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

RESOLVED FIXED in Firefox 34

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

Trunk
mozilla35
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

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).
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.
Created attachment 8483285 [details] [diff] [review]
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
Attachment #8483285 - Flags: review?(matt.woodrow)
Created attachment 8483286 [details] [diff] [review]
Part 1: Remove ItemCoversScrollableArea
Attachment #8483285 - Attachment is obsolete: true
Attachment #8483285 - Flags: review?(matt.woodrow)
Attachment #8483286 - Flags: review?(matt.woodrow)
Created attachment 8483287 [details] [diff] [review]
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
Attachment #8483287 - Flags: review?(matt.woodrow)
Attachment #8483286 - Flags: review?(matt.woodrow) → review+
Attachment #8483287 - Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/mozilla-central/rev/46803bf61e5f
https://hg.mozilla.org/mozilla-central/rev/c43865439dd9
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1066591
Backout out second patch to fix regressions.
https://hg.mozilla.org/integration/mozilla-inbound/rev/861da4eaaa2c
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

4 years ago
Depends on: 1066962
Depends on: 1067561
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).
No longer depends on: 1067561
I think the solution here is pretty straightforward: when we're flattening layers, SetupScrollMetadata should not be doing anything.
(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.
Created attachment 8490577 [details] [diff] [review]
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.
Attachment #8483287 - Attachment is obsolete: true
Attachment #8490577 - Flags: review?(tnikkel)
Created 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
Attachment #8490578 - Flags: review?(tnikkel)
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
See Also: bug 1056423
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.
Attachment #8490577 - Flags: review?(tnikkel) → review?(matt.woodrow)
Attachment #8490578 - Flags: review?(tnikkel) → review?(matt.woodrow)
Attachment #8490577 - Flags: review?(matt.woodrow) → review+
Attachment #8490578 - Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/mozilla-central/rev/baa4a1887133
https://hg.mozilla.org/mozilla-central/rev/1bdf7f5d1a6d
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
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.
Attachment #8490578 - Flags: approval-mozilla-aurora?
status-firefox34: --- → affected
status-firefox35: --- → fixed
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?
Attachment #8490578 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(roc)
Attachment #8483286 - Flags: approval-mozilla-aurora+
Attachment #8490577 - Flags: approval-mozilla-aurora+
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
status-b2g-v2.1: --- → fixed
status-b2g-v2.2: --- → fixed
status-firefox33: --- → wontfix
status-firefox34: affected → fixed
Flags: needinfo?(roc)
Flags: qe-verify-
See Also: → bug 1089380
No longer blocks: 1089382
Depends on: 1089382

Updated

4 years ago
Depends on: 1086985
You need to log in before you can comment on or make changes to this bug.