Summary: | <use> of text not displayed unless declared after | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jeff Schiller <jeffschiller> | ||||||||
Component: | SVG | Assignee: | 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
Jeff Schiller
2009-04-25 19:24:30 PDT
Created attachment 29793 [details]
Test Case showing the problem
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 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?
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. 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 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?
Fixed in ToT, closing bug. |