CupertinoContextMenu improvement #131030

xhzq233 · 2023-07-21T03:47:08Z

Why the conflict

If we put a TapGestureRecognizer at CupertinoContextMenu 's subtree.

For simplicity, we name the TapGestureRecognizer inside CupertinoContextMenu as tg2, the TapGestureRecognizer at CupertinoContextMenu 's subtree as tg1, and the press duration as t1.

When kPressTimeout+_previewLongPressTimeout/2 (500ms) < t1 < kPressTimeout+_previewLongPressTimeout (900ms), this moment

Although tg2 was rejected, _ContextMenuRoute was still opened.
And the onTap method of tg1 is triggered

The problem is that the GestureRecognizer in this ContextMenu is TapGestureRecognizer instead of LongPressGestureRecognizer, and Tap does not claim to victory, but GestureArena selects the deepest Tap to win.

When t1 is greater than 500ms, it means that it has passed PrimaryPointerGestureRecognizer.deadline plus half of the openController's duration, means that _ContextMenuRoute is about to be opened. After GestureArena received pointer up event, tg1 competed with tg2. The default "child-victory" worked, and the onTap of tg1 was called, so both were triggered.
When t1 is less than 900ms, because route is opened at this time, and both will be rejected by arena.

Solution

The reason is that TapGestureRecognizer will not declare victory by itself. Add the following code.

// call this when animation's value first reaches [_midpoint]

_tapGestureRecognizer.resolve(GestureDisposition.accepted);

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I signed the [CLA].
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is [test-exempt].
All existing and new tests are passing.

xhzq233 · 2023-07-23T15:57:17Z

I'm not sure ask who to review my code. Can you please give me some help? @MitchellGoodwin

MitchellGoodwin · 2023-07-24T17:54:55Z

@xhzq233 it looks like the failing check has to do with a plugin that this PR should not be interacting with. Would you mind rebasing to the current master branch to see if that fixes the check?

xhzq233 · 2023-07-25T04:29:46Z

@xhzq233 it looks like the failing check has to do with a plugin that this PR should not be interacting with. Would you mind rebasing to the current master branch to see if that fixes the check?

Merged. Now it has become a Windows build check error.= =

MitchellGoodwin · 2023-07-26T16:39:30Z

Merged. Now it has become a Windows build check error.= =

Looks like there was some other issues. Another rebase to current master might work.

MitchellGoodwin · 2023-07-26T16:57:39Z

packages/flutter/test/cupertino/context_menu_test.dart

+    await gesture.down(tester.getCenter(find.byKey(const Key('container'))));
+    await tester.pump();
+    // simulate a press with specified duration
+    for (int i = 0; i < 100; i++) {


Did you try await tester.pump(const Duration(milliseconds: pressDuration)); first? Is this for loop more consistent?

Yes, I tried it first. But _openAnimation won't reach midPoint since a pump only triggers one frame.

I need more frames to drive _openAnimation to reach the midPoint, so the route can be opened.

I think my main concern is that this loop just looks a little forced. Would pumpFrames work for this case?

According to https://github.com/flutter/flutter/blob/2c71881f505b0dc0997e959b3ac37664d221fad8/packages/flutter_test/lib/src/widget_tester.dart#L704C3-L719C4

pumpFrames use the same methodology as me, and it will pump another widget. So I think it probably isn't necessary to use pumpFrames.

Does it really matter that 100 frames need to be pumped, not just 1 frame over the same duration? I wouldn't think that would affect the gesture.

It aims to simulate the actual situation: the user keeps pressing and requesting frames. If there is only one frame, the animation is mutant and cannot drive the value of the animation controller.

Maybe the value of 100 is unreasonable. This is what I came up with casually.

MitchellGoodwin · 2023-07-26T17:01:58Z

packages/flutter/test/cupertino/context_menu_test.dart

+    await tester.pumpAndSettle();
+
+    // judge whether _ContextMenuRouteStatic present on the screen
+    final Finder static = find.byWidgetPredicate(


static is a protected variable in Dart. Could you change the name of this to something else?

Done 👍🏻

MitchellGoodwin

LGTM. Thank you for answering my questions and putting this together.

justinmc

LGTM with nits 👍

justinmc · 2023-08-02T16:48:52Z

packages/flutter/test/cupertino/context_menu_test.dart

    });
  });
+
+  testWidgets('Is gesture conflicts of ctx_menu resolved',


Nit: What is ctx_menu? Maybe change this to: testWidgets('Conflicting gesture detectors',

justinmc · 2023-08-02T16:49:53Z

packages/flutter/test/cupertino/context_menu_test.dart

+    int? onPointerDownTime;
+    int? onPointerUpTime;
+    bool insideTapTriggered = false;
+    // 500ms < duration < 900ms


Nit: Maybe explain where these values come from here.

justinmc · 2023-08-02T16:50:15Z

packages/flutter/test/cupertino/context_menu_test.dart

+    final TestGesture gesture = await tester.createGesture();
+    await gesture.down(tester.getCenter(find.byKey(const Key('container'))));
+    await tester.pump();
+    // simulate a press with specified duration


Nit: Capital letter at the beginning and period at the end of your comments here and below.

justinmc · 2023-08-02T16:53:52Z

packages/flutter/test/cupertino/context_menu_test.dart

+          (Widget w) => '${w.runtimeType}' == '_ContextMenuRouteStatic',
+    );
+    // can't present together
+    if (insideTapTriggered) {


Why is this an if, does it happen differently on different platforms?

This is used to detect whether the click gesture is detected by GestureDetector, so there will be no difference between different platforms.

justinmc · 2023-08-02T16:55:55Z

packages/flutter/test/cupertino/context_menu_test.dart

+    await gesture.down(tester.getCenter(find.byKey(const Key('container'))));
+    await tester.pump();
+    // simulate a press with specified duration
+    for (int i = 0; i < 100; i++) {


Does it really matter that 100 frames need to be pumped, not just 1 frame over the same duration? I wouldn't think that would affect the gesture.

xhzq233 · 2023-08-03T02:03:50Z

Done. It is my first time contributing to Flutter. Thank you for your review and guidance.

justinmc

Renewing my LGTM and merging. Thanks for fixing this!

flutter/flutter@ad0aa8d...436df69 2023-08-08 [email protected] Roll Flutter Engine from 9c83d90b01bd to 146c4c9487fc (6 revisions) (flutter/flutter#132112) 2023-08-08 [email protected] Roll Flutter Engine from c27109291e22 to 9c83d90b01bd (5 revisions) (flutter/flutter#132108) 2023-08-08 [email protected] Roll Flutter Engine from be085f6699b6 to c27109291e22 (3 revisions) (flutter/flutter#132086) 2023-08-08 [email protected] Revert "Replace TextField.canRequestFocus with TextField.focusNode.canRequestFocus" (flutter/flutter#132104) 2023-08-08 [email protected] [Impeller] add drawVertices and drawAtlas benchmarks. (flutter/flutter#132080) 2023-08-07 [email protected] Adds more documentations around ignoreSemantics deprecations. (flutter/flutter#131287) 2023-08-07 [email protected] [web] New HtmlElementView.fromTagName constructor (flutter/flutter#130513) 2023-08-07 [email protected] Move mock canvas to flutter_test (flutter/flutter#131631) 2023-08-07 [email protected] Add static_path_tessellation macrobenchmark (flutter/flutter#131837) 2023-08-07 [email protected] [web] Remove usage of `ui.webOnlyInitializePlatform()` (flutter/flutter#131344) 2023-08-07 [email protected] Roll Flutter Engine from 39a575f65d50 to be085f6699b6 (1 revision) (flutter/flutter#132069) 2023-08-07 [email protected] Android context menu theming and visual update (flutter/flutter#131816) 2023-08-07 [email protected] Roll Flutter Engine from 39ce1c097bce to 39a575f65d50 (2 revisions) (flutter/flutter#132064) 2023-08-07 [email protected] CupertinoContextMenu improvement (flutter/flutter#131030) 2023-08-07 [email protected] Roll Flutter Engine from 5b47c0577060 to 39ce1c097bce (3 revisions) (flutter/flutter#132057) 2023-08-07 [email protected] Slider should check `mounted` before start interaction (flutter/flutter#132010) 2023-08-07 [email protected] Roll Packages from ce53da1 to d7ee75a (7 revisions) (flutter/flutter#132058) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

xhzq233 added 2 commits July 21, 2023 11:39

Added test to check whether the gesture conflict is resolved.

b296617

Fix gesture conflicts of context_menu

1cff3bd

github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. f: cupertino flutter/packages/flutter/cupertino repository labels Jul 21, 2023

xhzq233 mentioned this pull request Jul 21, 2023

Fix gesture conflicts of context_menu #130977

Closed

8 tasks

MitchellGoodwin self-requested a review July 24, 2023 17:33

Merge branch 'flutter:master' into context_menu_improvement

c0302db

MitchellGoodwin reviewed Jul 26, 2023

View reviewed changes

xhzq233 added 2 commits July 27, 2023 10:32

Rename protected var in test

7580a12

Merge branch 'flutter:master' into context_menu_improvement

2ea8a7f

justinmc changed the title ~~Context menu improvement~~ CupertinoContextMenu improvement Jul 27, 2023

MitchellGoodwin approved these changes Aug 2, 2023

View reviewed changes

justinmc approved these changes Aug 2, 2023

View reviewed changes

xhzq233 added 2 commits August 3, 2023 09:56

Clarify comments

e529691

Merge branch 'flutter:master' into context_menu_improvement

d94d9ca

justinmc approved these changes Aug 7, 2023

View reviewed changes

justinmc merged commit ff5b0e1 into flutter:master Aug 7, 2023

engine-flutter-autoroll mentioned this pull request Aug 8, 2023

Roll Flutter from ad0aa8de7512 to 436df69a4684 (17 revisions) flutter/packages#4663

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 8, 2023

ff5b0e1 CupertinoContextMenu improvement (flutter/flutter#131030)

4681f8a

HansMuller mentioned this pull request Aug 11, 2023

Flutter Framework Team: Notable Commits #121415

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 15, 2023

ff5b0e1 CupertinoContextMenu improvement (flutter/flutter#131030)

96f47bc

Piinks mentioned this pull request May 21, 2025

Cupertino ContextMenu improved onTap support #81057

Closed

victorsanni mentioned this pull request Jun 6, 2025

CupertinoContextMenu onTap gesture interferes with child widget with onTap GestureRecognizer #169911

Closed

CupertinoContextMenu improvement #131030

CupertinoContextMenu improvement #131030

Uh oh!

Conversation

xhzq233 commented Jul 21, 2023

Why the conflict

Solution

Pre-launch Checklist

Uh oh!

xhzq233 commented Jul 23, 2023

Uh oh!

MitchellGoodwin commented Jul 24, 2023

Uh oh!

xhzq233 commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MitchellGoodwin commented Jul 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xhzq233 Aug 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xhzq233 Aug 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MitchellGoodwin left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xhzq233 commented Aug 3, 2023

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

xhzq233 commented Jul 25, 2023 •

edited

Loading

xhzq233 Aug 2, 2023 •

edited

Loading

xhzq233 Aug 3, 2023 •

edited

Loading