Bug 25401 – <use> of text not displayed unless declared after

• Component: SVG
• Status: RESOLVED FIXED

[Jeff Schiller]

Attached test case shows a bug in Webkit (Safari 4).  The <use> points at a <text> element that is defined later in the document.  Webkit does not dislay the black outline text behind the orange.

NOTE that if you put the <use> element after the <text> it shows up.

[Jeff Schiller]

Created attachment 29793 [details]
Test Case showing the problem

[Rob Buis]

Created attachment 32278 [details]
First attempt

The default SVGElement:insertedIntoDocument is not enough in this case, since the text content may not have been appended to the <text> element yet, therefore do
something extra in childrenChanged.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 32278 [details]
First attempt

Isn't:
8     // Invalidate all SVGElementInstances associated with us
 139     SVGElementInstance::invalidateAllInstancesOfElement(this);
needed in the root SVGElement childrenChanged method?  How do we handle updating <use> elements when other subtrees change?

[Rob Buis]

Hi Eric,

(In reply to comment #3)
> (From update of attachment 32278 [details])
> Isn't:
> 8     // Invalidate all SVGElementInstances associated with us
>  139     SVGElementInstance:: invalidateAllInstancesOfElement(this);
> needed in the root SVGElement childrenChanged method?  How do we handle
> updating <use> elements when other subtrees change?

I think <use> only references styled elements, so the hooks are only there, see
SVGStyledElement::childrenChanged and SVGStyledElement::svgAttributeChanged. However
that childrenChanged only calls invalidateAllInstancesOfElement after parsing is over, so that
is not enough in the use forward references to text, hence my patch overwriting it. Can you think of scenarios/testcases where the patch fails? Any other forward referencing cases that fail?
Cheers,

Rob.

[Rob Buis]

Created attachment 33019 [details]
More efificent

This version is slightly more efficient since it skips SVGStyledElement::invalidateAllInstancesOfElement. I still think the patch is correct so putting it up for review again.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 33019 [details]
More efificent

Please make your test dumpAsText, no need to use a full rendering test.

Why shouldn't this just be on SVGElement:
134 void SVGTextElement::childrenChanged(bool changedByParser, Node* beforeChange, Node* afterChange, int childCountDelta)
 135 {
 136     SVGElement::childrenChanged(changedByParser, beforeChange, afterChange, childCountDelta);
 137 
 138     // Invalidate all SVGElementInstances associated with us
 139     SVGElementInstance::invalidateAllInstancesOfElement(this);
 140 }
 141 

How do non-text nodes get around needing to make this call to update <use> stuff?

[Nikolas Zimmermann]

Fixed in ToT, closing bug.