Bug 71930 - Filters need to affect visual overflow
Summary: Filters need to affect visual overflow
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: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks: 68469
  Show dependency treegraph
 
Reported: 2011-11-09 10:12 PST by Simon Fraser (smfr)
Modified: 2011-12-16 10:04 PST (History)
5 users (show)

See Also:


Attachments
Patch (550.99 KB, patch)
2011-12-15 11:30 PST, Dean Jackson
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2011-11-09 10:12:18 PST
Filters need to affect visual overflow, like shadows do.
Comment 1 Radar WebKit Bug Importer 2011-11-09 10:13:25 PST
<rdar://problem/10420021>
Comment 2 Dean Jackson 2011-12-15 11:30:00 PST
Created attachment 119471 [details]
Patch
Comment 3 Simon Fraser (smfr) 2011-12-15 12:52:09 PST
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.
Comment 4 Dean Jackson 2011-12-16 10:03:39 PST
(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
Comment 5 Dean Jackson 2011-12-16 10:04:09 PST
http://trac.webkit.org/changeset/103076