blink: Layers that have a backdrop-filter paints contents

blink: Layers that have a backdrop-filter paints contents

Return true in CompositedDeprecatedPaintLayerMapping::containsPaintedContent
if it has a backdrop-filter.

BUG=527452

[hendrikw, 2015-09-03 21:27:37]

hendrikw@chromium.org changed reviewers:
+ pdr@chromium.org

[hendrikw, 2015-09-03 21:27:42]

PTAL, thanks!

[pdr., 2015-09-03 22:13:59]

On 2015/09/03 at 21:27:42, hendrikw wrote:
> PTAL, thanks!

Can you add a test for this?

[hendrikw, 2015-09-03 22:55:44]

Test added, PTAL thanks!

[pdr., 2015-09-03 23:21:07]

Test looks great.

The hasBackdropFilter check seems out of place though. Why don't we have
hasBackdropFilter in all the places we have hasFilter since they are roughly
equal (in terms of stacking contexts, etc)? The effect for this function should
be the same since you'd put the hasBackdropFilter check in
computedStyle::hasBoxDecorations which ends up getting called through
hasVisibleBoxDecorations().

[hendrikw, 2015-09-04 00:10:20]

On 2015/09/03 23:21:07, pdr wrote:
> Test looks great.
> 
> The hasBackdropFilter check seems out of place though. Why don't we have
> hasBackdropFilter in all the places we have hasFilter since they are roughly
> equal (in terms of stacking contexts, etc)? The effect for this function
should
> be the same since you'd put the hasBackdropFilter check in
> computedStyle::hasBoxDecorations which ends up getting called through
> hasVisibleBoxDecorations().

hm, interesting, well, they aren't exactly the same, backdrop filter will need
special cases where the layer is somehow transparent (e.g. no background color,
or fully transparent).  Also, backdrop filters are always handled by the
compositor, where filters may or may not be applied in the compositor.

But yeah, adding hasBackdropFilter to hasBoxDecorations would fix my problem,
but I don't think this is right.  Most of what's in hasBoxDecorations seems to
be operations that could change the size (outlines, shadows, etc), I'm not sure
backdrop filters fall into this?  But, it might make sense to add to
hasBackground()?  I'm looking through all the call sites, but not being too
familiar with blink, it's hard to understand the implications.

What's weird to here is that hasBoxDecorations with hasFilter() will cause
containsPaintedContent to return true when there's a filter, even when the div
is invisible.  Maybe this is rare enough that we don't care.

It does look like I completely missed the hasFilter cases when adding the
backdrop property.

I'll need to look at these case by case, for example, I probably wouldn't want
the one in inflatePaintInvalidationRectForReflectionAndFilter and
paintInvalidationReason.

Should I add to hasBackground?  Or hasBoxDecorations?  WDYT?

[pdr., 2015-09-04 17:10:12]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org

[pdr., 2015-09-04 17:12:50]

On 2015/09/04 at 00:10:20, hendrikw wrote:
> On 2015/09/03 23:21:07, pdr wrote:
> > Test looks great.
> > 
> > The hasBackdropFilter check seems out of place though. Why don't we have
> > hasBackdropFilter in all the places we have hasFilter since they are roughly
> > equal (in terms of stacking contexts, etc)? The effect for this function
should
> > be the same since you'd put the hasBackdropFilter check in
> > computedStyle::hasBoxDecorations which ends up getting called through
> > hasVisibleBoxDecorations().
> 
> hm, interesting, well, they aren't exactly the same, backdrop filter will need
special cases where the layer is somehow transparent (e.g. no background color,
or fully transparent).  Also, backdrop filters are always handled by the
compositor, where filters may or may not be applied in the compositor.
> 
> But yeah, adding hasBackdropFilter to hasBoxDecorations would fix my problem,
but I don't think this is right.  Most of what's in hasBoxDecorations seems to
be operations that could change the size (outlines, shadows, etc), I'm not sure
backdrop filters fall into this?  But, it might make sense to add to
hasBackground()?  I'm looking through all the call sites, but not being too
familiar with blink, it's hard to understand the implications.
> 
> What's weird to here is that hasBoxDecorations with hasFilter() will cause
containsPaintedContent to return true when there's a filter, even when the div
is invisible.  Maybe this is rare enough that we don't care.
> 
> It does look like I completely missed the hasFilter cases when adding the
backdrop property.
> 
> I'll need to look at these case by case, for example, I probably wouldn't want
the one in inflatePaintInvalidationRectForReflectionAndFilter and
paintInvalidationReason.
> 
> Should I add to hasBackground?  Or hasBoxDecorations?  WDYT?

I see, your point is that hasBoxDecorations is about stuff that can spill out of
the box, whereas hasBackground is purely paint? If so, that sounds reasonable to
me.

[hendrikw, 2015-09-04 22:26:20]

PTAL, thanks!

[pdr., 2015-09-04 22:42:14]

https://codereview.chromium.org/1327973002/diff/60001/Source/core/style/Compu...
File Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1327973002/diff/60001/Source/core/style/Compu...
Source/core/style/ComputedStyle.h:425: return hasBackgroundImage() ||
hasBackdropFilter();
I think we need to return hasBackdropFilter from
hasBoxDecorationsOrBackgroundImage and maybe rename it?

BoxDecorationData also looks like it needs a hasBackdropFilter().

Basically, just search cs.chromium.org for hasBackgroundImage().

It would be great if we could test some of these places just to make sure we're
doing the right thing.

[chrishtr, 2015-09-24 22:27:42]

FYI I took advantage of Hendrik being in SFO today to go over this patch and the
previous one,
and the complexity is justified. So looks good to me modulo pdr's latest
comment.