Bug 154576 – <g> wrapping <symbol> causes display of hidden <symbol>

• Component: SVG
• Status: RESOLVED FIXED

[Craig Mandsager]

Created attachment 271994 [details]
<g> wrapping <symbol> causes display of hidden <symbol>

A <symbol> should never be directly displayed. Yet <g> wrapping <symbol> causes display of hidden <symbol> if the <g> is referenced by another <use>. In the test file, the expected image should be two red rectangles aligned on a diagonal (displays correctly in Firefox), the black (unfilled) rectangle should never be visible (is visible in Safari).

[Said Abou-Hallawa]

Created attachment 272154 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/24825162>

[Darin Adler]

Comment on attachment 272154 [details]
Patch

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

> Source/WebCore/svg/SVGUseElement.cpp:313
> +static inline void removeChildrenFromParent(const Vector<Element*>& children)

This function has a bit of a peculiar name. It removes children from their multiple parents, plural, not from one parent, singular, and also it does the other work that is specific to removing disallowed elements from a cloned subtree. I’d like to see a different name.

> Source/WebCore/svg/SVGUseElement.cpp:349
> +    for (auto &&descendant : descendantsOfType<SVGSymbolElement>(subtree))

This should be:

    for (auto& descendant : descendantsOfType<SVGSymbolElement>(subtree))

No reason to write "auto&&" and certainly no reason to format it as "auto &&descendant".

> Source/WebCore/svg/SVGUseElement.cpp:427
> +    // The SVGSymbolElement is allowed only in the subtree if it's the cloned subtree root

It’s inelegant to have a long comment about one line of this function, but not about any of the others. We should find a way to explain this that is not so focused on this most recent change alone. Maybe it’s as simple as putting this comment on the removeSymbolElementsFromSubtree function rather than in the middle of the flow of this function.

Here’s my attempt at rewriting the comment:

    // Symbol elements inside the subtree should not be cloned for two reasons: 1) They are invisible and
    // don't need to be cloned to get correct rendering. 2) expandSymbolElementsInShadowTree will turn them
    // into use elements, which is correct for symbol elements directly referenced by use elements, but
    // incorrect for ones that just happen to be in a subtree.

> Source/WebCore/svg/SVGUseElement.cpp:428
> +    // element. Any descendant SVGSymbolElement should be removed from the sub tree because

"subtree" is one word, not two

[Said Abou-Hallawa]

Created attachment 272330 [details]
Patch

[Said Abou-Hallawa]

Created attachment 272343 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 272154 [details]
Patch

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

>> Source/WebCore/svg/SVGUseElement.cpp:313
>> +static inline void removeChildrenFromParent(const Vector<Element*>& children)
> 
> This function has a bit of a peculiar name. It removes children from their multiple parents, plural, not from one parent, singular, and also it does the other work that is specific to removing disallowed elements from a cloned subtree. I’d like to see a different name.

Done. I renamed it to disassociateAndRemoveClones() because it takes a vector of clones, dissociates their decedents from their originals and then delete the clones form their parents.

>> Source/WebCore/svg/SVGUseElement.cpp:349
>> +    for (auto &&descendant : descendantsOfType<SVGSymbolElement>(subtree))
> 
> This should be:
> 
>     for (auto& descendant : descendantsOfType<SVGSymbolElement>(subtree))
> 
> No reason to write "auto&&" and certainly no reason to format it as "auto &&descendant".

Done.

>> Source/WebCore/svg/SVGUseElement.cpp:427
>> +    // The SVGSymbolElement is allowed only in the subtree if it's the cloned subtree root
> 
> It’s inelegant to have a long comment about one line of this function, but not about any of the others. We should find a way to explain this that is not so focused on this most recent change alone. Maybe it’s as simple as putting this comment on the removeSymbolElementsFromSubtree function rather than in the middle of the flow of this function.
> 
> Here’s my attempt at rewriting the comment:
> 
>     // Symbol elements inside the subtree should not be cloned for two reasons: 1) They are invisible and
>     // don't need to be cloned to get correct rendering. 2) expandSymbolElementsInShadowTree will turn them
>     // into use elements, which is correct for symbol elements directly referenced by use elements, but
>     // incorrect for ones that just happen to be in a subtree.

Done. Comment was removed from here and added to removeSymbolElementsFromSubtree(). I only changed "2) expandSymbolElementsInShadowTree will turn them into use elements" to "2) expandSymbolElementsInShadowTree will turn them into <svg> elements".

>> Source/WebCore/svg/SVGUseElement.cpp:428
>> +    // element. Any descendant SVGSymbolElement should be removed from the sub tree because
> 
> "subtree" is one word, not two

Done. The whole comment was moved and changed.

[Said Abou-Hallawa]

Created attachment 272347 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 272347 [details]
Patch

Clearing flags on attachment: 272347

Committed r197194: <http://trac.webkit.org/changeset/197194>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.