Skip to content

Commit

Permalink
[CP-stable][web:semantics] fix double click due to long-press (#54735)
Browse files Browse the repository at this point in the history
This pull request is created by [automatic cherry pick workflow](https://github.com/flutter/flutter/blob/main/docs/releases/Flutter-Cherrypick-Process.md#automatically-creates-a-cherry-pick-request)
Please fill in the form below, and a flutter domain expert will evaluate this cherry pick request.

### Issue Link:
What is the link to the issue this cherry-pick is addressing?

flutter/flutter#147050

### Changelog Description:
Explain this cherry pick in one line that is accessible to most Flutter developers. See [best practices](https://github.com/flutter/flutter/blob/main/docs/releases/Hotfix-Documentation-Best-Practices.md) for examples

When semantics are enabled on Web, the `onTap` of various widgets (`GestureDetector`, `InkWell`) are called twice or the callback from one widget is called when another widget is pressed.

### Impact Description:
What is the impact (ex. visual jank on Samsung phones, app crash, cannot ship an iOS app)? Does it impact development (ex. flutter doctor crashes when Android Studio is installed), or the shipping production app (the app crashes on launch)

This causes multiple issues, e.g I have an `InputField` and a `InkWell` within the same page, tapping on the InputField executes the callback of the `InkWell`.

### Workaround:
Is there a workaround for this issue?

The workaround is to go through all clickable components and set `excludeFromSemantics: true`. Note that this is not always feasible when using external packages (those you don't have control over).

### Risk:
What is the risk level of this cherry-pick?

### Test Coverage:
Are you confident that your fix is well-tested by automated tests?

### Validation Steps:
What are the steps to validate that this fix works?

#### Steps to validate

1. Run the sample code below
2. Tap ONCE on the button

#### Expected results

The on tap callback is called once and `onTap` is printed once on the console:

```dart
onTap
```

#### Code sample

<details open><summary>Code sample</summary>

```dart
import 'package:flutter/material.dart';
import 'package:flutter/rendering.dart';

void main() {
  WidgetsFlutterBinding.ensureInitialized();
  SemanticsBinding.instance.ensureSemantics();
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @OverRide
  Widget build(BuildContext context) {
    return MaterialApp(
      debugShowCheckedModeBanner: false,
      home: Semantics(
        enabled: true,
        child: Scaffold(
          body: Center(
            child: GestureDetector(
              onTap: () => print('onTap'),
              child: const Text('Click me'),
            ),
          ),
        ),
      ),
    );
  }
}
```

</details>
  • Loading branch information
flutteractionsbot authored Aug 28, 2024
1 parent cd2627a commit f314ded
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
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
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 f314ded

Please sign in to comment.