We can easily detect some simple cases where a background image is fully obscured by an opaque child.
Created attachment 191961 [details] patch
Comment on attachment 191961 [details] patch Attachment 191961 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17050452 New failing tests: fast/table/invisible-cell-background.html compositing/reflections/backface-hidden-reflection.html platform/chromium/virtual/softwarecompositing/backface-visibility/backface-visibility-non3d.html platform/chromium/virtual/softwarecompositing/reflections/backface-hidden-reflection.html fast/clip/015.html fast/repaint/overflow-hide.html compositing/backface-visibility/backface-visibility-non3d.html fast/block/positioning/022.html ietestcenter/css3/bordersbackgrounds/background_color_padding_box.htm fast/block/positioning/019.html compositing/backface-visibility/backface-visibility-simple.html fast/repaint/overflow-show.html transforms/3d/general/background-visibility-layers.html platform/chromium/virtual/softwarecompositing/backface-visibility/backface-visibility-simple.html
Comment on attachment 191961 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=191961&action=review You should add a repaint test for this. > Source/WebCore/rendering/RenderBox.cpp:1145 > + if (childBox->style()->visibility() != VISIBLE) > + continue; > + if (childBox->style()->position() != StaticPosition && childBox->containingBlock() != this) > + continue; What about transforms?
(In reply to comment #3) > What about transforms? Don't transforms always get a layer? RenderLayer::contentsOpaqueInRect returns false if there is a transform.
(In reply to comment #4) > (In reply to comment #3) > > What about transforms? > > Don't transforms always get a layer? RenderLayer::contentsOpaqueInRect returns false if there is a transform. OK, but childBox->style()->position() != StaticPosition also means that the child will have a layer. Also, bailing on childBox->layer()->contentsOpaqueInRect() is wrong if the child's layer is composited.
Created attachment 192250 [details] patch2 No repaint test yet, I couldn't figure out how to make one. Usually this doesn't really avoid repaints just reduces overpaint. In gif case it kills repaints too but the case is asynchronous.
Comment on attachment 192250 [details] patch2 I can haz repaint test?
You should be able to change the background color of the obscured element and see that it doesn't cause a repaint. Some variations can test your new logic. Did we forget about opacity on the children?
(In reply to comment #8) > You should be able to change the background color of the obscured element and see that it doesn't cause a repaint. Some variations can test your new logic. That will trigger a repaint anyway (this patch only affects paint time). Is there some way to test overpaint? > Did we forget about opacity on the children? Do those always get a layer? If they do then no. If they don't then yes.
Comment on attachment 192250 [details] patch2 Attachment 192250 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17047350 New failing tests: fast/block/positioning/022.html ietestcenter/css3/bordersbackgrounds/background_color_padding_box.htm fast/exclusions/shape-outside-floats/shape-outside-floats-positioning.html fast/block/positioning/019.html
Yes, opacity creates layers, so should bail from the childBox->layer()->contentsOpaqueInRect() test, as should transforms, so you can remove the transform check (sorry). Note that RenderLayer::contentsOpaqueInRect() returns false if the z-order lists are dirty, which we would have to be careful about if this were run at layout time.
Comment on attachment 192250 [details] patch2 Attachment 192250 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17036462 New failing tests: fast/exclusions/shape-outside-floats/shape-outside-floats-positioning.html
Comment on attachment 192250 [details] patch2 Attachment 192250 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17006093 New failing tests: fast/exclusions/shape-outside-floats/shape-outside-floats-positioning.html
Created attachment 192545 [details] patch 3
Comment on attachment 192545 [details] patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=192545&action=review > Source/WebCore/ChangeLog:26 > + In case of multiple background layers the last one should determine background color clip. > + Tested by ietestcenter/css3/bordersbackgrounds/background_color_padding_box.htm So why not testcase? > Source/WebCore/rendering/RenderBox.cpp:1161 > + if (!childBox->layer()->contentsOpaqueInRect(childLocalBackgroundRect)) You should sync up with Alok, who is working on reverting the change that added contentsOpaqueInRect() in bug 70634. It had issues with paint phases in its original use.
(In reply to comment #15) > (From update of attachment 192545 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=192545&action=review > > > Source/WebCore/ChangeLog:26 > > + In case of multiple background layers the last one should determine background color clip. > > + Tested by ietestcenter/css3/bordersbackgrounds/background_color_padding_box.htm > > So why not testcase? This is a workaround for an existing bug with background-clip property when there are multiple background layers that specify different clips. The rest of the patch makes the bug observable. I didn't add a new test since it is already covered. As far as I see all places that use RenderStyle::backgroundClip() are wrong though it doesn't really matter in practice as the case is extremely obscure.
Created attachment 192607 [details] patch 4 now with test
Created attachment 192795 [details] patch 5 The opaqueness test functions were rolled out, add one sufficient for this patch back.
Comment on attachment 192795 [details] patch 5 View in context: https://bugs.webkit.org/attachment.cgi?id=192795&action=review > Source/WebCore/rendering/RenderBox.cpp:1135 > +bool RenderBox::isOpaqueInRect(const LayoutRect& localRect) const I think this should be backgroundIsOpaqueInRect() since it only checks the background. > Source/WebCore/rendering/RenderBox.cpp:1174 > + // Table background painting is special. > + if (isTable()) > + return false; The body/document elements are also special, because of root background propagation. > Source/WebCore/rendering/RenderBox.cpp:1176 > + LayoutRect backgroundRect = borderBoxRect(); Why doesn't this needs to get the right background rect by checking style()->backgroundClip() like the function above? > Source/WebCore/rendering/RenderBox.cpp:1189 > + if (childStyle->visibility() != VISIBLE || childStyle->zIndex() < zIndex || childStyle->shapeOutside()) > + continue; I don't think the z-index check makes sense if we're not a stacking context, or we're not positioned. It would be better to deal with this if the child is a layer, and it should be explicit about us being a stacking context. It's very hard for me to think about whether this check is correct. > LayoutTests/fast/repaint/obscured-background-no-repaint.html:26 > + setTimeout(logRepaints, 200); Do we really have to wait 200ms? This will be a very slow test.
(In reply to comment #19) > I think this should be backgroundIsOpaqueInRect() since it only checks the background. Ok, though caller doesn't really care and jumping through isOpaqueInRect() that just calls backgroundIsOpaqueInRect() seemed bit pointless. > > Source/WebCore/rendering/RenderBox.cpp:1176 > > + LayoutRect backgroundRect = borderBoxRect(); > > Why doesn't this needs to get the right background rect by checking style()->backgroundClip() like the function above? The cases are not equivalent, opaqueInRect() needs smallest fully opaque rect for correctness while here we need maximum rect with any background pixels. borderBoxRect() is the maximum and the common case. No need to over-optimize this. > I don't think the z-index check makes sense if we're not a stacking context, or we're not positioned. It would be better to deal with this if the child is a layer, and it should be explicit about us being a stacking context. It's very hard for me to think about whether this check is correct. I'll move the test to the layer branch. > Do we really have to wait 200ms? This will be a very slow test. Probably not. 0 should work.
Created attachment 192839 [details] patch 6
> The body/document elements are also special, because of root background propagation. This is not invoked for root and the logic should be fine for body. > Do we really have to wait 200ms? This will be a very slow test. Actually it needs to run an animation frame which is somewhere between 50-100ms. 200ms seems like a safe non-flaky number.
Comment on attachment 192839 [details] patch 6 View in context: https://bugs.webkit.org/attachment.cgi?id=192839&action=review > Source/WebCore/rendering/RenderBox.cpp:1202 > + if (layer() && layer()->isStackingContext() && childLayer->zIndex() < layer()->zIndex()) This should be childLayer->zIndex() < 0. > Source/WebCore/rendering/RenderBox.cpp:1204 > + if (childLayer->hasTransform() || childLayer->isTransparent() || childLayer->isSelfPaintingLayer()) I don't think you need to bail on childLayer->isSelfPaintingLayer()
Created attachment 192858 [details] for bots
Created attachment 192860 [details] for bots 2
Comment on attachment 192860 [details] for bots 2 Clearing flags on attachment: 192860 Committed r145680: <http://trac.webkit.org/changeset/145680>
All reviewed patches have been landed. Closing bug.
Comment on attachment 192860 [details] for bots 2 View in context: https://bugs.webkit.org/attachment.cgi?id=192860&action=review > Source/WebCore/rendering/RenderBox.h:595 > + bool backgroundIsOpaqueInRect(const LayoutRect& localRect) const; > + virtual bool backgroundIsObscured() const; For functions like these, the question I always ask is what they return when the answer is “I don’t know” or “it’s too complicated to compute”. I think in both cases, the function returns true when it’s sure the answer is true, and false when it’s either false or unsure. I wish we could come up with names are still short but that make that more explicit and clear.
Comment on attachment 192860 [details] for bots 2 View in context: https://bugs.webkit.org/attachment.cgi?id=192860&action=review >> Source/WebCore/rendering/RenderBox.h:595 >> + virtual bool backgroundIsObscured() const; > > For functions like these, the question I always ask is what they return when the answer is “I don’t know” or “it’s too complicated to compute”. I think in both cases, the function returns true when it’s sure the answer is true, and false when it’s either false or unsure. > > I wish we could come up with names are still short but that make that more explicit and clear. I can’t think of good ones, but here are some bad ones: backgroundKnownToBeObscured backgroundDefinitelyObscured
knownToBe is what popped into my head.
Or perhaps do it other way round: backgroundMaybeNotOpaque()
I find that one harder to think about. Using possiblyTransarent() may be more clear.
This caused bug 115840.