372147 - Move SVG onload event generation out of XML content sink

Status: RESOLVED WORKSFORME

(Core :: SVG, defect)

Description

[tor]

Comment 1

[tor]

Attachment: nonworking experiment firing SVGLoad from nsDocumentViewer

While this generates an event if you use addEventListener, onload attribute scripts don't fire.

Comment 2

[Jonathan Watt [:jwatt]]

All you need is:

  svgEvent.target = mDocument->GetRootContent();

I think. Dispatching the event to the document itself means the event never gets as far as the document element, so the onload attribute on the element can never be triggered. Of course the SVGLoad event should only be dispatched if mDocument->GetRootContent() is an SVG <svg> element so you should check for:

  mDocument->GetRootContent()->NodeInfo()->Equals(nsGkAtom::svg, kNameSpaceID_SVG)

first.

Comment 3

[tor]

(In reply to comment #2)
> All you need is:
> 
>   svgEvent.target = mDocument->GetRootContent();
> 
> I think. Dispatching the event to the document itself means the event never
> gets as far as the document element, so the onload attribute on the element can
> never be triggered.

No, that doesn't do it.

> Of course the SVGLoad event should only be dispatched if
> mDocument->GetRootContent() is an SVG <svg> element so you should check for:
> 
>   mDocument->GetRootContent()->NodeInfo()->Equals(nsGkAtom::svg,
> kNameSpaceID_SVG)
> 
> first.

That won't handle the case of a mixed-namespace document; a check of whether SVG is in the content tree at all will probably be needed.  I'm not worried about that at the moment, just about how to fire the event properly.

Comment 4

[Jonathan Watt [:jwatt]]

> No, that doesn't do it.

Ah, so the event dispatched from there seems to be used to dispatch a second one (the real event that goes to the document). I think you need to put the SVG event code here:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/dom/src/base/nsGlobalWindow.cpp&rev=1.912&mark=1939,1970#1938

If you put a breakpoint in there you'll see that the event dispatched from the code you're touching in DocumentViewerImpl::LoadComplete ends up there and triggers the second event.

Comment 5

[Jonathan Watt [:jwatt]]

Could be jumping the gun again though. How about we just CC smaug and he can tell us how all this crazy events stuff works. :-)

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

"The event is triggered at the point at which the user agent has fully parsed the element and its descendants and is ready to act appropriately upon that element, such as being ready to render the element to the target device. Referenced external resources that are required must be loaded, parsed and ready to render before the event is triggered. Optional external resources are not required to be ready for the event to be triggered."
So SVGLoad is dispatched to *all* elements. (though that can be ofc
optimized in cases when there are no event listeners for SVGLoad in the ancestor chain. )

What I don't quite understand is:
"A SVGLoad event is dispatched only to the element to which the event applies; it is not dispatched to its ancestors. For example, if an 'image' element and its parent 'g' element both have event listeners for SVGLoad events, when the 'image' element has been loaded, only its event listener will be invoked. (The 'g' element's event listener will indeed get invoked, but the invocation will happen when the 'g' itself has been loaded.)"
I think that makes sense if s/event listener/event listener for bubble phase/"

Using event listener attributes (=onload) the listener is registered
only for target/bubbling phase, but ofc one can use
.addEventsListeter("SVGLoad", handler, true)
I guess the current code doesn't quite handle the case where
svg element doesn't have onload attribute, but .addEventListener has been used.

Btw, In the Scalable Vector Graphics (SVG) Tiny 1.2 Specification, CR:
"This event (SVGLoad) is sent at the same time as the load event, but the load event must be sent before the SVGLoad event." ?!? :)

I don't think dispatching SVGLoad could be easily moved to 
documentviewer or globalwindow. I wonder what could be the best way
create the events. 

Comment 7

[Jonathan Watt [:jwatt]]

I think for the moment we're less interested in the SVG spec and more interested in why nsEventDispatcher::Dispatch doesn't work given the input in the patch. It's confusing that nsEventDispatcher::Dispatch can be fed two different targets. The docs could be better at explaining when you want to use one rather than the other.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Er, what is confusing?
http://lxr.mozilla.org/seamonkey/source/content/events/public/nsEventDispatcher.h#199

So if you want to dispatch something to documentElement, then use that
as the first parameter for the ::Dispatch. As the comment says, in 
that case the top of the event target chain will be the 
documentElement and event listeners of that element will be called.

It is different thing what you want event.target to be.
If you want it to be something else than documentElement, set 
event.target before calling ::Dispatch.
(Note, usually you don't want this. event.target is different than
the top of chain only in those few cases when event is dispatched to 
|window| but we want target to be |document| for backward 
compatibility)

Comment 9

[Jonathan Watt [:jwatt]]

What's confusing is that I wasn't sure whether aEvent->target played some part in deciding how part of the event target chain was created. Your above comment didn't really clear that up for me because you use the word "target" for different things. Anyway, our IRC conversation did and I've filed bug 373687.

Comment 10

[Jonathan Watt [:jwatt]]

Comment on attachment 256812 [details] [diff] [review]
nonworking experiment firing SVGLoad from nsDocumentViewer

Tim, what happens if you kill this line:

>+      svgEvent.target = mDocument;

and change this line:

>+      nsEventDispatcher::Dispatch(window, mPresContext, &svgEvent, nsnull,
>+                                  &status);

to:

      nsEventDispatcher::Dispatch(mDocument->GetRootContent(), mPresContext,
                                  &svgEvent, nsnull, &status);

Comment 11

[Robert Longson [:longsonr]]

Attachment: updated patch

(In reply to comment #10)

That seems to work, inasmuch as the testcase in bug 293581 gives sensible answers now.

Comment 12

[Jonathan Watt [:jwatt]]

Okay, good. So I guess now that we know how to dispatch events using nsEventDispatcher (bug 373687) we can come back to the problems raised in comment 6. Mainly, an SVGLoad event is currently dispatched to all SVG elements, but that would no longer happen with this patch. One solution I can see would be to reuse the SVGLoad event and iterate through the document tree dispatching it to every SVG element we can find. While that might meet the letter of the spec I suspect it doesn't quite meet the spirit/intent. :-)

Comment 13

[Robert Longson [:longsonr]]

Speaking from the position of knowing very little ;), could we do the dispatch in nsCSSFrameConstructor::ConstructSVGFrame for each frame we make?

Comment 14

[Jonathan Watt [:jwatt]]

Dunno. Frames can be constructed at any time during the document's lifetime. For example, when an element is moved in the DOM or when the display property is changed from display:none to something else. Also some elements don't (or shouldn't) have frames, such as anything starting it's life with display:none or perhaps anything inside <defs> for example?

Comment 15

[tor]

Attachment: slightly more spec compliant onload

Talked to Jonas about this and came up with this; still fires from the content sink but now doesn't do event propagation per spec.  This also appears to fix bug 374083.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 258733 [details] [diff] [review]
slightly more spec compliant onload


>+      nsEvent event(PR_TRUE, NS_SVG_LOAD);
>+      event.eventStructType = NS_SVG_EVENT;
>+      nsIPresShell *presShell = mDocument->GetShellAt(0);
>+      if (presShell) {
>+        FlushTags();
>+        nsCOMPtr<nsIDOMEvent> domEvent;
>+
>+        // Using HandleEvent directly because the SVGLoad event
>+        // isn't supposed to bubble or capture
>+        manager->HandleEvent(presShell->GetPresContext(),
>+                             &event,
>+                             getter_AddRefs(domEvent),
>+                             content,
>+                             NS_EVENT_FLAG_BUBBLE, // xxx
>+                             &status);
>+      }

This looks ..hmm.. ugly. Could you add PreHandleEvent to nsSVGElement and there
if the event is NS_SVG_LOAD, aVisitor.mParentTarget = nsnull; aVisitor.mCanHandle = PR_TRUE;
Otherwise call nsGenericElement::PreHandleEvent. And then just use the normal event dispatching.
nsEventDispatcher::Dispatch sets the right target/currentTarget/originalTarget for the event, 
so if you don't call that, you'll have to set them manually. nsEventDispatcher::Dispatch also
calls default and system event group handlers.

Though, there is the problem that what if someone creates SVGLoad event using scripts and
dispatches that. Should that event have normal capture/bubble phase?

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

What does the other implementations do with SVGLoad?

Comment 18

[Jonathan Watt [:jwatt]]

This patch seems to try to fix bug 320781 and bug 332227, but not this bug. Confusing. As for bug 374083, how about you just create a patch to make the code use nsEventDispatcher::Dispatch there?

Smaug, yeah, I suggest using PreHandleEvent in bug 332227. Hadn't quite got the details right though. To answer your question, right now Opera has a capture phase for SVGLoad. Don't know about anyone else. That makes me less keen to fix bug 332227 right now, especially mixed in with other fixes.

Comment 19

[tor]

(In reply to comment #18)
> This patch seems to try to fix bug 320781 and bug 332227, but not this bug.
> Confusing.

We talked about it and couldn't come up with a good alternative place to put the event firing, so I decided to just use this bug as a "get svg onload more spec compliant" bug.

Comment 20

[tor]

Attachment: set SVGLoad as non-bubbling event

It seems that the SVG working group is going to clarify the spec to be in line with 1.2 tiny, which just states that the event should not bubble.  However since there still is a capture phase, it seems that we need to fire SVGLoad for any SVG element.  That doesn't seem good for performance on complex documents.

Comment 21

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

I seriously think we need to object here. Firing an event for every parsed element is going to cost lots of performance, especially for SVG documents that are expected to be machine generated and huge.

Can we ask that it's only fired on elements that have an onload attribute specified?

Comment 22

[tor]

It actually gets worse than that in 1.2 tiny - there SVGLoad is depreciated and you need to fire both the SVGLoad and a "load" event when an element is ready.

  http://www.w3.org/TR/SVGMobile12/interact.html#EventsSVGLoad

Comment 23

[Jonathan Watt [:jwatt]]

(In reply to comment #21)
> Can we ask that it's only fired on elements that have an onload attribute
> specified?

The addition of HasListenerForEventType in the previous patch essentially does that.

Comment 24

[tor]

(In reply to comment #23)
> (In reply to comment #21)
> > Can we ask that it's only fired on elements that have an onload attribute
> > specified?
> 
> The addition of HasListenerForEventType in the previous patch essentially does
> that.

You'd need to check the entire parent chain for each element to see if anyone is lurking waiting for a capture event.  While doing that manually might be cheaper than firing the event (feedback on that point welcome), it's still a decent amount of work for element during parsing.

Comment 25

[tor]

s/for element/for each element/

Comment 26

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

Exactly, at which point just the checking becomes a big enough perf hit. We should just kick this back to the SVG WG as unimplementable and do what we're currently do whatever we want until then (personally I'd prefer to not fire any load/SVGLoad events until this mess is fixed)

Comment 27

[Jonathan Watt [:jwatt]]

My reading of the SVG spec is that SVGLoad does not have a capture phase or a bubble phase - only an "at target" phase - so there's no need to check the parent chain.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

AFAIK, that will change, or in other words there will be an (1.1) errata 
which clarifies that SVGLoad just doesn't bubble.
It makes no sense to have one event which doesn't have
capture/bubble.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

But could we somehow optimize SVGLoad dispatching.
That might need some ugly hacks but at least the hacks would be in
implementation, not in specification.

... whenever there is a capturing listener for svgload event, all 
descendant elements should dispatch svgload. If the listener is for
bubble, no need to handle descendants... hmm, something like that.
Maybe a node flag "NS_SVG_MAY_HAVE_SVG_LOAD_LISTENER" is needed...

Comment 30

[Jonathan Watt [:jwatt]]

Why does it make no sense?

And by far the most common case is to have a load listener on the root element, so hacks to only dispatch load events to children of elements that are listening for them would in practice mean we usually dispatch load to all the elements in the document.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #30)
> Why does it make no sense?

Maybe too strongly said, but at least it is strange for content authors to have one event which doesn't have capture phase.

> 
> And by far the most common case is to have a load listener on the root element,

Almost always those are registered for bubble phase, right?
onXXX attributes adds listeners for bubble phase. So because
SVGLoad doesn't bubble, they fire on AT_TARGET.

Comment 32

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

But why bother with all that hacking, extra uglyness and probably extra slowness for something that is adding very little value to the users. Like jwatt said, by far the most common case is to have onload on the root <svg>, so why not just dispatch it there.

This can be fixed on a spec level so lets try to do that.

Comment 33

[tor]

(In reply to comment #32)
> But why bother with all that hacking, extra uglyness and probably extra
> slowness for something that is adding very little value to the users. Like
> jwatt said, by far the most common case is to have onload on the root <svg>, so
> why not just dispatch it there.

In the sampling of SVG apps that I work with, there's at least one that makes use of multiple onloads on different elements.  Removing functionality as you suggest is problematic as we don't want to break currently working content.

Comment 34

[JP Fiset]

I have submitted bug 379971 which deals with dynamically created SVG elements. I do not know if you want to consider that case as well when trying to fix this reported bug.

Comment 35

[Robert Longson [:longsonr]]

This has all been rewritten for Firefox 4. I'm going to close it as WFM as Henri rewrote and optimised all of this.

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Did you Henri also change XML content sink, or only HTML/HTML5 parser/content sink?

Comment 37

[Henri Sivonen (:hsivonen)]

(In reply to comment #36)
> Did you Henri also change XML content sink, or only HTML/HTML5 parser/content
> sink?

I changed the XML content sink, too:
http://mxr.mozilla.org/mozilla-central/source/content/xml/document/src/nsXMLContentSink.cpp#1160