Bug 82521 – [CSS Filters] Trigger a repaint on elements with changed filter

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

[Alexandru Chiculita]

Currently the RenderStyle returns StyleDifferenceLayout when the filter property is changed.

Filter is not affecting the layout so a simple repaint should be sufficient. However, filter is a context dependent property and needs to return either StyleDifferenceRecompositeLayer or StyleDifferenceRepaintLayer depending on whether the layer is composited or not.

[Radar WebKit Bug Importer]

<rdar://problem/11141841>

[Alexandru Chiculita]

Created attachment 134457 [details]
Patch V1

[Dean Jackson]

Created attachment 134482 [details]
Patch

[Dean Jackson]

Oops. I didn't notice Alex had sent a patch.

[Dean Jackson]

Comment on attachment 134457 [details]
Patch V1

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

This is almost the same as my patch (I think better because mine had an error). I have one question though. Since you're testing layer->paintsWithFilters() won't that mean that if a filter property is completely added or removed you won't get marked as having a difference? I managed to get it into this state by toggling between none and blur(2px) for example.

Actually, another question is whether there is a reason for moving the test in ::diff?

> Source/WebCore/rendering/RenderObject.cpp:1729
> +#if ENABLE(CSS_FILTERS)
> +    if ((contextSensitiveProperties & ContextSensitivePropertyFilter) && hasLayer()) {
> +        RenderLayer* layer = toRenderBoxModelObject(this)->layer();
> +        if (layer->paintsWithFilters())
> +            diff = StyleDifferenceRepaintLayer;
> +        else if ((diff < StyleDifferenceRecompositeLayer) && layer->isComposited())
> +            diff = StyleDifferenceRecompositeLayer;
> +    }
> +#endif

We don't need the #if guards here. ContextSensitivePropertyFilter won't be set if ENABLE_CSS_FILTERS is off.

[Dean Jackson]

Created attachment 134487 [details]
filter -> none testcase

Here's an example of the case I mentioned. If you swap the filter and none to be the other way around, it works.

[Alexandru Chiculita]

(In reply to comment #5)
> (From update of attachment 134457 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134457&action=review
> 
> This is almost the same as my patch (I think better because mine had an error). I have one question though. Since you're testing layer->paintsWithFilters() won't that mean that if a filter property is completely added or removed you won't get marked as having a difference? I managed to get it into this state by toggling between none and blur(2px) for example.

Right. Maybe the right way is to reverse the condition like in the following example. This way we always fallback to a repaint of the layer.

if ((contextSensitiveProperties & ContextSensitivePropertyFilter) && hasLayer()) {
    RenderLayer* layer = toRenderBoxModelObject(this)->layer();
    if (!layer->isComposited() || layer->paintsWithFilters())
        diff = StyleDifferenceRepaintLayer;
    else if (diff < StyleDifferenceRecompositeLayer)
        diff = StyleDifferenceRecompositeLayer;
}

so the cases can be:

none -> software = to StyleDifferenceRepaintLayer
none -> composited = might go to either StyleDifferenceRepaintLayer or StyleDifferenceRecompositeLayer depending on whether the layer was composited before

software -> software = StyleDifferenceRepaintLayer (both should be true)
software -> composited = StyleDifferenceRepaintLayer -> which is needed anyway to clean the texture of any filter

composited -> software = StyleDifferenceRecompositeLayer -> which may be wrong, but we cannot do anything in this state anyway. This may need some help from the RenderLayer::styleChange to toggle the right repaint of the RenderLayer backing texture.
composited -> composited = StyleDifferenceRecompositeLayer

software -> none -> StyleDifferenceRepaintLayer
composited -> none -> StyleDifferenceRecompositeLayer

I will try to fix this tomorrow.

> 
> Actually, another question is whether there is a reason for moving the test in ::diff?
> 
> > Source/WebCore/rendering/RenderObject.cpp:1729
> > +#if ENABLE(CSS_FILTERS)
> > +    if ((contextSensitiveProperties & ContextSensitivePropertyFilter) && hasLayer()) {
> > +        RenderLayer* layer = toRenderBoxModelObject(this)->layer();
> > +        if (layer->paintsWithFilters())
> > +            diff = StyleDifferenceRepaintLayer;
> > +        else if ((diff < StyleDifferenceRecompositeLayer) && layer->isComposited())
> > +            diff = StyleDifferenceRecompositeLayer;
> > +    }
> > +#endif
> 
> We don't need the #if guards here. ContextSensitivePropertyFilter won't be set if ENABLE_CSS_FILTERS is off.

We need it for the paintsWithFilters call. I'm not sure if ContextSensitivePropertyFilter should also be in #ifdefs. It shouldn't hurt to be outside.

[WebKit Review Bot]

Comment on attachment 134482 [details]
Patch

Attachment 134482 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12204100

New failing tests:
css3/filters/repaint-compositing.html

[WebKit Review Bot]

Created attachment 134496 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 134482 [details]
Patch

Attachment 134482 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12184173

New failing tests:
css3/filters/repaint-compositing.html

[WebKit Review Bot]

Created attachment 134503 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Alexandru Chiculita]

(In reply to comment #5)
> (From update of attachment 134457 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134457&action=review
> 
> Actually, another question is whether there is a reason for moving the test in ::diff?

Seems like I've skipped this question. I moved because all the "returns" are in order in the ::diff function. It is important to return a layout diff before a repaint diff. That's because the layout will trigger a repaint anyway, but a repaint will not force a layout. That means that if there are other properties that change and need a new layout, but they're checks are below the "filter" check in the "::diff" function, it will not trigger a relayout.

Actually, I think that when a layout is returned instead (because some other "layout" property changed at the same time), we need to do something in RenderBoxModelObject::styleWillChange, so that we actually repaint the whole layer and not just the RenderObject (that means including descendants, because filters should apply on every child). 

That function is actually the one calling layer()->repaintIncludingDescendants(); when diff == StyleDifferenceRepaintLayer. It also calls it when diff is not StyleDifferenceRepaintLayer, but there is some other property change that required a StyleDifferenceRepaintLayer.

So in total there are 4 things that need to be done for this patch and I will try to do this morning:
1. Update the "::adjust" function with a version that catches all the changes correctly (I proposed one something in the previous comment)
2. Add  "filter" checks in RenderBoxModelObject::styleWillChange in the if (diff == StyleDifferenceLayout || diff == StyleDifferenceSimplifiedLayout) branch.
3. Add something in RenderLayer to do a repaint of the composited "texture" when the filter cannot be rendered in "platform" code anymore and needs to be backed into the "texture"
4. Add tests for each case listed before

[Alexandru Chiculita]

(In reply to comment #6)
> Created an attachment (id=134487) [details]
> filter -> none testcase
> 
> Here's an example of the case I mentioned. If you swap the filter and none to be the other way around, it works.

Hehe. This doesn't work for other reasons :) In CSSStyleSelector::createFilterOperations we do not set the filter if the value is not a value list, meaning that "none" will not overwrite the previous value in the RenderStyle. I will fix that part of this bug.

[Alexandru Chiculita]

Created attachment 134652 [details]
Patch V2

I've tried to fix all the cases with this patch.

[Alexandru Chiculita]

Created attachment 134653 [details]
Patch V3

Last one was still the first patch, uploaded again. This time it is the right patch.

[WebKit Review Bot]

Comment on attachment 134653 [details]
Patch V3

Attachment 134653 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12209020

New failing tests:
css3/filters/filter-change-repaint-composited.html
css3/filters/filter-change-repaint.html

[WebKit Review Bot]

Created attachment 134686 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Alexandru Chiculita]

Created attachment 134691 [details]
Patch V3

Fixed comments in test files. Marked tests for rebaseline on Chromium.

[Dean Jackson]

(In reply to comment #7)

> > We don't need the #if guards here. ContextSensitivePropertyFilter won't be set if ENABLE_CSS_FILTERS is off.
> 
> We need it for the paintsWithFilters call. 

Ah, yes.

> I'm not sure if ContextSensitivePropertyFilter should also be in #ifdefs. It shouldn't hurt to be outside.

I think it's fine to be outside.

[Dean Jackson]

Comment on attachment 134691 [details]
Patch V3

Nice catch on the shadow CA properties.

[WebKit Review Bot]

Comment on attachment 134691 [details]
Patch V3

Clearing flags on attachment: 134691

Committed r112644: <http://trac.webkit.org/changeset/112644>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexandru Chiculita]

*** Bug 82531 has been marked as a duplicate of this bug. ***