Allow multiple onloads per document

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: tor, Assigned: tor)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

13 years ago
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.
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... 
(Assignee)

Comment 2

13 years ago
Created attachment 159257 [details] [diff] [review]
multiple onload
(Assignee)

Updated

13 years ago
Attachment #159257 - Flags: superreview?(jst)
Attachment #159257 - Flags: review?(bugmail)
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.
(Assignee)

Comment 4

13 years ago
It appears only one will be removed.
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.
(Assignee)

Comment 6

13 years ago
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.
so what do you suggest we do?
Comment on attachment 159257 [details] [diff] [review]
multiple onload

sr=jst
Attachment #159257 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 9

13 years ago
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.
(Assignee)

Updated

13 years ago
Attachment #165603 - Flags: superreview?(jst)
Attachment #165603 - Flags: review?(bugmail)
(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?
(Assignee)

Comment 11

13 years ago
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?

(Assignee)

Updated

13 years ago
Attachment #159257 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #165603 - Attachment is obsolete: true
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.
There's a few hooks in nsIContent, but they're currently not consistently called
for all elements. IMHO we should make them that though.
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.
(Assignee)

Comment 15

13 years ago
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.
(Assignee)

Comment 16

13 years ago
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?
Attachment #165603 - Attachment is obsolete: false
Yeah, I guess...

Updated

13 years ago
Blocks: 269881

Comment 18

13 years ago
I assume the code has been tested with onload events that close the window,
remove the documentElement from the document, etc?
(Assignee)

Comment 19

13 years ago
Thanks for suggesting those tests.  My simple testcases for them passed
without obvious problems.
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 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
(Assignee)

Comment 22

13 years ago
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.
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

(Assignee)

Comment 24

13 years ago
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 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.
Attachment #165603 - Flags: review?(bugmail) → review+
(Assignee)

Updated

13 years ago
Attachment #159257 - Flags: review?(bugmail)
(Assignee)

Updated

13 years ago
Attachment #165603 - Flags: superreview?(jst)
(Assignee)

Comment 26

13 years ago
Created attachment 170832 [details] [diff] [review]
update "alternate approach" to tip, apply sicking's eventname change
Attachment #165603 - Attachment is obsolete: true
Attachment #170832 - Flags: superreview?(jst)

Comment 27

13 years ago
(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).
(Assignee)

Updated

13 years ago
Blocks: 277955
Remove "|| onload" from nsSVGSVGElement::IsEventName too.
(Assignee)

Comment 29

13 years ago
That's in the patch:

 PRBool
 nsSVGSVGElement::IsEventName(nsIAtom* aName)
 {
-  return IsGraphicElementEventName(aName) ||
-         aName == nsSVGAtoms::onload;
+  return IsGraphicElementEventName(aName);
 }
doh!
Comment on attachment 170832 [details] [diff] [review]
update "alternate approach" to tip, apply sicking's eventname change

sr=jst
Attachment #170832 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 32

13 years ago
Checked in.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.