Bug 111701 – Basic child obscuration test for backgrounds

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Antti Koivisto]

We can easily detect some simple cases where a background image is fully obscured by an opaque child.

[Antti Koivisto]

Created attachment 191961 [details]
patch

[WebKit Review Bot]

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

[Simon Fraser (smfr)]

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?

[Antti Koivisto]

(In reply to comment #3)
> What about transforms?

Don't transforms always get a layer? RenderLayer::contentsOpaqueInRect returns false if there is a transform.

[Simon Fraser (smfr)]

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

[Antti Koivisto]

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.

[Simon Fraser (smfr)]

Comment on attachment 192250 [details]
patch2

I can haz repaint test?

[Simon Fraser (smfr)]

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?

[Antti Koivisto]

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

[WebKit Review Bot]

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

[Simon Fraser (smfr)]

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.

[Build Bot]

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

[Build Bot]

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

[Antti Koivisto]

Created attachment 192545 [details]
patch 3

[Simon Fraser (smfr)]

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.

[Antti Koivisto]

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

[Antti Koivisto]

Created attachment 192607 [details]
patch 4

now with test

[Antti Koivisto]

Created attachment 192795 [details]
patch 5

The opaqueness test functions were rolled out, add one sufficient for this patch back.

[Simon Fraser (smfr)]

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.

[Antti Koivisto]

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

[Antti Koivisto]

Created attachment 192839 [details]
patch 6

[Antti Koivisto]

> 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.

[Simon Fraser (smfr)]

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

[Antti Koivisto]

Created attachment 192858 [details]
for bots

[Antti Koivisto]

Created attachment 192860 [details]
for bots 2

[WebKit Review Bot]

Comment on attachment 192860 [details]
for bots 2

Clearing flags on attachment: 192860

Committed r145680: <http://trac.webkit.org/changeset/145680>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[Darin Adler]

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

[Simon Fraser (smfr)]

knownToBe is what popped into my head.

[Antti Koivisto]

Or perhaps do it other way round:

backgroundMaybeNotOpaque()

[Simon Fraser (smfr)]

I find that one harder to think about. Using possiblyTransarent() may be more clear.

[Simon Fraser (smfr)]

This caused bug 115840.