Closed Bug 1549625 Opened 5 years ago Closed 5 years ago

Weird behaviour while running viewport-offset-manual.html

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- unaffected
firefox68 --- fixed

People

(Reporter: botond, Assigned: botond)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

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.

Attached file Reduced testcase

The behaviour here is a regression.

Has Regression Range: --- → no
Keywords: regression
Has Regression Range: no → yes
Regressed by: 1516056

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.

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.

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

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.

No longer blocks: visual-viewport-api

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.

Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dba3c6692a02
Avoid clobbering the visual scroll offset if the layout viewport shrinks in size r=kats
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Regressions: 1551582
Regressions: 1557424
Regressions: 1563938
Regressions: 1566714
Regressions: 1568826
You need to log in before you can comment on or make changes to this bug.