Closed Bug 1266066 Opened 8 years ago Closed 8 years ago

Implement 'passive' member of AddEventListenerOptions

Categories

(Core :: DOM: Events, defect)

Product:
Core
Component:
DOM: Events
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: overholt, Assigned: kats)

References

Details

(Keywords: dev-doc-complete, Whiteboard: btpp-fixlater)

Attachments

(3 files, 2 obsolete files)

Part 2 - Ignore passive listeners for APZ purposes
7.62 KB, patch
smaug
: review+
botond
: review+
Details | Diff | Splinter Review
Part 3 - Console warning
3.30 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Part 1 - Implement support for passive (v2)
11.54 KB, patch
smaug
: review+
Details | Diff | Splinter Review 
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.
Whiteboard: btpp-fixlater
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.
Attached 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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: