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

RESOLVED FIXED

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: glind, Assigned: smaug)

Tracking

({dev-doc-needed})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

# 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

6 years ago
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.
Summary: Wildcard for 'all events', <element>.addEventListener('*',...) → Wildcard for 'all events', nsIEventListenerService.addListenerForAllEvent(target, listener);
Summary: Wildcard for 'all events', nsIEventListenerService.addListenerForAllEvent(target, listener); → Wildcard for 'all events', nsIEventListenerService.addListenerForAllEvents(target, listener);
(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

6 years ago
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
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".)
* listener must not enable mutation events.
Assignee: nobody → bugs
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.
(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.
(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 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?
(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.
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.
Created attachment 648056 [details] [diff] [review]
with tests
Attachment #647969 - Attachment is obsolete: true
Attachment #648056 - Flags: review?(jst)
I'm not quite sure about the method names.
addAllEventListener or addListenerForAllEvents ?
(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();
Attachment #648056 - Attachment is obsolete: true
Attachment #648056 - Flags: review?(jst)
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?
(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.
Review ping.

Comment 20

6 years ago
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 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?
Attachment #648479 - Flags: review?(jst) → review+
Created attachment 677349 [details] [diff] [review]
up-to-date
Attachment #677324 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/482d32e2b348
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.