Bug 154576 - <g> wrapping <symbol> causes display of hidden <symbol>
Summary: <g> wrapping <symbol> causes display of hidden <symbol>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: Safari 9
Hardware: Unspecified iOS 9.2
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-02-23 01:01 PST by Craig Mandsager
Modified: 2019-05-02 16:18 PDT (History)
6 users (show)

See Also:


Attachments
<g> wrapping <symbol> causes display of hidden <symbol> (532 bytes, image/svg+xml)
2016-02-23 01:01 PST, Craig Mandsager
no flags Details
Patch (16.24 KB, patch)
2016-02-24 15:12 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (15.93 KB, patch)
2016-02-26 09:48 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (16.01 KB, patch)
2016-02-26 11:06 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (16.01 KB, patch)
2016-02-26 11:13 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Craig Mandsager 2016-02-23 01:01:39 PST
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).
Comment 1 Said Abou-Hallawa 2016-02-24 15:12:10 PST
Created attachment 272154 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2016-02-24 15:14:38 PST
<rdar://problem/24825162>
Comment 3 Darin Adler 2016-02-26 08:47:14 PST
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
Comment 4 Said Abou-Hallawa 2016-02-26 09:48:02 PST
Created attachment 272330 [details]
Patch
Comment 5 Said Abou-Hallawa 2016-02-26 11:06:17 PST
Created attachment 272343 [details]
Patch
Comment 6 Said Abou-Hallawa 2016-02-26 11:12:00 PST
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.
Comment 7 Said Abou-Hallawa 2016-02-26 11:13:16 PST
Created attachment 272347 [details]
Patch
Comment 8 WebKit Commit Bot 2016-02-26 12:58:26 PST
Comment on attachment 272347 [details]
Patch

Clearing flags on attachment: 272347

Committed r197194: <http://trac.webkit.org/changeset/197194>
Comment 9 WebKit Commit Bot 2016-02-26 12:58:30 PST
All reviewed patches have been landed.  Closing bug.