[web:semantics] fix double click due to long-press (#54697)

Remember the timestamp of _all_ `pointerup` events, not just those that were flushed. Clicks should be deduplicated after a `pointerup` even when not debouncing anything. This is because when not debouncing the engine already forwards all the pointer events to the framework, and sending click events on top only causes double-clicks.

Fixes flutter/flutter#147050

Author: yjbanov

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
+        // discarded.
+        _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<ui.PointerData> 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<ui.PointerData> aggregateData = <ui.PointerData>[];
     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;
   }
 }
 

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<void> 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<void>.delayed(const Duration(milliseconds: 250));
+    expect(
+      reason: '"pointerdown" should be flushed when the timer expires.',
+      pointerPackets,
+      <ui.PointerChange>[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>[ui.PointerChange.up],
+    );
+
+    // Use a delay that's short enough for the click to be deduped.
+    await Future<void>.delayed(const Duration(milliseconds: 10));
+
+    final DomEvent click = createDomMouseEvent(
+      'click',
+      <Object?, Object?>{
+        '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);