WICG / interventions

A place for browsers and web developers to collaborate on user agent interventions.
Other
176 stars 28 forks source link

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

Closed dtapuska closed 2 years ago

dtapuska commented 7 years ago

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 commented 7 years ago

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

PTAL as I value feedback from everyone.

smaug---- commented 7 years ago

So how is this different to #18 ? or https://github.com/w3c/touch-events/issues/74 ?

dtapuska commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

foolip commented 7 years ago

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

dtapuska commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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---- commented 7 years ago

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---- commented 7 years ago

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 commented 7 years ago

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

bzbarsky commented 7 years ago

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 commented 7 years ago

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

bzbarsky commented 7 years ago

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

foolip commented 7 years ago

https://github.com/WICG/interventions/issues/35#issuecomment-256384626 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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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}.

foolip commented 3 years ago

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

johannhof commented 2 years ago

(As noted in https://github.com/WICG/interventions/pull/72, we intend to archive this repository and are thus triaging and resolving all open issues)

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