Bug 111927 – Style updates are not propagated to <use> elements after creation

• Status: NEW
• Component: SVG

[Stuart P. Bentley]

Created attachment 192349 [details]
Testcase

Changing the style of an element through the DOM does not update the style of any <use> elements referencing that element if the style is updated after creation.

Repro: See test case. The change of the <circle>'s fill from green to navy that happens at initialization is copied by the <use> element, but the change on the timeout from navy to blue is not.

[Stuart P. Bentley]

Expected testcase behavior: Both circles turn blue (#00F).
Actual testcase behavior: The original circle on the left turns blue (#00F), while the <use> element referencing it on the right remains navy (#008).

<use> elements do currently update when the corresponding attributes are updated, so scripts setting style properties (ie. `element.style.fill = '#foo'`) can be rewritten to set attributes (ie. `element.setAttribute('fill','#foo')`) as a workaround.

Original bug: https://code.google.com/p/chromium/issues/detail?id=166438

[Takashi Sakamoto]

Created attachment 192422 [details]
Patch

[WebKit Review Bot]

Comment on attachment 192422 [details]
Patch

Attachment 192422 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17024155

New failing tests:
html5lib/generated/run-tests16-data.html

[Stephen Chenney]

Comment on attachment 192422 [details]
Patch

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

Basically good. Just fix the build for SVG disabled. The EWS chromium bot failure is a flake, not of any concern.

> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:31
> +#include "SVGElementInstance.h"

This needs to be protected with #if ENABLE(SVG)

> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:403
> +        SVGElementInstance::invalidateAllInstancesOfElement(toSVGElement(m_parentElement), false);

This also needs #if ENABLE(SVG) protection.

[Stephen Chenney]

Comment on attachment 192422 [details]
Patch

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

Oops, overlooked the test colors. Please fix to make it obvious what is pass and fail.

> LayoutTests/svg/custom/use-modify-target-style-property-expected.svg:3
> +<circle cx="50" cy="50" r="50" style="fill:blue;" id="circle" />

The "good" color should be green.

> LayoutTests/svg/custom/use-modify-target-style-property.svg:12
> +        c.style.fill= "blue";

Change it to green.

> LayoutTests/svg/custom/use-modify-target-style-property.svg:17
> +<circle cx="50" cy="50" r="50" style="fill:green;" id="circle" />

Start it red.

[Takashi Sakamoto]

Created attachment 192671 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 192422 [details]
Patch

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

Thank you for reviewing.

>> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:31
>> +#include "SVGElementInstance.h"
> 
> This needs to be protected with #if ENABLE(SVG)

Done.

>> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:403
>> +        SVGElementInstance::invalidateAllInstancesOfElement(toSVGElement(m_parentElement), false);
> 
> This also needs #if ENABLE(SVG) protection.

Done.

>> LayoutTests/svg/custom/use-modify-target-style-property-expected.svg:3
>> +<circle cx="50" cy="50" r="50" style="fill:blue;" id="circle" />
> 
> The "good" color should be green.

Done.

>> LayoutTests/svg/custom/use-modify-target-style-property.svg:12
>> +        c.style.fill= "blue";
> 
> Change it to green.

Done.

>> LayoutTests/svg/custom/use-modify-target-style-property.svg:17
>> +<circle cx="50" cy="50" r="50" style="fill:green;" id="circle" />
> 
> Start it red.

Done.

[Stephen Chenney]

Comment on attachment 192671 [details]
Patch

Thanks. R=me.

[Philip Rogers]

It looks like this was a good change but it never ended up landing.

Takashi, can you update this patch and land it?

[Ahmad Saleem]

I am still able to reproduce this bug using Safari 15.4 and Safari TP 146 based on attached Test case. Thanks!

[Radar WebKit Bug Importer]

<rdar://problem/97099415>

[Ahmad Saleem]

We might have to do changes here:

https://searchfox.org/wubkat/rev/c40451f6052e2805fb1c6abfb61fa322c67caf5b/Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp#443

I tried:

    if (m_parentElement->isSVGElement())
        invalidateInstances();

and

    if (m_parentElement->isSVGElement())
        SVGElement::invalidateInstances();

__

But get compiler error - invalidateInstances' is a private member of 'WebCore::SVGElement'

Added `SVGElement.h` header as well.

[Ahmad Saleem]

(In reply to Ahmad Saleem from comment #12)
> We might have to do changes here:
> 
> https://searchfox.org/wubkat/rev/c40451f6052e2805fb1c6abfb61fa322c67caf5b/
> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp#443
> 
> I tried:
> 
>     if (m_parentElement->isSVGElement())
>         invalidateInstances();
> 
> and
> 
>     if (m_parentElement->isSVGElement())
>         SVGElement::invalidateInstances();
> 
> __
> 
> But get compiler error - invalidateInstances' is a private member of
> 'WebCore::SVGElement'
> 
> Added `SVGElement.h` header as well.

Tried this as well:

    auto invalidateSVGInstance = SVGElement::invalidateInstances();
    if (m_parentElement->isSVGElement())
        invalidateSVGInstance;

____

but still got error about `private` member.

If I move `void invalidateInstances()` to even Public, it still not work. :-(