Currently, the HTML5 parser doesn't dispatch the SVG load event. Make the HTML5 parser dispatch the SVG load event upon </svg> like the XML content sink, except make the event async in text/html.

Waiting for the SVG WG to respond if they change the load event to be async or if they remove the whole thing.

I reached my timeout on waiting for the SVG WG to say something about this. Notes: * To avoid scripts running at non-</script> points when the tree op executor is performing tree operations, the event fires async from a runnable. I think we should change the XML side to fire async, too. AFAICT, Opera already fires the event async. * Just like the XML content sink, this implementation makes no effort to wait for external resources to load. * It seems to me that the check for the presence of the onload attribute and the MOZ_SMIL check for Tag() are bogus. There should probably be a follow-up bug for removing the check from both the XML and HTML code paths.

Oops. Here's the right version of the patch.

Since dispatching the SVG load event seems to have relevance to SMIL-in-SVG support, marking blocking1.9.3?

I would go the other way here. Unless we hear back from the WG that they made an explicit decision to keep this event, we should remove it. As implemented right now it's both buggy (since it doesn't wait for external resources, and since it only fires under certain conditions) as well as useless (since it doesn't wait for external resources). If implemented correctly this would be a big performance issue since for the event could fire thousands of times when loading a large SVG document.

Not blocking per Jonas.

(In reply to comment #5) > I would go the other way here. Unless we hear back from the WG that they made > an explicit decision to keep this event, we should remove it. As implemented > right now it's both buggy (since it doesn't wait for external resources, and > since it only fires under certain conditions) as well as useless (since it > doesn't wait for external resources). > > If implemented correctly this would be a big performance issue since for the > event could fire thousands of times when loading a large SVG document. Yeah, we certainly shouldn't implement the event on all SVG elements. (I hadn't event realized that the spec was so bad that it fired the event on elements other than <svg>. I had mis-read the XML content sink. Now the #ifdef MOZ_SMIL makes sense to me.) dholbert, Is it still necessary to fire the SVG load event on <svg> to make SMIL time bases work? The code and spec suggest the answer is affirmative, in which case I suggest making both the HTML5 parser and the XML content sink fire the event asynchronously and unconditionally on <svg> elements upon hitting the end tag for the element and not firing it on other elements.

(In reply to comment #7) > dholbert, Is it still necessary to fire the SVG load event on <svg> to make > SMIL time bases work? Yes -- the SVGLoad event defines "time 0" of the SMIL document timeline. We key off of it to start keeping track of time, here... http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGSVGElement.cpp#947 ...and scripts can key off of that event (or just use <svg onload="go()">) to schedule work to be done when the SMIL timeline begins (at which point svgRootElem.getCurrentTime() is guaranteed to return 0). > I suggest making both the HTML5 parser and the XML content sink fire > the event asynchronously and unconditionally on <svg> elements upon hitting the > end tag for the element and not firing it on other elements. That sounds reasonable to me.

(In reply to comment #8) > ...and scripts can key off of that event (or just use <svg onload="go()">) to > schedule work to be done when the SMIL timeline begins (at which point > svgRootElem.getCurrentTime() is guaranteed to return 0 ... and svgRootElem.animationsPaused() is guaranteed to return false. In other words, this provides a way for script to interact with the document at the instant that the SMIL timeline begins. (so they can e.g. pause or seek the timeline before any animations have taken effect)

dholbert: Would it make sense to start all animations from the document 'load' event rather than from the individual svg-fragments load event? Or is the latter explicitly intended? One advantage would be that animations in all fragments would be synchronized.

Possibly, but it would certainly be a change from our existing behavior. We currently have tests that assume the <svg> onload handlers see the timeline unpaused & at time 0, per comment 9 (and that wouldn't hold anymore, if we changed per comment 10). That change would also be awkward in cases where an SVG-fragment is generated entirely from scratch, *after* document load-time, and then added to the document -- e.g. http://brian.sol1.net/svg/tests/deferred-tree.xhtml. We currently start the fragment's timeline when it gets bound to our tree (or at least right around that time), but that might make less sense if we were synchronizing all the other svg-fragments with our document's load event. The current behavior was decided on & added to the SMIL patch well before I started working on it, so birtles/roc/jwatt may have better insight into the need for the existing behavior & any pitfalls (or lack thereof?) of potential behavior-changes like comment 10...

I think it's important for the animation start and SVGLoad event to be synchronised. I imagine a lot of content will rely on that behaviour as it's pretty clear in the spec that's what should happen. However, if you wanted to synchronise all the SVGLoad events with the document load event then that might be acceptable (excepting of course the situations Daniel mentioned in comment 11 about SVG document fragments generated entirely from script which we certainly want to support). Basically, SVG document fragments are defined as separate time containers in SVG/SMIL. So they can be paused and seeked independently and there's no requirement for them to be synchronised. It's currently possible to synchronise them using syncbase timing as we allow for cross-time container dependencies. However such synchronisation is at the element level not time-container level. It would probably be useful to be able to synchronise time containers as well but that sounds like implementing SMIL proper and not just SVG's subset (SMIL Animation). Regarding external resources, we have bug 277955 for implementing the externalResourcesRequired attribute but I don't know of any intention for fixing that. That attribute determines both when the SVGLoad event is dispatched and when animations start--again suggesting SVGLoad and animation start must be synchronised.

I think changing SMIL animations to use the document's load event is way out of scope of this bug. This patch: 1) Adds the SVG load event as async to <svg> in text/html. 2) Makes the SVG load event async in XML. 3) Removes the SVG load event from non-<svg> elements in XML.

On the Mac tryserver, the patch makes the assertion ###!!! ASSERTION: More UnblockOnload() calls than BlockOnload() calls; dropping call: 'Not Reached', file /opt/Projects/mozilla-html5/content/base/src/nsDocument.cpp, line 7044 go away from editor/composer/src/crashtests/removing-editable-xslt.html . I don't see why. On multiple tryserver platforms, the patch makes content/svg/content/src/crashtests/535691-1.svg assert twice, but I don't see the problem locally and the tryserver logs don't tell me which assertion fired. :-(

(In reply to comment #14) > On multiple tryserver platforms, the patch makes > content/svg/content/src/crashtests/535691-1.svg assert twice, but I don't see > the problem locally and the tryserver logs don't tell me which assertion fired. > :-( Actually the logs do show the reason. The patch for this bug regresses bug 535691 itself but only on the tryserver--not locally for me.

Comment on attachment 448353 [details] [diff] [review] Unify text/html and XML SVG load: only on <svg>, always on <svg>, async, v2 Putting this in the review pipeline anyway with the assumption that bug 535691 will be addressed by another patch.

Comment on attachment 448353 [details] [diff] [review] Unify text/html and XML SVG load: only on <svg>, always on <svg>, async, v2 >--- /dev/null >+++ b/parser/html/nsHtml5SvgLoadDispatcher.cpp Note that our SVG code always uses "SVG", not "Svg", in class names & filenames -- this added file would be the only file in mozilla-central to have "Svg" in its name. On the other hand, it looks like /parser/html/ never uses all-caps for acronyms (e.g. "Html5" instead of "HTML5"). So "Svg" is probably fine here, since it matches the more local style... >+++ b/parser/html/nsHtml5TreeBuilderCppSupplement.h >+#ifdef MOZ_SVG >+ if (aName == nsHtml5Atoms::svg) { >+ nsHtml5TreeOperation* treeOp = mOpQueue.AppendElement(); >+ NS_ASSERTION(treeOp, "Tree op allocation failed."); >+ treeOp->Init(eTreeOpSvgLoad, aElement); >+ return; > } > #endif >- // TODO soft flush the op queue every now and then > return; > } We don't actually need that added "return" statement, right? (after treeOp->Init) Without it, we'll just fall down the lower return statement when we exit the clause anyway. >+++ b/parser/html/nsHtml5TreeOperation.h >@@ -80,7 +80,8 @@ > eTreeOpSetScriptLineNumberAndFreeze, >- eTreeOpStartLayout >+ eTreeOpStartLayout, >+ eTreeOpSvgLoad The line for "eTreeOpSvgLoad" here should probably be wrapped in #ifdef MOZ_SVG, for consistency, and because we'll only need/use it in SVG-enabled builds. (I think the comma before it will need to be inside of the #ifdef, too, or else we'll get a compile warning in non-SVG builds for "comma at the end of an enum list".) >+++ b/parser/htmlparser/tests/mochitest/test_bug552938.html >+ <svg onload="svgLoadFired = true;"></svg> For completeness, you should probably add a test that listens specifically for the "SVGLoad" event. Something like: <!-- inside of an html5 document --> <svg> <script> var mySVGParent = /* get my <svg> parent */; mySVGParent.addEventListener("SVGLoad", function() { svgLoadFired = true; }) </script> </svg> r=dholbert with the above. (One of [jwatt,longsonr,roc] should probably also look at this, though.)

(In reply to comment #17) > (From update of attachment 448353 [details] [diff] [review]) > >--- /dev/null > >+++ b/parser/html/nsHtml5SvgLoadDispatcher.cpp > > Note that our SVG code always uses "SVG", not "Svg", in class names & filenames > -- this added file would be the only file in mozilla-central to have "Svg" in > its name. > > On the other hand, it looks like /parser/html/ never uses all-caps for acronyms > (e.g. "Html5" instead of "HTML5"). So "Svg" is probably fine here, since it > matches the more local style... I changed it to SVG, since the name leaks out of parser/html/ (where the naming is inconsistent with the rest of Gecko due to the parser core having been developed originally independently of Gecko). > >+++ b/parser/html/nsHtml5TreeBuilderCppSupplement.h > >+#ifdef MOZ_SVG > >+ if (aName == nsHtml5Atoms::svg) { > >+ nsHtml5TreeOperation* treeOp = mOpQueue.AppendElement(); > >+ NS_ASSERTION(treeOp, "Tree op allocation failed."); > >+ treeOp->Init(eTreeOpSvgLoad, aElement); > >+ return; > > } > > #endif > >- // TODO soft flush the op queue every now and then > > return; > > } > > We don't actually need that added "return" statement, right? (after > treeOp->Init) Without it, we'll just fall down the lower return statement when > we exit the clause anyway. The return is currently useless. I put it there to make the code less prone to bugs if/when edited. I removed the return. > >+++ b/parser/html/nsHtml5TreeOperation.h > >@@ -80,7 +80,8 @@ > > eTreeOpSetScriptLineNumberAndFreeze, > >- eTreeOpStartLayout > >+ eTreeOpStartLayout, > >+ eTreeOpSvgLoad > > The line for "eTreeOpSvgLoad" here should probably be wrapped in #ifdef > MOZ_SVG, for consistency, and because we'll only need/use it in SVG-enabled > builds. > > (I think the comma before it will need to be inside of the #ifdef, too, or else > we'll get a compile warning in non-SVG builds for "comma at the end of an enum > list".) I added the #ifdef. I avoided the comma issue by the new tree op not be the last item in the enum. > >+++ b/parser/htmlparser/tests/mochitest/test_bug552938.html > >+ <svg onload="svgLoadFired = true;"></svg> > > For completeness, you should probably add a test that listens specifically for > the "SVGLoad" event. Something like: > <!-- inside of an html5 document --> > <svg> > <script> > var mySVGParent = /* get my <svg> parent */; > mySVGParent.addEventListener("SVGLoad", function() { svgLoadFired = true; > }) > </script> > </svg> I added another test with the suggested code. > r=dholbert with the above. (One of [jwatt,longsonr,roc] should probably also > look at this, though.) Thanks. Requesting sr from roc.

Thanks. Landed: http://hg.mozilla.org/mozilla-central/rev/a8ac411e1653

Backed out, for causing randomorange Bug 571016 & Bug 571036. http://hg.mozilla.org/mozilla-central/rev/42a58530d419 I think both randomoranges are due to the fact that this patch changed SVGLoad to fire later, as I described in bug 571016 comment 3. Henri, can you fix nsXMLContentSink.cpp in this patch to match the old behavior? (i.e. you could probably just call event->Run() instead of NS_DispatchToMainThread(event), though there may be a cleaner / more lightweight way)

This also caused bug 571134

(In reply to comment #7) > > Yeah, we certainly shouldn't implement the event on all SVG elements. (I hadn't > event realized that the spec was so bad that it fired the event on elements > other than <svg>. I had mis-read the XML content sink. Now the #ifdef MOZ_SMIL > makes sense to me.) > Can you take this up with the w3c SVG working group please? The specification and test suite all say that load works on more than just <svg>

Maybe we could consider dispatching the load event to svg elements and to any elements that have an onload attribute?

(In reply to comment #22) > Can you take this up with the w3c SVG working group please? The specification > and test suite all say that load works on more than just <svg> OK, but it may still be necessary to draw our own conclusions in time to get sufficient beta testing. My November 2009 email about the load event hasn't yet resulted in reportable WG deliberation. That is, I haven't gotten any email back about the WG having discussed this. (In reply to comment #23) > Maybe we could consider dispatching the load event to svg elements and to any > elements that have an onload attribute? That's how the XML content sink used to be, but it seems pretty bogus to tie the firing of the event to an attribute when addEventListener exists. There are three possible points in time when the existence of the attribute could be checked: 1) When the start tag is parsed. This would require the HTML and XML parsers have additional bookkeeping that doesn't exist now. 2) When the end tag is parsed. In practice, this would mean enqueuing a tree op for each SVG-namespace end tag. I don't know what the perf cost would be for adding ops for all SVG-namespace end tags in the document, but it doesn't seem ideal to have such a ballooning of the quantity of ops. 3) When an async runnable for the event fires. Again, I don't know what the perf cost would be for one runnable per SVG-namespace end tag, but sicking said "big performance issue" in comment #5. Furthermore, the SVG WG as of 2000-2003 putting something in the spec is not a good reason to implement it. (After all, the SVG WG as of 2004 put XML Events and textArea in the spec...) Having a test suite test isn't a good reason, either. I think a relevant question is if there are good use cases for the load event on arbitrary SVG elements. (I concede that on the <svg> element, it's no longer worthwhile to try to get rid of the SVG load event, since SMIL is tied to it.) Since Gecko doesn't actually wait for subresource loads, I don't see a use case for the load event that wouldn't be equally addressed by placing a <script> element right after the end tag that'd trigger the load event. That is, instead of <g onload='doStuff();'> ... </g> the author could say <g> ... </g><script>doStuff();</script> On the other hand, if we ever want to make the SVG load event wait for subresource loads, I think it becomes even more important. If the event waited for subresource loads but fired sync, we'd have a new case where the parser gets blocked. Just for general complexity avoidance, I think new cases where the parser gets blocked should be avoided. I think the parser shouldn't be able to get blocked after any SVG element. It's bad enough that it can get blocked at </script>. document.write() makes things even worse, and in HTML5, you can call document.write() to inject an SVG fragment into the document. I *really* don't want new cases of the parser getting blocked in the middle of document.write(), because it's so easy to end up with the return condition of document.write() being racy with the network. See http://www.w3.org/Bugs/Public/show_bug.cgi?id=9843 for the pre-existing trouble. Thus, I have really hard time seeing justification for a sync load event on all SVG elements. If the event doesn't wait for subresources, it provides no value over </g><script>doStuff();</script> (AFAICT). If it does wait for subresources, making it sync is trouble, especially with document.write(). As for the load event on <svg> being sync or async, I would to make it async in order to avoid having to put nsHtml5TreeOpExecutor into a state where it's safe to run scripts in other situations than when dealing with </script>. (Earlier on IRC, I said this was about document.write(), but now I recalled there are other issues with the state of doc update batching.) If we migrate the XML side off-the-main-thread in the future or otherwise unify the XML code path with the tree op mechanism (e.g. to fix bug 483780), this concern would apply to XML, too. That's why it seems better to make the event async on the XML side, too, sooner than later to avoid getting into a situation where SVG takes off on the Web and a lot of content depends on the event being sync. Also, it seems that Opera fires the load event on <svg> async. See http://canvex.lazyilluminati.com/misc/dom-viewer/x.html?%3Chtml%20xmlns%3D%27http%3A%2F%2Fwww.w3.org%2F1999%2Fxhtml%27%3E%3Cscript%3EloadRan%3Dfalse%3C%2Fscript%3E%3Csvg%20xmlns%3D%27http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%27%20onload%3D%27loadRan%3Dtrue%27%2F%3E%3Cscript%3Ealert%28loadRan%3F%27sync%27%3A%27async%27%29%3C%2Fscript%3E%3C%2Fhtml%3E

(In reply to comment #24) > That's how the XML content sink used to be, but it seems pretty bogus to tie > the firing of the event to an attribute when addEventListener exists. Not quite I think. in Firefox 1.5-3.6 it dispatched to everything with an onload (which was wrong as you could not attach an event handler without the element having an onload attribute). Without this fix the current trunk dispatches to everything if SMIL is enabled which, as you say may be a bit much. So to move forward you could remove the dynamic-use reftest that fails, raise the "svg only for load events" to the SVG WG (again?), WONTFIX the svg test suite bug I raised and figure out why the smil mochitest fails and what to do about it.

And get something added to the dev documentation to note the change.

The problem is that even if we check for addEventListener-added listeners, and not just onload attributes, then we still won't always fire when needed. What we need to do is to check all the way up the parent chain to see if there are any listeners. And doing that for each svg-namespaced element would be very expensive. We could do magic tricks with nsINode flags etc, similar to what we do for mutation events, but even that is somewhat of a cost, and very complex. Not to to mention if someone attaches a listener to the document node, in which case we'd always fire the event. Which would be very expensive (event-firing is not cheap). So to sum up: * If this feature is used, it makes SVG parsing very slow. * If it isn't used, it only slows down parsing a little, but adds a lot of complexity and is by definition useless (since it isn't used). Doesn't sound like a good feature to me.

I think we're all on the same page. I'm just suggesting we ought to propose to the w3c svg wg (again) that they change the SVG specification and SVG test suite. I've never seen onload used on anything other than an svg element so it's unlikely we'd even break any of the rather small amount of SVG that there is out there.

Given that no-one replied the first time Henri raised this issue, I think a good way to get their attention is to change our code at the same time as we raise the issue again. That forces action on their part if they really want to keep the event.

Agreed.

Differences compared to the previous version: * Use reftest-wait in the random orange reftest. * Add an explicit svg load / page load order check to the random orange mochitest. * Make the code block/unblock onload.

(In reply to comment #31) > * Use reftest-wait in the random orange reftest. As described in bug 571036 comment 18 and bug 571036 comment 27, that doesn't actually fix anything. (We tried it already :) ). That reftest is kind of broken -- the thing that it tests is still broken, and it only passes if the onload handler fires synchronously. (see also the last few comments on bug 467498, which added the reftest) If anything, the test should be marked as random or removed.

The failure of test_smilReset.xhtml was caused by the lazy initialization of the animation controller of the document. Forcing initialization in this patch seems to fix the problem. Forcing initialization like this probably defeats the purpose of initializing the animation controller lazily, though. Unfortunately, I don't know the SMIL code well enough to be able to come up with a better but still reliable place where to initialize the animation controller. I'd appreciate advice. (Not requesting review yet, since this hasn't been through the tryserver, yet.)

(In reply to comment #33) > The failure of test_smilReset.xhtml was caused by the lazy initialization of > the animation controller of the document. Forcing initialization in this patch > seems to fix the problem. Only locally, it appears. Still fails on try. :-(

Fails on all platforms or just some?

(In reply to comment #35) > Fails on all platforms or just some? In that particular tryserver run, it failed on all platforms in debug builds. Linux opt failed but OS X and Windows opt builds passed the test.

Looks like following the discussion at the end of bug 515116 may be productive. jwatt seems to be encountering the same issue.

(In reply to comment #20) > Backed out, for causing randomorange Bug 571016 & Bug 571036. > http://hg.mozilla.org/mozilla-central/rev/42a58530d419 Per bug 571016 comment 14, I think we can just add "svg.setCurrentTime(0);" to the test_smilReset mochitest -- that should work around the problem in that mochitest. (and the reftest should be marked as random, per comment 32) With those test-changes, I think you should be able to roll back to the (un-bitrotted) patch that you previously landed, unless there was anything from one of the later patches that you'd like to keep. I'll file a bug on the underlying SMIL issue that caused the mochitest problem (described in more detail in bug 571016 comment 14).

Updated patch per dholbert's advice.

Comment on attachment 457825 [details] [diff] [review] Unify text/html and XML SVG load: only on <svg>, always on <svg>, async, v6 [sr=roc] Cool -- so this is the same as what landed in comment 19, plus: - the mochitest tweak & reftest random-marking - the change to block the document's load event from firing until after SVGLoad has fired. Looks great to me. >diff --git a/layout/reftests/svg/reftest.list b/layout/reftests/svg/reftest.list >-== dynamic-use-nested-01.svg dynamic-use-nested-01-ref.svg >+random == dynamic-use-nested-01.svg dynamic-use-nested-01-ref.svg Can you add a "# bug 467498" notation at the end of that line? (That's where most of the discussion has gone on about why the test is broken.) r=dholbert with that.

Comment on attachment 457825 [details] [diff] [review] Unify text/html and XML SVG load: only on <svg>, always on <svg>, async, v6 [sr=roc] (I think you can carry forward roc's earlier sr, since the new patch is the same as the already-sr'd one, aside from the minor changes noted in comment 40.) Landed new patch: http://hg.mozilla.org/mozilla-central/rev/d03339731d61