Closed Bug 1543485 Opened 5 years ago Closed 4 years ago

window.scrollTo() should preserve the relative offset between the visual and layout viewports

Categories

(Core :: Panning and Zooming, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
81 Branch
Tracking Status
firefox68 --- wontfix
firefox81 --- fixed

People

(Reporter: botond, Assigned: botond)

References

(Blocks 3 open bugs, )

Details

Attachments

(4 files)

This is a follow-up to bug 1516056 to track making sure we are passing http://w3c-test.org/visual-viewport/viewport-scroll-event-manual.html, which came up during discussion in bug 1516056 comment 7.

At the moment, I can't even get this test to recognize the first step (pinch-zooming near the "Continue" button as instructed does not cause the test to move on to the next step), even with dom.visualviewport.enabled=true.

Type: defect → enhancement
Priority: -- → P3

(In reply to Botond Ballo [:botond] from comment #0)

At the moment, I can't even get this test to recognize the first step (pinch-zooming near the "Continue" button as instructed does not cause the test to move on to the next step), even with dom.visualviewport.enabled=true.

In retrospect that's not surprising, as I just copied the test file onto my phone, but in fact there are JS files it depends on.

Jan, how did you run this manual test on Android?

Flags: needinfo?(jh+bugzilla)

I just ran it directly from w3c-test.org. (Or for running locally, make sure you have all the support files available.)

Flags: needinfo?(jh+bugzilla)

The test in question seems to expect that window.scrollTo() preserves the relative offset between the visual and layout viewports.

It's not entirely clear to me that this behaviour is desirable. When we initially contemplated this, we chose to make window.scrollTo() collapse the relative offset (i.e. scroll both the layout and visual viewports to the specified location), because we figured that a page performing window.scrollTo() might be trying to bring the specified location (e.g. a particular section header) into view, and preserving the relative scroll offset (which could be large) might get in the way of that.

I filed a spec issue seeking clarification on the intended behaviour and the motivation for it.

To summarize the discussion in the spec issue: Firefox's "collapse the relative offset to zero" behaviour and Chrome's "preserve the relative offset" behaviour are both largely artifacts of the respective implementation strategies.

There are arguments in favour of either behaviour, and either one is probably reasonable, but we should aim to be consistent for compat reasons.

That said, I don't think this needs to block the release of the Visual Viewport API. The compat issue here is not in the behaviour of the API, but in the underlying behaviour of the viewports themselves, and it's already observable without the API.

Summary: Make sure we are passing http://w3c-test.org/visual-viewport/viewport-scroll-event-manual.html → window.scrollTo() should preserve the relative offset between the visual and layout viewports

are both largely artifacts of the respective implementation strategies.

That would have been my guess, too, given how the events themselves are spec'd (only a change in the relative offset is supposed to fire the event).

Note to self: when this is fixed, the (more targeted and hacky) fix for bug 1549625 can be backed out.

Blocks: 1563938

This will be significantly easier to implement with bug 1519285 in place.

Depends on: 1519285
Blocks: 1566714
Blocks: 1568826

The hacky fix in bug 1549625 has caused a number of regressions.

I'd like to back it out, but I can't without providing an alternative fix because it also fixes an important regression.

This bug provides an alternative fix, so I'm going to prioritize getting this fixed so we can back out the existing fix in bug 1549625.

Assignee: nobody → botond
Priority: P3 → P1
No longer blocks: 1566714

We ended up solving the regressions another way in bug 1568826.

This is still something we want to fix, but not as urgent any more.

Priority: P1 → P3

This blocks desktop zooming, because otherwise enabling desktop zooming will regress bug 1566826.

Now that main thread scroll updates preserve the relative scroll offset
in APZ, this hacky fix is no longer necessary.

Depends on D87686

I verified that the patches:

The patches get several previously-failing assertions passing.

Depends on D87692

Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e995ac2f9e47
Preserve the relative scroll offset of the two viewports when accepting a main thread scroll update. r=kats
https://hg.mozilla.org/integration/autoland/rev/7397af51480f
Back out the hacky fix for bug 1549625. r=kats
https://hg.mozilla.org/integration/autoland/rev/7a196504e6d3
Also update the code in ScrollToImpl() that proactively updates the callback transform and main thread visual viewport offset. r=kats
https://hg.mozilla.org/integration/autoland/rev/a6dd2a21308a
Update wpt expectations. r=kats
Regressions: 1742241
You need to log in before you can comment on or make changes to this bug.