From da3278ca8aefd9fb17a7d624e23bd3fe0c800cd2 Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Wed, 21 Aug 2024 17:11:48 -0700 Subject: [PATCH 1/2] [web:semantics] fix double click due to long-press --- .../lib/src/engine/pointer_binding.dart | 29 +++++---- .../test/engine/pointer_binding_test.dart | 64 +++++++++++++++++++ 2 files changed, 82 insertions(+), 11 deletions(-) diff --git a/lib/web_ui/lib/src/engine/pointer_binding.dart b/lib/web_ui/lib/src/engine/pointer_binding.dart index 5bd9b6f7dc5fa..3635b39a5c42f 100644 --- a/lib/web_ui/lib/src/engine/pointer_binding.dart +++ b/lib/web_ui/lib/src/engine/pointer_binding.dart @@ -205,12 +205,13 @@ class ClickDebouncer { @visibleForTesting DebounceState? get debugState => _state; - // The timestamp of the last "pointerup" DOM event that was flushed. + // The timestamp of the last "pointerup" DOM event that was sent to the + // framework. // // 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; + Duration? _lastSentPointerUpTimeStamp; /// Returns true if the debouncer has a non-empty queue of pointer events that /// were withheld from the framework. @@ -244,6 +245,14 @@ class ClickDebouncer { } else if (event.type == 'pointerdown') { _startDebouncing(event, data); } else { + if (event.type == 'pointerup') { + // Record the last pointerup event even if not debouncing. This is + // because the sequence of pointerdown-pointerup could indicate a + // long-press, and the debounce timer is not long enough to capture it. + // If a "click" is observed after a long-press it should be + // debounced. + _lastSentPointerUpTimeStamp = _BaseAdapter._eventTimeStampToDuration(event.timeStamp!); + } _sendToFramework(event, data); } } @@ -295,6 +304,7 @@ class ClickDebouncer { EnginePlatformDispatcher.instance.invokeOnSemanticsAction( semanticsNodeId, ui.SemanticsAction.tap, null); + reset(); } void _startDebouncing(DomEvent event, List data) { @@ -351,10 +361,7 @@ class ClickDebouncer { // 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; + final bool targetChanged = event.target != state.target; if (targetChanged) { _flush(); } @@ -372,15 +379,15 @@ class ClickDebouncer { // already flushed to the framework, the click event is dropped to avoid // double click. bool _shouldSendClickEventToFramework(DomEvent click) { - final Duration? lastFlushedPointerUpTimeStamp = _lastFlushedPointerUpTimeStamp; + final Duration? lastSentPointerUpTimeStamp = _lastSentPointerUpTimeStamp; - if (lastFlushedPointerUpTimeStamp == null) { + if (lastSentPointerUpTimeStamp == 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; + final Duration delta = clickTimeStamp - lastSentPointerUpTimeStamp; return delta >= const Duration(milliseconds: 50); } @@ -393,7 +400,7 @@ class ClickDebouncer { final List aggregateData = []; for (final QueuedEvent queuedEvent in state.queue) { if (queuedEvent.event.type == 'pointerup') { - _lastFlushedPointerUpTimeStamp = queuedEvent.timeStamp; + _lastSentPointerUpTimeStamp = queuedEvent.timeStamp; } aggregateData.addAll(queuedEvent.data); } @@ -419,7 +426,7 @@ class ClickDebouncer { void reset() { _state?.timer.cancel(); _state = null; - _lastFlushedPointerUpTimeStamp = null; + _lastSentPointerUpTimeStamp = null; } } diff --git a/lib/web_ui/test/engine/pointer_binding_test.dart b/lib/web_ui/test/engine/pointer_binding_test.dart index 9e3354eb8d55d..f765c492f7226 100644 --- a/lib/web_ui/test/engine/pointer_binding_test.dart +++ b/lib/web_ui/test/engine/pointer_binding_test.dart @@ -4,6 +4,7 @@ import 'dart:js_util' as js_util; +import 'package:meta/meta.dart'; import 'package:test/bootstrap/browser.dart'; import 'package:test/test.dart'; import 'package:ui/src/engine.dart'; @@ -2757,6 +2758,7 @@ void _testClickDebouncer({required PointerBinding Function() getBinding}) { String description, Future Function() body, { Object? skip, + @doNotSubmit bool solo = false, }) { test( description, @@ -2768,6 +2770,7 @@ void _testClickDebouncer({required PointerBinding Function() getBinding}) { EngineSemantics.instance.semanticsEnabled = false; }, skip: skip, + solo: solo, // ignore: invalid_use_of_do_not_submit_member ); } @@ -3053,6 +3056,67 @@ void _testClickDebouncer({required PointerBinding Function() getBinding}) { // TODO(yjbanov): https://github.com/flutter/flutter/issues/142991. }, skip: ui_web.browser.operatingSystem == ui_web.OperatingSystem.windows); + // Regression test for https://github.com/flutter/flutter/issues/147050 + // + // This test emulates a long-press. Start with a "pointerdown" followed by no + // activity long enough that the debounce timer expires and the state of the + // ClickDebouncer is reset back to idle. Then a "pointerup" arrives seemingly + // standalone. Since no gesture is being debounced, the debouncer simply + // forwards it to the framework. However, "pointerup" will be immediately + // followed by a "click". Since we sent the "pointerdown" and "pointerup" to + // the framework already, the framework registered a tap. Forwarding the + // "click" would lead to a double-tap. This was the bug. + testWithSemantics('Dedupes click if pointer up happened recently without debouncing', () async { + expect(EnginePlatformDispatcher.instance.semanticsEnabled, true); + expect(PointerBinding.clickDebouncer.isDebouncing, false); + + final DomElement testElement = createDomElement('flt-semantics'); + testElement.setAttribute('flt-tappable', ''); + view.dom.semanticsHost.appendChild(testElement); + + // Begin a long-press with a "pointerdown". + testElement.dispatchEvent(context.primaryDown()); + + // Expire the timer causing the debouncer to reset itself. + await Future.delayed(const Duration(milliseconds: 250)); + expect( + reason: '"pointerdown" should be flushed when the timer expires.', + pointerPackets, + [ui.PointerChange.add, ui.PointerChange.down], + ); + pointerPackets.clear(); + + // Send a "pointerup" while the debouncer is not debouncing anything. + testElement.dispatchEvent(context.primaryUp()); + + // A standalone "pointerup" should not start debouncing anything. + expect(PointerBinding.clickDebouncer.isDebouncing, isFalse); + expect( + reason: 'The "pointerup" should be forwarded to the framework immediately', + pointerPackets, + [ui.PointerChange.up], + ); + + // Use a delay that's short enough for the click to be deduped. + await Future.delayed(const Duration(milliseconds: 10)); + + final DomEvent click = createDomMouseEvent( + 'click', + { + 'clientX': testElement.getBoundingClientRect().x, + 'clientY': testElement.getBoundingClientRect().y, + } + ); + PointerBinding.clickDebouncer.onClick(click, 42, true); + + expect( + reason: 'Because the DOM click event was deduped.', + semanticsActions, + isEmpty, + ); + // TODO(yjbanov): https://github.com/flutter/flutter/issues/142991. + }, skip: ui_web.browser.operatingSystem == ui_web.OperatingSystem.windows); + testWithSemantics('Forwards click if enough time passed after the last flushed pointerup', () async { expect(EnginePlatformDispatcher.instance.semanticsEnabled, true); expect(PointerBinding.clickDebouncer.isDebouncing, false); From 7dcfb99e3f16629ec8df64e3a875219ee252f9d4 Mon Sep 17 00:00:00 2001 From: Yegor Date: Thu, 22 Aug 2024 09:37:39 -0700 Subject: [PATCH 2/2] Update lib/web_ui/lib/src/engine/pointer_binding.dart Co-authored-by: Mouad Debbar --- lib/web_ui/lib/src/engine/pointer_binding.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/web_ui/lib/src/engine/pointer_binding.dart b/lib/web_ui/lib/src/engine/pointer_binding.dart index 3635b39a5c42f..11502593eb97e 100644 --- a/lib/web_ui/lib/src/engine/pointer_binding.dart +++ b/lib/web_ui/lib/src/engine/pointer_binding.dart @@ -250,7 +250,7 @@ class ClickDebouncer { // because the sequence of pointerdown-pointerup could indicate a // long-press, and the debounce timer is not long enough to capture it. // If a "click" is observed after a long-press it should be - // debounced. + // discarded. _lastSentPointerUpTimeStamp = _BaseAdapter._eventTimeStampToDuration(event.timeStamp!); } _sendToFramework(event, data);