Closed Bug 1349418 Opened 7 years ago Closed 7 years ago

Remove checkerboarding for sub-scroll frames

Categories

(Core :: Graphics: Layers, enhancement)

Product:
Core
Component:
Graphics: Layers
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

(Depends on 2 open bugs)

Details

Attachments

(3 files)

Remove checkboarding code and just use a solid color for the root scroll frame instead
22.25 KB, patch
kats
: review+
Details | Diff | Splinter Review
Require ASRs to match before marking things as opaque
8.30 KB, patch
mstange
: review+
Details | Diff | Splinter Review
Put the unscrolled item in the right place when we're doing container scrolling
7.94 KB, patch
mstange
: review+
Details | Diff | Splinter Review 
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: 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.

    
    

    
  
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

    
    

    
  
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.

    
    

    
  


  
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+

    
    

    
  
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

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/435e638babd1
https://hg.mozilla.org/mozilla-central/rev/3ccf396a61fe
Status: NEW → RESOLVED
Closed: 7 years ago

          status-firefox55:
          affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

    
    

    
  
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 → ---
Depends on: 1265561

    
    

    
  
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)

    
    

    
  
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.

    
    

    
  
(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+

    
    

    
  
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
No longer depends on: 1361970
Regressions: 1361970

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: