From 6dee3dd14ebb7e0834d4e533847c05fc3e5a8145 Mon Sep 17 00:00:00 2001 From: Yegor Date: Thu, 3 Aug 2023 14:43:19 -0700 Subject: [PATCH] [web] fix clicks on merged semantic nodes (flutter/engine#43620) This should significantly improve the situation described in https://github.com/flutter/flutter/issues/130162. When the framework merges semantics trees of multiple widgets into a single node, it can expand the tap area of one of the descendants to the size of the combined node as follows: Screenshot 2023-07-07 at 4 11 30 PM When a screen reader taps on the combined node, the pointer events and the click all land in the center. This produces a stalemate resulting in the user action never producing a tap/click gesture in the framework: * The web engine, seeing `pointerdown`/`pointerup`, switches to the gesture mode ignores the `click`, believing that the framework will interpret the pointer events as one. * The framework, seeing pointer events landing outside the checkbox, never reacts to the pointer events. This PR mostly solves the issue by delaying event sequences starting with pointerdown for up to 200ms. If within those 200ms a click is observed, `SemanticsAction.tap` is sent to the framework. Otherwise, the pointer events are sent to the framework and the DOM `click` event is dropped. The tradeoff is that even when the drag gesture detector is the only one present, there's a 200ms delay before dragging can start. However, it seems to be a better trade-off than missing clicks entirely. --- .../lib/src/engine/pointer_binding.dart | 341 +++++++++++++--- .../lib/src/engine/semantics/semantics.dart | 5 +- .../lib/src/engine/semantics/tappable.dart | 53 +-- .../lib/web_ui/test/common/matchers.dart | 20 +- .../test/engine/pointer_binding_test.dart | 379 ++++++++++++++++++ .../test/engine/semantics/semantics_test.dart | 46 +-- .../engine/semantics/semantics_tester.dart | 21 + 7 files changed, 750 insertions(+), 115 deletions(-) diff --git a/engine/src/flutter/lib/web_ui/lib/src/engine/pointer_binding.dart b/engine/src/flutter/lib/web_ui/lib/src/engine/pointer_binding.dart index e2305c50b5e..e3f43fde447 100644 --- a/engine/src/flutter/lib/web_ui/lib/src/engine/pointer_binding.dart +++ b/engine/src/flutter/lib/web_ui/lib/src/engine/pointer_binding.dart @@ -2,6 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'dart:async'; import 'dart:js_interop'; import 'dart:math' as math; @@ -25,7 +26,7 @@ const bool _debugLogPointerEvents = false; const bool _debugLogFlutterEvents = false; /// The signature of a callback that handles pointer events. -typedef _PointerDataCallback = void Function(Iterable); +typedef _PointerDataCallback = void Function(DomEvent event, List); // The mask for the bitfield of event buttons. Buttons not contained in this // mask are cut off. @@ -103,11 +104,14 @@ class PointerBinding { } } + final ClickDebouncer clickDebouncer = ClickDebouncer(); + /// Performs necessary clean up for PointerBinding including removing event listeners /// and clearing the existing pointer state void dispose() { _adapter.clearListeners(); _pointerDataConverter.clearPointerState(); + clickDebouncer.reset(); } final DomElement flutterViewElement; @@ -156,20 +160,247 @@ class PointerBinding { // TODO(dit): remove old API fallbacks, https://github.com/flutter/flutter/issues/116141 _BaseAdapter _createAdapter() { if (_detector.hasPointerEvents) { - return _PointerAdapter(_onPointerData, flutterViewElement, _pointerDataConverter, _keyboardConverter); + return _PointerAdapter(clickDebouncer.onPointerData, flutterViewElement, _pointerDataConverter, _keyboardConverter); } // Fallback for Safari Mobile < 13. To be removed. if (_detector.hasTouchEvents) { - return _TouchAdapter(_onPointerData, flutterViewElement, _pointerDataConverter, _keyboardConverter); + return _TouchAdapter(clickDebouncer.onPointerData, flutterViewElement, _pointerDataConverter, _keyboardConverter); } // Fallback for Safari Desktop < 13. To be removed. if (_detector.hasMouseEvents) { - return _MouseAdapter(_onPointerData, flutterViewElement, _pointerDataConverter, _keyboardConverter); + return _MouseAdapter(clickDebouncer.onPointerData, flutterViewElement, _pointerDataConverter, _keyboardConverter); } throw UnsupportedError('This browser does not support pointer, touch, or mouse events.'); } +} - void _onPointerData(Iterable data) { +@visibleForTesting +typedef QueuedEvent = ({ DomEvent event, Duration timeStamp, List data }); + +@visibleForTesting +typedef DebounceState = ({ + DomElement target, + Timer timer, + List queue, +}); + +/// Disambiguates taps and clicks that are produced both by the framework from +/// `pointerdown`/`pointerup` events and those detected as DOM "click" events by +/// the browser. +/// +/// The implementation is waiting for a `pointerdown`, and as soon as it sees +/// one stops forwarding pointer events to the framework, and instead queues +/// them in a list. The queuing process stops as soon as one of the following +/// two conditions happens first: +/// +/// * 200ms passes after the `pointerdown` event. Most clicks, even slow ones, +/// are typically done by then. Importantly, screen readers simulate clicks +/// much faster than 200ms. So if the timer expires, it is likely the user is +/// not interested in producing a click, so the debouncing process stops and +/// all queued events are forwarded to the framework. If, for example, a +/// tappable node is inside a scrollable viewport, the events can be +/// intrepreted by the framework to initiate scrolling. +/// * A `click` event arrives. If the event queue has not been flushed to the +/// framework, the event is forwarded to the framework as a +/// `SemanticsAction.tap`, and all the pointer events are dropped. If, by the +/// time the click event arrives, the queue was flushed (but no more than 50ms +/// ago), then the click event is dropped instead under the assumption that +/// the flushed pointer events are interpreted by the framework as the desired +/// gesture. +/// +/// This mechanism is in place to deal with https://github.com/flutter/flutter/issues/130162. +class ClickDebouncer { + DebounceState? _state; + + @visibleForTesting + DebounceState? get debugState => _state; + + // The timestamp of the last "pointerup" DOM event that was flushed. + // + // Not to be confused with the time when it was flushed. The two may be far + // apart because the flushing can happen after a delay due to timer, or events + // that happen after the said "pointerup". + Duration? _lastFlushedPointerUpTimeStamp; + + /// Returns true if the debouncer has a non-empty queue of pointer events that + /// were withheld from the framework. + /// + /// This value is normally false, and it flips to true when the first + /// pointerdown is observed that lands on a tappable semantics node, denoted + /// by the presence of the `flt-tappable` attribute. + bool get isDebouncing => _state != null; + + /// Processes a pointer event. + /// + /// If semantics are off, simply forwards the event to the framework. + /// + /// If currently debouncing events (see [isDebouncing]), adds the event to + /// the debounce queue, unless the target of the event is different from the + /// target that initiated the debouncing process, in which case stops + /// debouncing and flushes pointer events to the framework. + /// + /// If the event is a `pointerdown` and the target is `flt-tappable`, begins + /// debouncing events. + /// + /// In all other situations forwards the event to the framework. + void onPointerData(DomEvent event, List data) { + if (!EnginePlatformDispatcher.instance.semanticsEnabled) { + _sendToFramework(event, data); + return; + } + + if (isDebouncing) { + _debounce(event, data); + } else if (event.type == 'pointerdown') { + _startDebouncing(event, data); + } else { + _sendToFramework(event, data); + } + } + + /// Notifies the debouncer of the browser-detected "click" DOM event. + /// + /// Forwards the event to the framework, unless it is deduplicated because + /// the corresponding pointer down/up events were recently flushed to the + /// framework already. + void onClick(DomEvent click, int semanticsNodeId, bool isListening) { + assert(click.type == 'click'); + + if (!isDebouncing) { + // There's no pending queue of pointer events that are being debounced. It + // is a standalone click event. Unless pointer down/up were flushed + // recently and if the node is currently listening to event, forward to + // the framework. + if (isListening && _shouldSendClickEventToFramework(click)) { + EnginePlatformDispatcher.instance.invokeOnSemanticsAction( + semanticsNodeId, ui.SemanticsAction.tap, null); + } + return; + } + + if (isListening) { + // There's a pending queue of pointer events. Prefer sending the tap action + // instead of pointer events, because the pointer events may not land on the + // combined semantic node and miss the click/tap. + final DebounceState state = _state!; + _state = null; + state.timer.cancel(); + EnginePlatformDispatcher.instance.invokeOnSemanticsAction( + semanticsNodeId, ui.SemanticsAction.tap, null); + } else { + // The semantic node is not listening to taps. Flush the pointer events + // for the framework to figure out what to do with them. It's possible + // the framework is interested in gestures other than taps. + _flush(); + } + } + + void _startDebouncing(DomEvent event, List data) { + assert( + _state == null, + 'Cannot start debouncing. Already debouncing.' + ); + assert( + event.type == 'pointerdown', + 'Click debouncing must begin with a pointerdown' + ); + + final DomEventTarget? target = event.target; + if (target is DomElement && target.hasAttribute('flt-tappable')) { + _state = ( + target: target, + // The 200ms duration was chosen empirically by testing tapping, mouse + // clicking, trackpad tapping and clicking, as well as the following + // screen readers: TalkBack on Android, VoiceOver on macOS, Narrator/ + // NVDA/JAWS on Windows. 200ms seemed to hit the sweet spot by + // satisfying the following: + // * It was short enough that delaying the `pointerdown` still allowed + // drag gestures to begin reasonably soon (e.g. scrolling). + // * It was long enough to register taps and clicks. + // * It was successful at detecting taps generated by all tested + // screen readers. + timer: Timer(const Duration(milliseconds: 200), _onTimerExpired), + queue: [( + event: event, + timeStamp: _BaseAdapter._eventTimeStampToDuration(event.timeStamp!), + data: data, + )], + ); + } else { + // The event landed on an non-tappable target. Assume this won't lead to + // double clicks and forward the event to the framework. + _sendToFramework(event, data); + } + } + + void _debounce(DomEvent event, List data) { + assert( + _state != null, + 'Cannot debounce event. Debouncing state not established by _startDebouncing.' + ); + + final DebounceState state = _state!; + state.queue.add(( + event: event, + timeStamp: _BaseAdapter._eventTimeStampToDuration(event.timeStamp!), + data: data, + )); + + // It's only interesting to debounce clicks when both `pointerdown` and + // `pointerup` land on the same element. + if (event.type == 'pointerup') { + // TODO(yjbanov): this is a bit mouthful, but see https://github.com/dart-lang/sdk/issues/53070 + final DomEventTarget? eventTarget = event.target; + final DomElement stateTarget = state.target; + final bool targetChanged = eventTarget != stateTarget; + if (targetChanged) { + _flush(); + } + } + } + + void _onTimerExpired() { + if (!isDebouncing) { + return; + } + _flush(); + } + + // If the click event happens soon after the last `pointerup` event that was + // already flushed to the framework, the click event is dropped to avoid + // double click. + bool _shouldSendClickEventToFramework(DomEvent click) { + final Duration? lastFlushedPointerUpTimeStamp = _lastFlushedPointerUpTimeStamp; + + if (lastFlushedPointerUpTimeStamp == null) { + // We haven't seen a pointerup. It's standalone click event. Let it through. + return true; + } + + final Duration clickTimeStamp = _BaseAdapter._eventTimeStampToDuration(click.timeStamp!); + final Duration delta = clickTimeStamp - lastFlushedPointerUpTimeStamp; + return delta >= const Duration(milliseconds: 50); + } + + void _flush() { + assert(_state != null); + + final DebounceState state = _state!; + state.timer.cancel(); + + final List aggregateData = []; + for (final QueuedEvent queuedEvent in state.queue) { + if (queuedEvent.event.type == 'pointerup') { + _lastFlushedPointerUpTimeStamp = queuedEvent.timeStamp; + } + aggregateData.addAll(queuedEvent.data); + } + + _sendToFramework(null, aggregateData); + _state = null; + } + + void _sendToFramework(DomEvent? event, List data) { final ui.PointerDataPacket packet = ui.PointerDataPacket(data: data.toList()); if (_debugLogFlutterEvents) { for(final ui.PointerData datum in data) { @@ -178,6 +409,16 @@ class PointerBinding { } EnginePlatformDispatcher.instance.invokeOnPointerDataPacket(packet); } + + /// Cancels any pending debounce process and forgets anything that happened so + /// far. + /// + /// This object can be used as if it was just initialized. + void reset() { + _state?.timer.cancel(); + _state = null; + _lastFlushedPointerUpTimeStamp = null; + } } class PointerSupportDetector { @@ -198,65 +439,50 @@ class _Listener { required this.target, required this.handler, required this.useCapture, - required this.isNative, }); - /// Registers a listener for the given [event] on [target] using the Dart-to-JS API. + /// Registers a listener for the given `event` on a `target`. + /// + /// If `passive` is null uses the default behavior determined by the event + /// type. If `passive` is true, marks the handler as non-blocking for the + /// built-in browser behavior. This means the browser will not wait for the + /// handler to finish execution before performing the default action + /// 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({ required String event, required DomEventTarget target, required DartDomEventListener handler, bool capture = false, + bool? passive, }) { final DomEventListener jsHandler = createDomEventListener(handler); - final _Listener listener = _Listener._( - event: event, - target: target, - handler: jsHandler, - useCapture: capture, - isNative: false, - ); - target.addEventListener(event, jsHandler, capture); - return listener; - } + if (passive == null) { + target.addEventListener(event, jsHandler, capture); + } else { + final Map eventOptions = { + 'capture': capture, + 'passive': passive, + }; + target.addEventListenerWithOptions(event, jsHandler, eventOptions); + } - /// Registers a listener for the given [event] on [target] using the native JS API. - factory _Listener.registerNative({ - required String event, - required DomEventTarget target, - required DomEventListener jsHandler, - bool capture = false, - bool passive = false, - }) { - final Map eventOptions = { - 'capture': capture, - 'passive': passive, - }; - final _Listener listener = _Listener._( + return _Listener._( event: event, target: target, handler: jsHandler, useCapture: capture, - isNative: true, ); - target.addEventListenerWithOptions(event, jsHandler, eventOptions); - return listener; } final String event; - final DomEventTarget target; final DomEventListener handler; - final bool useCapture; - final bool isNative; void unregister() { - if (isNative) { - target.removeEventListener(event, handler, useCapture); - } else { - target.removeEventListener(event, handler, useCapture); - } + target.removeEventListener(event, handler, useCapture); } } @@ -496,10 +722,11 @@ mixin _WheelEventListenerMixin on _BaseAdapter { } void _addWheelEventListener(DartDomEventListener handler) { - _listeners.add(_Listener.registerNative( + _listeners.add(_Listener.register( event: 'wheel', target: flutterViewElement, - jsHandler: createDomEventListener(handler), + handler: handler, + passive: false, )); } @@ -509,7 +736,7 @@ mixin _WheelEventListenerMixin on _BaseAdapter { if (_debugLogPointerEvents) { print(event.type); } - _callback(_convertWheelEventToPointerData(event)); + _callback(e, _convertWheelEventToPointerData(event)); // Prevent default so mouse wheel event doesn't get converted to // a scroll event that semantic nodes would process. // @@ -759,7 +986,7 @@ class _PointerAdapter extends _BaseAdapter with _WheelEventListenerMixin { buttons: event.buttons!.toInt(), ); _convertEventsToPointerData(data: pointerData, event: event, details: down); - _callback(pointerData); + _callback(event, pointerData); }); // Why `domWindow` you ask? See this fiddle: https://jsfiddle.net/ditman/7towxaqp @@ -776,7 +1003,7 @@ class _PointerAdapter extends _BaseAdapter with _WheelEventListenerMixin { final _SanitizedDetails move = sanitizer.sanitizeMoveEvent(buttons: event.buttons!.toInt()); _convertEventsToPointerData(data: pointerData, event: event, details: move); } - _callback(pointerData); + _callback(event, pointerData); }); _addPointerEventListener(flutterViewElement, 'pointerleave', (DomPointerEvent event) { @@ -786,7 +1013,7 @@ class _PointerAdapter extends _BaseAdapter with _WheelEventListenerMixin { final _SanitizedDetails? details = sanitizer.sanitizeLeaveEvent(buttons: event.buttons!.toInt()); if (details != null) { _convertEventsToPointerData(data: pointerData, event: event, details: details); - _callback(pointerData); + _callback(event, pointerData); } }, useCapture: false, checkModifiers: false); @@ -799,7 +1026,7 @@ class _PointerAdapter extends _BaseAdapter with _WheelEventListenerMixin { _removePointerIfUnhoverable(event); if (details != null) { _convertEventsToPointerData(data: pointerData, event: event, details: details); - _callback(pointerData); + _callback(event, pointerData); } } }); @@ -815,7 +1042,7 @@ class _PointerAdapter extends _BaseAdapter with _WheelEventListenerMixin { final _SanitizedDetails details = _getSanitizer(device).sanitizeCancelEvent(); _removePointerIfUnhoverable(event); _convertEventsToPointerData(data: pointerData, event: event, details: details); - _callback(pointerData); + _callback(event, pointerData); } }, checkModifiers: false); @@ -954,7 +1181,7 @@ class _TouchAdapter extends _BaseAdapter { ); } } - _callback(pointerData); + _callback(event, pointerData); }); _addTouchEventListener(flutterViewElement, 'touchmove', (DomTouchEvent event) { @@ -973,7 +1200,7 @@ class _TouchAdapter extends _BaseAdapter { ); } } - _callback(pointerData); + _callback(event, pointerData); }); _addTouchEventListener(flutterViewElement, 'touchend', (DomTouchEvent event) { @@ -995,7 +1222,7 @@ class _TouchAdapter extends _BaseAdapter { ); } } - _callback(pointerData); + _callback(event, pointerData); }); _addTouchEventListener(flutterViewElement, 'touchcancel', (DomTouchEvent event) { @@ -1014,7 +1241,7 @@ class _TouchAdapter extends _BaseAdapter { ); } } - _callback(pointerData); + _callback(event, pointerData); }); } @@ -1112,7 +1339,7 @@ class _MouseAdapter extends _BaseAdapter with _WheelEventListenerMixin { buttons: event.buttons!.toInt(), ); _convertEventsToPointerData(data: pointerData, event: event, details: sanitizedDetails); - _callback(pointerData); + _callback(event, pointerData); }); // Why `domWindow` you ask? See this fiddle: https://jsfiddle.net/ditman/7towxaqp @@ -1124,7 +1351,7 @@ class _MouseAdapter extends _BaseAdapter with _WheelEventListenerMixin { } final _SanitizedDetails move = _sanitizer.sanitizeMoveEvent(buttons: event.buttons!.toInt()); _convertEventsToPointerData(data: pointerData, event: event, details: move); - _callback(pointerData); + _callback(event, pointerData); }); _addMouseEventListener(flutterViewElement, 'mouseleave', (DomMouseEvent event) { @@ -1132,7 +1359,7 @@ class _MouseAdapter extends _BaseAdapter with _WheelEventListenerMixin { final _SanitizedDetails? details = _sanitizer.sanitizeLeaveEvent(buttons: event.buttons!.toInt()); if (details != null) { _convertEventsToPointerData(data: pointerData, event: event, details: details); - _callback(pointerData); + _callback(event, pointerData); } }, useCapture: false); @@ -1142,7 +1369,7 @@ class _MouseAdapter extends _BaseAdapter with _WheelEventListenerMixin { final _SanitizedDetails? sanitizedDetails = _sanitizer.sanitizeUpEvent(buttons: event.buttons?.toInt()); if (sanitizedDetails != null) { _convertEventsToPointerData(data: pointerData, event: event, details: sanitizedDetails); - _callback(pointerData); + _callback(event, pointerData); } }); diff --git a/engine/src/flutter/lib/web_ui/lib/src/engine/semantics/semantics.dart b/engine/src/flutter/lib/web_ui/lib/src/engine/semantics/semantics.dart index 316c867a5fe..5fa474c8b2b 100644 --- a/engine/src/flutter/lib/web_ui/lib/src/engine/semantics/semantics.dart +++ b/engine/src/flutter/lib/web_ui/lib/src/engine/semantics/semantics.dart @@ -1980,8 +1980,9 @@ class EngineSemanticsOwner { } /// Receives DOM events from the pointer event system to correlate with the - /// semantics events; returns true if the event should be forwarded to the - /// framework. + /// semantics events. + /// + /// Returns true if the event should be forwarded to the framework. /// /// The browser sends us both raw pointer events and gestures from /// [SemanticsObject.element]s. There could be three possibilities: diff --git a/engine/src/flutter/lib/web_ui/lib/src/engine/semantics/tappable.dart b/engine/src/flutter/lib/web_ui/lib/src/engine/semantics/tappable.dart index 259a9c7d556..1bbf179c06f 100644 --- a/engine/src/flutter/lib/web_ui/lib/src/engine/semantics/tappable.dart +++ b/engine/src/flutter/lib/web_ui/lib/src/engine/semantics/tappable.dart @@ -2,12 +2,9 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'package:ui/src/engine.dart'; import 'package:ui/ui.dart' as ui; -import '../dom.dart'; -import '../platform_dispatcher.dart'; -import 'semantics.dart'; - /// Sets the "button" ARIA role. class Button extends PrimaryRoleManager { Button(SemanticsObject semanticsObject) : super.withBasics(PrimaryRole.button, semanticsObject) { @@ -33,41 +30,45 @@ class Button extends PrimaryRoleManager { /// the browser may not send us pointer events. In that mode we forward HTML /// click as [ui.SemanticsAction.tap]. class Tappable extends RoleManager { - Tappable(SemanticsObject semanticsObject) - : super(Role.tappable, semanticsObject); + Tappable(SemanticsObject semanticsObject) : super(Role.tappable, semanticsObject) { + _clickListener = createDomEventListener((DomEvent click) { + PointerBinding.instance!.clickDebouncer.onClick( + click, + semanticsObject.id, + _isListening, + ); + }); + semanticsObject.element.addEventListener('click', _clickListener); + } DomEventListener? _clickListener; + bool _isListening = false; @override void update() { - if (!semanticsObject.isTappable || semanticsObject.enabledState() == EnabledState.disabled) { - _stopListening(); - } else { - if (_clickListener == null) { - _clickListener = createDomEventListener((DomEvent event) { - if (semanticsObject.owner.gestureMode != GestureMode.browserGestures) { - return; - } - EnginePlatformDispatcher.instance.invokeOnSemanticsAction( - semanticsObject.id, ui.SemanticsAction.tap, null); - }); - semanticsObject.element.addEventListener('click', _clickListener); - } + final bool wasListening = _isListening; + _isListening = semanticsObject.enabledState() != EnabledState.disabled && semanticsObject.isTappable; + if (wasListening != _isListening) { + _updateAttribute(); } } - void _stopListening() { - if (_clickListener == null) { - return; + void _updateAttribute() { + // The `flt-tappable` attribute marks the element for the ClickDebouncer to + // to know that it should debounce click events on this element. The + // contract is that the element that has this attribute is also the element + // that receives pointer and "click" events. + if (_isListening) { + semanticsObject.element.setAttribute('flt-tappable', ''); + } else { + semanticsObject.element.removeAttribute('flt-tappable'); } - - semanticsObject.element.removeEventListener('click', _clickListener); - _clickListener = null; } @override void dispose() { + semanticsObject.element.removeEventListener('click', _clickListener); + _clickListener = null; super.dispose(); - _stopListening(); } } diff --git a/engine/src/flutter/lib/web_ui/test/common/matchers.dart b/engine/src/flutter/lib/web_ui/test/common/matchers.dart index a2bd9fe6610..169baaef8af 100644 --- a/engine/src/flutter/lib/web_ui/test/common/matchers.dart +++ b/engine/src/flutter/lib/web_ui/test/common/matchers.dart @@ -293,17 +293,23 @@ String canonicalizeHtml( html_package.Element.tag(replacementTag); if (mode != HtmlComparisonMode.noAttributes) { - original.attributes.forEach((dynamic name, String value) { - if (name is! String) { - throw ArgumentError('"$name" should be String but was ${name.runtimeType}.'); - } + // Sort the attributes so tests are not sensitive to their order, which + // does not matter in terms of functionality. + final List attributeNames = original.attributes.keys.cast().toList(); + attributeNames.sort(); + for (final String name in attributeNames) { + final String value = original.attributes[name]!; if (name == 'style') { - return; + // The style attribute is handled separately because it contains substructure. + continue; } - if (name.startsWith('aria-')) { + + // These are the only attributes we're interested in testing. This list + // can change over time. + if (name.startsWith('aria-') || name.startsWith('flt-') || name == 'role') { replacement.attributes[name] = value; } - }); + } if (original.attributes.containsKey('style')) { final String styleValue = original.attributes['style']!; diff --git a/engine/src/flutter/lib/web_ui/test/engine/pointer_binding_test.dart b/engine/src/flutter/lib/web_ui/test/engine/pointer_binding_test.dart index 3171e3213b1..8543210d302 100644 --- a/engine/src/flutter/lib/web_ui/test/engine/pointer_binding_test.dart +++ b/engine/src/flutter/lib/web_ui/test/engine/pointer_binding_test.dart @@ -3087,6 +3087,385 @@ void testMain() { packets.clear(); }, ); + + group('ClickDebouncer', () { + _testClickDebouncer(); + }); +} + +typedef CapturedSemanticsEvent = ({ + ui.SemanticsAction type, + int nodeId, +}); + +void _testClickDebouncer() { + final DateTime testTime = DateTime(2018, 12, 17); + late List pointerPackets; + late List semanticsActions; + late _PointerEventContext context; + late PointerBinding binding; + + void testWithSemantics( + String description, + Future Function() body, + ) { + test( + description, + () async { + EngineSemanticsOwner.instance + ..debugOverrideTimestampFunction(() => testTime) + ..semanticsEnabled = true; + await body(); + EngineSemanticsOwner.instance.semanticsEnabled = false; + }, + ); + } + + setUp(() { + context = _PointerEventContext(); + pointerPackets = []; + semanticsActions = []; + ui.window.onPointerDataPacket = (ui.PointerDataPacket packet) { + for (final ui.PointerData data in packet.data) { + pointerPackets.add(data.change); + } + }; + EnginePlatformDispatcher.instance.onSemanticsActionEvent = (ui.SemanticsActionEvent event) { + semanticsActions.add((type: event.type, nodeId: event.nodeId)); + }; + binding = PointerBinding.instance!; + binding.debugOverrideDetector(context); + binding.clickDebouncer.reset(); + }); + + tearDown(() { + binding.clickDebouncer.reset(); + }); + + test('Forwards to framework when semantics is off', () { + expect(EnginePlatformDispatcher.instance.semanticsEnabled, false); + expect(binding.clickDebouncer.isDebouncing, false); + flutterViewEmbedder.flutterViewElement.dispatchEvent(context.primaryDown()); + expect(pointerPackets, [ + ui.PointerChange.add, + ui.PointerChange.down, + ]); + expect(binding.clickDebouncer.isDebouncing, false); + expect(semanticsActions, isEmpty); + }); + + testWithSemantics('Forwards to framework when not debouncing', () async { + expect(EnginePlatformDispatcher.instance.semanticsEnabled, true); + expect(binding.clickDebouncer.isDebouncing, false); + + // This test DOM element is missing the `flt-tappable` attribute on purpose + // so that the debouncer does not debounce events and simply lets + // everything through. + final DomElement testElement = createDomElement('flt-semantics'); + flutterViewEmbedder.semanticsHostElement!.appendChild(testElement); + + testElement.dispatchEvent(context.primaryDown()); + testElement.dispatchEvent(context.primaryUp()); + expect(binding.clickDebouncer.isDebouncing, false); + + expect(pointerPackets, [ + ui.PointerChange.add, + ui.PointerChange.down, + ui.PointerChange.up, + ]); + expect(semanticsActions, isEmpty); + }); + + testWithSemantics('Accumulates pointer events starting from pointerdown', () async { + expect(EnginePlatformDispatcher.instance.semanticsEnabled, true); + expect(binding.clickDebouncer.isDebouncing, false); + + final DomElement testElement = createDomElement('flt-semantics'); + testElement.setAttribute('flt-tappable', ''); + flutterViewEmbedder.semanticsHostElement!.appendChild(testElement); + + testElement.dispatchEvent(context.primaryDown()); + expect( + reason: 'Should start debouncing at first pointerdown', + binding.clickDebouncer.isDebouncing, + true, + ); + + testElement.dispatchEvent(context.primaryUp()); + expect( + reason: 'Should still be debouncing after pointerup', + binding.clickDebouncer.isDebouncing, + true, + ); + + expect( + reason: 'Events are withheld from the framework while debouncing', + pointerPackets, + [], + ); + expect( + binding.clickDebouncer.debugState!.target, + testElement, + ); + expect( + binding.clickDebouncer.debugState!.timer.isActive, + isTrue, + ); + expect( + binding.clickDebouncer.debugState!.queue.map((QueuedEvent e) => e.event.type), + ['pointerdown', 'pointerup'], + ); + + await Future.delayed(const Duration(milliseconds: 250)); + expect( + reason: 'Should stop debouncing after timer expires.', + binding.clickDebouncer.isDebouncing, + false, + ); + expect( + reason: 'Queued up events should be flushed to the framework.', + pointerPackets, + [ + ui.PointerChange.add, + ui.PointerChange.down, + ui.PointerChange.up, + ], + ); + expect(semanticsActions, isEmpty); + }); + + testWithSemantics('Flushes events to framework when target changes', () async { + expect(EnginePlatformDispatcher.instance.semanticsEnabled, true); + expect(binding.clickDebouncer.isDebouncing, false); + + final DomElement testElement = createDomElement('flt-semantics'); + testElement.setAttribute('flt-tappable', ''); + flutterViewEmbedder.semanticsHostElement!.appendChild(testElement); + + testElement.dispatchEvent(context.primaryDown()); + expect( + reason: 'Should start debouncing at first pointerdown', + binding.clickDebouncer.isDebouncing, + true, + ); + + final DomElement newTarget = createDomElement('flt-semantics'); + newTarget.setAttribute('flt-tappable', ''); + flutterViewEmbedder.semanticsHostElement!.appendChild(newTarget); + newTarget.dispatchEvent(context.primaryUp()); + + expect( + reason: 'Should stop debouncing when target changes.', + binding.clickDebouncer.isDebouncing, + false, + ); + expect( + reason: 'The state should be cleaned up after stopping debouncing.', + binding.clickDebouncer.debugState, + isNull, + ); + expect( + reason: 'Queued up events should be flushed to the framework.', + pointerPackets, + [ + ui.PointerChange.add, + ui.PointerChange.down, + ui.PointerChange.up, + ], + ); + expect(semanticsActions, isEmpty); + }); + + testWithSemantics('Forwards click to framework when not debouncing but listening', () async { + expect(binding.clickDebouncer.isDebouncing, false); + + final DomElement testElement = createDomElement('flt-semantics'); + testElement.setAttribute('flt-tappable', ''); + flutterViewEmbedder.semanticsHostElement!.appendChild(testElement); + + final DomEvent click = createDomMouseEvent( + 'click', + { + 'clientX': testElement.getBoundingClientRect().x, + 'clientY': testElement.getBoundingClientRect().y, + } + ); + + binding.clickDebouncer.onClick(click, 42, true); + expect(binding.clickDebouncer.isDebouncing, false); + expect(pointerPackets, isEmpty); + expect(semanticsActions, [ + (type: ui.SemanticsAction.tap, nodeId: 42) + ]); + }); + + testWithSemantics('Forwards click to framework when debouncing and listening', () async { + expect(binding.clickDebouncer.isDebouncing, false); + + final DomElement testElement = createDomElement('flt-semantics'); + testElement.setAttribute('flt-tappable', ''); + flutterViewEmbedder.semanticsHostElement!.appendChild(testElement); + testElement.dispatchEvent(context.primaryDown()); + expect(binding.clickDebouncer.isDebouncing, true); + + final DomEvent click = createDomMouseEvent( + 'click', + { + 'clientX': testElement.getBoundingClientRect().x, + 'clientY': testElement.getBoundingClientRect().y, + } + ); + + binding.clickDebouncer.onClick(click, 42, true); + expect(pointerPackets, isEmpty); + expect(semanticsActions, [ + (type: ui.SemanticsAction.tap, nodeId: 42) + ]); + }); + + testWithSemantics('Dedupes click if debouncing but not listening', () async { + expect(binding.clickDebouncer.isDebouncing, false); + + final DomElement testElement = createDomElement('flt-semantics'); + testElement.setAttribute('flt-tappable', ''); + flutterViewEmbedder.semanticsHostElement!.appendChild(testElement); + testElement.dispatchEvent(context.primaryDown()); + expect(binding.clickDebouncer.isDebouncing, true); + + final DomEvent click = createDomMouseEvent( + 'click', + { + 'clientX': testElement.getBoundingClientRect().x, + 'clientY': testElement.getBoundingClientRect().y, + } + ); + + binding.clickDebouncer.onClick(click, 42, false); + expect( + reason: 'When tappable declares that it is not listening to click events ' + 'the debouncer flushes the pointer events to the framework and ' + 'lets it sort it out.', + pointerPackets, + [ + ui.PointerChange.add, + ui.PointerChange.down, + ], + ); + expect(semanticsActions, isEmpty); + }); + + testWithSemantics('Dedupes click if pointer down/up flushed recently', () async { + expect(EnginePlatformDispatcher.instance.semanticsEnabled, true); + expect(binding.clickDebouncer.isDebouncing, false); + + final DomElement testElement = createDomElement('flt-semantics'); + testElement.setAttribute('flt-tappable', ''); + flutterViewEmbedder.semanticsHostElement!.appendChild(testElement); + + testElement.dispatchEvent(context.primaryDown()); + + // Simulate the user holding the pointer down for some time before releasing, + // such that the pointerup event happens close to timer expiration. This + // will create the situation that the click event arrives just after the + // pointerup is flushed. Forwarding the click to the framework would look + // like a double-click, so the click event is deduped. + await Future.delayed(const Duration(milliseconds: 190)); + + testElement.dispatchEvent(context.primaryUp()); + expect(binding.clickDebouncer.isDebouncing, true); + expect( + reason: 'Timer has not expired yet', + pointerPackets, isEmpty, + ); + + // Wait for the timer to expire to make sure pointer events are flushed. + await Future.delayed(const Duration(milliseconds: 11)); + + expect( + reason: 'Queued up events should be flushed to the framework because the ' + 'time expired before the click event arrived.', + pointerPackets, + [ + ui.PointerChange.add, + ui.PointerChange.down, + ui.PointerChange.up, + ], + ); + + final DomEvent click = createDomMouseEvent( + 'click', + { + 'clientX': testElement.getBoundingClientRect().x, + 'clientY': testElement.getBoundingClientRect().y, + } + ); + binding.clickDebouncer.onClick(click, 42, true); + + expect( + reason: 'Because the DOM click event was deduped.', + semanticsActions, + isEmpty, + ); + }); + + + testWithSemantics('Forwards click if enough time passed after the last flushed pointerup', () async { + expect(EnginePlatformDispatcher.instance.semanticsEnabled, true); + expect(binding.clickDebouncer.isDebouncing, false); + + final DomElement testElement = createDomElement('flt-semantics'); + testElement.setAttribute('flt-tappable', ''); + flutterViewEmbedder.semanticsHostElement!.appendChild(testElement); + + testElement.dispatchEvent(context.primaryDown()); + + // Simulate the user holding the pointer down for some time before releasing, + // such that the pointerup event happens close to timer expiration. This + // makes it possible for the click to arrive early. However, this test in + // particular will delay the click to check that the delay is checked + // correctly. The inverse situation was already tested in the previous test. + await Future.delayed(const Duration(milliseconds: 190)); + + testElement.dispatchEvent(context.primaryUp()); + expect(binding.clickDebouncer.isDebouncing, true); + expect( + reason: 'Timer has not expired yet', + pointerPackets, isEmpty, + ); + + // Wait for the timer to expire to make sure pointer events are flushed. + await Future.delayed(const Duration(milliseconds: 100)); + + expect( + reason: 'Queued up events should be flushed to the framework because the ' + 'time expired before the click event arrived.', + pointerPackets, + [ + ui.PointerChange.add, + ui.PointerChange.down, + ui.PointerChange.up, + ], + ); + + final DomEvent click = createDomMouseEvent( + 'click', + { + 'clientX': testElement.getBoundingClientRect().x, + 'clientY': testElement.getBoundingClientRect().y, + } + ); + binding.clickDebouncer.onClick(click, 42, true); + + expect( + reason: 'The DOM click should still be sent to the framework because it ' + 'happened far enough from the last pointerup that it is unlikely ' + 'to be a duplicate.', + semanticsActions, + [ + (type: ui.SemanticsAction.tap, nodeId: 42) + ], + ); + }); } class MockSafariPointerEventWorkaround implements SafariPointerEventWorkaround { diff --git a/engine/src/flutter/lib/web_ui/test/engine/semantics/semantics_test.dart b/engine/src/flutter/lib/web_ui/test/engine/semantics/semantics_test.dart index 674fd04a98e..84ab72b21ca 100644 --- a/engine/src/flutter/lib/web_ui/test/engine/semantics/semantics_test.dart +++ b/engine/src/flutter/lib/web_ui/test/engine/semantics/semantics_test.dart @@ -377,7 +377,7 @@ void _testEngineSemanticsOwner() { expectSemanticsTree(''' - + '''); @@ -387,7 +387,7 @@ void _testEngineSemanticsOwner() { expectSemanticsTree(''' - + '''); @@ -397,7 +397,7 @@ void _testEngineSemanticsOwner() { expectSemanticsTree(''' - + '''); @@ -430,7 +430,7 @@ void _testEngineSemanticsOwner() { expectSemanticsTree(''' - + '''); @@ -440,7 +440,7 @@ void _testEngineSemanticsOwner() { expectSemanticsTree(''' - + '''); @@ -1388,7 +1388,7 @@ void _testIncrementables() { semantics().updateSemantics(builder.build()); expectSemanticsTree(''' - + '''); final SemanticsObject node = semantics().debugSemanticsTree![0]!; @@ -1421,7 +1421,7 @@ void _testIncrementables() { semantics().updateSemantics(builder.build()); expectSemanticsTree(''' - + '''); final DomHTMLInputElement input = @@ -1454,7 +1454,7 @@ void _testIncrementables() { semantics().updateSemantics(builder.build()); expectSemanticsTree(''' - + '''); final DomHTMLInputElement input = @@ -1489,7 +1489,7 @@ void _testIncrementables() { semantics().updateSemantics(builder.build()); expectSemanticsTree(''' - + '''); semantics().semanticsEnabled = false; @@ -1632,7 +1632,7 @@ void _testCheckables() { semantics().updateSemantics(builder.build()); expectSemanticsTree(''' - + '''); final SemanticsObject node = semantics().debugSemanticsTree![0]!; @@ -1690,7 +1690,7 @@ void _testCheckables() { semantics().updateSemantics(builder.build()); expectSemanticsTree(''' - + '''); semantics().semanticsEnabled = false; @@ -1716,7 +1716,7 @@ void _testCheckables() { semantics().updateSemantics(builder.build()); expectSemanticsTree(''' - + '''); semantics().semanticsEnabled = false; @@ -1766,7 +1766,7 @@ void _testCheckables() { semantics().updateSemantics(builder.build()); expectSemanticsTree(''' - + '''); semantics().semanticsEnabled = false; @@ -1793,7 +1793,7 @@ void _testCheckables() { semantics().updateSemantics(builder.build()); expectSemanticsTree(''' - + '''); semantics().semanticsEnabled = false; @@ -1845,7 +1845,7 @@ void _testCheckables() { semantics().updateSemantics(builder.build()); expectSemanticsTree(''' - + '''); semantics().semanticsEnabled = false; @@ -1918,7 +1918,7 @@ void _testTappable() { tester.apply(); expectSemanticsTree(''' - + '''); final SemanticsObject node = semantics().debugSemanticsTree![0]!; @@ -1979,14 +1979,14 @@ void _testTappable() { ''); updateTappable(enabled: true); - expectSemanticsTree(''); + expectSemanticsTree(''); updateTappable(enabled: false); expectSemanticsTree( ''); updateTappable(enabled: true); - expectSemanticsTree(''); + expectSemanticsTree(''); semantics().semanticsEnabled = false; }); @@ -2623,11 +2623,11 @@ void _testDialog() { tester.apply(); expectSemanticsTree(''' - + - + @@ -2716,7 +2716,7 @@ void _testDialog() { - + @@ -2853,9 +2853,9 @@ void _testFocusable() { } expectSemanticsTree(''' - + - + '''); diff --git a/engine/src/flutter/lib/web_ui/test/engine/semantics/semantics_tester.dart b/engine/src/flutter/lib/web_ui/test/engine/semantics/semantics_tester.dart index 881acdcfa6f..2634a99d758 100644 --- a/engine/src/flutter/lib/web_ui/test/engine/semantics/semantics_tester.dart +++ b/engine/src/flutter/lib/web_ui/test/engine/semantics/semantics_tester.dart @@ -353,6 +353,27 @@ class SemanticsTester { /// Verifies the HTML structure of the current semantics tree. void expectSemanticsTree(String semanticsHtml) { const List ignoredAttributes = ['pointer-events']; +// print('\n============================================================================'); +// print(''' +// semanticsHtml: +// $semanticsHtml +// '''.trim()); +// print('----------------------------------------------------------------------------'); +// print(''' +// canonicalizeHtml(semanticsHtml): +// ${canonicalizeHtml(semanticsHtml)} +// '''.trim()); +// print('----------------------------------------------------------------------------'); +// print(''' +// canonicalizeHtml(appHostNode.querySelector('flt-semantics')!.outerHTML!, ignoredAttributes: ignoredAttributes): +// ${canonicalizeHtml(appHostNode.querySelector('flt-semantics')!.outerHTML!, ignoredAttributes: ignoredAttributes)} +// '''.trim()); +// print(''' +// appHostNode.querySelector('flt-semantics')!.outerHTML!: +// ${appHostNode.querySelector('flt-semantics')!.outerHTML!} +// '''.trim()); +// print('============================================================================\n'); + expect( canonicalizeHtml(appHostNode.querySelector('flt-semantics')!.outerHTML!, ignoredAttributes: ignoredAttributes), canonicalizeHtml(semanticsHtml),