Bug 65643 – Repaint issues with -webkit-svg-shadow used on a container

• Component: SVG
• Status: RESOLVED FIXED

[Tim Horton]

Created attachment 102829 [details]
repro

Steps to Reproduce:

1. Open the attached repro.
2. Wait 600 ms.
3. Note how the shadow of the second element disappears.

Expected Results:

Two shapes, two shadows.

Actual results:

Two shapes, one shadow.

Notes:

Another side effect of this is that when an element is offscreen but its shadow is onscreen, the shadow is not drawn.

It seems that when you specify "-webkit-svg-shadow" on <svg>, or a <g>, etc., the shadow of elements within the container does not redraw when it should.

I'm not 100% certain that we should see *any* shadows drawn when applying -webkit-svg-shadow to a container; -webkit-box-shadow doesn't seem to affect children.

<rdar://problem/7600532>

[Tim Horton]

It seems like the child elements don't know about their shadow, because it's entirely contained within the parent. When they're asked to redraw, they don't know about the shadow rect, so they don't include it in the intersection test. The problem of course being that the child needs to repaint in order to repaint its shadow!

Ways that immediately come to mind to fix this:

a) have some way of informing children that they *have* a shadow (and should take that into consideration when determining whether or not to repaint), but don't need to *set* it/appear to have it.

b) stop allowing containers to have shadows. box-shadow on an HTML element isn't inherited by children, is there any reason -webkit-svg-shadow should be? This would change behavior, though, and I don't really want to do that.

Any ideas?

[Tim Horton]

Created attachment 103699 [details]
correct repro

Somehow the copy of the repro I uploaded had some extra characters in it. Here's a corrected copy.

[Tim Horton]

Created attachment 103710 [details]
preliminary patch sans changelog/test

[WebKit Review Bot]

Comment on attachment 103710 [details]
preliminary patch sans changelog/test

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

[Early Warning System Bot]

Comment on attachment 103710 [details]
preliminary patch sans changelog/test

Attachment 103710 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9357432

[Tim Horton]

Created attachment 103714 [details]
preliminary patch sans changelog/test

[WebKit Review Bot]

Comment on attachment 103714 [details]
preliminary patch sans changelog/test

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

New failing tests:
svg/css/shadow-changes.svg
fast/repaint/moving-shadow-on-container.html
svg/css/mask-with-shadow.svg
svg/css/composite-shadow-text.svg
svg/css/group-with-shadow.svg
svg/css/arrow-with-shadow.svg
svg/css/clippath-with-shadow.svg

[Nikolas Zimmermann]

Comment on attachment 103714 [details]
preliminary patch sans changelog/test

Tim, the idea behind this patch is correct. OTOH it slows down every common case by requiring these additional tree traversal.
This needs to be optimized. We need to know whether a child needs this logic, that means we have to cache somewhere if the parent of an arbitary child has shadow or not.

Storing this inside SVGRenderStylle of the child is not an option, as styles may be shared.
<g style="-webkit-svg-shadow: ...">
<rect/>
</g>
<rect/>

Both rects may share the same style, so we can't store the information whether the parent is shadowed in the childs style.
The renderers are unique though, so it might be possible to stash this info into RenderSVGContainer.

I'll bet you'll find an elegant solution :-)

[Tim Horton]

There are other issues I want to work out first before the performance, but that is a valid issue.

Primarily:

We already recursively include parent's shadows in the *repaint* rect via computeRectForRepaint; it's unfortunate that we're adding a second run up the tree, in *any* case (we shouldn't have to).

Another issue is that since clippedOverflowRectForRepaint uses repaintRectInLocalCoordinates and then computeRectForRepaints that, with the patch as-is, the shadow gets included *twice*, and we overpaint.

[Tim Horton]

(In reply to comment #9)
> Another issue is that since clippedOverflowRectForRepaint uses repaintRectInLocalCoordinates and then computeRectForRepaints that, with the patch as-is, the shadow gets included *twice*, and we overpaint.

Actually, this isn't my fault, I just make it happen more often.

Currently, if you apply -webkit-svg-shadow to a <rect/>, the repaint rect is adjusted for the shadow twice (thus, being quite a bit bigger than needed in my pathological cases).

[Tim Horton]

Created attachment 104596 [details]
expanded preliminary patch

[Tim Horton]

(In reply to comment #11)
> Created an attachment (id=104596) [details]
> expanded preliminary patch

This still has the issue of the overrepaint (which isn't due to this patch at all), but other than that is in good working order.

[Tim Horton]

Created attachment 104597 [details]
last patch plus image results that I missed

[WebKit Review Bot]

Comment on attachment 104597 [details]
last patch plus image results that I missed

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

New failing tests:
svg/css/shadow-changes.svg
fast/repaint/moving-shadow-on-container.html
svg/css/mask-with-shadow.svg
svg/css/composite-shadow-text.svg
svg/custom/repaint-webkit-svg-shadow.html
svg/css/arrow-with-shadow.svg
svg/css/clippath-with-shadow.svg
svg/css/group-with-shadow.svg

[Nikolas Zimmermann]

Comment on attachment 104597 [details]
last patch plus image results that I missed

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

It's getting better! Still some questions/suggestions.

> Source/WebCore/platform/graphics/mac/FontMac.mm:225
> +    bool hasSimpleShadow = context->textDrawingMode() == TextModeFill && shadowColor.isValid() && !shadowBlur && !platformData.isColorBitmapFont() && (!context->shadowsIgnoreTransforms() || context->getCTM().isIdentityOrTranslationOrFlipped()) && !context->inTransparencyLayer();

Aren't other platforms affected by this as well?
Why is this needed? Can you explain a bit more?

> Source/WebCore/rendering/RenderObject.cpp:1789
> +        setHasSVGShadow(TRUE);

s/TRUE/true/

> Source/WebCore/rendering/RenderObject.h:919
> +#if ENABLE(SVG)
> +    bool m_hasSVGShadow;
> +#endif

This bit of information is needed by all renderers which use intersectRepaintRectWithResources: RenderSVG(Container|Image|Path|Root|Text).

RenderSVG(Container|Image|Path) all inherit from RenderSVGModelObject.
RenderSVGRoot inherits from RenderBox, RenderSVGText from RenderSVGBlock, and thus from RenderBlock.

I'd rather store m_hasSVGShadow in RenderSVGModelObject, RenderSVGRoot and RenderSVGText, to avoid consuming more memory for HTML use cases.
hasSVGShadow could still be a virtual method in RenderObject, just like it's done for eg. nodeAtFloatPoint, and we'd override the getters/setters in RenderSVGModelObject/Root/Text.

What do you think?

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:287
> +
> +        if (!style)
> +            continue;

IIRC neither style nor svgStyle can be null, please double-check.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:301
> +        if (!currentObject->hasSVGShadow())
> +            return;
> +    } while (currentObject);

while (currentObject && currentObject->hasSVGShadow() ?

[Tim Horton]

Comment on attachment 104597 [details]
last patch plus image results that I missed

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

>> Source/WebCore/platform/graphics/mac/FontMac.mm:225
>> +    bool hasSimpleShadow = context->textDrawingMode() == TextModeFill && shadowColor.isValid() && !shadowBlur && !platformData.isColorBitmapFont() && (!context->shadowsIgnoreTransforms() || context->getCTM().isIdentityOrTranslationOrFlipped()) && !context->inTransparencyLayer();
> 
> Aren't other platforms affected by this as well?
> Why is this needed? Can you explain a bit more?

Oh, yeah, that actually needs explanation.

I'll provide an example:

If we set the shadow on a group, a transparency layer is created, and the shadow is cleared (because it's applied to the group, not the children). Then, when we go to draw the text shadow, if it has no blur (and can be rendered by simply redrawing the text), we a) store the shadow b) draw the text by hand in the shadow color c) restore the shadow. But, if we're in a transparency layer, the shadow is not clearable!

So what happens in effect is that we draw the text shadow into the transparency layer, then when it's composited onto the screen, the group's shadow is applied, leading to the *shadow* having its own shadow!

So, we can't use simple shadows inside a transparency layer. I assume the behavior is the same on other platforms (with the shadow clearing and all) because that makes the most sense. I think perhaps we should make the increment/decrement/inTransparencyLayer() stuff enabled on all platforms.

I might be able to concoct a test for this that still exhibits after we fix the overdraw, but for right now it's made blindingly obvious in a variety of other tests because of that flaw in this patch.

>> Source/WebCore/rendering/RenderObject.h:919
>> +#endif
> 
> This bit of information is needed by all renderers which use intersectRepaintRectWithResources: RenderSVG(Container|Image|Path|Root|Text).
> 
> RenderSVG(Container|Image|Path) all inherit from RenderSVGModelObject.
> RenderSVGRoot inherits from RenderBox, RenderSVGText from RenderSVGBlock, and thus from RenderBlock.
> 
> I'd rather store m_hasSVGShadow in RenderSVGModelObject, RenderSVGRoot and RenderSVGText, to avoid consuming more memory for HTML use cases.
> hasSVGShadow could still be a virtual method in RenderObject, just like it's done for eg. nodeAtFloatPoint, and we'd override the getters/setters in RenderSVGModelObject/Root/Text.
> 
> What do you think?

Sounds better, I'll do that.

>> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:287
>> +            continue;
> 
> IIRC neither style nor svgStyle can be null, please double-check.

I will double-check.

>> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:301
>> +    } while (currentObject);
> 
> while (currentObject && currentObject->hasSVGShadow() ?

Sure! I was using the space after the if(!currentObject->hasSVGShadow()) for debugging before, but that's certainly possible now.

[Tim Horton]

Created attachment 104713 [details]
previous patch plus make inTransparencyLayer/increment stuff generic, plus move hasSVGShadow into SVG renderers instead of RenderObject

[Early Warning System Bot]

Comment on attachment 104713 [details]
previous patch plus make inTransparencyLayer/increment stuff generic, plus move hasSVGShadow into SVG renderers instead of RenderObject

Attachment 104713 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9468804

[Gyuyoung Kim]

Comment on attachment 104713 [details]
previous patch plus make inTransparencyLayer/increment stuff generic, plus move hasSVGShadow into SVG renderers instead of RenderObject

Attachment 104713 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9467868

[WebKit Review Bot]

Comment on attachment 104713 [details]
previous patch plus make inTransparencyLayer/increment stuff generic, plus move hasSVGShadow into SVG renderers instead of RenderObject

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

[Tim Horton]

It's unclear to me how to fix the build of all of these other platforms; where is their implementation of GraphicsContext?

[Darin Adler]

Comment on attachment 104713 [details]
previous patch plus make inTransparencyLayer/increment stuff generic, plus move hasSVGShadow into SVG renderers instead of RenderObject

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

> Source/WebCore/platform/graphics/GraphicsContext.cpp:350
> +#if (OS(WINCE) && !PLATFORM(QT)) || PLATFORM(WX)

It seems highly likely  this list of platforms will get out of sync with the underlying machinery. Maybe instead there should be a private static inline member function that returns false or true that you could || with the transparency count.

> Source/WebCore/platform/graphics/GraphicsContext.h:373
> +        bool inTransparencyLayer() const;

This should probably be named isInTransparencyLayer.

[Collabora GTK+ EWS bot]

Comment on attachment 104713 [details]
previous patch plus make inTransparencyLayer/increment stuff generic, plus move hasSVGShadow into SVG renderers instead of RenderObject

Attachment 104713 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9463919

[Tim Horton]

So, this is still quite wrong, because it applies all of the shadows above an element in the tree to that element. So, for example

<svg xmlns="http://www.w3.org/2000/svg" id="asdf" style="-webkit-svg-shadow: 50px 50px 5px red;">
<g><g><g><g><g><g><g>
<rect x="10" y="10" width="20" height="20" fill="black"  />
</g></g></g></g></g></g></g>
</svg>

Each of the <g> elements will be expanded by the size of <svg>'s shadow, and by the time we get to <svg>, we're *way* overrepainting.

I wonder, is it appropriate to apply the shadow expansion only to leaf nodes? I'll have to think about that some more.

[Nikolas Zimmermann]

Comment on attachment 104713 [details]
previous patch plus make inTransparencyLayer/increment stuff generic, plus move hasSVGShadow into SVG renderers instead of RenderObject

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

Looks great, some further comments from my side and Darin lead to r- for now:

> Source/WebCore/platform/graphics/GraphicsContext.cpp:338
> +    m_transparencyCount++;

Use ++m_transparencyCount as micro optimization.

> Source/WebCore/platform/graphics/GraphicsContext.cpp:345
> +    m_transparencyCount--;

Use --m_transparencyCount as micro optimization.

>> Source/WebCore/platform/graphics/GraphicsContext.cpp:350
>> +#if (OS(WINCE) && !PLATFORM(QT)) || PLATFORM(WX)
> 
> It seems highly likely  this list of platforms will get out of sync with the underlying machinery. Maybe instead there should be a private static inline member function that returns false or true that you could || with the transparency count.

Agreed!

> Source/WebCore/platform/graphics/GraphicsContext.h:546
> +        int m_transparencyCount;

unsigned should be sufficient.

> Source/WebCore/rendering/RenderObject.h:403
> +    virtual void setHasSVGShadow(bool shadowedParent) { UNUSED_PARAM(shadowedParent); }

I'd rather just use setHasSVGShadow(bool) { } here.

[Dirk Schulze]

Comment on attachment 104713 [details]
previous patch plus make inTransparencyLayer/increment stuff generic, plus move hasSVGShadow into SVG renderers instead of RenderObject

Btw. The ChangeLog is still missing ;)

[Tim Horton]

Created attachment 105525 [details]
new patch, with per-platform fixes

This should probably be split into two bugs/patches, one about the simple text shadow bug, and this one, about shadow repaint.

[WebKit Review Bot]

Comment on attachment 105525 [details]
new patch, with per-platform fixes

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

New failing tests:
svg/css/shadow-changes.svg
fast/repaint/moving-shadow-on-container.html
svg/css/mask-with-shadow.svg
svg/css/composite-shadow-text.svg
svg/css/group-with-shadow.svg
svg/css/arrow-with-shadow.svg
svg/css/clippath-with-shadow.svg

[Tim Horton]

Niko, do you have any suggestion about how to prevent over-expanding the repaint rect because extend our repaint rect by the shadow every time? (the example in Comment #24, for starters)

[Nikolas Zimmermann]

(In reply to comment #29)
> Niko, do you have any suggestion about how to prevent over-expanding the repaint rect because extend our repaint rect by the shadow every time? (the example in Comment #24, for starters)

I rechecked your patch and thought about this. To be able to determine the point when the shadow rect extension should happen it's necessary to know _who_ is asking for the repaint rect.

Suppose following tree:
RenderSVGRoot (1) [shadowed]
    RenderSVGContainer (2)
        RenderSVGContainer (3)
            RenderSVGPath (4)

When asking (4) for it's repaint rect, we want to include the shadow from (1) at the point where the recursion hits (1) (call to (4) -> (3) -> (2) -> (1)). Same for (3), (2).

Your current solution extends each of the nodes (4) / (3) / (2) by the shadow of (1). Even when changing that, there's still the problem that we're caching the repaintRectInLocalCoordinates() that eg. (3) asked for in (3). We're doing the same for (2) and (1).

When asking for the total repaint rect starting from (4), computeRectForRepaint() comes into play which traverses the parent hierarchy from (4) to (1) mapping the cached repaintBoundingBox() through the localToParentTransform().

An obvious problem with your current code is that you're extending _each_ of the repaint rects of (4) / (3) / (2) with the shadow rect defined in local coordinates of (1), that ignores the fact that when traversing from (4) -> (3) -> (2) -> (1) the repaint rect gets mapped through the local trafos.

That will break cases like <svg style="-webkit-svg-shadow:.." <g transform="scale(2)"><g transform="scale(2)"> <rect ..>.

In another comments you've mentioned <g><g><g><g>... <rect> ... which is currently over-drawing a lot as well. The problem here is the usage of ShadowData::adjustRectforShadow(). Each time you're calculating the <g> repaint rect through computeRectForRepaint() you're just extending the repaint rect by the amount of shadow. The function is named "intersectRepaintRectWithShadow" but it's never intersecting, rather it just grows the boundaries on each invocation.

To summarize: The current approach is tricky. We should rethink about what we want to cache in repaintRectInLocalCoordinates(). The obvious solution for me is to stop including the shadow at all in repaintRectInLocalCoordinates() and only let computeRectForRepaint deal with it at all, to avoid mapping shadows of parents through localToParentTransform() of children, which is the root of all evil.

I don't see at all why the children of a shadowed container should know that it's container has shadow. When computing the rect for repaint for the rect in <g><g><g><rect/> you'll always walk up the parents and can call adjustRectForShadow() on the renderer which contains the shadow and extend the repaint rect once by the amount of shadow distance.

What do you think? :-)

[Tim Horton]

Moving all of the transparency layer and text shadow changes to https://bugs.webkit.org/show_bug.cgi?id=67543

[Tim Horton]

Created attachment 106521 [details]
test case of applying a shadow, and transforming the shadowed paths below the shadowed element

(In reply to comment #30)
> That will break cases like <svg style="-webkit-svg-shadow:.." <g transform="scale(2)"><g transform="scale(2)"> <rect ..>.

I'm a bit intrigued by what actually *does* happen here, right now. Since the shadow is on the parent and is applied before the transformations, we end up with a different result than I initially expected; thinking about it now, though, I think it makes sense. Still, it would be nice to hear if this is expected -- attaching a test case.

[Nikolas Zimmermann]

What's the status of this bug report? I'm a bit lost here.

[Tim Horton]

(In reply to comment #33)
> What's the status of this bug report? I'm a bit lost here.

I think the most recent patch still over expands the reprint rect; I forgot about this bug and haven't really worked through your long comment above. Thanks for reminding me, though!

[Tim Horton]

(In reply to comment #34)
> (In reply to comment #33)
> > What's the status of this bug report? I'm a bit lost here.
> 
> I think the most recent patch still over expands the reprint rect; I forgot about this bug and haven't really worked through your long comment above. Thanks for reminding me, though!

This is causing infinitely more problems now with tiled rendering. I'm going to take another look at this.

[Tim Horton]

(In reply to comment #30)
> To summarize: The current approach is tricky. We should rethink about what we want to cache in repaintRectInLocalCoordinates(). The obvious solution for me is to stop including the shadow at all in repaintRectInLocalCoordinates() and only let computeRectForRepaint deal with it at all, to avoid mapping shadows of parents through localToParentTransform() of children, which is the root of all evil.
> 
> I don't see at all why the children of a shadowed container should know that it's container has shadow. When computing the rect for repaint for the rect in <g><g><g><rect/> you'll always walk up the parents and can call adjustRectForShadow() on the renderer which contains the shadow and extend the repaint rect once by the amount of shadow distance.
> 
> What do you think? :-)

I don't see how children can get away without knowing that their container has shadow. There are places (RenderSVGShape::paint, for example) that use repaintRectInLocalCoordinates directly; the paint will bail there if the paintInfo doesn't intersect the repaintRectInLocalCoordinates (which you were proposing doesn't include the shadow), which is quite easy to cause in the tiled drawing case (or in the case where the shape is off-screen but the shadow is not).

[Tim Horton]

Created attachment 165758 [details]
patch

Needs a changelog, and I'd like to avoid adding anything to RenderObject if possible, and there's some minor cleanup required, and it might be crazysauce. However, it gets the repaint rects right for everything except text, with shadow on any parent, with arbitrary transforms in between, fixing a multitude of bugs (shadows not rendering when the object is off screen, shadows being clipped by tiles, shadows not invalidating correctly, etc.).

I don't have sufficient time at the moment, but next week, I plan on writing the changelog and putting the finishing touches on, and will file a followup bug to fix text (which is a bit crazier since it ends back up in non-SVG-land during painting).

[WebKit Review Bot]

Comment on attachment 165758 [details]
patch

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

New failing tests:
svg/custom/focus-ring.svg
svg/css/composite-shadow-with-opacity.html
svg/W3C-SVG-1.1/types-basicDOM-01-b.svg
svg/css/clippath-with-shadow.svg
svg/css/composite-shadow-example.html
svg/css/shadow-changes.svg
svg/batik/text/textFeatures.svg
fast/repaint/moving-shadow-on-container.html
svg/css/mask-with-shadow.svg
svg/batik/text/smallFonts.svg
svg/css/composite-shadow-text.svg
svg/css/arrow-with-shadow.svg
svg/W3C-SVG-1.1/animate-elem-82-t.svg
svg/custom/simple-text-double-shadow.svg
svg/W3C-SVG-1.1-SE/types-dom-01-b.svg
svg/W3C-SVG-1.1/animate-elem-24-t.svg

[Build Bot]

Comment on attachment 165758 [details]
patch

Attachment 165758 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14021612

New failing tests:
svg/custom/focus-ring.svg
svg/W3C-SVG-1.1/animate-elem-82-t.svg
svg/css/shadow-changes.svg
svg/W3C-SVG-1.1/types-basicDOM-01-b.svg
svg/css/clippath-with-shadow.svg
svg/css/composite-shadow-example.html
svg/css/composite-shadow-with-opacity.html
svg/batik/text/textFeatures.svg
fast/repaint/moving-shadow-on-container.html
svg/batik/text/smallFonts.svg
svg/css/composite-shadow-text.svg
svg/css/group-with-shadow.svg
svg/css/arrow-with-shadow.svg
svg/css/mask-with-shadow.svg
svg/custom/simple-text-double-shadow.svg
svg/W3C-SVG-1.1-SE/types-dom-01-b.svg
svg/W3C-SVG-1.1/animate-elem-24-t.svg

[Nikolas Zimmermann]

Comment on attachment 165758 [details]
patch

Great that you're back on this Tim. Looking forward to the detailed ChangeLog - even if this adds new stuff to RenderObject the approach in general looks much safer, though I only had a quick look.

[Tim Horton]

Created attachment 169072 [details]
patch

[Tim Horton]

I'm not going to handle the RenderSVGBlock-based cases (text and foreignobject) in this bug. Filed https://bugs.webkit.org/show_bug.cgi?id=99533 to cover those, I'll add it to my fixme.

[WebKit Review Bot]

Attachment 169072 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/rendering/svg/SVGRenderSupport.h:68:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/svg/SVGRenderSupport.h:81:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/svg/SVGRenderSupport.h:82:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 169072 [details]
patch

Attachment 169072 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14384432

New failing tests:
fast/repaint/moving-shadow-on-container.html

[WebKit Review Bot]

Comment on attachment 169072 [details]
patch

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

New failing tests:
svg/css/shadow-changes.svg
svg/repaint/repaint-webkit-svg-shadow.svg
svg/css/composite-shadow-example.html
svg/css/composite-shadow-with-opacity.html
fast/repaint/moving-shadow-on-container.html
svg/css/composite-shadow-text.svg
svg/custom/simple-text-double-shadow.svg

[Nikolas Zimmermann]

Did you already prototype a general solution based on your patch locally? Can you assure the interface will be the same for RenderSVGBlock & co?

[Tim Horton]

(In reply to comment #46)
> Did you already prototype a general solution based on your patch locally? Can you assure the interface will be the same for RenderSVGBlock & co?

I had it sort of working locally, but it required changes outside of SVG land.

The parts of the interface that I'm changing here will be the same, surely. There might be additional stuff required though.

[Dirk Schulze]

Now that we get filter: drop-shaow, can we remove -webkit-svg-shadow?

Even after promotion, it was never used by content.

Adding Beth who added this property.

[Beth Dakin]

(In reply to comment #48)
> Now that we get filter: drop-shaow, can we remove -webkit-svg-shadow?
> 
> Even after promotion, it was never used by content.
> 
> Adding Beth who added this property.

This would definitely need to be discussed on webkit-dev. We generally do not like to remove properties that have shipped, no matter how custom.

[Tim Horton]

(In reply to comment #48)
> Now that we get filter: drop-shaow, can we remove -webkit-svg-shadow?

No, we cannot.

> Even after promotion, it was never used by content.

This is not quite accurate.

Also, isn’t -webkit-svg-shadow faster? Does filter: drop-shadow always go through the filters code path?

[Dirk Schulze]

(In reply to comment #50)
> (In reply to comment #48)
> > Now that we get filter: drop-shaow, can we remove -webkit-svg-shadow?
> 
> No, we cannot.
> 

We had multiple issues with -webkit-svg-shadow in the past and this is just the next one. I spend a lot ot time fixing animations and it seems still not working correctly. We should ask ourself is is worth it spending so much time on it?

> > Even after promotion, it was never used by content.
> 
> This is not quite accurate.
> 
> Also, isn’t -webkit-svg-shadow faster? Does filter: drop-shadow always go through the filters code path?
Not sure what you mean with filter path. So yes, we create a FilterEffect object for drop-shadow


Instead of removing it, how about a compromise. Make -webkit-filter a shorthand for -webkit-svg-shadow with:

filter: drop-shadow(<shadow>)+

So the the CSSParser would set the -webkit-filter create a FilterEffect object like for the filter property (well it really sets the filter property). This way we have one code path and not multiple of features with the same effect, each of them hard to maintain.

[Tim Horton]

(In reply to comment #51)
> (In reply to comment #50)
> > (In reply to comment #48)
> > > Now that we get filter: drop-shaow, can we remove -webkit-svg-shadow?
> > 
> > No, we cannot.
> > 
> 
> We had multiple issues with -webkit-svg-shadow in the past and this is just the next one. I spend a lot ot time fixing animations and it seems still not working correctly. We should ask ourself is is worth it spending so much time on it?
> 
> > > Even after promotion, it was never used by content.
> > 
> > This is not quite accurate.
> > 
> > Also, isn’t -webkit-svg-shadow faster? Does filter: drop-shadow always go through the filters code path?
> Not sure what you mean with filter path. So yes, we create a FilterEffect object for drop-shadow
> 
> 
> Instead of removing it, how about a compromise. Make -webkit-filter a shorthand for -webkit-svg-shadow with:
> 
> filter: drop-shadow(<shadow>)+
> 
> So the the CSSParser would set the -webkit-filter create a FilterEffect object like for the filter property (well it really sets the filter property). This way we have one code path and not multiple of features with the same effect, each of them hard to maintain.

This would be better in the long term, yes.

In the short term, I feel that setting the CG shadow while painting is in many cases going to be enormously faster than painting into a buffer, blurring it in software, and compositing it back.

[Tim Horton]

Also, we're looking for a safe, easy fix for this terrible repaint problem; we should investigate more invasive long-term solutions in another bug.

[Simon Fraser (smfr)]

Comment on attachment 169072 [details]
patch

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

Why is this code so much more complex than the equivalent non-SVG code?

> Source/WebCore/rendering/RenderObject.cpp:277
> +#if ENABLE(SVG)
> +    SVGRenderSupport::setRendererHasSVGShadow(newChild, SVGRenderSupport::rendererHasSVGShadow(this) || SVGRenderSupport::rendererHasSVGShadow(newChild));
> +#endif

This is pretty gross. Can't we update this flag during layout or something?

> Source/WebCore/rendering/RenderObject.cpp:1917
> +#if ENABLE(SVG)
> +    SVGRenderSupport::setRendererHasSVGShadow(this, (m_parent && SVGRenderSupport::rendererHasSVGShadow(m_parent)) || (style()->svgStyle() && style()->svgStyle()->shadow()));
> +#endif

Ditto.

> Source/WebCore/rendering/svg/RenderSVGModelObject.h:80
> +    bool m_hasSVGShadow;

Is it OK to increase the size of all RenderSVGModelObjects by this?

[Tim Horton]

(In reply to comment #54)
> (From update of attachment 169072 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169072&action=review
> 
> Why is this code so much more complex than the equivalent non-SVG code?

We talked about this in person.

> > Source/WebCore/rendering/RenderObject.cpp:277
> > +#if ENABLE(SVG)
> > +    SVGRenderSupport::setRendererHasSVGShadow(newChild, SVGRenderSupport::rendererHasSVGShadow(this) || SVGRenderSupport::rendererHasSVGShadow(newChild));
> > +#endif
> 
> This is pretty gross. Can't we update this flag during layout or something?

Maybe!

> > Source/WebCore/rendering/RenderObject.cpp:1917
> > +#if ENABLE(SVG)
> > +    SVGRenderSupport::setRendererHasSVGShadow(this, (m_parent && SVGRenderSupport::rendererHasSVGShadow(m_parent)) || (style()->svgStyle() && style()->svgStyle()->shadow()));
> > +#endif
> 
> Ditto.
> 
> > Source/WebCore/rendering/svg/RenderSVGModelObject.h:80
> > +    bool m_hasSVGShadow;
> 
> Is it OK to increase the size of all RenderSVGModelObjects by this?

sizeof(RenderSVGModelObject) = 56 before and after. I wish we had compile-time-asserts for the size of everything.

[Tim Horton]

Created attachment 172699 [details]
patch trying to fix testexpectations

[Tim Horton]

Created attachment 172701 [details]
and the style bot

[WebKit Review Bot]

Attachment 172701 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/rendering/svg/RenderSVGModelObject.h:71:  Extra space before )  [whitespace/parens] [2]
Source/WebCore/rendering/svg/RenderSVGModelObject.h:71:  Too many spaces inside { }.  [whitespace/braces] [5]
Source/WebCore/rendering/svg/RenderSVGModelObject.h:80:  Should have a space between // and comment  [whitespace/comments] [4]
LayoutTests/platform/qt/TestExpectations:2399:  Path does not exist.  [test/expectations] [5]
Total errors found: 4 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 172701 [details]
and the style bot

Attachment 172701 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14732993

[Early Warning System Bot]

Comment on attachment 172701 [details]
and the style bot

Attachment 172701 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14745545

[Tim Horton]

(In reply to comment #58)
> Attachment 172701 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
> Source/WebCore/rendering/svg/RenderSVGModelObject.h:71:  Extra space before )  [whitespace/parens] [2]
> Source/WebCore/rendering/svg/RenderSVGModelObject.h:71:  Too many spaces inside { }.  [whitespace/braces] [5]
> Source/WebCore/rendering/svg/RenderSVGModelObject.h:80:  Should have a space between // and comment  [whitespace/comments] [4]
> LayoutTests/platform/qt/TestExpectations:2399:  Path does not exist.  [test/expectations] [5]
> Total errors found: 4 in 33 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Whoops, wrong patch.


> LayoutTests/platform/qt/TestExpectations:2399:  Path does not exist.  [test/expectations] [5]

That's webkit.org/b/101302 css3/line-break, which I didn't add.

[Tim Horton]

Created attachment 172706 [details]
post the right patch

[WebKit Review Bot]

Attachment 172706 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
LayoutTests/platform/qt/TestExpectations:2399:  Path does not exist.  [test/expectations] [5]
Total errors found: 1 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 172706 [details]
post the right patch

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

New failing tests:
svg/css/clippath-with-shadow.svg
svg/css/arrow-with-shadow.svg

[WebKit Review Bot]

Comment on attachment 172706 [details]
post the right patch

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

New failing tests:
svg/css/clippath-with-shadow.svg
svg/css/arrow-with-shadow.svg

[Tim Horton]

Created attachment 172853 [details]
patch

[Tim Horton]

Created attachment 172860 [details]
patch

[WebKit Review Bot]

Attachment 172860 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
LayoutTests/platform/qt/TestExpectations:2401:  Path does not exist.  [test/expectations] [5]
Total errors found: 1 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 172860 [details]
patch

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

> Source/WebCore/rendering/RenderObject.cpp:289
> +#if ENABLE(SVG)
> +    SVGRenderSupport::setRendererHasSVGShadow(newChild, SVGRenderSupport::rendererHasSVGShadow(this) || SVGRenderSupport::rendererHasSVGShadow(newChild));
> +#endif

This could be slightly less gross by having SVGRenderSupport::childAdded(RO*, RO*) and burying this inside it.

> Source/WebCore/rendering/RenderObject.cpp:1923
> +    SVGRenderSupport::setRendererHasSVGShadow(this, (m_parent && SVGRenderSupport::rendererHasSVGShadow(m_parent)) || (style()->svgStyle() && style()->svgStyle()->shadow()));

And SVGRenderSupport::styleChanged().

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:332
> +        ASSERT(style);

Not much point asserting there; you'll crash on the next line.

[Tim Horton]

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

[Nikolas Zimmermann]

(In reply to comment #70)
> http://trac.webkit.org/changeset/133834

Hmpf, just realized that the review I made wasn't posted :(
Anyhow, the bottom line was a r+ as well. Sorry Tim, for the long delays from my side :( The thesis keeps me busy.