Last Comment Bug 252631 - Allow multiple onloads per document
: Allow multiple onloads per document
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: tor
: Hixie (not reading bugmail)
Mentors:
Depends on:
Blocks: 269881 277955
  Show dependency treegraph
 
Reported: 2004-07-22 08:49 PDT by tor
Modified: 2005-01-26 06:46 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
multiple onload (12.72 KB, patch)
2004-09-17 15:10 PDT, tor
jst: superreview+
Details | Diff | Splinter Review
alternate approach (4.39 KB, patch)
2004-11-11 12:31 PST, tor
jonas: review+
Details | Diff | Splinter Review
update "alternate approach" to tip, apply sicking's eventname change (4.99 KB, patch)
2005-01-10 10:01 PST, tor
jst: superreview+
Details | Diff | Splinter Review

Description tor 2004-07-22 08:49:07 PDT
The patch for bug 242234 which adds svg events currently limits onload
to only one per document, on the root node.

Jonas has some ideas about how this limitation could be lifted.
Comment 1 Jonas Sicking (:sicking) No longer reading bugmail consistently 2004-07-22 15:38:38 PDT
I think the best way to handle onload for svg would probably be to have an
object living somewhere that keeps track of all elements that have onload set.
This object would then register as eventhandler on the windowobject for the
'load' event and just propagate the event too all elements.

This object could probably be managed by the top <svg> element. (or even be that
element itself).

The question is what to do after onload has fired. I can't think of a situation
where onload fires twice so we might be able to just let the object unregister
and die... 
Comment 2 tor 2004-09-17 15:10:21 PDT
Created attachment 159257 [details] [diff] [review]
multiple onload
Comment 3 Jonas Sicking (:sicking) No longer reading bugmail consistently 2004-09-22 12:16:44 PDT
What will happen if several "attribute events handlers" are added and then one
is removed? Will just the right one be removed or will they all?

I.e. if AddScriptEventListener is called several times and then
RemoveScriptEventListener is called once.
Comment 4 tor 2004-09-28 14:11:46 PDT
It appears only one will be removed.
Comment 5 Jonas Sicking (:sicking) No longer reading bugmail consistently 2004-09-28 19:16:50 PDT
Will it be the right one? This can acually happen if you have several elements
with onload attributes and then one attribute is removed before the page finish
loading.
Comment 6 tor 2004-09-29 12:55:17 PDT
No, I think it will always pick the first.

Your suggestion of how to do multiple onloads has its own problems; due
do the way things are parsed, SetAttr is called before the parent of an
element is hooked up, so the outer SVG can't be located.
Comment 7 Jonas Sicking (:sicking) No longer reading bugmail consistently 2004-09-29 16:14:09 PDT
so what do you suggest we do?
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2004-10-06 18:02:12 PDT
Comment on attachment 159257 [details] [diff] [review]
multiple onload

sr=jst
Comment 9 tor 2004-11-11 12:31:29 PST
Created attachment 165603 [details] [diff] [review]
alternate approach

This is a more localized change that gets closer to the specification
than the previous version: onload events are fired at the appropriate
time, the ordering is correct, and the event target is set correctly.
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2004-11-17 16:42:07 PST
(In reply to comment #9)
> Created an attachment (id=165603)
> alternate approach
> 
> This is a more localized change that gets closer to the specification
> than the previous version: onload events are fired at the appropriate
> time, the ordering is correct, and the event target is set correctly.

This seems wrong to me, the code fires the onload event when it sees the end
element, but what about pending sub-loads? Does SVG specifically say that onload
should fire independent of pending sub-loads that were triggerd by the SVG content?
Comment 11 tor 2004-11-17 17:10:20 PST
It says: "Referenced external resources that are required must be loaded,
parsed and ready to render before the event is triggered."  Currently none
of our elements do external references as far as I know.

Do elements get any notification that the content sink has finished
with them and added it to the tree, so we could hook at that point?

Comment 12 Jonas Sicking (:sicking) No longer reading bugmail consistently 2004-11-18 03:35:56 PST
both svg images and stylesheets can reference external files so at least the two
of them has to delay onload until these files are loaded.
Comment 13 Jonas Sicking (:sicking) No longer reading bugmail consistently 2004-11-18 03:40:49 PST
There's a few hooks in nsIContent, but they're currently not consistently called
for all elements. IMHO we should make them that though.
Comment 14 Johnny Stenback (:jst, jst@mozilla.com) 2004-11-19 21:07:27 PST
Seems like the actual firing of the onload event in SVG documents should happen
in nsDocumentViewer.cpp, that's where the onload event normally fires from, and
that code knows that all sub-loads are done etc.
Comment 15 tor 2004-11-19 21:22:04 PST
Does that wait until everything in the document (style, scripts, images)
has finished loading?  I was thinking of notifying the svg elements from
the content sink that they had finished and placing the logic in
content/svg to generate the events when the external resources finished
loading.
Comment 16 tor 2004-11-30 12:09:01 PST
Comment on attachment 165603 [details] [diff] [review]
alternate approach

I've started working on a full specification version of multiple
onload and the associated externalResourcesRequired attribute.
At the rate its going the patch will be more than an order of 
magnitude larger.

Since this patch corresponds to the default behavior of SVG, as
the default value of externalResourcesRequired is false, could
it be considered as a standin until the full spec version is
ready?
Comment 17 Johnny Stenback (:jst, jst@mozilla.com) 2004-11-30 16:42:34 PST
Yeah, I guess...
Comment 18 Boris Zbarsky [:bz] (TPAC) 2004-12-09 07:44:46 PST
I assume the code has been tested with onload events that close the window,
remove the documentElement from the document, etc?
Comment 19 tor 2004-12-09 08:19:45 PST
Thanks for suggesting those tests.  My simple testcases for them passed
without obvious problems.
Comment 20 Jonas Sicking (:sicking) No longer reading bugmail consistently 2005-01-02 14:29:11 PST
I'm not really happy with the fact that we're not waiting for external
stylesheets and images. This is especially bad on the top <svg> element since
it'll actually cause a regression there. Maybe you could treat the top <svg>
element as we do now (i.e. keep the code in nsSVGElement.cpp) and make an
exception for all svg elements in the nsXMLContentSink.cpp code?
Comment 21 Jonas Sicking (:sicking) No longer reading bugmail consistently 2005-01-02 14:31:07 PST
Comment on attachment 165603 [details] [diff] [review]
alternate approach

>-  return IsGraphicElementEventName(aName);
>+  return IsGraphicElementEventName(aName) ||
>+    aName == nsSVGAtoms::onload;

Just add onload to the list in IsGraphicElementEventName since it's included in
the list at http://www.w3.org/TR/SVG10/script.html#EventAttributes
Comment 22 tor 2005-01-02 15:25:00 PST
The previous way of delivering the svg onload event was wrong because
it was delivering the event to the SVG document, which is against spec.
Comment 23 Jonas Sicking (:sicking) No longer reading bugmail consistently 2005-01-03 03:14:18 PST
True, but for the onload event you in most cases care more about when the event
is fired then what the target of the event-object is.

Another issue (which I think we might want to ignore) is that it sounds like the
loadevent should not go through the capture and bubbeling phases. See the second
last paragraph of http://www.w3.org/TR/SVG10/interact.html#SVGEvents

Comment 24 tor 2005-01-04 11:11:33 PST
The w3c tests care about which object the event is delivered on, and
the wrong object causes most of the test cases using scripts to fail
silently as the scripts abort.
Comment 25 Jonas Sicking (:sicking) No longer reading bugmail consistently 2005-01-04 11:24:45 PST
Comment on attachment 165603 [details] [diff] [review]
alternate approach

hrm.. i still think the old way was more usefull for real life pages.

I'd prefer if you got Alex or someone more svg-ish then me to ok this change as
well as it could break existing (and conformant) content.
Comment 26 tor 2005-01-10 10:01:51 PST
Created attachment 170832 [details] [diff] [review]
update "alternate approach" to tip, apply sicking's eventname change
Comment 27 Alex Fritze 2005-01-11 10:05:05 PST
(In reply to comment #25)
> (From update of attachment 165603 [details] [diff] [review] [edit])
> hrm.. i still think the old way was more usefull for real life pages.
> 
> I'd prefer if you got Alex or someone more svg-ish then me to ok this change as
> well as it could break existing (and conformant) content.
> 
I'm happy with this change. It will only break existing code that uses features
that we don't support yet (externalResourcesRequired), and even then there is a
workaround for the user (e.g. poll in onload using setTimeout).
Comment 28 Jonas Sicking (:sicking) No longer reading bugmail consistently 2005-01-11 12:35:04 PST
Remove "|| onload" from nsSVGSVGElement::IsEventName too.
Comment 29 tor 2005-01-11 13:06:11 PST
That's in the patch:

 PRBool
 nsSVGSVGElement::IsEventName(nsIAtom* aName)
 {
-  return IsGraphicElementEventName(aName) ||
-         aName == nsSVGAtoms::onload;
+  return IsGraphicElementEventName(aName);
 }
Comment 30 Jonas Sicking (:sicking) No longer reading bugmail consistently 2005-01-11 13:08:05 PST
doh!
Comment 31 Johnny Stenback (:jst, jst@mozilla.com) 2005-01-25 17:34:04 PST
Comment on attachment 170832 [details] [diff] [review]
update "alternate approach" to tip, apply sicking's eventname change

sr=jst
Comment 32 tor 2005-01-26 06:46:55 PST
Checked in.

Note You need to log in before you can comment on or make changes to this bug.