1617427 - Text in an overflow-x: auto container fails to render while scrolling after opening dev tools

Status: VERIFIED FIXED

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

Description

[Hasan Ibraheem]

Attachment: test.html

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:72.0) Gecko/20100101 Firefox/72.0

Steps to reproduce:

1. Open test.html
2. Open dev tools (hitting f12)
3. Scrolling page

Actual results:

Region that I scrolled to was white until I clicked.

This behaviour goes away once you get rid of the overflow-x: auto on the pre element.

Expected results:

The scrolled to region should have the appropriately rendered text.

Comment 1

[Daniel Bodea [:danibodea]]

I can reproduce this issue on Windows 10 on the latest versions of the 4 main channels, however, this issue is somewhat partially reproduced on Mac OS. The content is rendered in Mac OS, just a little delayed.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

To be clear, the devtools need to be docked to the bottom for the bug to repro. I have my devtools in a different window and I couldn't initially repro.

This smells like APZ rather than layout. I know that devtools has some interactions with APZ... Botond, any ideas?

Comment 3

[Botond Ballo [:botond]]

What seems to be happening here is:

• Devtools disables APZ for the subframe (via this mechanism). APZ is still enabled for the root frame.
• As a result, we don't paint a displayport for the subframe, we only paint its viewport.
• Additionally, we clip the subframe's viewport to the viewport of the root frame. (This is to avoid rendering too much in case where a subframe has a huge viewport.)
• When you then scroll the root frame, it scrolls with APZ and paint skipping, but it brings into view parts of the subframe's viewport which haven't been rendered. Those parts are then never painted until something like a click event triggers a repaint.

Comment 4

[Botond Ballo [:botond]]

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

• Additionally, we clip the subframe's viewport to the viewport of the root frame. (This is to avoid rendering too much in case where a subframe has a huge viewport.)

I think conceptually the problem is here: we should be clipping the subframe's viewport to the displayport of the root frame. Then, the root frame can scroll with APZ and paint skipping just fine within the root frame's displayport. If we scroll the root frame far enough to move its displayport, we'll paint a new displayport including new regions of the subframe's viewport.

Comment 5

[Botond Ballo [:botond]]

P2 as this is a perma-checkerboarding issue (albeit one we've been shipping for a while).

Comment 6

[Kartikaya Gupta (email:[e-mail])]

I can take a look at this.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

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

What seems to be happening here is:

• Devtools disables APZ for the subframe (via this mechanism). APZ is still enabled for the root frame.
• As a result, we don't paint a displayport for the subframe, we only paint its viewport.
• Additionally, we clip the subframe's viewport to the viewport of the root frame. (This is to avoid rendering too much in case where a subframe has a huge viewport.)
• When you then scroll the root frame, it scrolls with APZ and paint skipping, but it brings into view parts of the subframe's viewport which haven't been rendered. Those parts are then never painted until something like a click event triggers a repaint.

This is mostly correct, except that strictly speaking, we do still have a displayport for the subframe. It's just that it uses zero margins to inflate (here) and so gets sized to the "display port base" size, rounded up to the nearest tile. The "display port base" size is computed in the block of code below this comment, and was the subject of much fiddling back in the day (see bug 1327095 and friends).

But yes, fundamentally the problem does seem to be that it uses the root composition bounds (i.e. the viewport of the root frame) as one of the clipping factors for the display port base, and increasing that to use the root displayport instead will likely fix the problem. Doing that might be nontrivial but I can try.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Braindump of current state:

• patches are at https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=7b25952b97afc2a34cc31701ffb185222727be72 and seem to work fine. I manually tested a few different scenarios including the case where the subframe has a CSS scale transform and things look good.
• trying to write a mochitest to exercise this bug is hard. My plan was to set up the situation that reproduces the checkerboard and then to somehow test for whether or not checkerboarding was happening
• setting up the checkerboard situation is working now, I had to add a DWU method to disable apz on an element but it wasn't too bad
• testing for checkerboarding is a little harder, because we need to compute the user-visible part of a scrollframe which involves walking up the APZ tree and doing intersections
• while doing this I discovered that the IsCurrentlyCheckerboarding function is not hooked up to anything

Comment 9

[Kartikaya Gupta (email:[e-mail])]

I finally got the mochitest working and hammered out all the intermittents I could find. Ran it with --verify without the patch to ensure it failed consistently and then ran it with --verify with the patch to validate the fix.

Try push is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=552500bc43d22d23a8ed7fa5063228c871afe28e

Assuming that's good, I'll rebase to current master, do any clang-formatting/static analysis fixes, and put it up for review.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Hm, test doesn't work on android because I used wheel events which aren't supported there. I can probably adjust the test to not use wheel events, will give that a shot first.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1617427 - Refactor to extract helper method; no functional changes. r?tnikkel

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1617427 - Prefer using the root frame's displayport to the root composition bounds. r?tnikkel

If we can get the root frame's displayport, then we should use that
rect instead of the root composition bounds when clipping the scrollframe's
displayport. That way if APZ is disabled on the scrollframe, but the root
frame scrolls to bring a part of it into view, it will be fully painted and
not perma-checkerboard-y.

Note that this patch is the main fix, but leaves a bunch of comments/variables
with bad names; the next patch cleans that up.

Depends on D66420

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1617427 - Rename things and fix up comments. r?tnikkel

Depends on D66421

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1617427 - Add a DOMWindowUtils API to disable APZ on a particular element. r?tnikkel

Depends on D66423

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1617427 - Miscellaenous documentation fixes. r?botond

Depends on D66424

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1617427 - Extract helper method to normalize visible rect computation. r?botond

Slight functional changes:

• the checkerboard event call site will now include mTestAsyncScrollOffset
  when calculating the visible rect, which should impact overall behaviour if
  there's a test that cares about checkerboard events (there currently isn't).
• the IsCurrentlyCheckerboarding call site will use the compositing effective
  scroll offset instead of the raw metrics scroll offset. This function is
  not called from anywhere so it doesn't matter, but it makes sense to align
  it with the other uses and I'll be using it in future patches.

Depends on D66425

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1617427 - Resurrect IsCurrentlyCheckerboarding and make it more correct. r?botond

This improves the implementation of IsCurrentlyCheckerboarding (which is not
invoked from anywhere prior to this patch) so that it takes into account the
recursive clipping applied by ancestor layers' composition bounds. In other
words, the visible rect for a layer may be additionally clipped because
ancestor scrollframes have scrolled, and this patch accounts for that.

It also records the currently-checkerboarding state into the APZTestData
at the time that the compositor APZTestData instance is fetched.

Depends on D66426

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1617427 - Add a test for the scenario being fixed. r?tnikkel,botond

Depends on D66427

Comment 19

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)

But yes, fundamentally the problem does seem to be that it uses the root composition bounds (i.e. the viewport of the root frame) as one of the clipping factors for the display port base, and increasing that to use the root displayport instead will likely fix the problem. Doing that might be nontrivial but I can try.

So this means that we could get a "multiply effect", where the displayport base of a non-root scroll frame is set based on the displayport of the root, and then the displayport of the non-root element is expanded from the displayport base to it's displayport. The multiplication though is limited to that, we can't get a chain of displayport bases that are in turn based on an ancestor displayport, since we always look at the root displayport. So there is still some finite upper bound, which is the important part. We are increasing the upper bound though.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

(In reply to Timothy Nikkel (:tnikkel) from comment #19)

So this means that we could get a "multiply effect", where the displayport base of a non-root scroll frame is set based on the displayport of the root, and then the displayport of the non-root element is expanded from the displayport base to it's displayport. The multiplication though is limited to that, we can't get a chain of displayport bases that are in turn based on an ancestor displayport, since we always look at the root displayport. So there is still some finite upper bound, which is the important part. We are increasing the upper bound though.

Yes, agreed.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/296aad2e1950
Refactor to extract helper method; no functional changes. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/275f9a696ae1
Prefer using the root frame's displayport to the root composition bounds. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/2e6aefd7a093
Rename things and fix up comments. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/c1c55cb6cf31
Add a DOMWindowUtils API to disable APZ on a particular element. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/ba8c8fe461d5
Miscellaenous documentation fixes. r=botond
https://hg.mozilla.org/integration/autoland/rev/6af812a825de
Extract helper method to normalize visible rect computation. r=botond
https://hg.mozilla.org/integration/autoland/rev/35d9d70da6c9
Resurrect IsCurrentlyCheckerboarding and make it more correct. r=botond
https://hg.mozilla.org/integration/autoland/rev/0edab0f8fa94
Add a test for the scenario being fixed. r=botond

Comment 22

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/296aad2e1950
https://hg.mozilla.org/mozilla-central/rev/275f9a696ae1
https://hg.mozilla.org/mozilla-central/rev/2e6aefd7a093
https://hg.mozilla.org/mozilla-central/rev/c1c55cb6cf31
https://hg.mozilla.org/mozilla-central/rev/ba8c8fe461d5
https://hg.mozilla.org/mozilla-central/rev/6af812a825de
https://hg.mozilla.org/mozilla-central/rev/35d9d70da6c9
https://hg.mozilla.org/mozilla-central/rev/0edab0f8fa94

Comment 23

[Catalin Sasca, Desktop QA [:csasca]]

Successfully reproduced the issue on Firefox Nightly 75.0a1 (2020-02-22) under macOS 10.15 using the STR provided in Comment 0.

The issue is no longer reproducible on latest Nightly 77.0a1 (2020-04-15) and Firefox 76.0b5. Tests were performed on Windows 10 (x64), Ubuntu 18.04 (x64) and macOS 10.15.