Summary: | [CSS Filters] Trigger a repaint on elements with changed filter | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexandru Chiculita <achicu> | ||||||||||||||||||||
Component: | Layout and Rendering | Assignee: | Alexandru Chiculita <achicu> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | dglazkov, dino, macpherson, menard, webkit-bug-importer, webkit.review.bot | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Attachments: |
|
Description
Alexandru Chiculita
2012-03-28 13:52:40 PDT
Created attachment 134457 [details]
Patch V1
Created attachment 134482 [details]
Patch
Oops. I didn't notice Alex had sent a patch. 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. 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.
(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 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 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 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 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
(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 (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. Created attachment 134652 [details]
Patch V2
I've tried to fix all the cases with this patch.
Created attachment 134653 [details]
Patch V3
Last one was still the first patch, uploaded again. This time it is the right patch.
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 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
Created attachment 134691 [details]
Patch V3
Fixed comments in test files. Marked tests for rebaseline on Chromium.
(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 on attachment 134691 [details]
Patch V3
Nice catch on the shadow CA properties.
Comment on attachment 134691 [details] Patch V3 Clearing flags on attachment: 134691 Committed r112644: <http://trac.webkit.org/changeset/112644> All reviewed patches have been landed. Closing bug. *** Bug 82531 has been marked as a duplicate of this bug. *** |