Visual viewport breaks scroll position saving for Session Store/Session History
Categories
(Firefox :: Session Restore, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox62 | --- | unaffected |
firefox63 | + | wontfix |
firefox64 | --- | wontfix |
firefox65 | --- | wontfix |
firefox66 | --- | fixed |
People
(Reporter: JanH, Assigned: JanH)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(11 files)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review |
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.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
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.
Assignee | ||
Comment 2•6 years ago
|
||
And it's not just the scroll events that are missing in that case - the scroll position itself as queried through ScrollPosition.jsm [1] is also some outdated value that no longer corresponds to the actual scroll position as visible on the screen. [1] (i.e. ultimately https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/toolkit/modules/sessionstore/ScrollPosition.jsm#43-45)
Assignee | ||
Updated•6 years ago
|
Comment 3•6 years ago
|
||
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 [1] for an illustration (the layout viewport is the blue rectangle), and this document [2] for more details. [1] http://bokand.github.io/viewport/index.html [2] https://github.com/bokand/bokand.github.io/blob/master/web_viewports_explainer.md
Assignee | ||
Comment 4•6 years ago
|
||
For the session store I definitively need the scroll position and scroll events for the visual viewport then, though.
Assignee | ||
Comment 5•6 years ago
|
||
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.
Comment 6•6 years ago
|
||
(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() [1] (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. [1] https://searchfox.org/mozilla-central/rev/26b40a44691e0710838130b614c2f2662bc91eec/dom/interfaces/base/nsIDOMWindowUtils.idl#851
Assignee | ||
Comment 7•6 years ago
|
||
That sounds good, but I'll also need a replacement for the "scroll" event - what about that?
Comment 8•6 years ago
|
||
(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?
Assignee | ||
Comment 9•6 years ago
|
||
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.
Assignee | ||
Comment 10•6 years ago
|
||
(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.
Comment 11•6 years ago
|
||
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?
Assignee | ||
Comment 12•6 years ago
|
||
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 [1] 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). [1] At least on Android, not quite sure about the Desktop implementation, plus on Desktop I think currently only extensions can trigger this, anyway.
Comment 13•6 years ago
|
||
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.
Comment 14•6 years ago
|
||
(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?
Assignee | ||
Comment 15•6 years ago
|
||
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.
Comment 16•6 years ago
|
||
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?
Comment 17•6 years ago
|
||
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).
Assignee | ||
Comment 18•6 years ago
|
||
Yes, it's certainly possible to switch to recording the visual offset without waiting for the VisualViewport.onscroll implementation.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 19•6 years ago
|
||
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.
Comment 20•6 years ago
|
||
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.
Assignee | ||
Comment 21•6 years ago
|
||
(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...
Comment 22•6 years ago
|
||
(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.
Assignee | ||
Comment 23•6 years ago
|
||
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.
Assignee | ||
Comment 24•6 years ago
|
||
Properly fixing this turned out more complicated, so this definitively won't make 63.
Comment 25•5 years ago
|
||
Getting late to fix in 64, but we could still take a patch for 65.
Updated•5 years ago
|
Comment 26•5 years ago
|
||
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.
Assignee | ||
Comment 28•5 years ago
|
||
Mainly required for testing.
Assignee | ||
Comment 29•5 years ago
|
||
... so it can be shared across multiple test files.
Assignee | ||
Comment 30•5 years ago
|
||
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.
Assignee | ||
Comment 31•5 years ago
|
||
Our internal Visual Viewport scroll events are dispatched system group-only, so this is the only way to catch them.
Assignee | ||
Comment 32•5 years ago
|
||
Likewise the only way to catch our internal visual viewport events.
Assignee | ||
Comment 33•5 years ago
|
||
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.
Assignee | ||
Comment 34•5 years ago
|
||
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.
Assignee | ||
Comment 35•5 years ago
|
||
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.
Assignee | ||
Comment 36•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 38•5 years ago
|
||
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.
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 39•5 years ago
|
||
So that the caller doesn't have to retrieve and compare the previous viewport offset himself.
Comment 40•5 years ago
|
||
(In reply to Jan Henning [:JanH] from comment #35)
Created attachment 9034289 [details]
Bug 1498812 - Part 9: Switch session store/session history to use visual
viewport for scroll position tracking. r?mikedeboer,snorpFor 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.
Without having looked at the patches, the immediate concern that rises with me is about how we capture and properly restore pinch-zoomed content. If it's yet another variation on full-page-zoom, then I'm not worried, of course.
Comment 41•5 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #40)
(In reply to Jan Henning [:JanH] from comment #35)
Created attachment 9034289 [details]
Bug 1498812 - Part 9: Switch session store/session history to use visual
viewport for scroll position tracking. r?mikedeboer,snorpFor 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.Without having looked at the patches, the immediate concern that rises with me is about how we capture and properly restore pinch-zoomed content. If it's yet another variation on full-page-zoom, then I'm not worried, of course.
Pinch-zoom is distinct from full-page-zoom. The state of where we've pinch-zoomed to is captured by the resolution (essentially, the zoom level) and the visual viewport offset. We are already saving and restoring the resolution in the session store, and with this patch we'll be doing that with the visual viewport offset as well, so the entire pinch-zoomed state will be saved and restored.
Comment 42•5 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #41)
Pinch-zoom is distinct from full-page-zoom. The state of where we've pinch-zoomed to is captured by the resolution (essentially, the zoom level) and the visual viewport offset. We are already saving and restoring the resolution in the session store, and with this patch we'll be doing that with the visual viewport offset as well, so the entire pinch-zoomed state will be saved and restored.
So does this mean that we need similar changes for desktop sessionstore?
Assignee | ||
Comment 43•5 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #42)
(In reply to Botond Ballo [:botond] from comment #41)
Pinch-zoom is distinct from full-page-zoom. The state of where we've pinch-zoomed to is captured by the resolution (essentially, the zoom level) and the visual viewport offset. We are already saving and restoring the resolution in the session store, and with this patch we'll be doing that with the visual viewport offset as well, so the entire pinch-zoomed state will be saved and restored.
So does this mean that we need similar changes for desktop sessionstore?
Once Desktop gets pinch-zooming, too, yes.
Comment 44•5 years ago
|
||
Pushed by mozilla@buttercookie.de: 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
Comment 45•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/466e822d2986
https://hg.mozilla.org/mozilla-central/rev/bbbb9e3c793d
https://hg.mozilla.org/mozilla-central/rev/bf744ce7867a
https://hg.mozilla.org/mozilla-central/rev/7b00521b6f31
https://hg.mozilla.org/mozilla-central/rev/710922bc053a
https://hg.mozilla.org/mozilla-central/rev/4eccacfc8801
https://hg.mozilla.org/mozilla-central/rev/c4fbfcb5239b
https://hg.mozilla.org/mozilla-central/rev/2d9a52630c04
https://hg.mozilla.org/mozilla-central/rev/39207d39e5c2
https://hg.mozilla.org/mozilla-central/rev/f0f5124781cc
https://hg.mozilla.org/mozilla-central/rev/a99bf382e5f7
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Description
•