1424714 - Checkerboarding is frequent and covers sticky elements in specific test case

Status: RESOLVED FIXED

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

Description

[fabioturrin]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.84 Safari/537.36

Steps to reproduce:

Please see this demo: https://codepen.io/bludev/pen/BmpwPP



Actual results:

If you drag the slider of the vertical scroll bar, the grid disappears until you stop scrolling.


Expected results:

The scrolling should be smooth, without flickering. The fixed headers (position: 'sticky') should always be visible, even while scrolling.

Comment 1

[Mats Palmgren (inactive)]

> If you drag the slider of the vertical scroll bar, the grid disappears until you stop scrolling.

I can't reproduce that in a Nightly build on Linux.
Please try a Nightly and let us know if it works.
https://www.mozilla.org/en-US/firefox/channel/desktop/#nightly

> The scrolling should be smooth, without flickering.

I do see a bit of flickering when dragging the scrollbar thumb "fast".
I believe this is a painting issue, not layout.

Comment 2

[fabioturrin]

Unfortunately, even with the Nightly build on Windows 10 64bit there are no appreciable improvements.

Comment 3

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

I can reproduce on OS X beta (first one I tried)

This looks like checkerboarding, but the page is simple enough that we should profile it and make sure nothing bad is happening in gecko. And yeah, the position:sticky items should be visible even during checkerboarding. So that's something that needs fixing.

Comment 4

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

I'll try to look at this in the near future (this week)

Comment 5

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

I started looking into this and I'm pretty sure (as alluded in various other bugs that I can't claim to fully understand) the scrolled-clip property for the sticky container layer is wrong. For this test case the clip and scrolled-clip are the same. I think that means that as the grid scrolls, the scrolled-clip will shift too and therefore ends up clipping the sticky item which isn't moving as the grid scrolls. The code in AsyncCompositionManager keeps the clip in place for sticky items (it goes into the mFixedClip part of the ClipParts) but the scrolled clip gets adjusted by the async transform (since it is in the mScrolledClip part of the ClipParts) and then they get intersected.

The scrolled-clip and clip are both set to the displayport, but I think it makes more sense to set the scrolled-clip to something larger - specifically, the entire range of positions that the sticky position can be in. Something like union(initial_position, final_position) where the initial_position and final_position are determined by the scroll range. Or maybe we can drop the scrolled-clip entirely, but I'm not sure if that would run afoul of the requirements listed in this comment (because I don't fully understand the comment yet).

Comment 6

[Botond Ballo [:botond]]

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

For this test case the clip and scrolled-clip are the same. I think that means that as the grid scrolls, the scrolled-clip will shift too and therefore ends up clipping the sticky item which isn't moving as the grid scrolls. The code in AsyncCompositionManager keeps the clip in place for sticky items (it goes into the mFixedClip part of the ClipParts) but the scrolled clip gets adjusted by the async transform (since it is in the mScrolledClip part of the ClipParts) and then they get intersected.

That sounds like the same diagnosis as in bug 1548397 comment 5, suggesting the same root cause!

Comment 7

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

I tried taking out the the "if sticky position" part of the clause here. That removes the scrolled-clip from the sticky container layer and does fix the problem, but it breaks the position-sticky-scrolled-clip-1.html testcase. Examining that testcase leads me to believe that there are at least some scenarios where it makes sense for the sticky-pos layer to have a scrolled-clip (that testcase being one such scenario). Thinking more about what the right answer here is...

Comment 8

[Markus Stange [:mstange]]

I'm cheering for you, kats. I had the checkerboarding problem in mind when I implemented this, but I couldn't think of a better solution. But I'm sure a solution exists.

Comment 9

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

If you couldn't think of a better solution I'm not sure I will be able to either. What I'm thinking right now is that basically we want to keep the existing behaviour exactly as-is, except in the case where the scrolled-clip assigned to the sticky position container is the displayport of the containing scrollframe. If there's anything other clips that get introduced in between the containing scrollframe and the sticky item, then we want to use those clips. This would take care of the position-sticky-scrolled-clip-1.html testcase, because in that case there's a div in between with its own clip.

In terms of implementing this in the code, one approach might be to set a flag on the scrolledRectClipState here that basically says "we're clipping to the displayport". When we generate nsDisplayStickyContainer we'd walk up the stack of DisplayListClipState::AutoSaveRestore instances. If we find an instance that has a clip set on it (pretty much where mUsed gets set to true) before we find an instance that has the "clipping to displayport" flag set on it, then we know that there's a clip being applied to the sticky container that we need to keep. If we hit the "clipping to displayport" flag then we know we can use a different rect.

I'll try implementing this and see what breaks.

Comment 10

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

I have a WIP at https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=128dd5e5a1628073a6706be7b97e35b62bbf2b83 that seems to pass tests and also fixes the problem. But it's pretty ugly. Will try to figure out a slightly cleaner way of propagating the necessary information.

Comment 11

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

The WIP also seems to fix bug 1548397.

Comment 12

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

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

The WIP also seems to fix bug 1548397.

Sorry, it doesn't fix that bug. I was testing incorrectly.

Comment 13

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

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=e2d2d9a58e17a79862dafe9b4bd64316e4892aa9 has cleaned up patches and a testcase. The fix definitely feels like hackery but I expect that it will evolve as I look into other related bugs (bug 1548397 being the first one).

Comment 14

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

Sigh. Test is failing on Android, looks like it's not checkerboarding in the "test" case at all, which is kind of weird. Will need to investigate.

Comment 15

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

Apparently the async-displayport properties in reftests end up changing the critical displayport on android, while the non-critical displayport is still unaffected. That seems to be the cause of the test failure.

Comment 16

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

Attachment: Bug 1424714 - Prevent the displayport clip from clipping sticky items. r?mstange

The displayport clip that is applied to sticky items from the enclosing
scrollframe can cause sticky items to checkerboard even when they might
reasonably be left visible. This is because the displayport clip moves as
the enclosing scrollframe is scrolled, but sticky items may remain fixed
during such scrolling. The displayport clip can therefore clip out sticky
items even if they are "stuck" and should be user-visible.

This patch sets a flag to identify when a sticky item is being clipped by
the displayport clip, and ensures that it doesn't actually get clipped. In
the case where other clips are being applied to the sticky item, we leave the
clips unaffected. This allows for other enclosing elements to clip the sticky
item as before.

Comment 17

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

Attachment: Bug 1424714 - Add a test to ensure sticky position items don't checkerboard unnecessarily. r?mstange

Depends on D68582

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/840f340afe5a
Prevent the displayport clip from clipping sticky items. r=mstange
https://hg.mozilla.org/integration/autoland/rev/925ec3ed99a4
Add a test to ensure sticky position items don't checkerboard unnecessarily. r=mstange

Comment 19

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/840f340afe5a
https://hg.mozilla.org/mozilla-central/rev/925ec3ed99a4

Comment 20

[fabioturrin]

Thanks for working on it.
Unfortunately, with my nightly 77.0a1 (2020-05-04) (64 bit) On Windows 10 64bit, the bug is still there, although it seems slightly improved.

The demo is the same as the first post:
https://codepen.io/bludev/pen/BmpwPP

Comment 21

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

If you can make a standalone test case (i.e. not one on codepen, which tends to introduce all sorts of other elements and nested scrollframes that impact scrolling behaviour) and file a new bug with that I can take a look. Thanks!

Comment 22

[fabioturrin]

I've submitted a new bug with attached demo. Thanks!