Bug 104959 – Style on dynamically created use element does not get overridden by referenced element style change

• Component: SVG
• Status: RESOLVED CONFIGURATION CHANGED

[Dirk Schulze]

Style on dynamically created use element does not get overridden by referenced element style change.

In the test, a 'use' element is created dynamically. The 'use' element references a circle, which did not set the fill property.In the same cycle as the use element creation, the fill color of the circle is set to 'green'. Green should override the fill property of the use element, but doesn't. Adding the setting of fill to a setTimeout solves the problem. Please see reference to test the issue.


<svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
    <circle cx="150" cy="150" r="100" id="circle"/>
    <g id="g"></g>
    <script type="text/ecmascript">
        var g = document.getElementById("g"),
            c = document.getElementById("circle"),
            use = document.createElementNS("http://www.w3.org/2000/svg", "use");
        use.setAttributeNS("http://www.w3.org/1999/xlink", "href", "#circle");
        use.setAttribute("x", 200);
        use.setAttribute("fill", "red");
        g.appendChild(use);
        // Fill color of circle is set to green, 'use' reference should follow, but doesn't.
        c.setAttribute("fill", "green");
        <!--setTimeout(function() { c.setAttribute("fill", "green"); },0);-->
    </script>
</svg>​

[Takashi Sakamoto]

Created attachment 179416 [details]
Patch

[Philip Rogers]

Comment on attachment 179416 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Since StyleResolver only checks styles of shadow tree elements, i.e.

This sentence is hard to read; do you mind rephrasing?

> Source/WebCore/svg/SVGUseElement.cpp:924
> +    if (m_needsShadowTreeRecreation)

Can you move this check above "if (!renderer())" ?

[Dirk Schulze]

Changing to P1 as discussed on IRC.

[Dirk Schulze]

Comment on attachment 179416 [details]
Patch

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

> Source/WebCore/svg/SVGUseElement.cpp:928
>      m_needsShadowTreeRecreation = true;
>      setNeedsStyleRecalc();

Can you do 

if (renderer) {
    m_needsShadowTreeRecreation = true;
    setNeedsStyleRecalc();
    return;
}

if (!target)
    return;

// the rest of your code in the if clause?

[Takashi Sakamoto]

Created attachment 181479 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 179416 [details]
Patch

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

Thank you for reviewing.

>> Source/WebCore/ChangeLog:10
>> +        Since StyleResolver only checks styles of shadow tree elements, i.e.
> 
> This sentence is hard to read; do you mind rephrasing?

I see. I tried to rewrite the bad description. Is it ok now?

>> Source/WebCore/svg/SVGUseElement.cpp:924
>> +    if (m_needsShadowTreeRecreation)
> 
> Can you move this check above "if (!renderer())" ?

Sure. Done.

>> Source/WebCore/svg/SVGUseElement.cpp:928
>>      setNeedsStyleRecalc();
> 
> Can you do 
> 
> if (renderer) {
>     m_needsShadowTreeRecreation = true;
>     setNeedsStyleRecalc();
>     return;
> }
> 
> if (!target)
>     return;
> 
> // the rest of your code in the if clause?

Done.

[Philip Rogers]

Comment on attachment 181479 [details]
Patch

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

I have two more minor nits about phrasing but this change looks awesome otherwise.

> Source/WebCore/ChangeLog:13
> +        So StyleResoler will miss the referred element's inline style changes.

Nit: I know what you mean here but this is still a little wonky because the sentence "Because...not" is missing a predicate. Can you rephrase:
"Because the StyleResolver doesn't check whether a given element is an element in a use element's shadow tree, it will miss the referenced element's inline style changes."

> Source/WebCore/svg/SVGUseElement.cpp:923
> +    // should copy attribute data, because styleResolver doesn't know

Nit from the style guide, can you capitalize "Should"?

[Takashi Sakamoto]

Created attachment 182024 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 181479 [details]
Patch

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

>> Source/WebCore/ChangeLog:13
>> +        So StyleResoler will miss the referred element's inline style changes.
> 
> Nit: I know what you mean here but this is still a little wonky because the sentence "Because...not" is missing a predicate. Can you rephrase:
> "Because the StyleResolver doesn't check whether a given element is an element in a use element's shadow tree, it will miss the referenced element's inline style changes."

Thanks. Done.

>> Source/WebCore/svg/SVGUseElement.cpp:923
>> +    // should copy attribute data, because styleResolver doesn't know
> 
> Nit from the style guide, can you capitalize "Should"?

Done.

[Takashi Sakamoto]

Created attachment 192401 [details]
Patch

[Build Bot]

Comment on attachment 192401 [details]
Patch

Attachment 192401 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17004315

New failing tests:
fast/css/sticky/sticky-both-sides.html
fast/css/sticky/inline-sticky.html

[Stephen Chenney]

Comment on attachment 192401 [details]
Patch

I have to think those mac tests are flakes.

[Andreas Kling]

Comment on attachment 192401 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

[Ahmad Saleem]

Created attachment 461447 [details]
Safari 15.6 matches with other browsers

I am unable to reproduce this issue using attached JSFiddle in URL field and all browsers as can be seen from attached screenshot fill circle with "green".

I am going to mark this as "RESOLVED CONFIGURATION CHANGED", if anything else is required or this is not fixed. Please reopen this bug. Thanks!