1519339 - scrollTo with behavior: smooth doesn't allow to move into overflow:hidden area

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P2)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: An example

Instead scrollTo with behavior:instant works. Chrome can do both.

Comment 1

[Botond Ballo [:botond]]

This is a side effect of behavior:smooth being implemented via APZ, which doesn't allow scrolling past the overflow:hidden bounds in general.

To fix this, we may have to tell APZ about the entire scrollable overflow area in addition to what we currently send as the scrollable rect.

Comment 3

[Botond Ballo [:botond]]

This seems like more of a defect than an enhancement to me. Also going to bump up its priority to P2 as we're seeing users run into it (bug 1774754).

Comment 4

[Botond Ballo [:botond]]

As a stopgap solution, we could consider having scrollTo() with behavior: smooth take the main-thread scrolling codepath when calling on an overflow:hidden scroll frame.

Comment 5

[Botond Ballo [:botond]]

Attachment: Bug 1519339 - Do not hand off smooth scrolls in an overflow:hidden direction to APZ. r=dlrobertson

APZ cannot currently scroll in directions which are overflow:hidden
(it does not know the full scroll range bounds in those directions).

Therefore, main-thread scrolling in these directions should not be
handed off to APZ.

Comment 6

[Botond Ballo [:botond]]

The posted patch implements the stopgap solution described in comment 4. I filed bug 1799238 to track a more proper solution.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/56bc7d891bd3
Do not hand off smooth scrolls in an overflow:hidden direction to APZ. r=dlrobertson

Comment 8

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on helper_overflowhidden_zoom.html

• backout: https://hg.mozilla.org/integration/autoland/rev/15a1a999b0f2bbb86dee38cb5fb91c18f55b674d
• push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=MpVtM6AMQkCdidL0moKhaw.0&revision=56bc7d891bd35c8d9f0e5413d0a8215a0900173f
• push where failures were noticed: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=MpVtM6AMQkCdidL0moKhaw.0&revision=da6fb931452a5489fde7469cfc17bdab5a837b7d&searchStr=mochitest
• failure log: https://treeherder.mozilla.org/logviewer?job_id=396517300&repo=autoland&lineNumber=2334

[task 2022-11-15T00:21:16.171Z] 00:21:16     INFO -  TEST-PASS | gfx/layers/apz/test/mochitest/test_group_zoom-2.html | helper_overflowhidden_zoom.html | shouldn't have scrolled (22)
[task 2022-11-15T00:21:16.171Z] 00:21:16     INFO -  Buffered messages finished
[task 2022-11-15T00:21:16.171Z] 00:21:16  WARNING -  TEST-UNEXPECTED-FAIL | gfx/layers/apz/test/mochitest/test_group_zoom-2.html | helper_overflowhidden_zoom.html | shouldn't have scrolled (23) - got 18880, expected +0
[task 2022-11-15T00:21:16.171Z] 00:21:16     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:487:14
[task 2022-11-15T00:21:16.171Z] 00:21:16     INFO -      spawnTest/w.is@gfx/layers/apz/test/mochitest/apz_test_utils.js:488:18
[task 2022-11-15T00:21:16.171Z] 00:21:16     INFO -      test@gfx/layers/apz/test/mochitest/helper_overflowhidden_zoom.html:73:9
[task 2022-11-15T00:21:16.171Z] 00:21:16  WARNING -  TEST-UNEXPECTED-FAIL | gfx/layers/apz/test/mochitest/test_group_zoom-2.html | helper_overflowhidden_zoom.html | shouldn't have scrolled (24) - got 18880, expected +0
[task 2022-11-15T00:21:16.171Z] 00:21:16     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:487:14
[task 2022-11-15T00:21:16.171Z] 00:21:16     INFO -      spawnTest/w.is@gfx/layers/apz/test/mochitest/apz_test_utils.js:488:18
[task 2022-11-15T00:21:16.172Z] 00:21:16     INFO -      test@gfx/layers/apz/test/mochitest/helper_overflowhidden_zoom.html:74:9
[task 2022-11-15T00:21:16.172Z] 00:21:16     INFO -  TEST-PASS | gfx/layers/apz/test/mochitest/test_group_zoom-2.html | helper_overflowhidden_zoom.html | shouldn't have scrolled (25)

Comment 9

[Botond Ballo [:botond]]

Backed out for failures on helper_overflowhidden_zoom.html

In this test, we call nsIDOMWindowUtils.scrollToVisual(20000,20000) on an overflow:hidden page, and expect it to only scroll to the end of the layout viewport, not the end of the page.

The failure is suggesting that it is in fact scrolling to the end of the page, suggesting that it's triggering one of the codepaths modified by the patch that would hand off the scroll to APZ previously (which would disallow scrolling past the layout viewport bounds), but now performs the scroll on the main thread (which allows it).

Since the purpose of the test is not to test the behaviour of scrollToVisual() itself, it would be easy to get around this failure by just changing the call to scroll to a destination that we know is within the layout viewport bounds.

However, two things about this failure are suspicious to me:

• By inspection, I don't see how scrollToVisual() would be triggering either of the modified codepaths.
• Why is the unexpected scroll offset only observed here and not here, when the scroll that scrollToVisual() is doing is an instant scroll?

Rather than papering over these issues, I'd like to investigate a bit further to understand these better.

Comment 10

[Botond Ballo [:botond]]

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

Rather than papering over these issues, I'd like to investigate a bit further to understand these better.

(However, since the test failure only occurs on Android, attempts to further investigate it locally are blocked on bug 1802618 at the moment.)

Comment 11

[Botond Ballo [:botond]]

I was able to get local Android test runs to work and arrive at a diagnosis of the failure of helper_overflowhidden_zoom.html. The sequence of events that leads to the failure is quite bespoke. Surprisingly, scrollToVisual() is not involved at all.

• Things start to go wrong when, after having scrolled the visual viewport away from the page origin, the test zooms back out using setResolutionAndScaleTo(1.0).
• This decreases the resolution without directly modifying the visual viewport offset (in a production scenario, APZ would modify both simultaneously), resulting in a visual viewport offset stored on the main thread that's temporarily (until the next APZ repaint) out-of-bounds .
• During this period that the main's thread copy of the visual viewport offset is out of bounds, the scroll frame is reflowed as a result of the resolution change.
  • During the reflow, the out-of-bounds value causes us to pass an inconsistent set of values to FinishReflowForScrollbar(). Specifically, we pass aMinXY=0 and aMaxXY=0 (because the scroll range is correctly computed to be empty and positioned at the layout scroll offset of (0,0)), but for aCurPosXY we pass the nonzero (out-of-bounds) visual viewport offset. This in turn causes us to set those inconsistent values as the scrollbar element's minpos, maxpos, and curpos attributes.
• Those inconsistent attribute values are read in nsSliderFrame::AttributeChanged(), where curpos being larger than maxpos triggers a call to ScrollByWhole(), scrolling all the way to bottom of the page! (Note, this is a scroll position much further down than the initial out-of-bounds value of the visual viewport offset).
  • It is this call that used to be a no-op before my patch, i.e. it was handed off to APZ which ignored it because the scroll frame is overflow:hidden, and now with my patch triggers main-thread scrolling to the bottom of the page.

Comment 12

[Botond Ballo [:botond]]

I'm thinking an appropriate fix here would be to prevent the main thread's copy of the visual viewport offset from becoming out of bounds as a result of zooming out, by re-clamping it in SetResolutionAndScaleTo().

Not sure if we should also make changes to the behaviour of nsSliderFrame::AttributeChanged() -- the fact that calling it with { minpos=0, maxpos=0, curPos=X } results in scrolling to the end of the page which may be a scroll offset > X seems pretty unfortunate. I'm open to suggestions on this point.

Comment 13

[Botond Ballo [:botond]]

Meanwhile, a scrollend-related regression (bug 1803455) has caused the test being added by my patch (helper_bug1519339_hidden_smoothscroll.html) to start failing. Marking that bug as a dependency.

Comment 14

[Botond Ballo [:botond]]

Attachment: Bug 1519339 - Reclamp the visual viewport offset in SetresolutionAndScaleTo(). r=tnikkel

Depends on D161351

Comment 15

[Botond Ballo [:botond]]

Attachment: Bug 1519339 - Clamp the pending visual scroll offset if using it as the main-thread's visual viewport offset. r=tnikkel

Depends on D163532

Comment 16

[Botond Ballo [:botond]]

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

Meanwhile, a scrollend-related regression (bug 1803455) has caused the test being added by my patch (helper_bug1519339_hidden_smoothscroll.html) to start failing. Marking that bug as a dependency.

This is now sorted -- thanks Dan for the quick fix in bug 1803455! This bug should be ready to re-land pending review of the added patches.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe78edcec148
Do not hand off smooth scrolls in an overflow:hidden direction to APZ. r=dlrobertson
https://hg.mozilla.org/integration/autoland/rev/b19ceb3b1ad9
Reclamp the visual viewport offset in SetresolutionAndScaleTo(). r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/4af9c56eb6d8
Clamp the pending visual scroll offset if using it as the main-thread's visual viewport offset. r=tnikkel

Comment 18

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/fe78edcec148
https://hg.mozilla.org/mozilla-central/rev/b19ceb3b1ad9
https://hg.mozilla.org/mozilla-central/rev/4af9c56eb6d8