-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Infra: Scroll on load with a sticky banner #3066
base: main
Are you sure you want to change the base?
Conversation
This ensures that the scroll-margin-top is respected, by triggering a scroll event after the corresponding JS is injected. Co-Authored-By: C.A.M. Gerlach <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this, @pradyunsg . It seems like this does resolve it on hard-(Shift-)reload on desktop for me with FF 107.0.1 on Windows.
I am still seeing one issue I saw previously on Firefox Mobile 110.1.0 on a Pixel 3a running Android 12, specifically with the banner height resizing after the scroll has occurred on a hard (unchached) reload (but not on a cached reload). This is a smaller change and happens to be a decrease in height, which is less of a problem than an increase.
However, this incorrect offset persists when I click another heading, which means that rather being an issue with the scroll-margin
being set after the scroll event occurs, this particular issue appears to in fact caused by the correct scroll-margin not being set at all. However, when I change the device orientation, it either doesn't seem to trigger a resize
event (maybe the device renders and caches a horizontal version of the page or something? no idea), or it otherwise stays stuck. Only a cached reload fixes it.
What appears to be causing it is that the webfont is only getting fetched and rendered presumably after the load
event occurs, which then changes the effective text size of the banner, which then results in the banner height reducing, but the new scroll margin doesn't get set.
After far too much trial and error I managed to capture the following screenshots showing this:
Page draw starts | Page initially loaded with fallback font | Webfont loads in, banner height updates but scroll-margin doesn't |
---|---|---|
Testing this on the FF mobile device simulator, for Pixel 2 and Pixel 5 simulating Chrome, on the latter I can sometimes repro an issue with the banner resizing after scroll (and covering some of the content, in this case) on hard-reload—if I apply heavy enough bandwidth throttling, I can repro it 100% of the time, and indeed in this case the resize after scroll is again caused by the webfont loading. However, in this case resize
apparently fires (or the correct scroll-margin
is otherwise set after the scroll) as either clicking another heading or running adjustBannerMarginAndScroll()
manually in the web console fixes it. Of course, in this case who knows how much this had to do what would happen on the actual device, and what is down to the specifics of the simulation.
Rendering | Fully loaded |
---|---|
Simulator details:
However, I'm not sure what to do about it other than firing another scrollandresize call on a 1s timer or something, or just switching to a pure system font stack (which we've talked about for a while) and hoping nothing else causes a delayed resize on render.
Oh and just to note, I can reproduce the first problem, the one on my actual mobile browser, exactly the same on the production site as on this PR (as expected, given what's happening). The second problem, the one in the device simulator, I actually cannot reproduce on the live site, but as I cannot reproduce it on my actual Android Pixel 3a device, and it could come down to timing variations between the preview platform on RTD and the live site on GHP (though I do have heavy throttling on), it's not totally clear if that's a real issue or not. |
This is basically @CAM-Gerlach's suggestion for how to resolve #3065, in the initial PR that introduced that functionality (#2992).
📚 Documentation preview 📚: https://pep-previews--3066.org.readthedocs.build/