Closed Bug 1498812 Opened Last year Closed 11 months ago
Visual viewport breaks scroll position saving for Session Store/Session History
STR: 1. Enable "browser.sessionstore.debug_logging" and restart. 2. To simplify things, also disable the dynamic toolbar from the settings (General -> Full-screen browsing). 3. Go to https://history.nasa.gov/afj/ap11fj/01launch.html and zoom in until the "Index to events" table approximately fills the screen (portrait mode). 4. Do small scroll movements back and forth and observe the logcat output for GeckoSessionStore. Expected: Lots of "scroll" events. Actual: For bigger scrolls/flings, or if you zoom out, you still get "scroll" events as expected, but when zoomed in and only doing small scrolls you can go for quite some time without getting any "scroll" events.
Summary: "scroll" events often not dispatched → "scroll" events often not dispatched, especially when zoomed in
Tracking requested for partially breaking dispatching of scroll events when scrolling with the page zoomed in, which means that e.g. the session store won't update its stored scroll position. Mozregression gives me https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=b62d582090e11edcdd01c2c707e0b07b33ad0b12&tochange=ecafdbe899a7b2e574922250e356f8610788ad75, so bug 1423011 broke this. (In reply to Jan Henning [:JanH] from comment #0) > 3. Go to https://history.nasa.gov/afj/ap11fj/01launch.html and zoom in until > the "Index to events" table approximately fills the screen (portrait mode). Actually any page is sufficient to reproduce this (e.g. https://hg.mozilla.org/mozilla-central/raw-file/c291143e24019097d087f9307e59b49facaf90cb/mobile/android/tests/browser/chrome/basic_article_mobile.html for a smaller example), just zoom in as far as you can.
And it's not just the scroll events that are missing in that case - the scroll position itself as queried through ScrollPosition.jsm  is also some outdated value that no longer corresponds to the actual scroll position as visible on the screen.  (i.e. ultimately https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/toolkit/modules/sessionstore/ScrollPosition.jsm#43-45)
Summary: "scroll" events often not dispatched, especially when zoomed in → Scroll position not properly updated when panning around while zoomed in
This is a deliberate consequence of bug 1423011, which implemented a layout vs. visual viewport distinction, to match the behaviour of Chrome and other mobile browsers. The scroll position reported to script now reflects the offset of the layout viewport. See this simulator  for an illustration (the layout viewport is the blue rectangle), and this document  for more details.  http://bokand.github.io/viewport/index.html  https://github.com/bokand/bokand.github.io/blob/master/web_viewports_explainer.md
For the session store I definitively need the scroll position and scroll events for the visual viewport then, though.
And so I don't forget it later on, this concerns not only the live scroll position as recorded through ScrollPosition.jsm, but also the scroll position that is stashed away in the PresState when we navigate away from a session history entry and then restored when we return to it.
(In reply to Jan Henning [:JanH] from comment #4) > For the session store I definitively need the scroll position and scroll > events for the visual viewport then, though. Internal consumers can access the visual viewport offset via nsIDOMWindowUtils.getVisualViewportOffsetRelativeToLayoutViewport()  (you would add the result of that to the scroll position (= layout viewport offset) to obtain the visual visual viewport offset. We can also add a plain getVisualViewportOffset() method if that would be more convenient.  https://searchfox.org/mozilla-central/rev/26b40a44691e0710838130b614c2f2662bc91eec/dom/interfaces/base/nsIDOMWindowUtils.idl#851
That sounds good, but I'll also need a replacement for the "scroll" event - what about that?
(In reply to Jan Henning [:JanH] from comment #7) > That sounds good, but I'll also need a replacement for the "scroll" event - > what about that? We don't currently have that, though we will once VisualViewport.onscroll is implemented (bug 1478776). Can I ask what your use case is?
As I mentioned, this is needed for the session store and session history in order to accurately restore the scroll position *as the user sees it*. E.g. saving and restoring horizontal scrolling on a "width=device-width" page (or a Desktop page without any meta viewport tag at all for that matter) is more or less completely broken now, because zooming in only affects the visual viewport now. As I understand it, the layout viewport continues taking up the full page width and therefore remains stuck at an y-position of 0. And even vertical scrolling is rather iffy, because the layout viewport's scroll position most likely won't match the visual viewport's position.
Summary: Scroll position not properly updated when panning around while zoomed in → Visual viewport breaks scroll position saving for Session Store/Session History
(In reply to Jan Henning [:JanH] from comment #9) > therefore remains stuck at an y-position of 0. Er, that should be x-position of course.
I meant, more specifically, what is your use case for listening to an event that fires whenever the visual viewport offset changes. Why isn't it sufficient to query the visual viewport offset when e.g. the page is navigated away from?
Because the session store needs to track the current scroll position in case the tab unexpectedly goes away (the case where this is really necessary is because Firefox crashed, although discarding a tab to save memory  work on the same principle that the tab data is kept reasonably current and therefore no extra state needs to be saved before the tab can be discarded).  At least on Android, not quite sure about the Desktop implementation, plus on Desktop I think currently only extensions can trigger this, anyway.
Ok, I see. I think what we can do then is implement VisualViewport.onscroll, and enable the VisualViewport API (currently behind a pref) for chrome consumers.
(In reply to Botond Ballo [:botond] from comment #13) > Ok, I see. I think what we can do then is implement VisualViewport.onscroll, > and enable the VisualViewport API (currently behind a pref) for chrome > consumers. I posted an outline of how to implement VisualViewport.onscroll in bug 1478776 comment 1. Would you like to give it a try?
If there was any chance of still fixing this for 63 I'd prefer if somebody who has more time could do this, but as I guess that this is illusory I might as well take it in that case.
Might it work, as an interim measure, to use the less accurate (layout) offset in case of a crash or discard, and the more accurate (visual) offset in case of a clean exit / navigation?
Or even to record the visual offset whenever a regular "scroll" event is fired? This way, an accurate offset would be recorded whenever the user's last scroll also scrolled the layout viewport (e.g. when the user has been scrolling consistently in one direction for the past few scrolls).
Yes, it's certainly possible to switch to recording the visual offset without waiting for the VisualViewport.onscroll implementation.
Component: Panning and Zooming → Session Restore
Product: Core → Firefox
I've just remembered that there is a third location where scroll positions are stored as well: within an SHEntry itself in case of anchor link/pushState navigation within the same document.
The user impact of the regression described in comment #1 doesn't seem big enough for taking a patch for uplift in RC week without QA, so tracking for 63 but marking as fix-optional to keep it under relman radar in case we have a fix soon and a dot release before 64 in which this bug could be evaluated again as a ride-along.
(In reply to Jan Henning [:JanH] from comment #18) > Yes, it's certainly possible to switch to recording the visual offset > without waiting for the VisualViewport.onscroll implementation. Although it seems that while calling scrollTo sets both layout and visual viewports to the requested position, by the time the "scroll" event has been received getVisualViewportOffset() doesn't necessarily yet return the new position. So maybe I do have to implement that event first after all...
(In reply to Jan Henning [:JanH] from comment #21) > Although it seems that while calling scrollTo sets both layout and visual > viewports to the requested position, by the time the "scroll" event has been > received getVisualViewportOffset() doesn't necessarily yet return the new > position. This is the case for JS-driven scrolling like scrollTo(), because moving the visual viewport has to go through a round-trip to the compositor. For user-driven scrolling, however, which originates from the compositor, the visual viewport will have been updated by the time the "scroll" event is fired.
Yes, but the existing session store tests all use scrollTo(), as does the session store itself when restoring the scroll position, so if I don't want to cause intermittent test failures, I need the other event as well I guess.
Properly fixing this turned out more complicated, so this definitively won't make 63.
Getting late to fix in 64, but we could still take a patch for 65.
Too late to fix in 64. Marking this issue as fix-optional for 65; if you land a patch in nightly and think it's low-risk for beta, please request uplift.
Mainly required for testing.
... so it can be shared across multiple test files.
The existing tests didn't catch this problem, because calling scrollTo(), which is both what a) the session store and session history itself are currently using to set the scroll position to be restored, as well as b) how the existing session store test is setting the page up to be scrolled ready for testing forces both the layout and visual viewport positions to the respective coordinates, even if the same configuration of visual and layout viewport offsets could never be achieved through manual scrolling (i.e. bug 1516056). This then triggers all the expected events and makes it so that reading the scroll position through the layout viewport returns the expected values, which is why the existing tests never noticed that something is off. Therefore, we introduce a test here that has a page where the layout viewport can never scroll (at least horizontally) and where we simulate scrolling by actually inputting fake touch events instead of simply calling scrollTo(). This will result in only the visual viewport scrolling, ensuring that we can properly test the new expected behaviour of the session store and session history. Because layout and visual viewport scroll positions aren't necessarily updated at exactly the same time and the session store is currently still relying on the conventional "scroll" events (relating to the layout viewport), which means the tests have to rely on the same events, too, we don't yet switch all session store tests to generally verify the current scroll position of the page using the visual viewport, and temporarily make this only opt-in via the corresponding helper function in head_scroll.js. I know that the proper way to reference "foreign" files in text manifests is to use !/absolute/path/to/file/helper.js, but as one of the files originally comes from a chrome mochitest and the other one (apz_test_utils.js) doesn't and this test itself is a chrome mochitest, this was the best way I found to get both files copied into the correct directory on the test device so the test could access them.
Our internal Visual Viewport scroll events are dispatched system group-only, so this is the only way to catch them.
Likewise the only way to catch our internal visual viewport events.
Easier than separately enabling it for each test in turn, and shouldn't have any bad side effects on tests that don't care about it, as this only exposes the new Window.visualViewport object, but doesn't change anything else.
This is now only being used as a purely internal helper function, so there's no need for mucking about with nsresults, out parameters, retrieving x- and y- coordinates separately, etc.
For simplicity's sake, for now we keep storing only one scroll position per history entry (bug 1499210), so if we have to choose between the layout and the visual viewport, the latter is a vastly better choice, as it more accurately represents the scroll position as perceived by the user, especially when the page has been pinch-zoomed. This also means that instead of the normal scroll events, the session store now has to listen for the corresponding events specific to the visual viewport. We also extend the scroll position test to check that the scroll position isn't just properly saved, but also actually properly restored in practice as well. We only add this test now instead of already adding it beforehand like we did with the rest of the test - to avoid having to temporarily extend the checkScroll() helper function to deal with todo()/todo_is etc. - because getting that part of the test to complete without timing out (which would be one of its natural failure modes, because the expected events would be missing) would require faking even more scroll events - because we already have the todo() tests that are telling us the we didn't *store* any scroll position in the first place, so there's no point in trying to actually restore anything For the GeckoView saveAndRestoreState test, we now spin the event loop once before setting the scroll position in order to give APZ opportunity to settle down after the initial page load.
No longer depends on: 1516782
Duplicate of this bug: 1516782
Given the usage example of pull-to-refresh in bug 1371796, downstream consumers will probably more interested in the true visible scroll position of the user within the page, i.e. the visual viewport. Listening for *visual* viewport events will also definitively be required to get the saveAndRestoreState GeckoView test properly working once we switch Gecko's session store helper function to use the *visual* viewport scroll position.
Attachment #9034283 - Attachment description: Bug 1498812 - Part 2: Move scroll position test helper functions into separate file. r?mikedeboer → Bug 1498812 - Part 3: Move scroll position test helper functions into separate file. r?mikedeboer
Attachment #9034284 - Attachment description: Bug 1498812 - Part 3: Add scroll position test that is specifically testing the Visual Viewport. r?mikedeboer → Bug 1498812 - Part 4: Add scroll position test that is specifically testing the Visual Viewport. r?mikedeboer
Attachment #9034285 - Attachment description: Bug 1498812 - Part 4: Allow registering System event listeners through nsSessionStoreUtils. r?nika → Bug 1498812 - Part 5: Allow registering System event listeners through nsSessionStoreUtils. r?nika
Attachment #9034286 - Attachment description: Bug 1498812 - Part 5: Allow promiseBrowserEvent to listen in system group. r?mikedeboer → Bug 1498812 - Part 6: Allow promiseBrowserEvent to listen in system group. r?mikedeboer
Attachment #9034287 - Attachment description: Bug 1498812 - Part 6: Generally enable Visual Viewport for Mochitests. r?botond → Bug 1498812 - Part 7: Generally enable Visual Viewport for Mochitests. r?botond
Attachment #9034288 - Attachment description: Bug 1498812 - Part 7: Simplify docshell's GetCurScrollPos() function. r?nika → Bug 1498812 - Part 8: Simplify docshell's GetCurScrollPos() function. r?nika
Attachment #9034289 - Attachment description: Bug 1498812 - Part 8: Switch session store/session history to use visual viewport for scroll position tracking. r?mikedeboer!,snorp! → Bug 1498812 - Part 9: Switch session store/session history to use visual viewport for scroll position tracking. r?mikedeboer,snorp
Attachment #9034290 - Attachment description: Bug 1498812 - Part 9: Use Visual Viewport for storing scroll position in the PresState. r?botond → Bug 1498812 - Part 11: Use Visual Viewport for storing scroll position in the PresState. r?botond,tnikkel
So that the caller doesn't have to retrieve and compare the previous viewport offset himself.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/466e822d2986 Part 1: Add helper method for directly retrieving the visual viewport's position. r=botond https://hg.mozilla.org/integration/autoland/rev/bbbb9e3c793d Part 2: Switch GeckoViewScrollChild to use the visual viewport. r=snorp https://hg.mozilla.org/integration/autoland/rev/bf744ce7867a Part 3: Move scroll position test helper functions into separate file. r=mikedeboer https://hg.mozilla.org/integration/autoland/rev/7b00521b6f31 Part 4: Add scroll position test that is specifically testing the Visual Viewport. r=mikedeboer https://hg.mozilla.org/integration/autoland/rev/710922bc053a Part 5: Allow registering System event listeners through nsSessionStoreUtils. r=nika https://hg.mozilla.org/integration/autoland/rev/4eccacfc8801 Part 6: Allow promiseBrowserEvent to listen in system group. r=mikedeboer https://hg.mozilla.org/integration/autoland/rev/c4fbfcb5239b Part 7: Generally enable Visual Viewport for Mochitests. r=botond https://hg.mozilla.org/integration/autoland/rev/2d9a52630c04 Part 8: Simplify docshell's GetCurScrollPos() function. r=nika https://hg.mozilla.org/integration/autoland/rev/39207d39e5c2 Part 9: Switch session store/session history to use visual viewport for scroll position tracking. r=mikedeboer,snorp https://hg.mozilla.org/integration/autoland/rev/f0f5124781cc Part 10: Return whether SetVisualViewportOffset was a no-op. r=botond https://hg.mozilla.org/integration/autoland/rev/a99bf382e5f7 Part 11: Use Visual Viewport for storing scroll position in the PresState. r=botond,tnikkel
You need to log in before you can comment on or make changes to this bug.