Closed Bug 1498812 Opened 6 years ago Closed 5 years ago

Visual viewport breaks scroll position saving for Session Store/Session History

Categories

(Firefox :: Session Restore, defect, P1)

63 Branch
All
Android
defect

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)

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.
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.
Blocks: 1423011
Severity: normal → major
Flags: needinfo?(kshvmdn)
Flags: needinfo?(botond)
Version: Trunk → 63 Branch
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)
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 [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)
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() [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
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 [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.
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.
Assignee: nobody → jh+bugzilla
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.
Blocks: 1499210
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.
Blocks: 1497144
See Also: → 1500554
(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.
Blocks: 1509575
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.
See Also: → 1516471
See Also: 1516471
No longer blocks: 1509575
Depends on: 1517103
See Also: → 1511823
Depends on: 1516782
... 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.
Priority: -- → P1
See Also: 15005541509575
Blocks: 1516056
Blocks: 1517895
No longer blocks: 1516056
No longer depends on: 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.

(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,snorp

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.

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.

(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,snorp

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.

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.

Blocks: 1517976

(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)

(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)
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

Too late in the cycle to take this for 65.

Flags: in-testsuite+
Depends on: 1519621
Depends on: 1531057
Depends on: 1538762
No longer depends on: 1538762
Regressions: 1538762
No longer depends on: 1531057
Regressions: 1531057
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: