1062100 - Background color not hidden in talos/page_load_test/scroll/tiled-fixed.html

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This probably contributes to bug 1042186.

There is an nsDisplayCanvasBackgroundColor with an nsDisplayCanvasBackgroundImage on top. The former's animated geometry root is the nsCanvasFrame since it scrolls (at least in principle). The latter is fixed so its animated geometry root is the viewport of the chrome document.

Because they have different animated geometry roots we don't let the nsDisplayCanvasBackgroundImage subtract from the nsDisplayCanvasBackgroundColor's layer's visible region, so we paint the ColorLayer when we don't need to (and wouldn't have, previously).

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is a bit tricky to fix.

In FrameLayerBuilder::PostprocessRetainedLayers we know that the image's layer has the same animated geometry root and fixed-pos frame as the container frame we're building a ContainerLayer for (that's the viewport frame of the HTML document). So we know it doesn't move. However, it doesn't cover the whole container frame bounds; the vertical scrollbar is visible, and the image's layer only covers the scrollport. It does cover the bounds of the ColorLayer, however.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Assign clip rects to non-async-scrollable scrolled layers, and hide clipped layers if some non-moving layer covers their entire clip rect

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: Remove ItemCoversScrollableArea

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Assign clip rects to non-async-scrollable scrolled layers, and hide clipped layers if some non-moving layer covers their entire clip rect

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/46803bf61e5f
https://hg.mozilla.org/integration/mozilla-inbound/rev/c43865439dd9

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/46803bf61e5f
https://hg.mozilla.org/mozilla-central/rev/c43865439dd9

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Backout out second patch to fix regressions.
https://hg.mozilla.org/integration/mozilla-inbound/rev/861da4eaaa2c

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This caused regressions where the ContainerLayer we create for a scrollbar had the scroll-port clip rect applied to it, making the scrollbar effectively disappear.

That happened because the nsDisplayOwnLayer that creates the ContainerLayer had its animated geometry root set to the scrolled nsCanvasFrame, and *that* happened because we set NS_FRAME_NO_COMPONENT_ALPHA on the content document ViewportFrame, which means every display item for the content document --- including its scrollbars --- is forced to have the same animated geometry root (which is somewhat arbitrarily chosen).

I am not pleased that when we disable component alpha for the document, scrollbars are forced to have the wrong animated geometry root, but perhaps that's OK. They will generally be invalidated while we scroll anyway (though we'll be needlessly invalidating the scrollbar on the other axis, when both scrollbars are showing).

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think the solution here is pretty straightforward: when we're flattening layers, SetupScrollMetadata should not be doing anything.

Comment 10

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> This caused regressions where the ContainerLayer we create for a scrollbar
> had the scroll-port clip rect applied to it, making the scrollbar
> effectively disappear.

This sounds exactly like bug 1056423.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2 v2

This seems to fix the regressions. When we're flattening layers, just skip the SetupScrollingMetadata step completely, since it's pointless/harmful.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3: When layers are flattened, let all the layers contribute opaqueness to the container layer, whatever their animated geometry roots are

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Turns out that we need part 3 to unregress the layer tree properly. Without it, we don't mark the content document's ContainerLayer (the one that flattens its descendant layers) as opaque, so the chrome ThebesLayer covers the entire window instead of just the top strip.

Comment 14

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> > This caused regressions where the ContainerLayer we create for a scrollbar
> > had the scroll-port clip rect applied to it, making the scrollbar
> > effectively disappear.
> 
> This sounds exactly like bug 1056423.

Never mind, that bug was entirely on the compositor side.

Comment 15

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8490577 [details] [diff] [review]
Part 2 v2

Didn't get to this while Matt was on PTO, I'll give it back to him.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/baa4a1887133
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bdf7f5d1a6d

Comment 17

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/baa4a1887133
https://hg.mozilla.org/mozilla-central/rev/1bdf7f5d1a6d

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8490578 [details] [diff] [review]
Part 3: When layers are flattened, let all the layers contribute opaqueness to the container layer, whatever their animated geometry roots are

Approval Request Comment
[Feature/regressing bug #]: 1022612
[User impact if declined]: Slower scrolling on certain pages
[Describe test coverage new/current, TBPL]: Lots of tests cover affected code
[Risks and why]: Low risk. Code has been on central for a while and no regressions have been reported.
[String/UUID change made/needed]: none

Need approval for all 3 patches in this bug.

Comment 19

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8490578 [details] [diff] [review]
Part 3: When layers are flattened, let all the layers contribute opaqueness to the container layer, whatever their animated geometry roots are

This fix should address the perf regression in bug 1042186. Aurora+

roc - We have no sheriff coverage this weekend. Can you land on Aurora yourself?

Comment 20

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Surprise!

https://hg.mozilla.org/releases/mozilla-aurora/rev/422d79a278be
https://hg.mozilla.org/releases/mozilla-aurora/rev/231126bd4179
https://hg.mozilla.org/releases/mozilla-aurora/rev/d261f0165d94

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Thanks!!!