-
Notifications
You must be signed in to change notification settings - Fork 29.8k
[CP-beta][web] ClickDebouncer workaround for iOS Safari click behavior #173072
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CP-beta][web] ClickDebouncer workaround for iOS Safari click behavior #173072
Conversation
…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
|
@mdebbar please fill out the PR description above, afterwards the release team will review this request. |
|
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. |
There was a problem hiding this 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.
| 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, | ||
| ), | ||
| ], | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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,
),
],
);
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Based on Gemini's comment: #173072 (comment)
Based on Gemini's comment: flutter#173072 (comment)
justinmc
left a comment
There was a problem hiding this 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 👍
659d955
into
flutter:flutter-3.35-candidate.0
Picks up two beta cherry picks: https://github.com/flutter/flutter/commits/flutter-3.35-candidate.0/ #173304 #173072
Based on Gemini's comment: flutter#173072 (comment)
Based on Gemini's comment: flutter#173072 (comment)
Based on Gemini's comment: flutter#173072 (comment)
Based on Gemini's comment: flutter#173072 (comment)
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?
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?
Follow repro steps in #172180