Remove checkerboarding for sub-scroll frames

RESOLVED FIXED in Firefox 55

Status

()

Core
Graphics: Layers
RESOLVED FIXED
2 months ago
25 days ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

(Depends on: 2 bugs)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

2 months ago
The checkerboarding code is complex, and a pain to support with WR.

If we just allow sub-frames to overscroll and show the contents underneath, then we can make this much simpler by including a ColorLayer at the back of the root scroll frame.

This was Markus' idea, I just rebased the patches and made sure they pass Try.
(Assignee)

Comment 1

2 months ago
Created attachment 8849804 [details] [diff] [review]
Remove checkboarding code and just use a solid color for the root scroll frame instead
Assignee: nobody → matt.woodrow
Attachment #8849804 - Flags: review?(bugmail)
Comment on attachment 8849804 [details] [diff] [review]
Remove checkboarding code and just use a solid color for the root scroll frame instead

Review of attachment 8849804 [details] [diff] [review]:
-----------------------------------------------------------------

Not sure this will produce the correct results, see comment below.

::: layout/reftests/async-scrolling/checkerboard-2-ref.html
@@ +1,4 @@
>  <!DOCTYPE HTML>
>  <html>
>  <body style="background-color: green; overflow:hidden">
> +  <div style="position:fixed; left: 0px; top: 0px; width: 100px; height: 500px; background-color: purple; z-index: -1"></div>

Don't these reftest changes mean that the new code is not doing the same thing as the old code? In particular these changes seem to violate the thing the test is testing for - that no purple is visible. I strongly suspect this will break proper checkerboarding on "hero image" pages such as blog.wanderview.com. Related: bug 1013385 comment 6 (and the rest of that bug).
Attachment #8849804 - Flags: review?(bugmail)
Also for the record comment 0 talks about subframes specifically, but it's not clear to me why subframes are different from the root scrollable frame in this context. I thought the checkerboarding code applied to all scrolling frames, and your patch also seems to apply to all scrolling frames.
It does, but it also makes sure that for the root scroll frame there's a non-scrolling nsDisplaySolidColor underneath everything.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> In particular these changes seem to violate the thing
> the test is testing for - that no purple is visible. I strongly suspect this
> will break proper checkerboarding on "hero image" pages such as
> blog.wanderview.com. Related: bug 1013385 comment 6 (and the rest of that
> bug).

Uh oh. This is a case that I never actually thought about before.

It seems to be quite tricky to support reliably, even with support from the compositor. The current code works on blog.wanderview.com because the background color of the foreground layer happens to match the root scroll frame's background color, and because the foreground layer happens to be opaque. It will break down if either of these things aren't true.

Kats, how bad would it be to remove this behavior?

Now that we properly support scrolling clips on fixed layers, web sites can be written in such a way that this problem doesn't happen. It might be worth finding out whether we can contribute a change to this blog theme.
(In reply to Markus Stange [:mstange] from comment #5)
> It seems to be quite tricky to support reliably, even with support from the
> compositor. The current code works on blog.wanderview.com because the
> background color of the foreground layer happens to match the root scroll
> frame's background color, and because the foreground layer happens to be
> opaque. It will break down if either of these things aren't true.

Even if the colors didn't match it would at least show something solid instead of the content underneath.

> Kats, how bad would it be to remove this behavior?

To be honest, I'm not sure. I seem to recall there were a number of similar issues that were fixed by the original code, but I think most or all of them were in Gaia, which did all sorts of crazy things. I don't know how widespread this issue would be in the real world.

If my mental model is correct, webrender won't have any checkerboarding at all, because it will have the whole display list, correct? Maybe then we can use this solid background color display item in the webrender codepath but keep the existing layer bounds stuff as-is, to be eventually phased out when we drop all that code?
Does that mean we will generate display items for stuff outside the displayport?
I don't think webrender will change anything about display ports or checkerboarding in the short term. Once we've improved display list building performance enough that it becomes possible to increase the display port size without a huge performance hit, we may want to do so. Retaining a display list for the whole page is not part of the webrender plans, as far as I know.

With the BlobImage stuff, we might even see a new type of checkerboarding for layers whose BlobImage rasterization hasn't completed yet, but I think I managed to convince Jeff that that would be madness.
Ah, ok. And I guess the problem is that porting all this complex checkerboarding code into webrender doesn't seem so great. I agree with that. So how about we land this patch and see if people start complaining about glitchy checkerboarding. If it turns up a lot, we can back out the patch and consider alternatives. If it's low frequency, we can leave it in and try to figure out a better way to fix in the webrender world.

If we do land this, I'd like to make sure we don't land additional stuff on top of this just yet, in case we need to back it out. Alternatively we disable the existing checkerboard code without removing it, so that if we need to do a backout it's less risky. I'm just worried that given the size of the patch it will result in messy conflicts if we try to back it out even a week or two from now.
Attachment #8849804 - Flags: review+

Comment 10

2 months ago
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3910de7acce3
Remove checkerboarding code and just use an opaque background color behind root scroll frames. r=kats

Comment 11

2 months ago
sorry had to back this out for reftest failing in async-scrolling like https://treeherder.mozilla.org/logviewer.html#?job_id=90012846&repo=mozilla-inbound&lineNumber=45944

https://hg.mozilla.org/integration/mozilla-inbound/rev/a31c50d1afec
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 12

a month ago
For some reason we're getting scrollbars drawn in the failing tests, though I'm not sure why checkerboarding changed that. Were we drawing over them previously?

We probably don't want the extra scrollbars though, does anyone remember how we've worked around this in the past?
Flags: needinfo?(matt.woodrow)
We have lots of async-scrolling tests that set overflow:hidden on the html element, but we still give them a layer + AGR + displayport because of the displayport reftest attributes. We can probably do the same here. But it would be nice to know why the scrollbar only shows in the bottom part and not in the rest of the page.
(Assignee)

Comment 14

a month ago
Created attachment 8859427 [details] [diff] [review]
Require ASRs to match before marking things as opaque

The nsDisplayBlendContainer created for the failing test has the same AGR as its contents, but a different ASR (it's fixed, since it contains the fixed background item).

Since FLB was only using the AGR, it marked the container as opaque and then when we async scrolled within it, we had a transparent area, but the compositor left it blank since it though the entire container was drawing opaque content.

Merging AGRs and ASRs will stop this sort of bug from being possible, but for now lets just check both.
Attachment #8859427 - Flags: review?(mstange)
Comment on attachment 8859427 [details] [diff] [review]
Require ASRs to match before marking things as opaque

Review of attachment 8859427 [details] [diff] [review]:
-----------------------------------------------------------------

This isn't very pretty, but hopefully only a transitory state.
Attachment #8859427 - Flags: review?(mstange) → review+

Comment 16

a month ago
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/435e638babd1
Remove checkerboarding code and just use an opaque background color behind root scroll frames. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ccf396a61fe
Require ASRs to match before allow occlusions between layers. r=mstange

Comment 17

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/435e638babd1
https://hg.mozilla.org/mozilla-central/rev/3ccf396a61fe
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 18

a month ago
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/mozilla-central/rev/7b7aa29bb6ed
Backed out changeset 3ccf396a61fe 
https://hg.mozilla.org/mozilla-central/rev/0b1d1dfe7055
Backed out changeset 435e638babd1 for failing checkerboard-{1,2,3}.html on Android 4.3. r=backout a=backout
Backed out for failing checkerboard-{1,2,3}.html on Android 4.3:

https://hg.mozilla.org/mozilla-central/rev/0b1d1dfe7055a8568e1cc7ea5f74c6fd63ada14c
https://hg.mozilla.org/mozilla-central/rev/7b7aa29bb6ed8a6fcfcd6391eb778619625c05c1

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=3ccf396a61fe9d2c4170d52051a58472486dda6d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/async-scrolling/checkerboard-1.html == http://10.0.2.2:8854/tests/layout/reftests/async-scrolling/checkerboard-1-ref.html | image comparison, max difference: 255, number of differing pixels: 640000
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/async-scrolling/checkerboard-2.html == http://10.0.2.2:8854/tests/layout/reftests/async-scrolling/checkerboard-2-ref.html | image comparison, max difference: 255, number of differing pixels: 610000
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/async-scrolling/checkerboard-3.html == http://10.0.2.2:8854/tests/layout/reftests/async-scrolling/checkerboard-3-ref.html | image comparison, max difference: 255, number of differing pixels: 750000

Please give me a few minutes to back this out from autoland and inbound.
Status: RESOLVED → REOPENED
Flags: needinfo?(matt.woodrow)
Resolution: FIXED → ---

Updated

a month ago
Depends on: 1265561
(Assignee)

Comment 20

a month ago
Looks like the background color didn't get added, or maybe it got scrolled?

Kats, do we still use Containerful scrolling on android?
Flags: needinfo?(matt.woodrow) → needinfo?(bugmail)
I believe we do, yeah.

http://searchfox.org/mozilla-central/rev/baf47b352e873d4516d7656186d6d7c7447d3873/modules/libpref/init/all.js#727
Flags: needinfo?(bugmail)
(Assignee)

Comment 22

a month ago
Hmm, do we have plans to not do that?

The other solution is to try put the ColorLayer outside of the nsDisplaySubDocument (which has the ScrollMetadata attached) so that it doesn't scroll.

That's proving to be a pain, because of resolution/zoom items, and because reftest invalidation events are sent from the nsDisplaySubDocument ContainerLayer, and thus don't register changes to the color outside of it.
No immediate plans, I think. IIRC it made it hard to do zooming on Android. Timothy might know remember more.
(Assignee)

Comment 24

a month ago
Created attachment 8862681 [details] [diff] [review]
Put the unscrolled item in the right place when we're doing container scrolling
Attachment #8862681 - Flags: review?(mstange)
(In reply to Matt Woodrow (:mattwoodrow) from comment #22)
> Hmm, do we have plans to not do that?

I tried. I don't remember the details. A lot of issues came up where it was like "do we even want to do this? it seems to make more sense to do this containerful (in the android situation)". I think I stopped working on it because there wasn't anything specific blocked on it and there was more important things to do.
Comment on attachment 8862681 [details] [diff] [review]
Put the unscrolled item in the right place when we're doing container scrolling

Review of attachment 8862681 [details] [diff] [review]:
-----------------------------------------------------------------

Ugghhhh so much code

I hope once we switch to the visual viewport model, some of this can go away.
Attachment #8862681 - Flags: review?(mstange) → review+

Comment 27

27 days ago
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9316ff651216
Remove checkerboarding code and just use an opaque background color behind root scroll frames. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/d08357559284
Require ASRs to match before allow occlusions between layers. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fa3d6e54b5b
Put the unscrolled item in the right place when we're doing container scrolling. r=mstange

Comment 28

27 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9316ff651216
https://hg.mozilla.org/mozilla-central/rev/d08357559284
https://hg.mozilla.org/mozilla-central/rev/9fa3d6e54b5b
Status: REOPENED → RESOLVED
Last Resolved: a month ago27 days ago
Resolution: --- → FIXED

Updated

25 days ago
Depends on: 1361970
You need to log in before you can comment on or make changes to this bug.