1682919 - Top and bottom of the page flicker when scroll with scroll up/down button

Status: VERIFIED FIXED

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

Description

[Alice0775 White]

Attachment: reduced.html

Reproducible: always

Steps to reproduce:

1. Open attached reduced.html (https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions)
2. Scroll with scroll up/down button

Actual Results:
Top and bottom of the page flicker while scrolling.
Screencast: https://youtu.be/LdwbDvs2Q-M

Expected Results:
Scroll without such glitch

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5dc0cfd11b2215c75bdd10a395855586ca9b5199&tochange=f0a8a5a5f78baa5e246082b71c2c2f56759e0518

Comment 1

[Alice0775 White]

Attachment: about:support

Comment 2

[Botond Ballo [:botond]]

Thanks for the report, I'll have a look.

Comment 3

[Botond Ballo [:botond]]

For some reason, content is painting a displayport that is unusually short (just as tall as the viewport), resulting in an async scroll of just a few pixels resulting in checkerboarding.

Comment 4

[Botond Ballo [:botond]]

The displayport is short because it's suppressed here.

Comment 5

[Botond Ballo [:botond]]

The suppression logic was added in bug 1295019. The motivation there was to avoid painting a large displayport when scrolling "far away", such as via the Home or End keys, and does not seem relevant to scrollbar buttons. It's also specific to main-thread scroll animations, which is what scrollbar button clicks trigger (if smooth scrolling is enabled).

We could avoid displayport suppression for scrollbar button clicks, or even offload the scroll animation triggered by scrollbar button clicks to APZ. However, there are a couple of things I do not understand yet:

• How exactly the checkerboarding arises as a result of repeated scrollbar button clicks. If the animations are all main-thread, there should be no async scrolling.
• Why bug 1669861 regressed this.

Comment 6

[Timothy Nikkel (:tnikkel)]

Hmm, I thought scrollbar button clicks would trigger

https://searchfox.org/mozilla-central/rev/31ddf859c57e812878a5f817e4097efb06de4d97/layout/generic/nsGfxScrollFrame.cpp#4640

of the "desktop zooming" scrollbar code, but I must be misremembering.

Comment 7

[Botond Ballo [:botond]]

The scrolling I'm seeing is triggered from here.

Comment 8

[Botond Ballo [:botond]]

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

However, there are a couple of things I do not understand yet:

• How exactly the checkerboarding arises as a result of repeated scrollbar button clicks. If the animations are all main-thread, there should be no async scrolling.
• Why bug 1669861 regressed this.

I've investigated further. The answers to these two questions are related.

We get an async scroll offset because we are getting paint-skipped transactions.

Paint-skipping does not make sense when the displayport is suppressed. It's supposed to be prevented by the existing check here which compares the old and new displayports. If the displayport is suppressed, such that it tightly fits the viewport, then any amount of scrolling will cause the old and new displayports to be different, and thereby cause the comparison to fail.

However, bug 1669861 caused a regression that resulted in a false positive pass of this comparison.

In that bug (and bug 1664101 to which that is a follow-up), we changed the way display port margins are stored. Instead of storing them relative to the layout viewport, we store them relative to the visual viewport, while also recording the visual and layout scroll offsets at the same time, such that we can adjust the margins to be relative to the layout viewport when necessary. (The motivation there was related to fixed-position content which requires a different adjustment.)

The visual and layout scroll offsets used for this adjustment are queried at the time the DisplayPortMargins object is constructed, which is typically when a repaint request from APZ is processed. However, if the displayport is supressed, we construct a new DisplayPortMargins object using DisplayPortMargins::Empty() at the time the displayport is queried. This means the visual and layout offsets are also queried at this time.

Unfortunately, for the purposes of the paint-skipping check, we query the new displayport here which is just after the layout scroll offset is updated, but just before the visual scroll offset is. That is, we are querying it in the short interval of time when the two offsets as stored on the scroll frame are inconsistent.

This inconsistent pair of values is then stored in the DisplayPortMargins, and results in an incorrect adjustment to the displayport rect as expressed relative to the layout viewport. This value then incorrectly passes the comparison with the old displayport rect, resulting in the paint-skipping codepath being entered.

Comment 9

[Botond Ballo [:botond]]

Attachment: Bug 1682919 - Avoid creating a new DisplayPortMargins object as a side effect of querying the displayport. r=tnikkel

DisplayPortMargins objects are only meant to be created when setting
display port margins, not when querying them, because the object's
constructor records the visual and layout scroll offsets at the time
of construction to use for adjusting the margins to be layout-relative.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e65982f45643
Avoid creating a new DisplayPortMargins object as a side effect of querying the displayport. r=tnikkel

Comment 13

[Sandor Molnar[:smolnar]]

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

Comment 14

[Cornel Ionce [:noni] [Hubs QA]]

Reproduced the issue on Windows 10x64 using 85.0.1 with the reduced testcase.
Confirming the issue is fixed with 86.0b9, buildID 20210211195159.

Comment 15

[Timothy Nikkel (:tnikkel)]

Very rarely I'm able to reproduce something similar to this -> bug 1693224.