Bug 158601 - Add support for passive event listeners on touch events
Summary: Add support for passive event listeners on touch events
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on: 149466
Blocks:
  Show dependency treegraph
 
Reported: 2016-06-09 20:00 PDT by Benjamin Poulain
Modified: 2016-06-10 18:17 PDT (History)
12 users (show)

See Also:


Attachments
Patch (77.15 KB, patch)
2016-06-09 20:17 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (1.05 MB, application/zip)
2016-06-09 21:20 PDT, Build Bot
no flags Details
Patch (82.76 KB, patch)
2016-06-10 14:01 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2016-06-09 20:00:35 PDT
Add support for passive event listeners on touch events
Comment 1 Benjamin Poulain 2016-06-09 20:17:27 PDT
Created attachment 280982 [details]
Patch
Comment 2 WebKit Commit Bot 2016-06-09 20:28:21 PDT
Attachment 280982 [details] did not pass style-queue:


ERROR: Source/WebKit2/Shared/Scrolling/RemoteScrollingCoordinatorTransaction.cpp:488:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Chris Dumez 2016-06-09 20:36:10 PDT
\o/
Comment 4 Build Bot 2016-06-09 21:20:26 PDT
Comment on attachment 280982 [details]
Patch

Attachment 280982 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1476207

New failing tests:
tiled-drawing/scrolling/frames/scroll-region-after-frame-layout.html
tiled-drawing/scrolling/frames/coordinated-frame.html
tiled-drawing/scrolling/frames/coordinated-frame-lose-scrolling-ancestor.html
tiled-drawing/scrolling/frames/coordinated-frame-in-fixed.html
tiled-drawing/scrolling/fixed/fixed-in-overflow.html
tiled-drawing/scrolling/frames/coordinated-frame-gain-scrolling-ancestor.html
Comment 5 Build Bot 2016-06-09 21:20:30 PDT
Created attachment 280987 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 6 Benjamin Poulain 2016-06-09 21:37:27 PDT
I'll check the WK2 tests tomorrow. Piano tonight.
Comment 7 Rick Byers 2016-06-10 07:11:21 PDT
This is awesome, thanks for working on it Ben!

One suggestion.  It looks like you're treating all touch event types the same, right?  One common scenario is to have passive touchstart and touchmove listeners but a non-passive touchend listener (so you can still prevent the generation of mouse/click events and associated behavior - eg. as is common today with fastclick libraries as much I want those to disappear).  In blink we were careful to ensure that case still didn't block scrolling (http://crbug.com/601990).  You can see this at http://rbyers.net/scroll-latency.html - set the listener to touchend, enable showing non-blocking events, and tap.  You'll get an uncancelable touchstart but a cancelable touchend event.  Basically we treat touchstart, touchmove and wheel/mousewheel specially as the only "scroll blocking events".

That's just a performance optimization so I don't think interoperability here matters much (although maybe it would be nice to spec the precise behavior here and add some web-platform-tests if we end up matching).  But figured I'd mention it in case it matters to you.
Comment 8 Simon Fraser (smfr) 2016-06-10 10:03:06 PDT
Comment on attachment 280982 [details]
Patch

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

> Source/WebCore/ChangeLog:32
> +        If a Target has multiple listener for an event type, we want to know

listeners

> Source/WebCore/dom/EventTarget.cpp:252
> +    return hasActiveEventListeners(names.touchstartEvent)
> +        || hasActiveEventListeners(names.touchmoveEvent)
> +        || hasActiveEventListeners(names.touchendEvent)
> +        || hasActiveEventListeners(names.touchcancelEvent)
> +        || hasActiveEventListeners(names.touchforcechangeEvent);

Maybe adjust this given Rick's comment.

> Source/WebCore/page/DebugPageOverlays.cpp:137
> +        if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator()) {
> +            EventTrackingRegions eventTrackingRegions = scrollingCoordinator->absoluteEventTrackingRegions();
> +            *region = eventTrackingRegions.synchronousDispatchRegion;
> +        }

We could fix the overlay to show both regions.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1994
> +        // But, here we know that every events handlers that can handle this events are passive.

"every events handlers" -> "all events handlers"
Comment 9 Benjamin Poulain 2016-06-10 11:43:15 PDT
(In reply to comment #7)
> One suggestion.  It looks like you're treating all touch event types the
> same, right?  One common scenario is to have passive touchstart and
> touchmove listeners but a non-passive touchend listener (so you can still
> prevent the generation of mouse/click events and associated behavior - eg.
> as is common today with fastclick libraries as much I want those to
> disappear).  In blink we were careful to ensure that case still didn't block
> scrolling (http://crbug.com/601990).  You can see this at
> http://rbyers.net/scroll-latency.html - set the listener to touchend, enable
> showing non-blocking events, and tap.  You'll get an uncancelable touchstart
> but a cancelable touchend event.  Basically we treat touchstart, touchmove
> and wheel/mousewheel specially as the only "scroll blocking events".

That's a very good point.
Comment 10 Benjamin Poulain 2016-06-10 14:01:20 PDT
Created attachment 281044 [details]
Patch
Comment 11 WebKit Commit Bot 2016-06-10 14:03:29 PDT
Attachment 281044 [details] did not pass style-queue:


ERROR: Source/WebKit2/Shared/Scrolling/RemoteScrollingCoordinatorTransaction.cpp:488:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Benjamin Poulain 2016-06-10 18:17:06 PDT
Comment on attachment 281044 [details]
Patch

Clearing flags on attachment: 281044

Committed r201958: <http://trac.webkit.org/changeset/201958>
Comment 13 Benjamin Poulain 2016-06-10 18:17:11 PDT
All reviewed patches have been landed.  Closing bug.