Bug 139451 – Only when the SVG is inline and only when a shape is referenced before it is defined, this shape will not be drawn

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Said Abou-Hallawa]

Created attachment 242942 [details]
<defs> after <use> (inline)

Open the attached test cases in WebKit.

Result: When the SVG is inline (included in an HTML) and when a shape is referenced before it is defined, this shape will not be drawn.  When the same SVG (the one which has the <defs> comes after the <use>) is opened by itself in WebKit, everything is drawn as expected.
Expected: Regardless whether the SVG is inline or not and regardless whether the shapes are defined before they are referenced or after they are referenced, the shapes should be drawn.

[Said Abou-Hallawa]

Created attachment 242943 [details]
<defs> after <use> (non inline)

[Said Abou-Hallawa]

Created attachment 242944 [details]
<defs> before <use> (inline)

[Said Abou-Hallawa]

NOTE: FireFox and Chrome render the first test case as expected.

[Radar WebKit Bug Importer]

<rdar://problem/19192678>

[Said Abou-Hallawa]

Created attachment 243135 [details]
<defs> after <use> (inline - xhtml)

[Said Abou-Hallawa]

It looks like this bug is only related to the html parser. It does not happen if the file extension is xhtml

[Said Abou-Hallawa]

Created attachment 243173 [details]
Patch

[Dirk Schulze]

Comment on attachment 243173 [details]
Patch

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

> Source/WebCore/ChangeLog:34
> +        This actually might not be the most efficient solution here. This invalidate/
> +        update mechanism is very aggressive. While parsing an inline SVG and with
> +        every child gets appended to a target element we will invalidate/update all
> +        shadow trees of the referencing elements till all the children elements get
> +        inserted. If the target SVG element has an 'n' children nodes and there 'm'
> +        other nodes referencing this target elment then invalidating and updating the
> +        shadow tree nodes will be O(n*n*m).

I do not understand what that has to do with the characterData node?

> Source/WebCore/svg/SVGElement.cpp:1099
> +    if (hasID() || change.source != ChildChangeSourceParser)
> +        SVGElementInstance::invalidateAllInstancesOfElement(this);

Ok, so if I understand this part and the comment above, we are invalidating more often. Every time a child changes and the element has an id we always mark all instance for invalidation and rebuild the trees, right?  To your comment above, this is one of the critical code paths. <use> element is specifically slow in WebKit.

If there is a difference between XML and HTML on characterData, why do we fix the issue here?

> LayoutTests/svg/in-html/defs-after-use-expected.html:4
> +    <g transform="translate(10,10)">

Is the transform needed? just putting x="10" y="10" should work for rectangles. (There might be AA issues on circles and curves though.)

> LayoutTests/svg/in-html/defs-after-use.html:16
> +    <defs>
> +      <g id="red-rect">
> +        <rect x="0" y="0" width="100" height="50" fill="red"/>
> +      </g>
> +    </defs>
> +    <use x="10" y="10" xlink:href="#red-rect"/>

IMO this can just use the rect directly. If that actual test works we don't know if this referencing works. The second <use> overdraws this one.

[Said Abou-Hallawa]

Created attachment 243209 [details]
Patch

[Said Abou-Hallawa]

(In reply to comment #8)
> Comment on attachment 243173 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243173&action=review
> 
> > Source/WebCore/ChangeLog:34
> > +        This actually might not be the most efficient solution here. This invalidate/
> > +        update mechanism is very aggressive. While parsing an inline SVG and with
> > +        every child gets appended to a target element we will invalidate/update all
> > +        shadow trees of the referencing elements till all the children elements get
> > +        inserted. If the target SVG element has an 'n' children nodes and there 'm'
> > +        other nodes referencing this target elment then invalidating and updating the
> > +        shadow tree nodes will be O(n*n*m).
> 
> I do not understand what that has to do with the characterData node?

When parsing the following SVG:

  <svg xmlns="http://www.w3.org/2000/svg"
  	  xmlns:xlink="http://www.w3.org/1999/xlink"
  	  width="1000px" height="1000px"
  	  viewBox="0 0 1000 1000">
    <use x="425" y="80" xlink:href="#helmet"/>
    <defs>
      <g id="helmet">
        <path d="M 0 0 L 150 0 A75,75 0 0,0 0 0 z" fill="#6E808B"/>
        <rect x="0" y="0" width="150" height="20" fill="#48565e"/>
      </g>
    </defs>
  </svg>

We get the following DOM tree:

#document  0x116818600 (renderer 0x11650c7f0) (child needs style recalc)
  svg 0x119f02460 (renderer 0x10c171670) (child needs style recalc)
    #text  0x628000080190 "\n "
    use 0x111204e20 (renderer 0x640000180b60) 
      #document-fragment 0x6480001c0e10 (renderer 0x0) 
        g  0x111009000 (renderer 0x6400001809c0) 
          #text  0x640000080820 "\n   "
          path  0x111014500 (renderer 0x6400001e0100) 
          #text  0x640000080910 "\n   "
          rect  0x11100a190 (renderer 0x11100f170) 
          #text  0x640000080780 "\n  "
    #text  0x648000082670 "\n "
    defs  0x110705860 (renderer 0x600000105220) (child needs style recalc)
      #text  0x610000081ea0 "\n  "
      g  0x111311580 (renderer 0x600000185210) 
        #text  0x650000080f50 "\n   "
        path  0x10c176380 (renderer 0x6080001e2f00) 
        #text  0x60000008ef60 "\n   "
        rect  0x10c6259c0 (renderer 0x116517480) 
        #text  0x608000087940 "\n  "
      #text  0x640000080a50 "\n "
    #text  0x650000080ff0 "\n"

The HTML and the XML parsers create a Text element after each SVG element. In the XML code path and when we start processing a new tag, we append the buffered text to the leaf text element which is XMLDocumentParser::m_leafTextNode, see XMLDocumentParser::exitText().  This function causes the following stack of calls:

#0	WebCore::SVGElement::childrenChanged() 
#1	WebCore::CharacterData::dispatchModifiedEvent() 
#2	WebCore::CharacterData::setDataAndUpdate()
#3	WebCore::CharacterData::appendData() 
#4	WebCore::XMLDocumentParser::exitText() 
#5	WebCore::XMLDocumentParser::startElementNs()

WebCore::CharacterData::dispatchModifiedEvent() sends the source of change as ContainerNode::ChildChangeSourceAPI to SVGElement::childrenChanged() which considers this as a legitimate reason to invalidate the referencing elements because it assumes the children were changed by something but not the parser. This might be a bug but it should not be specific to the SVG shadow tree invalidate/update.

> 
> > Source/WebCore/svg/SVGElement.cpp:1099
> > +    if (hasID() || change.source != ChildChangeSourceParser)
> > +        SVGElementInstance::invalidateAllInstancesOfElement(this);
> 
> Ok, so if I understand this part and the comment above, we are invalidating
> more often. Every time a child changes and the element has an id we always
> mark all instance for invalidation and rebuild the trees, right?  To your
> comment above, this is one of the critical code paths. <use> element is
> specifically slow in WebKit.
> 
> If there is a difference between XML and HTML on characterData, why do we
> fix the issue here?
> 

Fixed: I moved the invalidation to SVGElement::finishParsingChildren() which gets called when all the children are parsed. And for the difference between HTML and XML, I explained above why the bug was not repro in XML code path which, I think, happened accidentally. Although we are doing more invalidation for the XML with this change than we do for the HTML, I think this is the right place to have the invalidation. We should fix the XML Text element issue to not cause any update. But I think this problem should be addressed in a separate bug.

> > LayoutTests/svg/in-html/defs-after-use-expected.html:4
> > +    <g transform="translate(10,10)">
> 
> Is the transform needed? just putting x="10" y="10" should work for
> rectangles. (There might be AA issues on circles and curves though.)
> 

Done.

> > LayoutTests/svg/in-html/defs-after-use.html:16
> > +    <defs>
> > +      <g id="red-rect">
> > +        <rect x="0" y="0" width="100" height="50" fill="red"/>
> > +      </g>
> > +    </defs>
> > +    <use x="10" y="10" xlink:href="#red-rect"/>
> 
> IMO this can just use the rect directly. If that actual test works we don't
> know if this referencing works. The second <use> overdraws this one.

Done. But I changed the test a little to include two <use> elements:
1st: references two <rect>s, the first <rect> comes before the user and the second <rect> comes after the <use>.
2nd: references two <rect>s, the two <rect>s come after the <use>.

[Said Abou-Hallawa]

*** Bug 105257 has been marked as a duplicate of this bug. ***

[Said Abou-Hallawa]

*** Bug 136128 has been marked as a duplicate of this bug. ***

[WebKit Commit Bot]

Comment on attachment 243209 [details]
Patch

Clearing flags on attachment: 243209

Committed r177576: <http://trac.webkit.org/changeset/177576>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.