Closed
Bug 1498812
Opened 6 years ago
Closed 6 years ago
Visual viewport breaks scroll position saving for Session Store/Session History
Categories
(Firefox :: Session Restore, defect, P1)
Tracking
()
RESOLVED
FIXED
Firefox 66
| 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)
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details |
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
|
Summary: "scroll" events often not dispatched → "scroll" events often not dispatched, especially when zoomed in
|
|
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.
Blocks: 1423011
Severity: normal → major
tracking-firefox63:
--- → ?
Flags: needinfo?(kshvmdn)
Flags: needinfo?(botond)
Version: Trunk → 63 Branch
|
|
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
|
Summary: "scroll" events often not dispatched, especially when zoomed in → Scroll position not properly updated when panning around while zoomed in
|
||
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
Flags: needinfo?(kshvmdn)
Flags: needinfo?(botond)
|
|
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.
Summary: Scroll position not properly updated when panning around while zoomed in → Visual viewport breaks scroll position saving for Session Store/Session History
|
|
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: nobody → jh+bugzilla
|
|
Assignee | |
Updated•6 years ago
|
Component: Panning and Zooming → Session Restore
Product: Core → Firefox
|
|
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 | |
Updated•6 years ago
|
|
|
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.
Getting late to fix in 64, but we could still take a patch for 65.
status-firefox66:
--- → affected
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.
|
||
Updated•6 years ago
|
|
|
||
Updated•6 years ago
|
|
|
Assignee | |
Updated•6 years ago
|
|
|
Assignee | |
Comment 28•6 years ago
|
||
Mainly required for testing.
|
|
Assignee | |
Comment 29•6 years ago
|
||
... so it can be shared across multiple test files.
|
|
Assignee | |
Comment 30•6 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•6 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•6 years ago
|
||
Likewise the only way to catch our internal visual viewport events.
|
|
Assignee | |
Comment 33•6 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•6 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•6 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•6 years ago
|
||
|
|
Assignee | |
Updated•6 years ago
|
Priority: -- → P1
|
|
Assignee | |
Updated•6 years ago
|
|
|
Assignee | |
Comment 38•6 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•6 years ago
|
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
|
|
||
Updated•6 years ago
|
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
|
|
||
Updated•6 years ago
|
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
|
|
||
Updated•6 years ago
|
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
|
|
||
Updated•6 years ago
|
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
|
|
||
Updated•6 years ago
|
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
|
|
||
Updated•6 years ago
|
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
|
|
||
Updated•6 years ago
|
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
|
|
Assignee | |
Comment 39•6 years ago
|
||
So that the caller doesn't have to retrieve and compare the previous viewport
offset himself.
|
||
Comment 40•6 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•6 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•6 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?
Flags: needinfo?(botond)
|
|
Assignee | |
Comment 43•6 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.
Flags: needinfo?(botond)
|
||
Comment 44•6 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•6 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
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
|
||
Updated•6 years ago
|
status-firefox-esr60:
--- → unaffected
|
|
||
Updated•6 years ago
|
|
|
||
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•