Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

Default to passive: true on root document touch scroll blocking event listeners #35

Closed
dtapuska opened this issue Oct 25, 2016 · 26 comments

Comments

@dtapuska
Copy link

Chromium/Blink would like to propose an intervention where touch scroll blocking event listeners (touchstart and touchmove) added to Document Level objects would default the AddEventListenerOptions passive field to true.

Document Level objects are defined to be the window, document and body.
Touch scroll blocking event listeners are touchstart and touchmove.

Empirically we have determined that over 80% of touch event listeners invoked don't end up preventing scrolling. There are two opt outs that developers can use:

  1. Using touch-action
  2. Explicitly specifying passive: false

See more details in Document Level Passive Event Listeners Intervention

and #18

@dtapuska
Copy link
Author

/cc @teddink, @smaug----, @RByers, @foolip, @BenjaminPoulain

PTAL as I value feedback from everyone.

@smaug----
Copy link

So how is this different to #18 ?
or w3c/touch-events#74 ?

@dtapuska
Copy link
Author

By placing this in the wicg we are hoping that we can solicit feedback from our colleagues at Microsoft and Apple.

#18 covers more than just this root document intervention it also covers making it by default for everything and making events non-blocking during fling. So this is a concrete implementation of a example used in #18 .

@foolip
Copy link
Member

foolip commented Oct 26, 2016

Doing it in this venue makes sense, but showing the changes as a patch in DOM would I think make it easier to review.

@RByers
Copy link
Member

RByers commented Oct 26, 2016

Thanks for filing this specific concrete issue Dave. Note that there's also an active intent thread on blink-dev. We'll do the cross-vendor discussion here and bring any conclusions to the intent discussion.

@RByers
Copy link
Member

RByers commented Oct 26, 2016

And to be concrete, the precise effect of this change is:

  • During addEventListener
    • If type is touchstart or touchmove, AND
    • If passive option is unspecified, AND
    • If target is window, document, document.documentElement or document.body, THEN
    • set passive to true.

@foolip
Copy link
Member

foolip commented Oct 26, 2016

Is that any Window, Document or HTMLBodyElement, or some specific ones?

@dtapuska
Copy link
Author

dtapuska commented Oct 26, 2016

  1. Any Window
  2. Any Document
  3. Any node that has target.ownerDocument.documentElement == target
  4. Any node that has target.ownerDocument.body == target

In Blink the code is here.

@foolip
Copy link
Member

foolip commented Oct 26, 2016

I guess that's if the target is a Node, and using the "node document". That's unambiguous to me, thanks.

@smaug----, thoughts on that?

@RByers
Copy link
Member

RByers commented Oct 26, 2016

I've filed this WebKit bug in the hopes that we can get some public signal on whether this is something WebKit would ever consider doing.

@smaug----
Copy link

hmm, target.document == target? What does that mean? Or target.document.documentElement == target? Is this only for events dispatched to Window, Document or Body or what?
I thought this was about listeners on Window, Document or Body.

@smaug----
Copy link

And does this mean any document, or only documents which have .defaultView != null ?
And any body or only body elements which are in document which has defaultView != null?

@dtapuska
Copy link
Author

target is instance of EventTarget to which the addEventListener is being called on. It has nothing to do with dispatch target at all.

@bzbarsky
Copy link

target is instance of EventTarget to which the addEventListener is being called on

Sure, but what exactly is target.document? That doesn't make any sense.

I think what you mean is something more like (based on looking at the Blink implementation):

  1. Any Window.
  2. Any Document.
  3. Any node which is the https://dom.spec.whatwg.org/#document-element of its https://dom.spec.whatwg.org/#concept-node-document.
  4. Any node which is the https://html.spec.whatwg.org/multipage/dom.html#the-body-element-2 of its https://dom.spec.whatwg.org/#concept-node-document.

@dtapuska
Copy link
Author

Yes there was an implicit predicate if target is a node on 2, 3, 4.

@bzbarsky
Copy link

My point was that .document on nodes is nonsense, except in the Blink codebase.

@foolip
Copy link
Member

foolip commented Nov 1, 2016

#35 (comment) is correct AFAICT. @smaug---- @bzbarsky @annevk, any concerns with this?

Just to spell out the obvious, yes, it does mean that { passive: false } and { passive: undefined } will not always mean the same thing. I don't think this can be avoided while still defining this in terms of passive.

@dtapuska, can you poke more people working on Edge to get feedback here?

@annevk
Copy link

annevk commented Nov 1, 2016

I'm very much confused. addEventListener() doesn't know the target of an event. Are we talking about the this object (context object in DOM) or something else? Also, why is there no upstream issue?

@foolip
Copy link
Member

foolip commented Nov 1, 2016

Yes, it's the context object. I think a DOM PR would be good, and perhaps publishing the output somewhere if other vendors want to see this shipped before committing, see blink-dev.

@annevk
Copy link

annevk commented Nov 1, 2016

My initial impression is that it seems rather hairy to bake a dependency on the Window object and HTML's body element in addEventListener(). I guess we already added a hack for service workers, but that was not an invitation.

@RByers
Copy link
Member

RByers commented Nov 1, 2016

I agree it's hacky to special case the Window object etc. Our ultimate goal is to treat all all context objects the same - passive would default to true for all touchstart and touchmove events (similar to how pointer events behave). But we suspect that's not sufficiently web compatible to be something we could reasonably ship yet. We're hopeful that by taking this first step we can validate that it's indeed rare for sites to be impacted and easy for developers to adopt touch-action. Then we can better evaluate if / how we can achieve the full goal.

@annevk do you think it could be reasonable to add a generic hook to DOM for getting the default passive value (as @smaug--- suggests). That hook would be the same now and for the ultimate state. Then the hacky details of how we transition can be confined to the Touch Events spec.

If you think that's plausible then we can file a DOM spec issue and start working on a PR.

@dtapuska
Copy link
Author

@BenjaminPoulain; do you have any feelings on whatwg/dom#365 ?
@teddink; do you have any feelings on whatwg/dom#365, w3c/touch-events#75 ?

Olli has promised it's on his todo list; I really value other vendors input in this space.

@BenjaminPoulain
Copy link

Maybe I missed a merge request but is there a property exposed anywhere to query and/or change the default behavior? Or is it all in w3c/touch-events#75?

Note that I have not worked in WebCore for a few years. It would be good to contact Beth Dakin for an official position.

@RByers
Copy link
Member

RByers commented Jan 12, 2017

Note that this is on track to ship in Chrome 56 - hitting stable in a few weeks.

Maybe I missed a merge request but is there a property exposed anywhere to query and/or change the default behavior

Sorry I missed this question. No there's no way to change the default, just override it by specifying {passive:false}.

@RByers RByers removed their assignment Feb 21, 2017
hubot pushed a commit to WebKit/WebKit-http that referenced this issue Aug 23, 2017
https://bugs.webkit.org/show_bug.cgi?id=175346
<rdar://problem/33164597>

Reviewed by Sam Weinig.

Source/WebCore:

Make any touchstart or touchmove event listeners passive by default
if they are on the document, window, body or document element targets.
This follows the "intervention" first implemented by Chrome/Blink:

WICG/interventions#35
https://docs.google.com/document/d/1II7oSIpd8pK91V5kEM3tDLKcIj398jOJn8Niqy6_loI/edit
whatwg/dom#365

If the event listener explicitly defines "passive" to false in their
options dictionary, then they'll still get a non-passive listener.

NOTE: Any fallout from this bug should be collected in:
https://bugs.webkit.org/show_bug.cgi?id=175869
Please do not revert this change just because a site is broken. We'll
gather the issues and see if we can evangelise or detect via code.

Tests: fast/events/touch/ios/passive-by-default-on-document-and-window.html
       fast/events/touch/ios/passive-by-default-overridden-on-document-and-window.html

* dom/EventNames.h:
(WebCore::EventNames::isTouchScrollBlockingEventType const): Added this helper
to identify the types of touches we want to check for.
* dom/EventTarget.cpp:
(WebCore::EventTarget::addEventListener): Check for the event being one of the touch-types
that we care about, and the target being one of the Node/Window types we care about. If
so, tell the event listener to be passive.
* dom/EventTarget.h: Use an optional for the passive member.
(WebCore::EventTarget::AddEventListenerOptions::AddEventListenerOptions):
* dom/EventTarget.idl: Change "passive" to not have a default value, so we
can detect if it was explicitly set to false.

LayoutTests:

* fast/events/touch/ios/passive-by-default-on-document-and-window-expected.txt: Added.
* fast/events/touch/ios/passive-by-default-on-document-and-window.html: Added.
* fast/events/touch/ios/passive-by-default-overridden-on-document-and-window-expected.txt: Added.
* fast/events/touch/ios/passive-by-default-overridden-on-document-and-window.html: Added.
* fast/events/touch/ios/tap-with-active-listener-on-window.html: Explicitly set passive to false.
* fast/events/touch/ios/touch-event-regions/document.html: Ditto.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@221092 268f45cc-cd09-0410-ab3c-d52691b4dbfc
foolip added a commit to foolip/wpt that referenced this issue May 19, 2021
These behaviors aren't backed by any spec, but these issues are linked:
WICG/interventions#35
WICG/interventions#64
@foolip
Copy link
Member

foolip commented May 19, 2021

What is the status of getting this behavior into a spec? In whatwg/dom#365 (comment) it was noted that we have tests for this in WPT, but seemingly no spec, so I've renamed the test in web-platform-tests/wpt#29039.

foolip added a commit to web-platform-tests/wpt that referenced this issue May 19, 2021
These behaviors aren't backed by any spec, but these issues are linked:
WICG/interventions#35
WICG/interventions#64
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue May 21, 2021
…e, a=testonly

Automatic update from web-platform-tests
Make 2 passive-by-default tests tentative (#29039)

These behaviors aren't backed by any spec, but these issues are linked:
WICG/interventions#35
WICG/interventions#64
--

wpt-commits: cc6fee62b855f6f93ac9c56bf763f91607d75146
wpt-pr: 29039
@johannhof
Copy link
Member

(As noted in #72, we intend to archive this repository and are thus triaging and resolving all open issues)

Standardization of this particular intervention is tracked in whatwg/dom#365 and Domenic added a comment summarizing the latest status. There's some wpt work going on in web-platform-tests/wpt#34464 now and I think we can close this issue here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants