Bug 71930 – Filters need to affect visual overflow

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

Filters need to affect visual overflow, like shadows do.

[Radar WebKit Bug Importer]

<rdar://problem/10420021>

[Dean Jackson]

Created attachment 119471 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 119471 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119471&action=review

> Source/WebCore/rendering/FilterEffectRenderer.h:74
> +    IntRect outputRect() { return lastEffect()->hasResult() ? lastEffect()->requestedRegionOfInputImageData(IntRect(m_filterRegion)) : IntRect(); }

Should be const.

> Source/WebCore/rendering/RenderBox.cpp:3516
>  void RenderBox::addBoxShadowAndBorderOverflow()

The method name is wrong after this change. Maybe it should be renamed to addBoxDecorationOverflow, addVisualEffectOverflow or something?

> Source/WebCore/rendering/RenderLayerBacking.cpp:741
>  static bool hasBorderOutlineOrShadow(const RenderStyle* style)

Method name is wrong now.

> Source/WebCore/rendering/style/FilterOperations.cpp:35
> +static inline void outsetSizeForBlur(unsigned& outsetX, unsigned& outsetY, float stdX, float stdY)

Can't this return an IntSize?

> Source/WebCore/rendering/style/FilterOperations.cpp:68
> +        if (operationType == FilterOperation::BLUR || operationType == FilterOperation::DROP_SHADOW)

Is it worth checking for non-zero blur radius/shadow offset?

> Source/WebCore/rendering/style/RenderStyle.h:406
> +        if (hasFilter())
> +            filter().getOutsets(top, right, bottom, left, borderBoxSize);

I think you should set the parameters to zero if there is no filter.

> LayoutTests/ChangeLog:4
> +        Filters need to affect visual overflow
> +        https://bugs.webkit.org/show_bug.cgi?id=71930

You should probably add some margins between the elements in these filter tests to avoid adjacent blurs overlapping.

[Dean Jackson]

(In reply to comment #3)
> (From update of attachment 119471 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119471&action=review
> 
> > Source/WebCore/rendering/FilterEffectRenderer.h:74
> > +    IntRect outputRect() { return lastEffect()->hasResult() ? lastEffect()->requestedRegionOfInputImageData(IntRect(m_filterRegion)) : IntRect(); }
> 
> Should be const.

fixed

> 
> > Source/WebCore/rendering/RenderBox.cpp:3516
> >  void RenderBox::addBoxShadowAndBorderOverflow()
> 
> The method name is wrong after this change. Maybe it should be renamed to addBoxDecorationOverflow, addVisualEffectOverflow or something?

addVisualEffectOverflow it is

> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:741
> >  static bool hasBorderOutlineOrShadow(const RenderStyle* style)
> 
> Method name is wrong now.

hasBoxDecorations (matches existing functions nicely)

> 
> > Source/WebCore/rendering/style/FilterOperations.cpp:35
> > +static inline void outsetSizeForBlur(unsigned& outsetX, unsigned& outsetY, float stdX, float stdY)
> 
> Can't this return an IntSize?

fixed

> 
> > Source/WebCore/rendering/style/FilterOperations.cpp:68
> > +        if (operationType == FilterOperation::BLUR || operationType == FilterOperation::DROP_SHADOW)
> 
> Is it worth checking for non-zero blur radius/shadow offset?

I don't think so. That would require casting to a real operation, etc.

Although now that I've committed, I do see that people will use a blur(0) as a transition starting point to be a common case. I'll file a followup.

> 
> > Source/WebCore/rendering/style/RenderStyle.h:406
> > +        if (hasFilter())
> > +            filter().getOutsets(top, right, bottom, left, borderBoxSize);
> 
> I think you should set the parameters to zero if there is no filter.

fixed

> 
> > LayoutTests/ChangeLog:4
> > +        Filters need to affect visual overflow
> > +        https://bugs.webkit.org/show_bug.cgi?id=71930
> 
> You should probably add some margins between the elements in these filter tests to avoid adjacent blurs overlapping.

fixed

[Dean Jackson]

http://trac.webkit.org/changeset/103076