Skip to content

Commit

Permalink
[web:semantics] fix double click due to long-press (flutter#54697)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@yjbanov
yjbanov authored and GitHub Actions Bot committed Aug 23, 2024
1 parent c9b9d57 commit 150f579
Show file tree
Hide file tree
Showing 2 changed files with 82 additions and 11 deletions.
29 changes: 18 additions & 11 deletions lib/web_ui/lib/src/engine/pointer_binding.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -295,6 +304,7 @@ class ClickDebouncer {

EnginePlatformDispatcher.instance.invokeOnSemanticsAction(
semanticsNodeId, ui.SemanticsAction.tap, null);
reset();
}

void _startDebouncing(DomEvent event, List<ui.PointerData> data) {
Expand Down Expand Up @@ -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();
}
Expand All @@ -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);
}

Expand All @@ -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);
}
Expand All @@ -419,7 +426,7 @@ class ClickDebouncer {
void reset() {
_state?.timer.cancel();
_state = null;
_lastFlushedPointerUpTimeStamp = null;
_lastSentPointerUpTimeStamp = null;
}
}

Expand Down
64 changes: 64 additions & 0 deletions lib/web_ui/test/engine/pointer_binding_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -2757,6 +2758,7 @@ void _testClickDebouncer({required PointerBinding Function() getBinding}) {
String description,
Future<void> Function() body, {
Object? skip,
@doNotSubmit bool solo = false,
}) {
test(
description,
Expand All @@ -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
);
}

Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit 150f579

Please sign in to comment.