779306 - Wildcard for 'all events', nsIEventListenerService.addListenerForAllEvents(target, listener);

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

# Proposal:

<element>.addEventListener('*',....).

# Uses:

* log all events, even if you don't know their names before hand.  (Test Pilot, debugging, etc., requested by #ur and #ux)
* easier access for general breakpoints (requested by #devtools)

# Drawbacks and Pointy bits

* could be a slowdown?

# Mitigation

* assume people are adults?  (lol!)
* limit to chrome privilege?  
* other people already have ways of slagging the browser pretty well (registering onHover, for example).

# Not Covered

* 'for an element, what are all the events registered on you?' 

In this proposal, you either listen to *everything* or use the usual methods.

# Prior art:

We allow a '*' for ObserverService [1].  

[1] https://developer.mozilla.org/en/Observer_Notifications  http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsObserverService.cpp#152

Also, I believe this has come up as an idea before, but my search skills are failing.


# Possible related:

http://stackoverflow.com/questions/446892/how-to-find-event-listeners-on-a-dom-node

Comment 1

[Jim Blandy :jimb]

Here are the properties I'd like to see to make this really useful for use by developers' tools:

- Adding a wildcard event listener shouldn't have any effect visible to content (beyond whatever the wildcard handler decides to do). Debugging tools shouldn't disturb content unless that's what the dev asked them to do.

- It should be easy to have multiple, independent listeners. If two people write two independent developer tools that use a wildcard event listener, I should be able to apply both of those tools to the same content page, and have them both function. Granted, there's no good way to decide what order to invoke the listeners in, but for tools that just want to observe, that shouldn't matter.

- I'd like to be able to use a wildcard listener like a form of breakpoint or watchpoint, and have my handler function actually spin up a nested event loop so that the dev can poke around in the content, and then hit "continue".

- A wildcard event listener should run before any content event listeners. Perhaps there should be another chrome-level listener to run after all content event listeners, but that doesn't seem as critical.

Comment 2

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

(In reply to Gregg Lind (User Research - Test Pilot) from comment #0)

> 
> * 'for an element, what are all the events registered on you?' 

If you mean "for which events is there a listener in some event target ", 
nsIEventListenerService can give you that information.

Comment 3

[Ilana]

Hello Gregg and others,

I am an innocent bystander whose username was (I assume) accidentally placed on the cc list.  I removed myself, and in case my username took the place of one belonging to a more relevant user, I'm letting you know.

Thanks and carry on,
ipercher

Comment 4

[Alex Vincent [:WeirdAl]]

Beware mutation events.  They can cause great pain.

(In other words, the API should, at least until mutations are completely removed, possibly have a third argument for "include/exclude mutation events".)

Comment 5

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

* listener must not enable mutation events.

Comment 6

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

Attachment: WIP

Comment 7

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

Re the WIP:

1. Thanks for tackling this so fast!

2. This is adding a new method 'addAllEventListener', and not doing any magic with any '*' sigils.  Is that correct?  It would be convenient to have that as a shortcut as well, even if it's less specific.  This would make it consistent with the observer service.

3.  WIP is agnostic on mutation events, right?  People requested an arg for that.

Comment 8

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

(In reply to Gregg Lind (User Research - Test Pilot) from comment #7)
> 2. This is adding a new method 'addAllEventListener', and not doing any
> magic with any '*' sigils.  Is that correct? 
Yes


> It would be convenient to have
> that as a shortcut as well, even if it's less specific.  This would make it
> consistent with the observer service.
Well, observer service isn't exposed to web pages, but add/removeEventListener are.
I don't want to add privileged-caller-only specific special cases to add/removeEventListener.

> 
> 3.  WIP is agnostic on mutation events, right?  People requested an arg for
> that.
I won't add anything to mutation events. Mutation events are dispatched *only* if there are
listener specific to them. The "all" listener doesn't enable mutation events.

Comment 9

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

(Also, smaug, looks like this is fulfilling some of the ideas from https://bugzilla.mozilla.org/show_bug.cgi?id=448602#c11 as well!)

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 647969 [details] [diff] [review]
WIP

Review of attachment 647969 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/events/src/nsEventListenerManager.cpp
@@ +60,5 @@
>  
> +#define EVENT_TYPE_EQUALS(ls, type, userType, allEvents) \
> +  ((ls->mEventType == type && \
> +   (ls->mEventType != NS_USER_DEFINED_EVENT || ls->mTypeAtom == userType)) || \
> +   (allEvents && ls->mAllEvents == allEvents))

(allEvents && ls->mAllEvents)?

Can't this be an inline function, btw?

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Olli Pettay [:smaug] from comment #8)
> > 3.  WIP is agnostic on mutation events, right?  People requested an arg for
> > that.
> I won't add anything to mutation events. Mutation events are dispatched
> *only* if there are
> listener specific to them. The "all" listener doesn't enable mutation events.

So if you add a specific mutev listener, the "all" listener will get them? I guess that works for a chrome-only API.

Comment 12

[Alex Vincent [:WeirdAl]]

To be clear, I wasn't asking for mutation events support.  I was just calling that out as something that users of this API need to be aware of.

Comment 13

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

Attachment: with tests

Comment 14

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

I'm not quite sure about the method names.
addAllEventListener or addListenerForAllEvents ?

Comment 15

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

(In reply to Jim Blandy :jimb from comment #1)
> - A wildcard event listener should run before any content event listeners.
> Perhaps there should be another chrome-level listener to run after all
> content event listeners, but that doesn't seem as critical.
I forgot this.
ls = mListeners.AppendElement(); should become
ls = aAllEvents ? mListeners.InsertElementAt(0) : mListeners.AppendElement();

Comment 16

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

Attachment: patch

Comment 17

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

Still no hint of the "*" sigil in this patch (ie., `addEventListener('*')`).  I assume that is by design?   It would be nice (from a dev perspective), to have that shortcut, similar to ObserverService.  


Also, if this bug goes through, there will need to be doc updates, blog post, and developer education.  

When is this landing?

Comment 18

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

(In reply to Gregg Lind (User Research - Test Pilot) from comment #17)
> Still no hint of the "*" sigil in this patch (ie., `addEventListener('*')`).
> I assume that is by design?
Yes. addEventListener is an API exposed also to web pages, and we don't want
to expose '*' functionality to untrusted scripts.
 
> When is this landing?
Patches need review(s) before they can land.

Comment 19

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

Review ping.

Comment 20

[Sunil]

It will good to have the option of calling the chrome event listener *after* all content event listeners are called. In my specific case, want to know if the content  event handler prevented default or not. Currently there's no easy way to achieve that.

And meanwhile, any update when the patch will land?

Comment 21

[Johnny Stenback (:jst)]

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

Sorry for letting this review request fall through the cracks :(

This patch looks good, but it feels NS_EVENT_TYPE_DUMMY could have a better name, since it's pretty specific to this case of wildcard event listeners, so maybe make the type name reflect that?

Comment 22

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

Attachment: up-to-date

Comment 23

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

https://tbpl.mozilla.org/?tree=Try&rev=958815b255e7

Comment 24

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

Attachment: up-to-date

Comment 25

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

https://hg.mozilla.org/mozilla-central/rev/482d32e2b348