320781 - addEventListener doesn't work for SVGLoad

Status: RESOLVED WORKSFORME

(Core :: SVG, defect)

Description

[Klaus Förster]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

The SVGLoad DOM2 event (http://www.w3.org/TR/SVG11/interact.html#EventsLoad)
is not implemented on SVGSVGElement when added as eventListener using
document.documentElement.addEventListener("SVGLoad",doSomething,false)

Reproducible: Always

Steps to Reproduce:
1. load URL that demonstrates the problem
2. alert "doing something onload" should be shown


Actual Results:  
alert triggered by SVGLoad is not shown

Expected Results:  
alert "doing something onload" triggered by SVGLoad should be shown

Comment 1

[Bjoern Hoehrmann]

As pointed out in http://svg.jibbering.com/svg/2005-12-19.html#T06-00-13 this can be worked around using an empty onload attribute on the svg element (or any other element, it seems); I think a more accurate summary would be that SVG specific events are not dispatched unless there is a corresponding event attribute in the SVG tree.

Comment 2

[Jonathan Watt [:jwatt]]

This is because we only dispatch SVGLoad if there is an onload attribute on the element.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/xml/document/src/nsXMLContentSink.cpp&rev=1.408&mark=1113#1110

Comment 3

[Jonathan Watt [:jwatt]]

The way to fix this is probably to add a HasListenersFor(type) method to nsIEventListenerManager similar to the HasMutationListeners method it has already.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch

This came up at the SVG WG F2F. Here's a patch.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 382408 [details] [diff] [review]
patch

I don't think this is right, due to chrome event listeners etc. Can we just drop this check or does that hurt perf a lot? Also, you should get review from Olli on this.

Comment 6

[Jonathan Watt [:jwatt]]

Removing the check would mean that we'd dispatch an SVGLoad event to every single SVG element in a document, and each of those events would be passed along the parent chain for the capture phase. That could be a lot of events, and consume a lot of cycles. Since this flood of events would occur while the document loads, I was assuming there would be a noticeable, possibly substantial, page load perf hit.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Do you mean every <svg> element or every SVG element? The spec currently seems to say every SVG element, but our code says ever <svg> element.

Does that include <svg> elements that are children of other SVG elements? Could we get the spec to restrict this to outermost <svg> elements only?

I think your patch is wrong not just for chrome event listeners but also because you're not checking for capturing ancestors. So if we need to optimize here, we need something al ot more fancy.

Comment 8

[Jonathan Watt [:jwatt]]

I mean every SVG element, as the spec says. That's what our current code does BTW. The nsGkAtoms::svg check there doesn't restrict the event dispatch to <svg>, but rather it forces <svg> to always get an SVGLoad regardless of whether there is a listener (because starting the SMIL timeline depends on it).

(In reply to comment #7)
> I think your patch is wrong not just for chrome event listeners but also
> because you're not checking for capturing ancestors. So if we need to optimize
> here, we need something al ot more fancy.

Indeed, you're correct on that point. I actually have a half written email about that, but for this bug I was only trying to solve the reported issue.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Perhaps what we need here is this patch, plus an extra state in the document or window to indicate whether any capturing SVGLoad listeners have been registered. Then we can check that flag and do the super-slow thing only if it's set (which should be very rare).

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If that's right, then I'm fine with your patch here, but Olli should still do the review.

Comment 11

[Jonathan Watt [:jwatt]]

(In reply to comment #5)
> I don't think this is right, due to chrome event listeners etc.

Why do you say that? Are you saying that nsIEventListenerManager::HasListenersFor() may return false when chrome code has registered listeners?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes. Although since SVGLoad doesn't bubble, I'm not sure if that matters.

Comment 13

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

If chrome adds event listeners for SVGLoad it probably adds listeners to
capturing phase so that it can get access to the event before content
can call stopPropagation.

Comment 14

[Jonathan Watt [:jwatt]]

Ugh! Are you saying there'd be no performant alternative for chrome code if it wanted to get at the event before script? I'm quite rusty on this, but isn't there a two pass thing for events (generally, the event is captured, bubbles, is captured again, and then bubbles again)? I thought I remembered something like that from the last time I looked at the event code, and the first pass was for trusted script.

If not, maybe we need an addChromeEventListener for adding event listeners that will get first pass at the event.

Otherwise I don't have any ideas on how to solve this.

Comment 15

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

Event handling goes like this:
- Creating event target chain (PreHandleEvent)
- capture phase in chrome, default event group
- capture phase in content, default event group
- target phase in content, default event group
- bubble phase in content, default event group
- bubble phase in chrome, default event group
(- nsIFrame::HandleEvent(), if needed)
- capture phase in chrome, system event group
- capture phase in content, system event group
- target phase in content  + PostHandleEvent, system event group
- bubble phase in content + PostHandleEvent, system event group
- bubble phase in chrome + PostHandleEvent, system event group
 
Whenever JS uses onfoo attributes or .addEventListener, listener is added
to default event group. C++, and actually also XBL can use also system event group.

But still, I don't understand what has that to do with this bug.
We don't want to dispatch any extra SVGLoad events if there are no listeners
for it anywhere in the event target chain.
See what is done with MozAfterPaint event.

Comment 16

[Robert Longson [:longsonr]]

I think one of Henri's patches fixed this some time ago. The URL works now.