Implement boolean or EventListenerOptions as 3rd param to addEventListener

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: smaug, Assigned: baku)

Tracking

({dev-doc-complete})

36 Branch
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed, firefox-esr38- wontfix, firefox-esr4547+ fixed)

Details

(Whiteboard: btpp-fixlater)

Attachments

(3 attachments)

This is the more important part, and support for 'passive' can be added later.
Whiteboard: btpp-fixlater
Assignee: nobody → amarchesini
Posted patch event.patchSplinter Review
In this patch I'm not implementing "passive" nor "once".
Attachment #8745144 - Flags: review?(masayuki)
Blocks: 1267505
Comment on attachment 8745144 [details] [diff] [review]
event.patch

Not enough information about this bug in comment 0. Redirecting this review to smaug...
Attachment #8745144 - Flags: review?(masayuki) → review?(bugs)
yeah, I can take a look given that I was fighting against this syntax, but failed :/
Posted patch event2.patchSplinter Review
Or maybe you prefer to have the dictionary propagated up to the EventListenerManager...
Attachment #8745164 - Flags: review?(bugs)
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
Attachment #8745144 - Flags: review?(bugs) → review+
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.
Attachment #8745164 - Flags: review?(bugs) → review+
> So, we need some tests too, wpt tests hopefully.

I and smaug tested it locally.
and wpt tests are coming from blink folks hopefully later this week.
https://hg.mozilla.org/mozilla-central/rev/5b7777972750
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
[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.
Flags: needinfo?(amarchesini)
Posted patch esr-45Splinter Review
[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.
Flags: needinfo?(amarchesini)
Attachment #8745881 - Flags: approval-mozilla-esr45?
Sylvestre, do you know who handles ESR approvals?  This has been waiting for a month now...

Thanks!
Flags: needinfo?(sledru)
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.
Flags: needinfo?(sledru)
Attachment #8745881 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
I guess 47 & 48 are fixed somewhere else, right?
You need to log in before you can comment on or make changes to this bug.