Implement 'passive' member of AddEventListenerOptions

RESOLVED FIXED in Firefox 49

Status

()

RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: overholt, Assigned: kats)

Tracking

({dev-doc-complete})

unspecified
mozilla49
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: btpp-fixlater)

Attachments

(3 attachments, 2 obsolete attachments)

Comment 1

3 years ago
FWIW I think implementing the passive member can happen separately from supporting the union of boolean and EventListenerOptions being passed as the third argument to addEventListener...
Yeah, getting the latter in sooner is more important, as it reduces the risk of breakage from future websites on old browsers.

I'm happy to do the APZ side of the implementation once the DOM side is done and there is some API to detect if a listener is passive or not.
(Reporter)

Updated

3 years ago
Whiteboard: btpp-fixlater
Depends on: 1266194
Keywords: dev-doc-needed
Summary: Implement 'passive' member of EventListenerOptions → Implement 'passive' member of AddEventListenerOptions
I started looking into implementing this. Parking with me, but if somebody else wants it please get in touch!
Assignee: nobody → bugmail.mozilla
Seemed pretty straightforward to implement, top three patches at https://github.com/staktrace/gecko-dev/commits/passive - although I don't know if there's any feature detection hooks we need as well. Also it needs tests.
Comment on attachment 8748906 [details] [diff] [review]
Part 2 - Ignore passive listeners for APZ purposes

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

::: gfx/layers/apz/test/mochitest/helper_tap_passive.html
@@ +2,5 @@
> +<html>
> +<head>
> +  <meta charset="utf-8">
> +  <meta name="viewport" content="width=device-width; initial-scale=1.0">
> +  <title>Ensure we get a touch-cancel after a contextmenu comes up</title>

Copypasta title?

@@ +49,5 @@
> +}
> +
> +function registerListeners() {
> +  window.addEventListener('touchstart', recordEvent, { passive: true, capture: true });
> +  window.addEventListener('contextmenu', recordEvent, true);

IIUC, this "true" means "capture: true"?
Attachment #8748906 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #8)
> > +  <title>Ensure we get a touch-cancel after a contextmenu comes up</title>
> 
> Copypasta title?

Whoops, thanks. I'll update that.

> @@ +49,5 @@
> > +}
> > +
> > +function registerListeners() {
> > +  window.addEventListener('touchstart', recordEvent, { passive: true, capture: true });
> > +  window.addEventListener('contextmenu', recordEvent, true);
> 
> IIUC, this "true" means "capture: true"?

Yes, that's the old-style (sans EventListenerOptions) listener registration. It doesn't really matter if that listener is capturing or bubbling, I just usually default to capturing listeners unless there's a reason to make them bubbling.
Posted patch Part 3 - Console warning (obsolete) — Splinter Review
dtapuska pointed out that the spec suggests logging a console warning for these things. He also pointed me to the warning in Chromium at https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/events/Event.cpp&q=Event::preventDe&sq=package:chromium&l=236 - they log it every time it happens. My inclination was to just log it once per document, but maybe we should do it every time too? Not really sure.
Attachment #8749214 - Flags: review?(bugs)
once per document sounds better.
(In reply to Olli Pettay [:smaug] from comment #11)
> once per document sounds better.

Cool, that's what the patch does. Updated version just takes out the "XXX" comment about this.
Attachment #8749214 - Attachment is obsolete: true
Attachment #8749214 - Flags: review?(bugs)
Attachment #8751325 - Flags: review?(bugs)
Comment on attachment 8751325 [details] [diff] [review]
Part 3 - Console warning

> Event::PreventDefaultInternal(bool aCalledByDefaultHandler)
> {
>   if (!mEvent->mFlags.mCancelable) {
>     return;
>   }
>   if (mEvent->mFlags.mInPassiveListener) {
>+    nsCOMPtr<nsPIDOMWindowInner> win(do_QueryInterface(mOwner));
>+    if (win) {
>+      if (nsIDocument* doc = win->GetExtantDoc()) {
>+        nsString type;
nsAutoString

I assume you tested this manually.
Attachment #8751325 - Flags: review?(bugs) → review+
Comment on attachment 8748905 [details] [diff] [review]
Part 1 - Implement support for passive

>   EventListenerFlags flags;
>   flags.mCapture =
>     aOptions.IsBoolean() ? aOptions.GetAsBoolean()
>                          : aOptions.GetAsAddEventListenerOptions().mCapture;
>+  if (!aOptions.IsBoolean()) {
>+    flags.mPassive = aOptions.GetAsAddEventListenerOptions().mPassive;
>+  }
Might be nice to have aOptions.IsBoolean() check only once.

Perhaps
  if (aOptions.IsBoolean()) {
    flags.mCapture = aOptions.GetAsBoolean();
  } else {
    flags.mCapture = aOptions.GetAsAddEventListenerOptions().mCapture;
    flags.mPassive = aOptions.GetAsAddEventListenerOptions().mPassive;
  }


>   bool Equals(const EventListenerFlags& aOther) const
>   {
>     return (mCapture == aOther.mCapture &&
>             mInSystemGroup == aOther.mInSystemGroup &&
>             mListenerIsJSListener == aOther.mListenerIsJSListener &&
>-            mAllowUntrustedEvents == aOther.mAllowUntrustedEvents);
>+            mAllowUntrustedEvents == aOther.mAllowUntrustedEvents &&
>+            mPassive == aOther.mPassive);
This makes addEventListener behave against the spec. passive isn't part of the "key".
Sounds like we're missing a test for that case.

>+function doTest(description, passiveArg)
>+{
>+  listenerHitCount = 0;
>+
>+  elem.addEventListener('test', listener, { passive: passiveArg });
So you could add the same listener again with passive: false to ensure the first registration isn't changed.
But anyhow, please ensure addEventListener behavior follows the spec.
Attachment #8748905 - Flags: review?(bugs) → review-
Comment on attachment 8748906 [details] [diff] [review]
Part 2 - Ignore passive listeners for APZ purposes

So simple!
Attachment #8748906 - Flags: review?(bugs) → review+
Updated the test and code to handle the case you described. I got rid of the operator== because it doesn't seem to be used except for this one place where we add event listeners, and I felt it was better to have a more explicitly named method.
Attachment #8748905 - Attachment is obsolete: true
Attachment #8751838 - Flags: review?(bugs)
Comment on attachment 8751838 [details] [diff] [review]
Part 1 - Implement support for passive (v2)

>+function testAddListenerKey()
>+{
>+  listenerHitCount = 0;
>+  doPreventDefault = true;
>+
>+  elem.addEventListener('test', listener, { capture: false, passive: false });
>+  // This second listener should not be registered, because the "key" of
>+  // { type, callback, capture } is the same.
>+  elem.addEventListener('test', listener, { capture: false, passive: true });
Would be nice to test also other way round, that passive listener doesn't become non-passive
Attachment #8751838 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #17)
> Would be nice to test also other way round, that passive listener doesn't
> become non-passive

I updated the test function to take a passiveListenerFirst argument, and conditioned the behaviour on that. I called the function twice from the main test driver function so now it tests both ways. Will land the patches shortly.

Comment 20

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f66aa2a433d8
https://hg.mozilla.org/mozilla-central/rev/e5880e371c16
https://hg.mozilla.org/mozilla-central/rev/cda76e80a47c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Made a PR for platform-status.mozilla.org:

https://github.com/mozilla/platform-status/pull/445
You need to log in before you can comment on or make changes to this bug.