1266194 - Implement boolean or EventListenerOptions as 3rd param to addEventListener

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

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

This is the more important part, and support for 'passive' can be added later.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: event.patch

In this patch I'm not implementing "passive" nor "once".

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8745144 [details] [diff] [review]
event.patch

Not enough information about this bug in comment 0. Redirecting this review to smaug...

Comment 3

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

yeah, I can take a look given that I was fighting against this syntax, but failed :/

Comment 4

[Andrea Marchesini [:baku]]

Attachment: event2.patch

Or maybe you prefer to have the dictionary propagated up to the EventListenerManager...

Comment 5

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

Comment on attachment 8745144 [details] [diff] [review]
event.patch


>+dictionary EventListenerOptions {
>+  boolean capture = false;
>+};
>+
>+dictionary AddEventListenerOptions : EventListenerOptions {
>+  boolean passive = false;
>+  boolean once = false;
Could you actually comment the properties we don't support out.
That way one can do feature testing by having a getter for the property and making it to throw. If exception
is thrown, property is supported in the dictionary.
To ease future patches we can still leave AddEventListenerOptions, even though it becomes just empty dictionary inheriting
EventListenerOptions

Comment 6

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

Comment on attachment 8745164 [details] [diff] [review]
event2.patch


>+EventListenerManager::RemoveEventListener(
>+                        const nsAString& aType,
>+                        const EventListenerHolder& aListenerHolder,
>+                        const dom::EventListenerOptionsOrBoolean& aOptions)
>+{
>+  EventListenerFlags flags;
>+  flags.mCapture =
>+    aOptions.IsBoolean() ? aOptions.GetAsBoolean()
>+                         : aOptions.GetAsEventListenerOptions().mCapture;
>+  RemoveEventListenerByType(aListenerHolder, aType, flags);
>+}
I guess this is better than calling the RemoveEventListener above, since we'll need to set
other flags once we support passive and once.


So, we need some tests too, wpt tests hopefully.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b7777972750

Comment 8

[Andrea Marchesini [:baku]]

> So, we need some tests too, wpt tests hopefully.

I and smaug tested it locally.

Comment 9

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

and wpt tests are coming from blink folks hopefully later this week.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5b7777972750

Comment 11

[(no longer active)]

[Tracking Requested - why for this release]:

We should backport this to ESR so that once websites start to use this API, they will still work fine on ESR even if they don't feature detect it properly.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: esr-45

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 

If this optional parameter is used, ESR45 will not be able to handle it. This patch is for web compat.

Fix Landed on Version: 49.

Risk to taking this patch (and alternatives if risky): not really. The patch is quite simple.

String or UUID changes made by this patch: none.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 13

[(no longer active)]

Sylvestre, do you know who handles ESR approvals?  This has been waiting for a month now...

Thanks!

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8745881 [details] [diff] [review]
esr-45

Improve the ESR status.

Sorry about the delay, we start dealing with esr uplifts usually at this time in the beta cycle.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

I guess 47 & 48 are fixed somewhere else, right?

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr45/rev/02df988a56ae

Comment 17

[Jean-Yves Perrier [:teoli]]

Updated:
https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener
and
https://developer.mozilla.org/en-US/Firefox/Releases/49#DOM_HTML_DOM