Bug 175346 - Default passive touch event listeners on the root
Summary: Default passive touch event listeners on the root
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar, WebExposed
: 164016 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-08-08 15:02 PDT by Dean Jackson
Modified: 2019-02-06 09:18 PST (History)
12 users (show)

See Also:


Attachments
Patch (18.25 KB, patch)
2017-08-22 20:00 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (19.99 KB, patch)
2017-08-22 20:11 PDT, Dean Jackson
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2017-08-08 15:02:39 PDT
Interacting/scrolling pages on iOS is often a terrible experience because the page has poorly performing synchronous touch event handlers. This proposed intervention makes document-level listeners passive by default (meaning they can't call preventDefault to stop scrolling).

https://github.com/WICG/interventions/issues/35
https://docs.google.com/document/d/1II7oSIpd8pK91V5kEM3tDLKcIj398jOJn8Niqy6_loI/edit

https://github.com/whatwg/dom/issues/365
Comment 1 Dean Jackson 2017-08-08 15:03:00 PDT
<rdar://problem/33164597>
Comment 2 Rick Byers 2017-08-09 19:14:43 PDT
See also https://github.com/w3c/touch-events/issues/74

If WebKit ships the same behavior as Chromium then I will argue for updating the touch events spec to define it.
Comment 3 Rick Byers 2017-08-09 19:18:11 PDT
More details (and some of the debate) here: https://groups.google.com/a/chromium.org/d/msg/blink-dev/BW3qrkisqIs/v5Au-HVTAwAJ

In particular, note the detailed behavior description:
Touch Scroll blocking (touchstart, touchmove) event listeners are treated as passive="true" if added to a root node (window, document, body) and if AddEventListenerOptions passive is not specified.

Are you planning on doing the same (window, document and body) or just "document"?

Also be sure not to apply this to "touchend" (just start/move).  Cancelling touchend is important for suppressing click events etc. (eg. sites relying on old fastclick libraries).
Comment 4 Simon Fraser (smfr) 2017-08-09 19:38:47 PDT
We'll follow you.
Comment 5 Dean Jackson 2017-08-21 15:01:55 PDT
*** Bug 164016 has been marked as a duplicate of this bug. ***
Comment 6 Dean Jackson 2017-08-22 20:00:32 PDT
Created attachment 318847 [details]
Patch
Comment 7 Dean Jackson 2017-08-22 20:11:11 PDT
Created attachment 318849 [details]
Patch
Comment 8 Sam Weinig 2017-08-22 22:08:01 PDT
Comment on attachment 318849 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318849&action=review

> Source/WebCore/ChangeLog:12
> +        now in the DOM specification:

This doesn't seem to be in the DOM spec yet.  It still has passive as having a default value of false.

> Source/WebCore/ChangeLog:19
> +        If the event listener explicitly defines "passive" to false in their
> +        options dictionary, then the behaviour is overridden.

This sentence took me a while to figure out what you mean by "the behavior".

> Source/WebCore/dom/EventTarget.cpp:72
> +    std::optional<bool> passive = options.passive;

You could use auto here.
Comment 9 Simon Fraser (smfr) 2017-08-22 22:24:24 PDT
Comment on attachment 318849 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318849&action=review

>> Source/WebCore/ChangeLog:19
>> +        options dictionary, then the behaviour is overridden.
> 
> This sentence took me a while to figure out what you mean by "the behavior".

Yeah. Which behavior; the "passive by default", or do you override the request for passive=false?

> Source/WebCore/ChangeLog:24
> +        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.

Seem weird to put this in the changelog.

> Source/WebCore/dom/EventTarget.cpp:79
> +        if (auto* node = toNode()) {
> +            if (node->isDocumentNode() || node->document().documentElement() == node || node->document().body() == node)
> +                passive = true;
> +        } else if (toDOMWindow())
> +            passive = true;

Maybe flip these around because the toDOMWindow check is cheaper than the node-related checks.

> LayoutTests/fast/events/touch/ios/passive-by-default-on-document-and-window.html:98
> +<div style="height: 500vh">
> +    This is a tall element to make sure there is something to scroll.
> +</div>

Lame. Just style the body with height.

> LayoutTests/fast/events/touch/ios/passive-by-default-overridden-on-document-and-window.html:98
> +<div style="height: 500vh">
> +    This is a tall element to make sure there is something to scroll.
> +</div>

Lame.
Comment 10 David Tapuska 2017-08-23 03:37:27 PDT
Comment on attachment 318849 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318849&action=review

> Source/WebCore/ChangeLog:15
> +        https://docs.google.com/document/d/1II7oSIpd8pK91V5kEM3tDLKcIj398jOJn8Niqy6_loI/edit

Any devtools warnings? I know we generate them in chromium to help developers.
Comment 11 Dean Jackson 2017-08-23 12:15:32 PDT
Comment on attachment 318849 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318849&action=review

>> Source/WebCore/ChangeLog:12
>> +        now in the DOM specification:
> 
> This doesn't seem to be in the DOM spec yet.  It still has passive as having a default value of false.

Fixed.

>> Source/WebCore/ChangeLog:15
>> +        https://docs.google.com/document/d/1II7oSIpd8pK91V5kEM3tDLKcIj398jOJn8Niqy6_loI/edit
> 
> Any devtools warnings? I know we generate them in chromium to help developers.

We'll do that in a separate bug.

>>> Source/WebCore/ChangeLog:19
>>> +        options dictionary, then the behaviour is overridden.
>> 
>> This sentence took me a while to figure out what you mean by "the behavior".
> 
> Yeah. Which behavior; the "passive by default", or do you override the request for passive=false?

I made this more clear - you get a non-passive listener.

>> Source/WebCore/ChangeLog:24
>> +        gather the issues and see if we can evangelise or detect via code.
> 
> Seem weird to put this in the changelog.

I want to make sure that, in 3 months time, when we autospade a regression, we know what to do with it.

>> Source/WebCore/dom/EventTarget.cpp:72
>> +    std::optional<bool> passive = options.passive;
> 
> You could use auto here.

Fixed.

>> Source/WebCore/dom/EventTarget.cpp:79
>> +            passive = true;
> 
> Maybe flip these around because the toDOMWindow check is cheaper than the node-related checks.

Fixed.

>> LayoutTests/fast/events/touch/ios/passive-by-default-on-document-and-window.html:98
>> +</div>
> 
> Lame. Just style the body with height.

Fixed.
Comment 12 Dean Jackson 2017-08-23 12:23:22 PDT
Committed r221092: <http://trac.webkit.org/changeset/221092>
Comment 13 Rick Byers 2018-03-19 08:12:53 PDT
See https://bugs.webkit.org/show_bug.cgi?id=182521 for discussion of the fall-out of shipping this without also shipping touch-action.
Comment 14 Lucas Forschler 2019-02-06 09:18:55 PST
Mass move bugs into the DOM component.