Prevent issues when storing the top offset in AdjustableSidebarWidth

[marinaaisa]

Bug/issue #128197719, if applicable:

Summary

Make sure the page is fully loaded before storing top offset in AdjustableSidebarWidth

Dependencies

NA

Testing

Steps:

1. Run a doccarchive with a navigator
2. Make sure that the navigator works as expected

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

• Added tests
• Ran npm test, and it succeeded
• Updated documentation if necessary

[mportiz08]

@marinaaisa are there testing instructions you can add to this?

[marinaaisa]

@marinaaisa are there testing instructions you can add to this?

Ops, sure, I just added them.

[hqhhuang]

Thank you Marina for investigating this issue and explaining it. I think I may (or may not) have found a better way to fix this.

If I understood this correctly, the issue is that the offset we fetch of baseNavStickyAnchor is sometimes inaccurate, causing issues in our other logic.
As I was reviewing your changes, I noticed that we use a initialLoadingPlaceHolder component here. I think the better solution is to move the placeholder component to be under the baseNavStickyAnchor here. Although delaying the timing of fetching this offset solves the issue, I think the actual issue is not necessarily the timing. What's probably happening is that the baseNavStickyAnchor is below the initialLoadingPlaceHolder, so the offset we are currently fetching is sometimes too big by the height of the placeholder component. If we move the placeholder to below the anchor, then the offset will always be correctly. In that case, we don't need to delay the logic for fetching the offset.

It's a bit hard to explain as well, but let me know if you think this makes sense?

[hqhhuang]

Not sure what this was initially meant to wait for. But would we still need this to ensure everything is updated before focus trapping?

[mportiz08]

Thank you Marina for investigating this issue and explaining it. I think I may (or may not) have found a better way to fix this.

If I understood this correctly, the issue is that the offset we fetch of baseNavStickyAnchor is sometimes inaccurate, causing issues in our other logic. As I was reviewing your changes, I noticed that we use a initialLoadingPlaceHolder component here. I think the better solution is to move the placeholder component to be under the baseNavStickyAnchor here. Although delaying the timing of fetching this offset solves the issue, I think the actual issue is not necessarily the timing. What's probably happening is that the baseNavStickyAnchor is below the initialLoadingPlaceHolder, so the offset we are currently fetching is sometimes too big by the height of the placeholder component. If we move the placeholder to below the anchor, then the offset will always be correctly. In that case, we don't need to delay the logic for fetching the offset.

It's a bit hard to explain as well, but let me know if you think this makes sense?

Sounds right, but I think it's still a timing problem as you describe it. If I remember correctly, this placeholder is added to avoid an awkward situation before the routing and data fetching occurs so that the page doesn't look super compact and weird with the header/footer squashed together.

I would still consider it a timing issue though, because basically you need to wait for the time required for $router.onReady to fire (after which the placeholder is removed).

That also explains why this bug could happen, because I assume it's adding the height of the placeholder to the calculation if the calculation happens before it is removed (which is variable depending on the height of the window)—and by moving the placeholder below this anchor, its height wouldn't impact that calculation anymore.

[marinaaisa]

Sounds right, but I think it's still a timing problem as you describe it. If I remember correctly, this placeholder is added to avoid an awkward situation before the routing and data fetching occurs so that the page doesn't look super compact and weird with the header/footer squashed together.
I would still consider it a timing issue though, because basically you need to wait for the time required for $router.onReady to fire (after which the placeholder is removed).
That also explains why this bug could happen, because I assume it's adding the height of the placeholder to the calculation if the calculation happens before it is removed (which is variable depending on the height of the window)—and by moving the placeholder below this anchor, its height wouldn't impact that calculation anymore.

It's a really good point. I'm going to leave it as it is, then.

[hqhhuang]

Ops I was working on fixing tests. In case we still want to go with the placeholder approach I have that here: hqhhuang@78edb51

[mportiz08]

I think the placeholder approach might be better—just wanted to highlight that it is still a problem caused by the timing of the placeholder getting removed. Sorry if I was unclear!

[marinaaisa]

Ops I was working on fixing tests. In case we still want to go with the placeholder approach I have that here: hqhhuang@78edb51

Okay, @hqhhuang , could you cherry pick your commit into this branch then?

[hqhhuang]

@mportiz08 Do you mind giving it another look?

[hqhhuang]

@swift-ci test