252631 - Allow multiple onloads per document

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

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]

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]

Attachment: multiple onload

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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]

It appears only one will be removed.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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]

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]

so what do you suggest we do?

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 159257 [details] [diff] [review]
multiple onload

sr=jst

Comment 9

[tor]

Attachment: 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)]

(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]

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]

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]

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)]

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]

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]

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)]

Yeah, I guess...

Comment 18

[Boris Zbarsky [:bzbarsky]]

I assume the code has been tested with onload events that close the window,
remove the documentElement from the document, etc?

Comment 19

[tor]

Thanks for suggesting those tests.  My simple testcases for them passed
without obvious problems.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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]

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]

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]

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]

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]

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]

Attachment: update "alternate approach" to tip, apply sicking's eventname change

Comment 27

[Alex Fritze]

(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]

Remove "|| onload" from nsSVGSVGElement::IsEventName too.

Comment 29

[tor]

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]

doh!

Comment 31

[Johnny Stenback (:jst)]

Comment on attachment 170832 [details] [diff] [review]
update "alternate approach" to tip, apply sicking's eventname change

sr=jst

Comment 32

[tor]

Checked in.