Bug 82521 - [CSS Filters] Trigger a repaint on elements with changed filter
Summary: [CSS Filters] Trigger a repaint on elements with changed filter
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: Alexandru Chiculita
URL:
Keywords: InRadar
: 82531 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-03-28 13:52 PDT by Alexandru Chiculita
Modified: 2012-03-30 13:01 PDT (History)
6 users (show)

See Also:


Attachments
Patch V1 (21.55 KB, patch)
2012-03-28 18:05 PDT, Alexandru Chiculita
dino: review-
Details | Formatted Diff | Diff
Patch (12.16 KB, patch)
2012-03-28 19:58 PDT, Dean Jackson
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
filter -> none testcase (337 bytes, text/html)
2012-03-28 20:28 PDT, Dean Jackson
no flags Details
Archive of layout-test-results from ec2-cr-linux-03 (9.96 MB, application/zip)
2012-03-28 22:02 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from ec2-cr-linux-04 (10.12 MB, application/zip)
2012-03-28 22:59 PDT, WebKit Review Bot
no flags Details
Patch V2 (21.55 KB, patch)
2012-03-29 13:14 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff
Patch V3 (40.79 KB, patch)
2012-03-29 13:16 PDT, Alexandru Chiculita
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (10.34 MB, application/zip)
2012-03-29 16:07 PDT, WebKit Review Bot
no flags Details
Patch V3 (41.79 KB, patch)
2012-03-29 16:37 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandru Chiculita 2012-03-28 13:52:40 PDT
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.
Comment 1 Radar WebKit Bug Importer 2012-03-28 14:44:46 PDT
<rdar://problem/11141841>
Comment 2 Alexandru Chiculita 2012-03-28 18:05:54 PDT
Created attachment 134457 [details]
Patch V1
Comment 3 Dean Jackson 2012-03-28 19:58:52 PDT
Created attachment 134482 [details]
Patch
Comment 4 Dean Jackson 2012-03-28 20:00:20 PDT
Oops. I didn't notice Alex had sent a patch.
Comment 5 Dean Jackson 2012-03-28 20:22:48 PDT
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.
Comment 6 Dean Jackson 2012-03-28 20:28:46 PDT
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.
Comment 7 Alexandru Chiculita 2012-03-28 21:17:51 PDT
(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.
Comment 8 WebKit Review Bot 2012-03-28 22:02:31 PDT
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
Comment 9 WebKit Review Bot 2012-03-28 22:02:38 PDT
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
Comment 10 WebKit Review Bot 2012-03-28 22:59:33 PDT
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
Comment 11 WebKit Review Bot 2012-03-28 22:59:41 PDT
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
Comment 12 Alexandru Chiculita 2012-03-29 04:53:32 PDT
(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
Comment 13 Alexandru Chiculita 2012-03-29 11:41:45 PDT
(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.
Comment 14 Alexandru Chiculita 2012-03-29 13:14:07 PDT
Created attachment 134652 [details]
Patch V2

I've tried to fix all the cases with this patch.
Comment 15 Alexandru Chiculita 2012-03-29 13:16:23 PDT
Created attachment 134653 [details]
Patch V3

Last one was still the first patch, uploaded again. This time it is the right patch.
Comment 16 WebKit Review Bot 2012-03-29 16:07:29 PDT
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
Comment 17 WebKit Review Bot 2012-03-29 16:07:38 PDT
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
Comment 18 Alexandru Chiculita 2012-03-29 16:37:14 PDT
Created attachment 134691 [details]
Patch V3

Fixed comments in test files. Marked tests for rebaseline on Chromium.
Comment 19 Dean Jackson 2012-03-29 21:09:57 PDT
(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.
Comment 20 Dean Jackson 2012-03-29 21:17:24 PDT
Comment on attachment 134691 [details]
Patch V3

Nice catch on the shadow CA properties.
Comment 21 WebKit Review Bot 2012-03-29 22:07:24 PDT
Comment on attachment 134691 [details]
Patch V3

Clearing flags on attachment: 134691

Committed r112644: <http://trac.webkit.org/changeset/112644>
Comment 22 WebKit Review Bot 2012-03-29 22:08:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Alexandru Chiculita 2012-03-30 13:01:26 PDT
*** Bug 82531 has been marked as a duplicate of this bug. ***