Skip to content

Conversation

@flutteractionsbot
Copy link

@flutteractionsbot flutteractionsbot commented Jul 31, 2025

This pull request is created by automatic cherry pick workflow

Issue Link:

What is the link to the issue this cherry-pick is addressing?

#172180

Changelog Description:

Fix a bug that causes the web engine to double report taps on dropdown menu items on iOS Safari.

Impact Description:

It breaks Flutter Web apps that use dropdown menus with semantics enabled on iOS Safari.

Workaround:

Is there a workaround for this issue?

No.

Risk:

What is the risk level of this cherry-pick?

  • Low
  • Medium
  • High

Test Coverage:

Are you confident that your fix is well-tested by automated tests?

  • Yes
  • No

Validation Steps:

What are the steps to validate that this fix works?

Follow repro steps in #172180

…r#172995)

It turns out iOS Safari in some cases tracks timers that are scheduled
from within a `pointerdown` listener, and it delays the `click` event
until those timers have expired (with a max waiting time of 350ms or
so).

The `ClickDebouncer` sets a timer of 200ms to see if a `click` event is
received by then. But because of the Safari behavior explained above,
the `click` event will always arrive right after the `ClickDebouncer`'s
timer, so we always misattribute the `click` event.

Fixes flutter#172180
@flutteractionsbot flutteractionsbot added the cp: review Cherry-picks in the review queue label Jul 31, 2025
@flutteractionsbot
Copy link
Author

@mdebbar please fill out the PR description above, afterwards the release team will review this request.

@flutter-dashboard
Copy link

This pull request was opened from and to a release candidate branch. This should only be done as part of the official Flutter release process. If you are attempting to make a regular contribution to the Flutter project, please close this PR and follow the instructions at Tree Hygiene for detailed instructions on contributing to Flutter.

Reviewers: Use caution before merging pull requests to release branches. Ensure the proper procedure has been followed.

@github-actions github-actions bot added engine flutter/engine related. See also e: labels. platform-web Web applications specifically labels Jul 31, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a workaround for an issue with click debouncing on iOS Safari. The change defers the start of the debouncing logic to the end of the event loop using Timer.run(). This is achieved by introducing a new state flag _isDebouncing and splitting the debouncing logic into synchronous and asynchronous parts. The tests have been updated to accommodate this asynchronous behavior, and a new test has been added to verify it. The changes look good, but I've identified a potential race condition that could lead to a minor resource leak, and I've suggested a fix.

Comment on lines 352 to 374
void _doStartDebouncing(DomEvent event, List<ui.PointerData> data) {
_state = (
target: event.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: <QueuedEvent>[
(
event: event,
timeStamp: _BaseAdapter._eventTimeStampToDuration(event.timeStamp!),
data: data,
),
],
);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There's a potential race condition here. If reset() is called after _maybeStartDebouncing sets _isDebouncing = true but before this method runs (e.g. due to a hot restart), _isDebouncing will be false. This method would still proceed to set _state and create a Timer, which would be leaked.

To prevent this, you should add a check at the beginning of this method to ensure debouncing is still active before proceeding.

  void _doStartDebouncing(DomEvent event, List<ui.PointerData> data) {
    // It's possible that debouncing was cancelled between the pointerdown event
    // and the execution of this method.
    if (!isDebouncing) {
      return;
    }
    _state = (
      target: event.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: <QueuedEvent>[
        (
          event: event,
          timeStamp: _BaseAdapter._eventTimeStampToDuration(event.timeStamp!),
          data: data,
        ),
      ],
    );
  }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mdebbar I think this is a valid concern here? Otherwise CPLGTM.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this is yet another nice catch by Gemini!

This race condition may not happen in practice (or very rarely), but it's still a good idea to cover our bases.

I'll do the fix in a normal PR first (to master), then update this CP to reflect the changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gemini-code-assist nice catch! Can you help me write a test for the race condition you found?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great question! To test the race condition where reset() is called after _maybeStartDebouncing schedules _doStartDebouncing but before it executes, you can simulate this exact timing in a test. The key is to call reset() on the ClickDebouncer instance before allowing the Timer.run() callback to execute.

Here's a test case you can add to engine/src/flutter/lib/web_ui/test/engine/pointer_binding_test.dart:

testWithSemantics('Does not start debouncing if reset before scheduled execution', () async {
  expect(EnginePlatformDispatcher.instance.semanticsEnabled, isTrue);
  expect(PointerBinding.clickDebouncer.isDebouncing, isFalse);
  expect(PointerBinding.clickDebouncer.debugState, isNull);

  final DomElement testElement = createDomElement('flt-semantics');
  testElement.setAttribute('flt-tappable', '');
  view.dom.semanticsHost.appendChild(testElement);

  // 1. Trigger _maybeStartDebouncing, which sets _isDebouncing = true and schedules _doStartDebouncing.
  testElement.dispatchEvent(context.primaryDown());

  // At this point, _isDebouncing is true, but _doStartDebouncing (which sets _state and creates the Timer)
  // has not yet executed because it was scheduled with Timer.run().
  expect(PointerBinding.clickDebouncer.isDebouncing, isTrue);
  expect(PointerBinding.clickDebouncer.debugState, isNull); // _state is still null

  // 2. Simulate a scenario where reset() is called before _doStartDebouncing gets a chance to run.
  // This could happen due to a hot restart or other lifecycle events.
  PointerBinding.clickDebouncer.reset();

  // After reset(), _isDebouncing should be false and _state should still be null.
  expect(PointerBinding.clickDebouncer.isDebouncing, isFalse);
  expect(PointerBinding.clickDebouncer.debugState, isNull);

  // 3. Allow the scheduled _doStartDebouncing to run. With the fix, it should now check
  // `!isDebouncing` and return early.
  await nextEventLoop();

  // Verify that _doStartDebouncing did not proceed to set _state or create a Timer.
  expect(PointerBinding.clickDebouncer.isDebouncing, isFalse);
  expect(PointerBinding.clickDebouncer.debugState, isNull);

  // Ensure no events were sent to the framework as debouncing was effectively cancelled.
  expect(pointerPackets, isEmpty);
  expect(semanticsActions, isEmpty);
});

This test ensures that if reset() is called between the scheduling and execution of _doStartDebouncing, the _doStartDebouncing method correctly aborts, preventing a leaked Timer and _state.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created PR here: #173294, and updated this CP to reflect the new changes.

github-merge-queue bot pushed a commit that referenced this pull request Aug 5, 2025
danilozhang pushed a commit to danilozhang/flutter that referenced this pull request Aug 6, 2025
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CPLGTM with the fix for the Gemini case 👍

@justinmc justinmc added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 6, 2025
@auto-submit auto-submit bot merged commit 659d955 into flutter:flutter-3.35-candidate.0 Aug 6, 2025
163 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 14, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 14, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2025
ksokolovskyi pushed a commit to ksokolovskyi/flutter that referenced this pull request Aug 19, 2025
mboetger pushed a commit to mboetger/flutter that referenced this pull request Sep 18, 2025
korca0220 pushed a commit to korca0220/flutter that referenced this pull request Sep 22, 2025
lucaantonelli pushed a commit to lucaantonelli/flutter that referenced this pull request Nov 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App cp: review Cherry-picks in the review queue engine flutter/engine related. See also e: labels. platform-web Web applications specifically

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants