Closed
Bug 467498
Opened 16 years ago
Closed 5 years ago
Second level of <svg:use> is not live to changes
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
DUPLICATE
of bug 1032124
People
(Reporter: jruderman, Unassigned)
References
Details
(Keywords: testcase)
Attachments
(3 files, 2 obsolete files)
The three stripes (blue, green, and red) should all extend to the bottom. Based on layout/reftests/svg/use-01-extref.svg, but all references in the reduced testcase are internal. (Is that the only reftest that relies on multiple levels of <svg:use>?)
Reporter | ||
Comment 1•16 years ago
|
||
Comment 2•16 years ago
|
||
I suspect it's the only <use>-within-<use> reftest, yes.
Comment 3•16 years ago
|
||
So the way <use> handles mutations is by setting itself up as the listener on the used node. So if we have <use xlink:href="#x"/> <svg id="x"><use xlink:href="#y"/></svg> And we change an attribute on a kid of id="y", then the second ("inner") <use> will get notified and reclone. But that reclone fires no mutation notificatons, so there's no notification dispatched to the first (outer) <use>. Ideally, any time a <use> element reclones all <use> elements that have it as mOriginal will also reclone, no? That should fix this bug, I would think... Is that a reasonable approach?
Why would that fix this bug? The first <use> does not point directly to the second <use> in your example. Why doesn't reclone fire a mutation observer? Is it because the cloned nodes are anonymous? I think even changes to anonymous children should cause <use> observers to reclone (and the anonymous children should be cloned, too).
Comment 5•16 years ago
|
||
The first use doesn't point to the second use, but the clone of the second use that's in the first use's cloned tree does point to the second use, and if it recloned when the second use reclones that would fix the bug, no? Recloning doesn't trigger mutation events because it just uses nsIAnonymousContentCreator, yeah. Cloning doesn't explicitly clone the anonymous kids right now. It just clones the other <use>, which then creates its own anonymous kids when its frame is constructed....
Right, OK.
Comment 7•14 years ago
|
||
Testcase seems to work on trunk now.
Reporter | ||
Updated•14 years ago
|
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WORKSFORME
Reporter | ||
Comment 8•14 years ago
|
||
Reftest: http://hg.mozilla.org/mozilla-central/rev/2f0055db840c
Flags: in-testsuite+
Comment 9•14 years ago
|
||
This issue is actually still broken -- we only get lucky on the testcase posted here because it does all the JS work before our first reflow, I think. If you change the onload handler to use a setTimeout (particularly with a timeout-length longer than a few milliseconds), then it re-breaks the test. Reopening.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Comment 10•14 years ago
|
||
here's the testcase with a setTimeout added in the onload handler. It's broken on every single load for me, in both today's nightly and yesterday's nightly. (yesterday's nightly is relevant because it's from before this reftest started sporadically failing, as tracked in bug 571036)
Comment 11•14 years ago
|
||
I think we should back out the testcase in the tree since it isn't testing what we think it should.
Comment 12•14 years ago
|
||
(In reply to comment #11) Well, the test *is* testing *something* that didn't used to work but now does work. It would be useful to know if that part of this issue regresses. As I noted in comment 9, the test does currently only pass if the dynamic modifications occurring synchronously -- we could preserve that synchronicity (after onload becomes async in bug 552938) by using an inline <script> tag towards the end of the document instead of an onload handler, right?
Comment 13•12 years ago
|
||
So if we have use1->use2->content and the content changes we're going to get use2 to send a modification event as if it was itself modified that use1 picks up using its existing observer infrastructure.
Assignee: nobody → longsonr
Attachment #634014 -
Flags: review?(dholbert)
Updated•12 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Comment 14•12 years ago
|
||
Please ignore the layout/svg/base/src/nsSVGUseFrame.cpp change I don't need that, the reclone is enough to cause a complete refresh.
Comment 15•12 years ago
|
||
Comment on attachment 634014 [details] [diff] [review] patch >diff --git a/content/svg/content/src/nsSVGSVGElement.h b/content/svg/content/src/nsSVGSVGElement.h >--- a/content/svg/content/src/nsSVGSVGElement.h >+++ b/content/svg/content/src/nsSVGSVGElement.h >@@ -168,18 +168,16 @@ public: >- // Copy our width or height to the target >- void SyncWidthOrHeight(nsIAtom* aName, nsSVGElement *aTarget) const; I'm confused... nsSVGUseFrame.cpp still calls this method, like so: > 128 static_cast<nsSVGUseElement*>(mContent)->SyncWidthOrHeight(aAttribute); Is that call no longer needed? I'd guess that this patch wouldn't compile as long as that line remains. (Also: it looks like we don't have any remaining callers for the other "SyncWidthOrHeight" impl, which lives on nsSVGSVGElement. Somewhat unrelated, but it looks like that impl can go away too?)
Comment 16•12 years ago
|
||
We're keeping the still-used nsSVGUseElement::SyncWidthOrHeight implemented but we can ditch the unused nsSVGSVGElement::SyncWidthOrHeight
Comment 17•12 years ago
|
||
Oh, of course -- sorry, I glossed over the filename and assumed the removed chunk was from nsSVGUseElement. Sorry / thanks for the correction. :)
Comment 18•12 years ago
|
||
And one reason for removing the unused method is that it's quite confusing ;-)
Comment 19•12 years ago
|
||
Comment on attachment 634014 [details] [diff] [review] patch Yup. :) Thanks for that. So just to make sure I understand: this works because our TriggerReclone() makes us call "nsSVGUseElement::AttributeChanged()" on any <use> elements that target us, which hits their own "TriggerReclone()" method (etc.)? Assuming I'm understanding that correctly -- does this cause problems if we have a <use> cycle, like so: <use id="a" xlink:href="#b"/> <use id="b" xlink:href="#a"/> <script> // Some script that tweaks "a" and causes a TriggerReclone call </script> It seems like this could cause endless ping-ponging. Does it? (Even if it we're safe against that for some reason, I'd still like this patch to include a test like that -- either as a crashtest or a trivial reftest of some sort -- so that a future patch won't combine with this to accidentally open ourselves up to that in the future.)
Comment 20•12 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #19) > > So just to make sure I understand: this works because our TriggerReclone() > makes us call "nsSVGUseElement::AttributeChanged()" on any <use> elements > that target us, which hits their own "TriggerReclone()" method (etc.)? use1->use2->content use2's nsSVGUseElement::AttributeChanged picks up a change in content then calls TriggerReclone which now calls nsNodeUtils::AttributeChanged this is picked up by use1's nsSVGUseElement::AttributeChanged as an attribute change and that reclones it's contents which is use2. > > Assuming I'm understanding that correctly -- does this cause problems if we > have a <use> cycle, like so: > > <use id="a" xlink:href="#b"/> > <use id="b" xlink:href="#a"/> > <script> > // Some script that tweaks "a" and causes a TriggerReclone call > </script> > > It seems like this could cause endless ping-ponging. Does it? nsSVGUseElement::CreateAnonymousContent stops this as it checks for loops and refuses to do carry on cloning. > > (Even if it we're safe against that for some reason, I'd still like this > patch to include a test like that -- either as a crashtest or a trivial > reftest of some sort -- so that a future patch won't combine with this to > accidentally open ourselves up to that in the future.) I can turn your snippet into a crashtest.
Comment 21•12 years ago
|
||
Comment on attachment 634014 [details] [diff] [review] patch (In reply to Robert Longson from comment #20) > nsSVGUseElement::CreateAnonymousContent stops this as it checks for loops > and refuses to do carry on cloning. Awesome. I thought we must already have protection against that... Glad we do. > I can turn your snippet into a crashtest. That would be awesome, thanks! r=me with that.
Attachment #634014 -
Flags: review?(dholbert) → review+
Comment 22•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce4dbc71a506
Target Milestone: --- → mozilla16
Comment 23•12 years ago
|
||
backed out the change that fixes this bug as content/svg/content/src/crashtests/405639-1.svg intermittently fails on Windows.
Target Milestone: mozilla16 → ---
Comment 24•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd3a8bc49325 is the backout
Whiteboard: [leave open] fix backed out
Comment 25•12 years ago
|
||
Here are some logs of the crashes in 405639-1.svg, for reference: WinOpt: https://tbpl.mozilla.org/php/getParsedLog.php?id=12795462&tree=Mozilla-Inbound WinDbg: https://tbpl.mozilla.org/php/getParsedLog.php?id=12795487&tree=Mozilla-Inbound OSX Dbg: https://tbpl.mozilla.org/php/getParsedLog.php?id=12795446&tree=Mozilla-Inbound
Comment 26•12 years ago
|
||
It looks like the crashtest failure is something like the ping-ponging that I mentioned in comment 19, BTW -- at least, one of the full logs from my previous comment has a 1024-level backtrace that looks like: > 1017 xul.dll!nsSVGUseElement::TriggerReclone() [nsSVGUseElement.cpp:16b70566af66 : 445 + 0xf] > 1018 xul.dll!nsSVGUseElement::AttributeChanged(nsIDocument *,mozilla::dom::Element *,int,nsIAtom *,int) [nsSVGUseElement.cpp:16b70566af66 : 182 + 0x6] > 1019 xul.dll!nsNodeUtils::AttributeChanged(mozilla::dom::Element *,int,nsIAtom *,int) [nsNodeUtils.cpp:16b70566af66 : 108 + 0x77] > 1020 xul.dll!nsSVGUseElement::TriggerReclone() [nsSVGUseElement.cpp:16b70566af66 : 445 + 0xf] > 1021 xul.dll!nsSVGUseElement::AttributeChanged(nsIDocument *,mozilla::dom::Element *,int,nsIAtom *,int) [nsSVGUseElement.cpp:16b70566af66 : 182 + 0x6] > 1022 xul.dll!nsNodeUtils::AttributeChanged(mozilla::dom::Element *,int,nsIAtom *,int) [nsNodeUtils.cpp:16b70566af66 : 108 + 0x77] > 1023 xul.dll!nsSVGUseElement::TriggerReclone() [nsSVGUseElement.cpp:16b70566af66 : 445 + 0xf] > 1024 xul.dll!nsSVGUseElement::AttributeChanged(nsIDocument *,mozilla::dom::Element *,int,nsIAtom *,int) [nsSVGUseElement.cpp:16b70566af66 : 182 + 0x6] Interestingly, all of the TriggerReclone()/AttributeChanged() calls are for the same element (at address 16b70566af66) -- it must be registering as its own listener somehow, I guess?
Comment 27•12 years ago
|
||
Comment 28•12 years ago
|
||
I've sent this to try. The idea is to send the attribute changed later and only when the listener for updates to the current use element are disabled.
Updated•12 years ago
|
Whiteboard: [leave open] fix backed out
Updated•12 years ago
|
Assignee: longsonr → nobody
Comment 31•10 years ago
|
||
Wow... i accidentally made a dup of this. Disappointed to see it is now 6 years old. Seeing as it looks like this is not going to be fixed any time soon, i was forced to come up with a generalised workaround, for others who are interested: It involves triggering redraws by modifying an attribute in any way on affected use elements. You could be clever about this and walk the references recursively picking up all the elements related to the target you are modifying but the overhead likely exceeds the simplicity of doing a basic global 'use' query selection. Ether way you should only do your query when new use elements might be injected into the DOM, then save the reference to the use elements for updating in your animation step. I've put together a fix at the gist bellow... it gives you a constructor that returns an object with two methods: "gather" and "redraw" gather does your query selection, redraw is what you want to call in your animation draw steps. It takes two arguments "root" and "auto" If your feeling lazy and want a quick drop in fix then set the auto argument to true, that will set up a redraw loop for you... for performance reasons this does not also call the gather method automatically. Ideally you should call the redraw method in your animation step callback. SvgUseFix: https://gist.github.com/anonymous/d006a644618033ffda2b Example: http://jsbin.com/piricejuvo/5/edit
Comment 33•9 years ago
|
||
Robert, how do you feel about picking this bug up again? Do you still have the patch that you sent to try in comment 28?
Flags: needinfo?(longsonr)
Comment 34•9 years ago
|
||
It caused hangs due to infinite loops. I'd have to figure out how to resolve that. I'd be more inclined to fix use not to clone which would likely fix this bug as fall out. I almost had that implemented but display list conversion broke it.
Flags: needinfo?(longsonr)
Comment 35•9 years ago
|
||
(In reply to Robert Longson from comment #34) > I almost had that implemented but display list > conversion broke it. Are those patches in a bug somewhere? Maybe I can take it over the finish line.
Updated•9 years ago
|
Flags: needinfo?(longsonr)
Updated•5 years ago
|
Status: REOPENED → RESOLVED
Closed: 14 years ago → 5 years ago
Resolution: --- → DUPLICATE
Updated•5 years ago
|
Attachment #634014 -
Attachment is obsolete: true
Updated•5 years ago
|
Attachment #634959 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•