Bug 25401

Summary: <use> of text not displayed unless declared after
Product: WebKit Reporter: Jeff Schiller <jeffschiller>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: rwlbuis, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
Test Case showing the problem
none
First attempt
eric: review-
More efificent eric: review-

Description Jeff Schiller 2009-04-25 19:24:30 PDT
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.
Comment 1 Jeff Schiller 2009-04-25 19:25:02 PDT
Created attachment 29793 [details]
Test Case showing the problem
Comment 2 Rob Buis 2009-07-05 12:30:30 PDT
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.
Comment 3 Eric Seidel (no email) 2009-07-07 00:44:01 PDT
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?
Comment 4 Rob Buis 2009-07-18 01:52:20 PDT
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.
Comment 5 Rob Buis 2009-07-18 01:57:09 PDT
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.
Comment 6 Eric Seidel (no email) 2009-08-07 09:27:55 PDT
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?
Comment 7 Nikolas Zimmermann 2010-01-19 07:57:53 PST
Fixed in ToT, closing bug.