From b4ea656857284d6a88d73e8d390027aa9ba5a8ce Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Fri, 22 Sep 2023 15:39:50 -0700 Subject: [PATCH 1/4] [web] fix clicks on merged semantic nodes --- .../lib/src/engine/pointer_binding.dart | 330 ++++++++++++--- .../lib/src/engine/semantics/semantics.dart | 5 +- .../lib/src/engine/semantics/tappable.dart | 54 +-- lib/web_ui/test/common/matchers.dart | 20 +- .../test/engine/pointer_binding_test.dart | 379 ++++++++++++++++++ .../test/engine/semantics/semantics_test.dart | 46 +-- 6 files changed, 728 insertions(+), 106 deletions(-) diff --git a/lib/web_ui/lib/src/engine/pointer_binding.dart b/lib/web_ui/lib/src/engine/pointer_binding.dart index ce20224042438..5d199b3f73a5e 100644 --- a/lib/web_ui/lib/src/engine/pointer_binding.dart +++ b/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.'); } +} + +@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.' + ); - void _onPointerData(Iterable data) { + 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 { @@ -197,53 +438,47 @@ class _Listener { required this.event, required this.target, required this.handler, - 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? passive, }) { final DomEventListener jsHandler = createDomEventListener(handler); - final _Listener listener = _Listener._( - event: event, - target: target, - handler: jsHandler, - isNative: false, - ); - target.addEventListener(event, jsHandler); - return listener; - } - /// 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 passive = false, - }) { - final Map eventOptions = { - 'passive': passive, - }; + if (passive == null) { + target.addEventListener(event, jsHandler); + } else { + final Map eventOptions = { + 'passive': passive, + }; + target.addEventListenerWithOptions(event, jsHandler, eventOptions); + } + final _Listener listener = _Listener._( event: event, target: target, handler: jsHandler, - isNative: true, ); - target.addEventListenerWithOptions(event, jsHandler, eventOptions); + target.addEventListener(event, jsHandler); return listener; } final String event; - final DomEventTarget target; final DomEventListener handler; - final bool isNative; - void unregister() { target.removeEventListener(event, handler); } @@ -477,10 +712,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, )); } @@ -490,7 +726,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. // @@ -739,7 +975,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 @@ -756,7 +992,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) { @@ -766,7 +1002,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); } }, checkModifiers: false); @@ -779,7 +1015,7 @@ class _PointerAdapter extends _BaseAdapter with _WheelEventListenerMixin { _removePointerIfUnhoverable(event); if (details != null) { _convertEventsToPointerData(data: pointerData, event: event, details: details); - _callback(pointerData); + _callback(event, pointerData); } } }); @@ -795,7 +1031,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); @@ -934,7 +1170,7 @@ class _TouchAdapter extends _BaseAdapter { ); } } - _callback(pointerData); + _callback(event, pointerData); }); _addTouchEventListener(flutterViewElement, 'touchmove', (DomTouchEvent event) { @@ -953,7 +1189,7 @@ class _TouchAdapter extends _BaseAdapter { ); } } - _callback(pointerData); + _callback(event, pointerData); }); _addTouchEventListener(flutterViewElement, 'touchend', (DomTouchEvent event) { @@ -975,7 +1211,7 @@ class _TouchAdapter extends _BaseAdapter { ); } } - _callback(pointerData); + _callback(event, pointerData); }); _addTouchEventListener(flutterViewElement, 'touchcancel', (DomTouchEvent event) { @@ -994,7 +1230,7 @@ class _TouchAdapter extends _BaseAdapter { ); } } - _callback(pointerData); + _callback(event, pointerData); }); } @@ -1091,7 +1327,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 @@ -1103,7 +1339,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) { @@ -1111,7 +1347,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); } }); @@ -1121,7 +1357,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/lib/web_ui/lib/src/engine/semantics/semantics.dart b/lib/web_ui/lib/src/engine/semantics/semantics.dart index 82d9a2d874659..d700d58e5d7a2 100644 --- a/lib/web_ui/lib/src/engine/semantics/semantics.dart +++ b/lib/web_ui/lib/src/engine/semantics/semantics.dart @@ -2028,8 +2028,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/lib/web_ui/lib/src/engine/semantics/tappable.dart b/lib/web_ui/lib/src/engine/semantics/tappable.dart index cba0fafeb7650..5adf4d0ec0807 100644 --- a/lib/web_ui/lib/src/engine/semantics/tappable.dart +++ b/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) { @@ -34,42 +31,45 @@ class Button extends PrimaryRoleManager { /// click as [ui.SemanticsAction.tap]. class Tappable extends RoleManager { Tappable(SemanticsObject semanticsObject, PrimaryRoleManager owner) - : super(Role.tappable, semanticsObject, owner); + : super(Role.tappable, semanticsObject, owner) { + _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 (_clickListener == null) { - _clickListener = createDomEventListener((DomEvent event) { - // Stop dom from reacting since it will be handled entirely on the - // flutter side. - event.preventDefault(); - if (!semanticsObject.isTappable || semanticsObject.enabledState() == EnabledState.disabled) { - return; - } - if (semanticsObject.owner.gestureMode != GestureMode.browserGestures) { - return; - } - EnginePlatformDispatcher.instance.invokeOnSemanticsAction( - semanticsObject.id, ui.SemanticsAction.tap, null); - }); - owner.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'); } - - owner.removeEventListener('click', _clickListener); - _clickListener = null; } @override void dispose() { + owner.removeEventListener('click', _clickListener); + _clickListener = null; super.dispose(); - _stopListening(); } } diff --git a/lib/web_ui/test/common/matchers.dart b/lib/web_ui/test/common/matchers.dart index a2bd9fe66102a..169baaef8af95 100644 --- a/lib/web_ui/test/common/matchers.dart +++ b/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/lib/web_ui/test/engine/pointer_binding_test.dart b/lib/web_ui/test/engine/pointer_binding_test.dart index f93d1a2e966a6..dadfef69c26e2 100644 --- a/lib/web_ui/test/engine/pointer_binding_test.dart +++ b/lib/web_ui/test/engine/pointer_binding_test.dart @@ -3129,6 +3129,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: 20)); + + 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/lib/web_ui/test/engine/semantics/semantics_test.dart b/lib/web_ui/test/engine/semantics/semantics_test.dart index f9abdde052297..b5f1d96b11768 100644 --- a/lib/web_ui/test/engine/semantics/semantics_test.dart +++ b/lib/web_ui/test/engine/semantics/semantics_test.dart @@ -390,7 +390,7 @@ void _testEngineSemanticsOwner() { expectSemanticsTree(''' - + '''); @@ -400,7 +400,7 @@ void _testEngineSemanticsOwner() { expectSemanticsTree(''' - + '''); @@ -410,7 +410,7 @@ void _testEngineSemanticsOwner() { expectSemanticsTree(''' - + '''); @@ -482,7 +482,7 @@ void _testEngineSemanticsOwner() { expectSemanticsTree(''' - + '''); @@ -492,7 +492,7 @@ void _testEngineSemanticsOwner() { expectSemanticsTree(''' - + '''); @@ -1440,7 +1440,7 @@ void _testIncrementables() { semantics().updateSemantics(builder.build()); expectSemanticsTree(''' - + '''); final SemanticsObject node = semantics().debugSemanticsTree![0]!; @@ -1473,7 +1473,7 @@ void _testIncrementables() { semantics().updateSemantics(builder.build()); expectSemanticsTree(''' - + '''); final DomHTMLInputElement input = @@ -1506,7 +1506,7 @@ void _testIncrementables() { semantics().updateSemantics(builder.build()); expectSemanticsTree(''' - + '''); final DomHTMLInputElement input = @@ -1541,7 +1541,7 @@ void _testIncrementables() { semantics().updateSemantics(builder.build()); expectSemanticsTree(''' - + '''); semantics().semanticsEnabled = false; @@ -1684,7 +1684,7 @@ void _testCheckables() { semantics().updateSemantics(builder.build()); expectSemanticsTree(''' - + '''); final SemanticsObject node = semantics().debugSemanticsTree![0]!; @@ -1742,7 +1742,7 @@ void _testCheckables() { semantics().updateSemantics(builder.build()); expectSemanticsTree(''' - + '''); semantics().semanticsEnabled = false; @@ -1768,7 +1768,7 @@ void _testCheckables() { semantics().updateSemantics(builder.build()); expectSemanticsTree(''' - + '''); semantics().semanticsEnabled = false; @@ -1818,7 +1818,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; @@ -1897,7 +1897,7 @@ void _testCheckables() { semantics().updateSemantics(builder.build()); expectSemanticsTree(''' - + '''); semantics().semanticsEnabled = false; @@ -1970,7 +1970,7 @@ void _testTappable() { tester.apply(); expectSemanticsTree(''' - + '''); final SemanticsObject node = semantics().debugSemanticsTree![0]!; @@ -2031,14 +2031,14 @@ void _testTappable() { ''); updateTappable(enabled: true); - expectSemanticsTree(''); + expectSemanticsTree(''); updateTappable(enabled: false); expectSemanticsTree( ''); updateTappable(enabled: true); - expectSemanticsTree(''); + expectSemanticsTree(''); semantics().semanticsEnabled = false; }); @@ -2675,11 +2675,11 @@ void _testDialog() { tester.apply(); expectSemanticsTree(''' - + - + @@ -2768,7 +2768,7 @@ void _testDialog() { - + @@ -2905,9 +2905,9 @@ void _testFocusable() { } expectSemanticsTree(''' - + - + '''); From 56c1246a07906f707510452c9b91f7c1555895a7 Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Thu, 26 Oct 2023 13:29:49 -0700 Subject: [PATCH 2/4] reland click disambiguation with fix for nested tappables --- .../lib/src/engine/pointer_binding.dart | 20 ++++- .../lib/src/engine/semantics/semantics.dart | 7 +- .../lib/src/engine/semantics/tappable.dart | 6 +- lib/web_ui/test/common/matchers.dart | 24 ++--- .../test/engine/semantics/semantics_test.dart | 87 ++++++++++++++++++- .../engine/semantics/semantics_tester.dart | 4 +- 6 files changed, 123 insertions(+), 25 deletions(-) diff --git a/lib/web_ui/lib/src/engine/pointer_binding.dart b/lib/web_ui/lib/src/engine/pointer_binding.dart index 5d199b3f73a5e..85fca6d417605 100644 --- a/lib/web_ui/lib/src/engine/pointer_binding.dart +++ b/lib/web_ui/lib/src/engine/pointer_binding.dart @@ -272,8 +272,7 @@ class ClickDebouncer { // 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); + _sendSemanticsTapToFramework(click, semanticsNodeId); } return; } @@ -285,8 +284,7 @@ class ClickDebouncer { final DebounceState state = _state!; _state = null; state.timer.cancel(); - EnginePlatformDispatcher.instance.invokeOnSemanticsAction( - semanticsNodeId, ui.SemanticsAction.tap, null); + _sendSemanticsTapToFramework(click, semanticsNodeId); } 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 @@ -295,6 +293,20 @@ class ClickDebouncer { } } + void _sendSemanticsTapToFramework(DomEvent click, int semanticsNodeId) { + // Tappable nodes can be nested inside other tappable nodes. If a click + // lands on an inner element and is allowed to propagate, it will also + // land on the ancestor tappable, leading to both the descendant and the + // ancestor sending SemanticsAction.tap to the framework, creating a double + // tap/click, which is wrong. More details: + // + // https://github.com/flutter/flutter/issues/134842 + click.stopPropagation(); + + EnginePlatformDispatcher.instance.invokeOnSemanticsAction( + semanticsNodeId, ui.SemanticsAction.tap, null); + } + void _startDebouncing(DomEvent event, List data) { assert( _state == null, diff --git a/lib/web_ui/lib/src/engine/semantics/semantics.dart b/lib/web_ui/lib/src/engine/semantics/semantics.dart index d700d58e5d7a2..b27c9233f10a2 100644 --- a/lib/web_ui/lib/src/engine/semantics/semantics.dart +++ b/lib/web_ui/lib/src/engine/semantics/semantics.dart @@ -426,6 +426,7 @@ abstract class PrimaryRoleManager { /// Initializes a role for a [semanticsObject] that includes basic /// functionality for focus, labels, live regions, and route names. PrimaryRoleManager.withBasics(this.role, this.semanticsObject) { + element = _initElement(createElement(), semanticsObject); addFocusManagement(); addLiveRegion(); addRouteName(); @@ -438,9 +439,11 @@ abstract class PrimaryRoleManager { /// Use this constructor for highly specialized cases where /// [RoleManager.withBasics] does not work, for example when the default focus /// management intereferes with the widget's functionality. - PrimaryRoleManager.blank(this.role, this.semanticsObject); + PrimaryRoleManager.blank(this.role, this.semanticsObject) { + element = _initElement(createElement(), semanticsObject); + } - late final DomElement element = _initElement(createElement(), semanticsObject); + late final DomElement element; /// The primary role identifier. final PrimaryRole role; diff --git a/lib/web_ui/lib/src/engine/semantics/tappable.dart b/lib/web_ui/lib/src/engine/semantics/tappable.dart index 5adf4d0ec0807..d3612ae80348c 100644 --- a/lib/web_ui/lib/src/engine/semantics/tappable.dart +++ b/lib/web_ui/lib/src/engine/semantics/tappable.dart @@ -39,7 +39,7 @@ class Tappable extends RoleManager { _isListening, ); }); - semanticsObject.element.addEventListener('click', _clickListener); + owner.element.addEventListener('click', _clickListener); } DomEventListener? _clickListener; @@ -60,9 +60,9 @@ class Tappable extends RoleManager { // 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', ''); + owner.element.setAttribute('flt-tappable', ''); } else { - semanticsObject.element.removeAttribute('flt-tappable'); + owner.element.removeAttribute('flt-tappable'); } } diff --git a/lib/web_ui/test/common/matchers.dart b/lib/web_ui/test/common/matchers.dart index 169baaef8af95..51698843d66a8 100644 --- a/lib/web_ui/test/common/matchers.dart +++ b/lib/web_ui/test/common/matchers.dart @@ -227,23 +227,23 @@ enum HtmlComparisonMode { /// Rewrites [htmlContent] by removing irrelevant style attributes. /// -/// If [throwOnUnusedAttributes] is `true`, throws instead of rewriting. Set -/// [throwOnUnusedAttributes] to `true` to check that expected HTML strings do +/// If [throwOnUnusedStyleProperties] is `true`, throws instead of rewriting. Set +/// [throwOnUnusedStyleProperties] to `true` to check that expected HTML strings do /// not contain irrelevant attributes. It is ok for actual HTML to contain all /// kinds of attributes. They only need to be filtered out before testing. String canonicalizeHtml( String htmlContent, { HtmlComparisonMode mode = HtmlComparisonMode.nonLayoutOnly, - bool throwOnUnusedAttributes = false, - List? ignoredAttributes, + bool throwOnUnusedStyleProperties = false, + List? ignoredStyleProperties, }) { if (htmlContent.trim().isEmpty) { return ''; } - String? unusedAttribute(String name) { - if (throwOnUnusedAttributes) { - fail('Provided HTML contains style attribute "$name" which ' + String? unusedStyleProperty(String name) { + if (throwOnUnusedStyleProperties) { + fail('Provided HTML contains style property "$name" which ' 'is not used for comparison in the test. The HTML was:\n\n$htmlContent'); } @@ -329,7 +329,7 @@ String canonicalizeHtml( if (parts.length == 2) { final String name = parts.first; - if (ignoredAttributes != null && ignoredAttributes.contains(name)) { + if (ignoredStyleProperties != null && ignoredStyleProperties.contains(name)) { return null; } @@ -343,7 +343,7 @@ String canonicalizeHtml( ].contains(name); if (isStaticAttribute) { - return unusedAttribute(name); + return unusedStyleProperty(name); } // Whether the attribute is set by the layout system. @@ -363,7 +363,7 @@ String canonicalizeHtml( if (forLayout && !isLayoutAttribute || !forLayout && isLayoutAttribute) { - return unusedAttribute(name); + return unusedStyleProperty(name); } } } @@ -378,7 +378,7 @@ String canonicalizeHtml( replacement.attributes['style'] = processedAttributes; } } - } else if (throwOnUnusedAttributes && original.attributes.isNotEmpty) { + } else if (throwOnUnusedStyleProperties && original.attributes.isNotEmpty) { fail('Provided HTML contains attributes. However, the comparison mode ' 'is $mode. The HTML was:\n\n$htmlContent'); } @@ -414,7 +414,7 @@ String canonicalizeHtml( void expectHtml(DomElement element, String expectedHtml, {HtmlComparisonMode mode = HtmlComparisonMode.nonLayoutOnly}) { expectedHtml = - canonicalizeHtml(expectedHtml, mode: mode, throwOnUnusedAttributes: true); + canonicalizeHtml(expectedHtml, mode: mode, throwOnUnusedStyleProperties: true); final String actualHtml = canonicalizeHtml(element.outerHTML!, mode: mode); expect(actualHtml, expectedHtml); } diff --git a/lib/web_ui/test/engine/semantics/semantics_test.dart b/lib/web_ui/test/engine/semantics/semantics_test.dart index b5f1d96b11768..8534fe009b77c 100644 --- a/lib/web_ui/test/engine/semantics/semantics_test.dart +++ b/lib/web_ui/test/engine/semantics/semantics_test.dart @@ -433,7 +433,7 @@ void _testEngineSemanticsOwner() { expectSemanticsTree(''' - + '''); @@ -448,7 +448,7 @@ void _testEngineSemanticsOwner() { expectSemanticsTree(''' - + '''); expect(existingParent, tree[1]!.element.parent); @@ -2111,6 +2111,89 @@ void _testTappable() { semantics().semanticsEnabled = false; }); + + // Regression test for: https://github.com/flutter/flutter/issues/134842 + // + // If the click event is allowed to propagate through the hierarchy, then both + // the descendant and the parent will generate a SemanticsAction.tap, causing + // a double-tap to happen on the framework side. + test('inner tappable overrides ancestor tappable', () async { + semantics() + ..debugOverrideTimestampFunction(() => _testTime) + ..semanticsEnabled = true; + + final List capturedActions = []; + EnginePlatformDispatcher.instance.onSemanticsActionEvent = (ui.SemanticsActionEvent event) { + capturedActions.add((event.nodeId, event.type, event.arguments)); + }; + + final SemanticsTester tester = SemanticsTester(semantics()); + tester.updateNode( + id: 0, + isFocusable: true, + hasTap: true, + hasEnabledState: true, + isEnabled: true, + isButton: true, + rect: const ui.Rect.fromLTRB(0, 0, 100, 50), + children: [ + tester.updateNode( + id: 1, + isFocusable: true, + hasTap: true, + hasEnabledState: true, + isEnabled: true, + isButton: true, + rect: const ui.Rect.fromLTRB(0, 0, 100, 50), + ), + ], + ); + tester.apply(); + + expectSemanticsTree(''' + + + + + +'''); + + // Tap on the outer element + { + final DomElement element = tester.getSemanticsObject(0).element; + final DomRect rect = element.getBoundingClientRect(); + + element.dispatchEvent(createDomMouseEvent('click', { + 'clientX': (rect.left + (rect.right - rect.left) / 2).floor(), + 'clientY': (rect.top + (rect.bottom - rect.top) / 2).floor(), + })); + + expect(capturedActions, [ + (0, ui.SemanticsAction.tap, null), + ]); + } + + // Tap on the inner element + { + capturedActions.clear(); + final DomElement element = tester.getSemanticsObject(1).element; + final DomRect rect = element.getBoundingClientRect(); + + element.dispatchEvent(createDomMouseEvent('click', { + 'bubbles': true, + 'clientX': (rect.left + (rect.right - rect.left) / 2).floor(), + 'clientY': (rect.top + (rect.bottom - rect.top) / 2).floor(), + })); + + // The click on the inner element should not propagate to the parent to + // avoid sending a second SemanticsAction.tap action to the framework. + expect(capturedActions, [ + (1, ui.SemanticsAction.tap, null), + ]); + } + + semantics().semanticsEnabled = false; + }); } void _testImage() { diff --git a/lib/web_ui/test/engine/semantics/semantics_tester.dart b/lib/web_ui/test/engine/semantics/semantics_tester.dart index f0c61e92ca541..8a40c918f6d42 100644 --- a/lib/web_ui/test/engine/semantics/semantics_tester.dart +++ b/lib/web_ui/test/engine/semantics/semantics_tester.dart @@ -353,9 +353,9 @@ class SemanticsTester { /// Verifies the HTML structure of the current semantics tree. void expectSemanticsTree(String semanticsHtml) { - const List ignoredAttributes = ['pointer-events']; + const List ignoredStyleProperties = ['pointer-events']; expect( - canonicalizeHtml(rootElement.querySelector('flt-semantics')!.outerHTML!, ignoredAttributes: ignoredAttributes), + canonicalizeHtml(rootElement.querySelector('flt-semantics')!.outerHTML!, ignoredStyleProperties: ignoredStyleProperties), canonicalizeHtml(semanticsHtml), ); } From ed46554e02b8a2dab60354d04584fe1f9385acab Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Tue, 7 Nov 2023 15:18:44 -0800 Subject: [PATCH 3/4] update pointer_binding_test.dart --- .../test/engine/pointer_binding_test.dart | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/web_ui/test/engine/pointer_binding_test.dart b/lib/web_ui/test/engine/pointer_binding_test.dart index dadfef69c26e2..695f3d2ea8152 100644 --- a/lib/web_ui/test/engine/pointer_binding_test.dart +++ b/lib/web_ui/test/engine/pointer_binding_test.dart @@ -3187,7 +3187,7 @@ void _testClickDebouncer() { test('Forwards to framework when semantics is off', () { expect(EnginePlatformDispatcher.instance.semanticsEnabled, false); expect(binding.clickDebouncer.isDebouncing, false); - flutterViewEmbedder.flutterViewElement.dispatchEvent(context.primaryDown()); + binding.flutterViewElement.dispatchEvent(context.primaryDown()); expect(pointerPackets, [ ui.PointerChange.add, ui.PointerChange.down, @@ -3204,7 +3204,7 @@ void _testClickDebouncer() { // so that the debouncer does not debounce events and simply lets // everything through. final DomElement testElement = createDomElement('flt-semantics'); - flutterViewEmbedder.semanticsHostElement!.appendChild(testElement); + window.dom.semanticsHost.appendChild(testElement); testElement.dispatchEvent(context.primaryDown()); testElement.dispatchEvent(context.primaryUp()); @@ -3224,7 +3224,7 @@ void _testClickDebouncer() { final DomElement testElement = createDomElement('flt-semantics'); testElement.setAttribute('flt-tappable', ''); - flutterViewEmbedder.semanticsHostElement!.appendChild(testElement); + window.dom.semanticsHost.appendChild(testElement); testElement.dispatchEvent(context.primaryDown()); expect( @@ -3282,7 +3282,7 @@ void _testClickDebouncer() { final DomElement testElement = createDomElement('flt-semantics'); testElement.setAttribute('flt-tappable', ''); - flutterViewEmbedder.semanticsHostElement!.appendChild(testElement); + window.dom.semanticsHost.appendChild(testElement); testElement.dispatchEvent(context.primaryDown()); expect( @@ -3293,7 +3293,7 @@ void _testClickDebouncer() { final DomElement newTarget = createDomElement('flt-semantics'); newTarget.setAttribute('flt-tappable', ''); - flutterViewEmbedder.semanticsHostElement!.appendChild(newTarget); + window.dom.semanticsHost.appendChild(newTarget); newTarget.dispatchEvent(context.primaryUp()); expect( @@ -3323,7 +3323,7 @@ void _testClickDebouncer() { final DomElement testElement = createDomElement('flt-semantics'); testElement.setAttribute('flt-tappable', ''); - flutterViewEmbedder.semanticsHostElement!.appendChild(testElement); + window.dom.semanticsHost.appendChild(testElement); final DomEvent click = createDomMouseEvent( 'click', @@ -3346,7 +3346,7 @@ void _testClickDebouncer() { final DomElement testElement = createDomElement('flt-semantics'); testElement.setAttribute('flt-tappable', ''); - flutterViewEmbedder.semanticsHostElement!.appendChild(testElement); + window.dom.semanticsHost.appendChild(testElement); testElement.dispatchEvent(context.primaryDown()); expect(binding.clickDebouncer.isDebouncing, true); @@ -3370,7 +3370,7 @@ void _testClickDebouncer() { final DomElement testElement = createDomElement('flt-semantics'); testElement.setAttribute('flt-tappable', ''); - flutterViewEmbedder.semanticsHostElement!.appendChild(testElement); + window.dom.semanticsHost.appendChild(testElement); testElement.dispatchEvent(context.primaryDown()); expect(binding.clickDebouncer.isDebouncing, true); @@ -3402,7 +3402,7 @@ void _testClickDebouncer() { final DomElement testElement = createDomElement('flt-semantics'); testElement.setAttribute('flt-tappable', ''); - flutterViewEmbedder.semanticsHostElement!.appendChild(testElement); + window.dom.semanticsHost.appendChild(testElement); testElement.dispatchEvent(context.primaryDown()); @@ -3457,7 +3457,7 @@ void _testClickDebouncer() { final DomElement testElement = createDomElement('flt-semantics'); testElement.setAttribute('flt-tappable', ''); - flutterViewEmbedder.semanticsHostElement!.appendChild(testElement); + window.dom.semanticsHost.appendChild(testElement); testElement.dispatchEvent(context.primaryDown()); From 5e360277f788519c8f29ec372163d04c2eaa9f5e Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Tue, 7 Nov 2023 16:04:46 -0800 Subject: [PATCH 4/4] use the recommended semanticsHost expression --- .../test/engine/pointer_binding_test.dart | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/web_ui/test/engine/pointer_binding_test.dart b/lib/web_ui/test/engine/pointer_binding_test.dart index 695f3d2ea8152..d119a7a311b8b 100644 --- a/lib/web_ui/test/engine/pointer_binding_test.dart +++ b/lib/web_ui/test/engine/pointer_binding_test.dart @@ -3204,7 +3204,7 @@ void _testClickDebouncer() { // so that the debouncer does not debounce events and simply lets // everything through. final DomElement testElement = createDomElement('flt-semantics'); - window.dom.semanticsHost.appendChild(testElement); + EnginePlatformDispatcher.instance.implicitView!.dom.semanticsHost.appendChild(testElement); testElement.dispatchEvent(context.primaryDown()); testElement.dispatchEvent(context.primaryUp()); @@ -3224,7 +3224,7 @@ void _testClickDebouncer() { final DomElement testElement = createDomElement('flt-semantics'); testElement.setAttribute('flt-tappable', ''); - window.dom.semanticsHost.appendChild(testElement); + EnginePlatformDispatcher.instance.implicitView!.dom.semanticsHost.appendChild(testElement); testElement.dispatchEvent(context.primaryDown()); expect( @@ -3282,7 +3282,7 @@ void _testClickDebouncer() { final DomElement testElement = createDomElement('flt-semantics'); testElement.setAttribute('flt-tappable', ''); - window.dom.semanticsHost.appendChild(testElement); + EnginePlatformDispatcher.instance.implicitView!.dom.semanticsHost.appendChild(testElement); testElement.dispatchEvent(context.primaryDown()); expect( @@ -3293,7 +3293,7 @@ void _testClickDebouncer() { final DomElement newTarget = createDomElement('flt-semantics'); newTarget.setAttribute('flt-tappable', ''); - window.dom.semanticsHost.appendChild(newTarget); + EnginePlatformDispatcher.instance.implicitView!.dom.semanticsHost.appendChild(newTarget); newTarget.dispatchEvent(context.primaryUp()); expect( @@ -3323,7 +3323,7 @@ void _testClickDebouncer() { final DomElement testElement = createDomElement('flt-semantics'); testElement.setAttribute('flt-tappable', ''); - window.dom.semanticsHost.appendChild(testElement); + EnginePlatformDispatcher.instance.implicitView!.dom.semanticsHost.appendChild(testElement); final DomEvent click = createDomMouseEvent( 'click', @@ -3346,7 +3346,7 @@ void _testClickDebouncer() { final DomElement testElement = createDomElement('flt-semantics'); testElement.setAttribute('flt-tappable', ''); - window.dom.semanticsHost.appendChild(testElement); + EnginePlatformDispatcher.instance.implicitView!.dom.semanticsHost.appendChild(testElement); testElement.dispatchEvent(context.primaryDown()); expect(binding.clickDebouncer.isDebouncing, true); @@ -3370,7 +3370,7 @@ void _testClickDebouncer() { final DomElement testElement = createDomElement('flt-semantics'); testElement.setAttribute('flt-tappable', ''); - window.dom.semanticsHost.appendChild(testElement); + EnginePlatformDispatcher.instance.implicitView!.dom.semanticsHost.appendChild(testElement); testElement.dispatchEvent(context.primaryDown()); expect(binding.clickDebouncer.isDebouncing, true); @@ -3402,7 +3402,7 @@ void _testClickDebouncer() { final DomElement testElement = createDomElement('flt-semantics'); testElement.setAttribute('flt-tappable', ''); - window.dom.semanticsHost.appendChild(testElement); + EnginePlatformDispatcher.instance.implicitView!.dom.semanticsHost.appendChild(testElement); testElement.dispatchEvent(context.primaryDown()); @@ -3457,7 +3457,7 @@ void _testClickDebouncer() { final DomElement testElement = createDomElement('flt-semantics'); testElement.setAttribute('flt-tappable', ''); - window.dom.semanticsHost.appendChild(testElement); + EnginePlatformDispatcher.instance.implicitView!.dom.semanticsHost.appendChild(testElement); testElement.dispatchEvent(context.primaryDown());