From e4c06c358b652d6560b69720c0fcae1cd2e36955 Mon Sep 17 00:00:00 2001 From: David Iglesias Teixeira Date: Fri, 28 Jun 2024 12:46:48 -0700 Subject: [PATCH 1/3] [web] Stop 'touchstart' events. This prevents (mobile) browsers from sending a 'touchcancel' event shortly after users start to drag on a scrollable list, when the app is embedded in a host element. This PR also ensures: * all pointer events are registered on the _viewTarget. This uses `setPointerCapture` so move/up/cancel events keep being reported even when the pointer leaves the _viewTarget. * pointer events are registered only ONCE :) --- lib/web_ui/lib/src/engine/dom.dart | 5 +++ .../lib/src/engine/pointer_binding.dart | 38 +++++++++++++------ 2 files changed, 32 insertions(+), 11 deletions(-) diff --git a/lib/web_ui/lib/src/engine/dom.dart b/lib/web_ui/lib/src/engine/dom.dart index b11e9e97a7458..059b66bf201e9 100644 --- a/lib/web_ui/lib/src/engine/dom.dart +++ b/lib/web_ui/lib/src/engine/dom.dart @@ -431,6 +431,9 @@ extension DomEventExtension on DomEvent { external JSString get _type; String get type => _type.toDart; + external JSBoolean? get _cancelable; + bool get cancelable => _cancelable?.toDart ?? true; + external JSVoid preventDefault(); external JSVoid stopPropagation(); @@ -720,6 +723,8 @@ extension DomElementExtension on DomElement { removeChild(firstChild!); } } + + external void setPointerCapture(num? pointerId); } @JS() diff --git a/lib/web_ui/lib/src/engine/pointer_binding.dart b/lib/web_ui/lib/src/engine/pointer_binding.dart index f0b2b75c8e521..2f7afc60c9003 100644 --- a/lib/web_ui/lib/src/engine/pointer_binding.dart +++ b/lib/web_ui/lib/src/engine/pointer_binding.dart @@ -83,7 +83,11 @@ class SafariPointerEventWorkaround { // We only need to attach the listener once. if (_listener == null) { _listener = createDomEventListener((_) {}); - domDocument.addEventListener('touchstart', _listener); + domDocument.addEventListenerWithOptions( + 'touchstart', _listener!, + { + 'passive': true, + }); } } @@ -470,7 +474,7 @@ class _Listener { target: target, handler: jsHandler, ); - target.addEventListener(event, jsHandler); + return listener; } @@ -501,7 +505,7 @@ abstract class _BaseAdapter { bool _lastWheelEventWasTrackpad = false; bool _lastWheelEventAllowedDefault = false; - DomEventTarget get _viewTarget => _view.dom.rootElement; + DomElement get _viewTarget => _view.dom.rootElement; DomEventTarget get _globalTarget => _view.embeddingStrategy.globalEventTarget; /// Each subclass is expected to override this method to attach its own event @@ -518,10 +522,12 @@ abstract class _BaseAdapter { /// Adds a listener for the given [eventName] to [target]. /// - /// Generally speaking, down and leave events should use [_rootElement] - /// as the [target], while move and up events should use [domWindow] - /// instead, because the browser doesn't fire the latter two for DOM elements - /// when the pointer is outside the window. + /// In general, all events should use `_viewTarget` as their `target`. + /// In order for the browser to fire 'move' and 'up' events outside of the app, + /// we must `setPointerCapture` on the `target` on 'down'/'start' events. + /// + /// See: https://developer.mozilla.org/en-US/docs/Web/API/Element/setPointerCapture + /// See also: https://jsfiddle.net/ditman/7towxaqp void addEventListener( DomEventTarget target, String eventName, @@ -966,8 +972,20 @@ class _PointerAdapter extends _BaseAdapter with _WheelEventListenerMixin { @override void setup() { + // Prevents the browser auto-canceling pointer events. + _addPointerEventListener(_viewTarget, 'touchstart', (DomEvent event) { + // This is one of the ways I've seen this done. PixiJS does a similar thing. + // ThreeJS seems to subscribe move/leave in the pointerdown handler instead? + if (event.cancelable) { + event.preventDefault(); + } + }, checkModifiers: false); + _addPointerEventListener(_viewTarget, 'pointerdown', (DomPointerEvent event) { final int device = _getPointerId(event); + // Ensure pointer events for `event.pointerId` are relative to `_viewTarget`. + // See this fiddle: https://jsfiddle.net/ditman/7towxaqp + // _viewTarget.setPointerCapture(event.pointerId); final List pointerData = []; final _ButtonSanitizer sanitizer = _ensureSanitizer(device); final _SanitizedDetails? up = @@ -984,8 +1002,7 @@ class _PointerAdapter extends _BaseAdapter with _WheelEventListenerMixin { _callback(event, pointerData); }); - // Why `domWindow` you ask? See this fiddle: https://jsfiddle.net/ditman/7towxaqp - _addPointerEventListener(_globalTarget, 'pointermove', (DomPointerEvent event) { + _addPointerEventListener(_viewTarget, 'pointermove', (DomPointerEvent event) { final int device = _getPointerId(event); final _ButtonSanitizer sanitizer = _ensureSanitizer(device); final List pointerData = []; @@ -1012,8 +1029,7 @@ class _PointerAdapter extends _BaseAdapter with _WheelEventListenerMixin { } }, checkModifiers: false); - // TODO(dit): This must happen in the flutterViewElement, https://github.com/flutter/flutter/issues/116561 - _addPointerEventListener(_globalTarget, 'pointerup', (DomPointerEvent event) { + _addPointerEventListener(_viewTarget, 'pointerup', (DomPointerEvent event) { final int device = _getPointerId(event); if (_hasSanitizer(device)) { final List pointerData = []; From 47dc0f2f8e68d47fa00294b0dc3d49afbbff2fc7 Mon Sep 17 00:00:00 2001 From: David Iglesias Teixeira Date: Fri, 28 Jun 2024 16:08:27 -0700 Subject: [PATCH 2/3] Simplify solution. --- .../lib/src/engine/pointer_binding.dart | 25 +++++++------------ 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/lib/web_ui/lib/src/engine/pointer_binding.dart b/lib/web_ui/lib/src/engine/pointer_binding.dart index 2f7afc60c9003..0e09d89aedead 100644 --- a/lib/web_ui/lib/src/engine/pointer_binding.dart +++ b/lib/web_ui/lib/src/engine/pointer_binding.dart @@ -83,11 +83,7 @@ class SafariPointerEventWorkaround { // We only need to attach the listener once. if (_listener == null) { _listener = createDomEventListener((_) {}); - domDocument.addEventListenerWithOptions( - 'touchstart', _listener!, - { - 'passive': true, - }); + domDocument.addEventListener('touchstart', _listener); } } @@ -522,12 +518,10 @@ abstract class _BaseAdapter { /// Adds a listener for the given [eventName] to [target]. /// - /// In general, all events should use `_viewTarget` as their `target`. - /// In order for the browser to fire 'move' and 'up' events outside of the app, - /// we must `setPointerCapture` on the `target` on 'down'/'start' events. - /// - /// See: https://developer.mozilla.org/en-US/docs/Web/API/Element/setPointerCapture - /// See also: https://jsfiddle.net/ditman/7towxaqp + /// Generally speaking, down and leave events should use [_rootElement] + /// as the [target], while move and up events should use [domWindow] + /// instead, because the browser doesn't fire the latter two for DOM elements + /// when the pointer is outside the window. void addEventListener( DomEventTarget target, String eventName, @@ -983,9 +977,6 @@ class _PointerAdapter extends _BaseAdapter with _WheelEventListenerMixin { _addPointerEventListener(_viewTarget, 'pointerdown', (DomPointerEvent event) { final int device = _getPointerId(event); - // Ensure pointer events for `event.pointerId` are relative to `_viewTarget`. - // See this fiddle: https://jsfiddle.net/ditman/7towxaqp - // _viewTarget.setPointerCapture(event.pointerId); final List pointerData = []; final _ButtonSanitizer sanitizer = _ensureSanitizer(device); final _SanitizedDetails? up = @@ -1002,7 +993,8 @@ class _PointerAdapter extends _BaseAdapter with _WheelEventListenerMixin { _callback(event, pointerData); }); - _addPointerEventListener(_viewTarget, 'pointermove', (DomPointerEvent event) { + // Why `domWindow` you ask? See this fiddle: https://jsfiddle.net/ditman/7towxaqp + _addPointerEventListener(_globalTarget, 'pointermove', (DomPointerEvent event) { final int device = _getPointerId(event); final _ButtonSanitizer sanitizer = _ensureSanitizer(device); final List pointerData = []; @@ -1029,7 +1021,8 @@ class _PointerAdapter extends _BaseAdapter with _WheelEventListenerMixin { } }, checkModifiers: false); - _addPointerEventListener(_viewTarget, 'pointerup', (DomPointerEvent event) { + // TODO(dit): This must happen in the flutterViewElement, https://github.com/flutter/flutter/issues/116561 + _addPointerEventListener(_globalTarget, 'pointerup', (DomPointerEvent event) { final int device = _getPointerId(event); if (_hasSanitizer(device)) { final List pointerData = []; From 729999ad35708ee230aa7f98eeb4651e80c6139e Mon Sep 17 00:00:00 2001 From: David Iglesias Teixeira Date: Fri, 28 Jun 2024 18:22:02 -0700 Subject: [PATCH 3/3] Add tests. --- .../lib/src/engine/pointer_binding.dart | 37 ++++++----- .../test/engine/pointer_binding_test.dart | 66 +++++++++++++++++++ 2 files changed, 88 insertions(+), 15 deletions(-) diff --git a/lib/web_ui/lib/src/engine/pointer_binding.dart b/lib/web_ui/lib/src/engine/pointer_binding.dart index 0e09d89aedead..f51c16a0d7e20 100644 --- a/lib/web_ui/lib/src/engine/pointer_binding.dart +++ b/lib/web_ui/lib/src/engine/pointer_binding.dart @@ -432,8 +432,10 @@ class PointerSupportDetector { String toString() => 'pointers:$hasPointerEvents'; } -class _Listener { - _Listener._({ +/// Encapsulates a DomEvent registration so it can be easily unregistered later. +@visibleForTesting +class Listener { + Listener._({ required this.event, required this.target, required this.handler, @@ -448,7 +450,7 @@ class _Listener { /// associated with this event. If `passive` is false, the browser will wait /// for the handler to finish execution before performing the respective /// action. - factory _Listener.register({ + factory Listener.register({ required String event, required DomEventTarget target, required DartDomEventListener handler, @@ -465,7 +467,7 @@ class _Listener { target.addEventListenerWithOptions(event, jsHandler, eventOptions); } - final _Listener listener = _Listener._( + final Listener listener = Listener._( event: event, target: target, handler: jsHandler, @@ -496,7 +498,7 @@ abstract class _BaseAdapter { PointerDataConverter get _pointerDataConverter => _owner._pointerDataConverter; KeyboardConverter? get _keyboardConverter => _owner._keyboardConverter; - final List<_Listener> _listeners = <_Listener>[]; + final List _listeners = []; DomWheelEvent? _lastWheelEvent; bool _lastWheelEventWasTrackpad = false; bool _lastWheelEventAllowedDefault = false; @@ -510,7 +512,7 @@ abstract class _BaseAdapter { /// Cleans up all event listeners attached by this adapter. void dispose() { - for (final _Listener listener in _listeners) { + for (final Listener listener in _listeners) { listener.unregister(); } _listeners.clear(); @@ -546,7 +548,7 @@ abstract class _BaseAdapter { handler(event); } } - _listeners.add(_Listener.register( + _listeners.add(Listener.register( event: eventName, target: target, handler: loggedHandler, @@ -719,7 +721,7 @@ mixin _WheelEventListenerMixin on _BaseAdapter { } void _addWheelEventListener(DartDomEventListener handler) { - _listeners.add(_Listener.register( + _listeners.add(Listener.register( event: 'wheel', target: _viewTarget, handler: handler, @@ -967,13 +969,18 @@ class _PointerAdapter extends _BaseAdapter with _WheelEventListenerMixin { @override void setup() { // Prevents the browser auto-canceling pointer events. - _addPointerEventListener(_viewTarget, 'touchstart', (DomEvent event) { - // This is one of the ways I've seen this done. PixiJS does a similar thing. - // ThreeJS seems to subscribe move/leave in the pointerdown handler instead? - if (event.cancelable) { - event.preventDefault(); - } - }, checkModifiers: false); + // Register into `_listener` directly so the event isn't forwarded to semantics. + _listeners.add(Listener.register( + event: 'touchstart', + target: _viewTarget, + handler: (DomEvent event) { + // This is one of the ways I've seen this done. PixiJS does a similar thing. + // ThreeJS seems to subscribe move/leave in the pointerdown handler instead? + if (event.cancelable) { + event.preventDefault(); + } + }, + )); _addPointerEventListener(_viewTarget, 'pointerdown', (DomPointerEvent event) { final int device = _getPointerId(event); diff --git a/lib/web_ui/test/engine/pointer_binding_test.dart b/lib/web_ui/test/engine/pointer_binding_test.dart index 04cbaeba92cc1..e8d90c465836a 100644 --- a/lib/web_ui/test/engine/pointer_binding_test.dart +++ b/lib/web_ui/test/engine/pointer_binding_test.dart @@ -305,6 +305,18 @@ void testMain() { }, ); + test('prevents default on touchstart events', () async { + final event = createDomEvent('Event', 'touchstart'); + + rootElement.dispatchEvent(event); + + expect( + event.defaultPrevented, + isTrue, + reason: 'touchstart events should be prevented so pointer events are not cancelled later.', + ); + }); + test( 'can receive pointer events on the app root', () { @@ -2670,6 +2682,60 @@ void testMain() { ); }); + group('Listener', () { + late DomElement eventTarget; + late DomEvent expected; + late bool handled; + + setUp(() { + eventTarget = createDomElement('div'); + expected = createDomEvent('Event', 'custom-event'); + handled = false; + }); + + test('listeners can be registered', () { + Listener.register( + event: 'custom-event', + target: eventTarget, + handler: (event) { + expect(event, expected); + handled = true; + }, + ); + + // Trigger the event... + eventTarget.dispatchEvent(expected); + expect(handled, isTrue); + }); + + test('listeners can be unregistered', () { + final Listener listener = Listener.register( + event: 'custom-event', + target: eventTarget, + handler: (event) { + handled = true; + }, + ); + listener.unregister(); + + eventTarget.dispatchEvent(expected); + expect(handled, isFalse); + }); + + test('listeners are registered only once', () { + int timesHandled = 0; + Listener.register( + event: 'custom-event', + target: eventTarget, + handler: (event) { + timesHandled++; + }, + ); + eventTarget.dispatchEvent(expected); + expect(timesHandled, 1, reason: 'The handler ran multiple times for a single event.'); + }); + }); + group('ClickDebouncer', () { _testClickDebouncer(getBinding: () => instance); });