1549625 - Weird behaviour while running viewport-offset-manual.html

Status: RESOLVED FIXED

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

Description

[Botond Ballo [:botond]]

While running the Visual Viewport API's manual web platform tests, I noticed a weird behaviour while running viewport-offset-manual.html.

At step 5, when you're instructed to scroll fully to the bottom right, whenever you do so, the scroll position jumps back to a lower value.

The test still passes if you're careful to scroll far enough to the bottom right that you can click the button and proceed with the test, but not all the way to the jumping is not triggered, so it's not a failure per se, but it should be investigated. Chrome does not have this behaviour.

Comment 1

[Botond Ballo [:botond]]

Attachment: Reduced testcase

Comment 2

[Botond Ballo [:botond]]

The behaviour here is a regression.

Comment 3

[Botond Ballo [:botond]]

From bug 1516056.

Comment 4

[Botond Ballo [:botond]]

What's happening here is:

• Reaching the bottom of the page causes the URL bar to transition back onscreen.
• The page is reflowed into a smaller ICB size.
• The layout viewport size on this page is equal to the ICB size, due to the min-scale: 1.
• The content size on this page is 150% of the ICB size.
• Therefore, both the layout viewport size and the content size shrink, but the content size shrinks more (due to the 150%), so the layout scroll range also shrinks.
• ScrollFrameHelper::ReflowFinished() re-clamps the layout scroll offset to the new (smaller) layout scroll range.
• Since this is different from the previous layout scroll offset, we don't take the early-exit in ScrollToImpl(), and update the scroll origin to be a main-thread origin.
• This causes the APZ (visual) scroll offset to be clobbered by the new layout scroll offset, causing the visual viewport to jump back there as well.

Comment 5

[Botond Ballo [:botond]]

It's tempting to blame this on the fact that URL bar transitions cause the ICB size to change, which doesn't happen with Chrome (bug 1515980), but I think that's a red herring.

One can easily imagine a similar scenario in a case where the ICB size is legitimately expected to change (for example, with desktop zooming because the user resizes the browser window). I think the visual scroll offset jumping by a potentially large amount in response to a small change to the ICB size is unexpected, regardless of what triggered the change to the ICB size.

I think the real issue is that we rely on the early-exit in ScrollToImpl() to avoid the visual scroll offset being clobbered by the layout scroll offset in cases where the layout scroll offset "didn't change". I think the issue here hinges on our precise definition of "change"; arguably, being re-clamped to account for the layout scroll range having shrunk shouldn't be considered a "change" for this purpose.

I have a candidate fix for this; I don't love it because it feels like we're heaping more and more logic related to scroll origins into ScrollToImpl(), but I don't have a better idea at this time.

Comment 6

[Botond Ballo [:botond]]

Attachment: Bug 1549625 - Avoid clobbering the visual scroll offset if the layout viewport shrinks in size

Comment 7

[Botond Ballo [:botond]]

If we end up fixing bug 1543485 by making layout scrolls preserve the relative offset between the visual and layout viewport, then this problem would go away as a consequence (without a need for the patch currently posted).

Comment 8

[Botond Ballo [:botond]]

While this issue was discovered while running a manual web platform test for the Visual Viewport API, it's not actually related to the Visual Viewport API, it's just a general scrolling regression.

Comment 9

[Botond Ballo [:botond]]

I would like to proceed with the targeted patch for now, to avoid shipping this regression in 68.

We will fix bug 1543485 in due course by making layout scrolls preserve the relative offset (and we can then remove this targeted fix), but that's a larger and more risky change that I don't want to depend on here.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dba3c6692a02
Avoid clobbering the visual scroll offset if the layout viewport shrinks in size r=kats

Comment 11

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/dba3c6692a02