addEventListener doesn't work for SVGLoad

RESOLVED WORKSFORME

Status

()

RESOLVED WORKSFORME
13 years ago
8 years ago

People

(Reporter: klaus.foerster, Assigned: jwatt)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

13 years ago
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
Component: General → SVG
Product: Firefox → Core
Version: unspecified → 1.0 Branch

Comment 1

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

Updated

13 years ago
Assignee: nobody → general
QA Contact: general → ian
(Assignee)

Comment 2

12 years ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
Summary: SVGLoad not implemented on SVGSVGElement → addEventListener doesn't work for SVGLoad
Version: 1.0 Branch → Trunk
(Assignee)

Comment 3

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

Comment 4

9 years ago
Created attachment 382408 [details] [diff] [review]
patch

This came up at the SVG WG F2F. Here's a patch.
Assignee: general → jwatt
Status: NEW → ASSIGNED
Attachment #382408 - Flags: review?(roc)
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.
(Assignee)

Comment 6

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

Comment 8

9 years ago
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.
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).
If that's right, then I'm fine with your patch here, but Olli should still do the review.
(Assignee)

Comment 11

9 years ago
(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?
(Assignee)

Updated

9 years ago
Attachment #382408 - Flags: review?(roc)
Yes. Although since SVGLoad doesn't bubble, I'm not sure if that matters.
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.
(Assignee)

Comment 14

9 years ago
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.
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.
QA Contact: ian → general
I think one of Henri's patches fixed this some time ago. The URL works now.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.