Bug 111701 - Basic child obscuration test for backgrounds
Summary: Basic child obscuration test for backgrounds
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-07 02:10 PST by Antti Koivisto
Modified: 2013-05-08 18:28 PDT (History)
9 users (show)

See Also:


Attachments
patch (6.45 KB, patch)
2013-03-07 02:53 PST, Antti Koivisto
simon.fraser: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
patch2 (6.88 KB, patch)
2013-03-08 10:56 PST, Antti Koivisto
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
patch 3 (5.81 KB, patch)
2013-03-11 13:42 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch 4 (10.40 KB, patch)
2013-03-11 17:36 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch 5 (10.59 KB, patch)
2013-03-12 13:26 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch 6 (10.73 KB, patch)
2013-03-12 17:22 PDT, Antti Koivisto
simon.fraser: review+
Details | Formatted Diff | Diff
for bots (10.72 KB, patch)
2013-03-12 19:35 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
for bots 2 (10.63 KB, patch)
2013-03-12 19:43 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2013-03-07 02:10:27 PST
We can easily detect some simple cases where a background image is fully obscured by an opaque child.
Comment 1 Antti Koivisto 2013-03-07 02:53:50 PST
Created attachment 191961 [details]
patch
Comment 2 WebKit Review Bot 2013-03-07 05:46:31 PST
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 3 Simon Fraser (smfr) 2013-03-07 09:09:51 PST
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?
Comment 4 Antti Koivisto 2013-03-07 10:20:09 PST
(In reply to comment #3)
> What about transforms?

Don't transforms always get a layer? RenderLayer::contentsOpaqueInRect returns false if there is a transform.
Comment 5 Simon Fraser (smfr) 2013-03-07 10:28:15 PST
(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.
Comment 6 Antti Koivisto 2013-03-08 10:56:11 PST
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 7 Simon Fraser (smfr) 2013-03-08 11:01:02 PST
Comment on attachment 192250 [details]
patch2

I can haz repaint test?
Comment 8 Simon Fraser (smfr) 2013-03-08 11:19:35 PST
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?
Comment 9 Antti Koivisto 2013-03-08 11:28:14 PST
(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 10 WebKit Review Bot 2013-03-08 11:56:01 PST
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
Comment 11 Simon Fraser (smfr) 2013-03-08 12:05:32 PST
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 12 Build Bot 2013-03-08 20:08:19 PST
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 13 Build Bot 2013-03-09 00:29:10 PST
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
Comment 14 Antti Koivisto 2013-03-11 13:42:19 PDT
Created attachment 192545 [details]
patch 3
Comment 15 Simon Fraser (smfr) 2013-03-11 13:58:01 PDT
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.
Comment 16 Antti Koivisto 2013-03-11 14:48:09 PDT
(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.
Comment 17 Antti Koivisto 2013-03-11 17:36:51 PDT
Created attachment 192607 [details]
patch 4

now with test
Comment 18 Antti Koivisto 2013-03-12 13:26:47 PDT
Created attachment 192795 [details]
patch 5

The opaqueness test functions were rolled out, add one sufficient for this patch back.
Comment 19 Simon Fraser (smfr) 2013-03-12 15:51:41 PDT
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.
Comment 20 Antti Koivisto 2013-03-12 16:08:56 PDT
(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.
Comment 21 Antti Koivisto 2013-03-12 17:22:28 PDT
Created attachment 192839 [details]
patch 6
Comment 22 Antti Koivisto 2013-03-12 17:26:48 PDT
> 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 23 Simon Fraser (smfr) 2013-03-12 17:58:49 PDT
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()
Comment 24 Antti Koivisto 2013-03-12 19:35:37 PDT
Created attachment 192858 [details]
for bots
Comment 25 Antti Koivisto 2013-03-12 19:43:18 PDT
Created attachment 192860 [details]
for bots 2
Comment 26 WebKit Review Bot 2013-03-12 21:57:26 PDT
Comment on attachment 192860 [details]
for bots 2

Clearing flags on attachment: 192860

Committed r145680: <http://trac.webkit.org/changeset/145680>
Comment 27 WebKit Review Bot 2013-03-12 21:57:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Darin Adler 2013-03-13 09:34:28 PDT
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 29 Darin Adler 2013-03-13 09:35:33 PDT
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
Comment 30 Simon Fraser (smfr) 2013-03-13 09:36:12 PDT
knownToBe is what popped into my head.
Comment 31 Antti Koivisto 2013-03-13 13:51:40 PDT
Or perhaps do it other way round:

backgroundMaybeNotOpaque()
Comment 32 Simon Fraser (smfr) 2013-03-13 14:04:55 PDT
I find that one harder to think about. Using possiblyTransarent() may be more clear.
Comment 33 Simon Fraser (smfr) 2013-05-08 18:28:45 PDT
This caused bug 115840.