这是indexloc提供的服务,不要输入任何密码
Skip to content

Improve CupertinoContextMenu to match native more #117698

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

Merged
merged 19 commits into from
Feb 15, 2023
Merged

Improve CupertinoContextMenu to match native more #117698

merged 19 commits into from
Feb 15, 2023

Conversation

manuthebyte
Copy link
Contributor

@manuthebyte manuthebyte commented Dec 27, 2022

This PR adds an improved look of the CupertinoContextMenu and optional Haptic Feedback. Fixes #92260

What I also tried to do, but didn't get to achieve is the "Continue Swipe" feature. In iOS if you hold your finger, as an example, on an App and don't lift off the finger you can "swipe" through the actions and the one where you let your finger off, is being selected.
I tried achieving this but since a decoy child is being created, the original onTap Event is being cancelled and so the decoy child doesn't get the event passed on.
If anyone has an idea on how to make this happen, the context menu would 100% feel like native.

The comparison is outdated, it now looks even better, see #117698 (comment) and #117698 (comment)

Before After
IMG_0131 IMG_0126

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for subåmitting 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.

@flutter-dashboard flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels. labels Dec 27, 2022
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@bernaferrari
Copy link
Contributor

bernaferrari commented Dec 30, 2022

if someone knows how to make that work, it is @gspencergoog. I really like that behavior, even Android had in a few places and macOS also allows on menu.

@manuthebyte
Copy link
Contributor Author

manuthebyte commented Dec 31, 2022

if someone knows how to make that work, it is @gspencergoog. I really like that behavior, even Android had in a few places and macOS also allows on menu.

That would be awesome!

@MitchellGoodwin MitchellGoodwin self-requested a review January 3, 2023 17:07
Copy link
Contributor

@MitchellGoodwin MitchellGoodwin left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! Can you please add a test to check for this fix? It does not need to be something too complicated. Just a check that the expected width of the action widgets was reached.

@MitchellGoodwin
Copy link
Contributor

Also that swipe gesture sounds like a good thing to separate into it's own issue. @manuthebyte would you mind making a new issue and possibly adding an example? Conversation about that can be moved to there.

@manuthebyte
Copy link
Contributor Author

manuthebyte commented Jan 3, 2023

Summary of what I improved/changed/added:

  • Change when the haptic feedback happens
  • Change the type of haptic feedback
  • Don't play the haptic feedback when the user aborts
  • Add test

And lastly I improved the looks (the above is flutter, below is iOS):

Darkmode Lightmode
image image

All in All, it now feels so near native, the last thing holding back is the swipe continue, for what I will open a new issue.

@manuthebyte
Copy link
Contributor Author

Also that swipe gesture sounds like a good thing to separate into it's own issue. @manuthebyte would you mind making a new issue and possibly adding an example? Conversation about that can be moved to there.

Created issue #117936 for that.

@paldepind
Copy link
Contributor

Thank you for working on this @manuthebyte. This looks great and like a huge improvement!

Just to note, HapticFeedback.heavyImpact() is close but not the same as the native haptic feedback. But unfortunately, the native press-and-hold haptic feedback is not exposed by iOS except through the native components (as far as I can tell at least).

@manuthebyte
Copy link
Contributor Author

Thank you for working on this @manuthebyte. This looks great and like a huge improvement!

Just to note, HapticFeedback.heavyImpact() is close but not the same as the native haptic feedback. But unfortunately, the native press-and-hold haptic feedback is not exposed by iOS except through the native components (as far as I can tell at least).

Tysm for your nice feedback!
Yeah so you're right, the haptic feedback is not exactly the same, but I think it's the closest we can get :)

@bernaferrari
Copy link
Contributor

Question: isn't it possible to just reduce the ms in the hapitic feedback?

@manuthebyte
Copy link
Contributor Author

Question: isn't it possible to just reduce the ms in the hapitic feedback?

I don't think that's possible that easily because the HapticFeedback class calls native functions. But I don't know it for sure.

@manuthebyte
Copy link
Contributor Author

@MitchellGoodwin What exactly is the Google Testing check and why hasn't it started yet?

@bernaferrari
Copy link
Contributor

bernaferrari commented Jan 17, 2023

Try to rebase on top of latest master (open the fork in github and click to sync, then local and rebase to the top) and force push.

@MitchellGoodwin
Copy link
Contributor

@MitchellGoodwin What exactly is the Google Testing check and why hasn't it started yet?

The Google Testing is a test that runs separately against other products at google that use Flutter to make sure we aren't introducing a breaking change. As mentioned, it gets stuck sometimes, and rebasing tends to unstick it.

I like the idea of adding the haptic feedback, even if it doesn't match native 100%, but can we make it optional? As in add a boolean to the constructor (maybe something like enableHapticFeedback?) that defaults to true so developers can opt out.

@manuthebyte
Copy link
Contributor Author

manuthebyte commented Jan 17, 2023

@MitchellGoodwin What exactly is the Google Testing check and why hasn't it started yet?

The Google Testing is a test that runs separately against other products at google that use Flutter to make sure we aren't introducing a breaking change. As mentioned, it gets stuck sometimes, and rebasing tends to unstick it.

I like the idea of adding the haptic feedback, even if it doesn't match native 100%, but can we make it optional? As in add a boolean to the constructor (maybe something like enableHapticFeedback?) that defaults to true so developers can opt out.

Ty for the explanation :)

Yeah I think it's a great idea to make it optional! Also, now that you mention it, making the haptic feedback not optional would produce two haptic feedbacks at once at some times, because I have seen some Flutter apps already having Haptic Feedback on the context menus.

@manuthebyte
Copy link
Contributor Author

Would love to optimize the dialog a little bit! But I think I would prefer that we merge this PR and then I can make a seperate one :)

As a warning, if you want to get the divider.dart looking perfect in the dialog file as well, that may be a bit of an undertaking for a little thing. They need to scale according to screen size, so it will have to match native on iOS, macOS, and web, and will likely break a fair amount of golden files that we will have to go through. But it would be nice to get that looking good.

Hmm okay. Will look into it - as you say, would be nice to have that clean as well.

@delfme
Copy link

delfme commented Feb 13, 2023

To be clear, the line between actions is done here

The one in dialog.dart just controls similar looking widgets. It'd be great if we could get that divider a little closer to native, and I don't think it will take too much tinkering, but if weird issues pop up @manuthebyte and it's not that simple, put a comment in here and we can get this PR in as a win.

Thx for the reference. If it can help, I recall I tested the 0,5px with lighten color but eventually opted for 0,25px. Fidelity is almost 100%.

Hopefully hairlines would be soon avaiable on Impelller #118947

Copy link
Contributor

@MitchellGoodwin MitchellGoodwin left a comment

Choose a reason for hiding this comment

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

Looks good to me, except for the comment on the timeout. If we can do this without using one, that would be ideal.

manuthebyte added 2 commits February 14, 2023 20:47
…ynamically to the controller. Fix not having to meet the value 0.500 exactly, only the first above 0.5.
Copy link
Contributor

@MitchellGoodwin MitchellGoodwin left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for the contribution and going along with my nitpicks! Let me find someone to get a secondary review for first-time contributors, and this will be ready to merge in.

@manuthebyte
Copy link
Contributor Author

manuthebyte commented Feb 14, 2023

LGTM. Thank you for the contribution and going along with my nitpicks! Let me find someone to get a secondary review for first-time contributors, and this will be ready to merge in.

Thank you! I am very happy with your nitpicks, because I also want it to be perfect! :)
Now the last thing is the continuous swipe. Hoping we also can achieve that. But I'm positive about that 💯

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

LGTM stamp from a Japanese personal seal

Co-authored-by: Chris Bracken <chris@bracken.jp>
@MitchellGoodwin MitchellGoodwin merged commit a12e242 into flutter:master Feb 15, 2023
@manuthebyte manuthebyte deleted the cupertinocontextmenu-improve-look branch February 15, 2023 19:31
@manuthebyte manuthebyte restored the cupertinocontextmenu-improve-look branch February 15, 2023 19:31
@manuthebyte manuthebyte deleted the cupertinocontextmenu-improve-look branch February 15, 2023 19:32
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Feb 16, 2023
* 3ad7ea3 Roll Plugins from 9c312d4d2f5f to 2ce625f1a87e (5 revisions) (flutter/flutter#120793)

* 7865713 Roll Flutter Engine from 1328c4bc6299 to 4db9673d48d6 (2 revisions) (flutter/flutter#120796)

* 541a8bf Fix switching from scrollable and non-scrollable tab bars throws (flutter/flutter#120771)

* ab1390e Use black30 for CupertinoTabBar's border (flutter/flutter#119509)

* a513d4e Fix `flutter_localizations` README references (flutter/flutter#120800)

* a664f08 In test of --(no-)fatal-infos analyzer flags, pin missing_return to info (flutter/flutter#120797)

* ef49f56 Add Android unit tests to plugin template (flutter/flutter#120720)

* a12e242 Improve CupertinoContextMenu to match native more (flutter/flutter#117698)

* a9f4366 Fix the `flutter run -d linux` tests (flutter/flutter#120721)

* dff0955 09da59a5a Roll Dart SDK from c022d475e9d8 to 5d17a336bdfe (1 revision) (flutter/engine#39649) (flutter/flutter#120816)

* f35de0c Adds wide gamut saveLayer integration test (flutter/flutter#120131)

* 99dcaa2 Roll Flutter Engine from 09da59a5adcf to a8b3d1af55b6 (3 revisions) (flutter/flutter#120821)

* 8d15083 Use the impellerc GLES output flag when compiling shaders for Android (flutter/flutter#120647)

* c6b636f [flutter_tools] Replace Future.catchError() with Future.then(onError: ...) (flutter/flutter#120637)

* 2b7d709 Add `@widgetFactory` annotation (flutter/flutter#117455)

* e65dfba Add Linux unit tests to plugin template (flutter/flutter#120814)

* dccec41 5de007b90 Remove "bringup: true" from "Linux Fuchsia FEMU" (flutter/engine#39651) (flutter/flutter#120826)

* d6de6bc 9f3b061b7 Roll buildroot to 64b0c3deecaff8e66c2deb74e2171e8297b2bfcd (flutter/engine#39653) (flutter/flutter#120830)

* da2508c bb1ff84b6 Add a white background to app anatomy diagram (flutter/engine#39638) (flutter/flutter#120832)

* 1f85497 [flutter_tools] Add the NoProfile parameter to the PowerShell execution statement (flutter/flutter#120786)

* 4ad47fb Fix `StretchingOverscrollIndicator` not handling directional changes correctly (flutter/flutter#116548)

* 9a721c4 Update AndroidManifest.xml.tmpl (flutter/flutter#120527)

* c0b7d2d Roll Flutter Engine from bb1ff84b6c4f to 02a379db1d38 (4 revisions) (flutter/flutter#120845)

* a10e295 Added identical(a,b) short circuit to Material Library lerp methods (flutter/flutter#120829)

* efde350 Roll Flutter Engine from 02a379db1d38 to a966cf878ffd (2 revisions) (flutter/flutter#120846)

* cc473e4 Roll Flutter Engine from a966cf878ffd to 3fc40ca5beb9 (3 revisions) (flutter/flutter#120850)

* d125242 Roll Flutter Engine from 3fc40ca5beb9 to 9fa2a5c3cfbd (2 revisions) (flutter/flutter#120856)

* 22e17bb ea1d087c4 Roll Skia from b8b36146c7a0 to 7b3fb04bc3d4 (3 revisions) (flutter/engine#39673) (flutter/flutter#120860)

* f85438b c8b1d2ffa Roll Fuchsia Mac SDK from YpQKlqmyn8r_snx06... to xl9Y8o-9FDyvPogki... (flutter/engine#39675) (flutter/flutter#120887)

* 174a562 d699b4a91 Roll Flutter from e3471f0 to df41e58 (83 revisions) (flutter/plugins#7184) (flutter/flutter#120888)

* 170539f Roll Flutter Engine from c8b1d2ffaec8 to 0d8d93306822 (2 revisions) (flutter/flutter#120891)
auto-submit bot pushed a commit to flutter/plugins that referenced this pull request Feb 16, 2023
* 3ad7ea3c9 Roll Plugins from 9c312d4 to 2ce625f (5 revisions) (flutter/flutter#120793)

* 786571368 Roll Flutter Engine from 1328c4bc6299 to 4db9673d48d6 (2 revisions) (flutter/flutter#120796)

* 541a8bfd9 Fix switching from scrollable and non-scrollable tab bars throws (flutter/flutter#120771)

* ab1390e0a Use black30 for CupertinoTabBar's border (flutter/flutter#119509)

* a513d4e7b Fix `flutter_localizations` README references (flutter/flutter#120800)

* a664f08a5 In test of --(no-)fatal-infos analyzer flags, pin missing_return to info (flutter/flutter#120797)

* ef49f5661 Add Android unit tests to plugin template (flutter/flutter#120720)

* a12e242c0 Improve CupertinoContextMenu to match native more (flutter/flutter#117698)

* a9f43665c Fix the `flutter run -d linux` tests (flutter/flutter#120721)

* dff09558d 09da59a5a Roll Dart SDK from c022d475e9d8 to 5d17a336bdfe (1 revision) (flutter/engine#39649) (flutter/flutter#120816)

* f35de0c80 Adds wide gamut saveLayer integration test (flutter/flutter#120131)

* 99dcaa2d9 Roll Flutter Engine from 09da59a5adcf to a8b3d1af55b6 (3 revisions) (flutter/flutter#120821)

* 8d150833b Use the impellerc GLES output flag when compiling shaders for Android (flutter/flutter#120647)

* c6b636fa5 [flutter_tools] Replace Future.catchError() with Future.then(onError: ...) (flutter/flutter#120637)

* 2b7d709fd Add `@widgetFactory` annotation (flutter/flutter#117455)

* e65dfba8e Add Linux unit tests to plugin template (flutter/flutter#120814)

* dccec41d5 5de007b90 Remove "bringup: true" from "Linux Fuchsia FEMU" (flutter/engine#39651) (flutter/flutter#120826)

* d6de6bc68 9f3b061b7 Roll buildroot to 64b0c3deecaff8e66c2deb74e2171e8297b2bfcd (flutter/engine#39653) (flutter/flutter#120830)

* da2508c9f bb1ff84b6 Add a white background to app anatomy diagram (flutter/engine#39638) (flutter/flutter#120832)

* 1f85497ef [flutter_tools] Add the NoProfile parameter to the PowerShell execution statement (flutter/flutter#120786)

* 4ad47fb47 Fix `StretchingOverscrollIndicator` not handling directional changes correctly (flutter/flutter#116548)

* 9a721c456 Update AndroidManifest.xml.tmpl (flutter/flutter#120527)

* c0b7d2ddd Roll Flutter Engine from bb1ff84b6c4f to 02a379db1d38 (4 revisions) (flutter/flutter#120845)

* a10e295a0 Added identical(a,b) short circuit to Material Library lerp methods (flutter/flutter#120829)

* efde35081 Roll Flutter Engine from 02a379db1d38 to a966cf878ffd (2 revisions) (flutter/flutter#120846)

* cc473e4f1 Roll Flutter Engine from a966cf878ffd to 3fc40ca5beb9 (3 revisions) (flutter/flutter#120850)

* d1252428c Roll Flutter Engine from 3fc40ca5beb9 to 9fa2a5c3cfbd (2 revisions) (flutter/flutter#120856)

* 22e17bb71 ea1d087c4 Roll Skia from b8b36146c7a0 to 7b3fb04bc3d4 (3 revisions) (flutter/engine#39673) (flutter/flutter#120860)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
waegari pushed a commit to waegari/flutter_plugins that referenced this pull request Jul 3, 2025
…r#7186)

* 3ad7ea3c9 Roll Plugins from 9c312d4 to 2ce625f (5 revisions) (flutter/flutter#120793)

* 786571368 Roll Flutter Engine from 1328c4bc6299 to 4db9673d48d6 (2 revisions) (flutter/flutter#120796)

* 541a8bfd9 Fix switching from scrollable and non-scrollable tab bars throws (flutter/flutter#120771)

* ab1390e0a Use black30 for CupertinoTabBar's border (flutter/flutter#119509)

* a513d4e7b Fix `flutter_localizations` README references (flutter/flutter#120800)

* a664f08a5 In test of --(no-)fatal-infos analyzer flags, pin missing_return to info (flutter/flutter#120797)

* ef49f5661 Add Android unit tests to plugin template (flutter/flutter#120720)

* a12e242c0 Improve CupertinoContextMenu to match native more (flutter/flutter#117698)

* a9f43665c Fix the `flutter run -d linux` tests (flutter/flutter#120721)

* dff09558d 09da59a5a Roll Dart SDK from c022d475e9d8 to 5d17a336bdfe (1 revision) (flutter/engine#39649) (flutter/flutter#120816)

* f35de0c80 Adds wide gamut saveLayer integration test (flutter/flutter#120131)

* 99dcaa2d9 Roll Flutter Engine from 09da59a5adcf to a8b3d1af55b6 (3 revisions) (flutter/flutter#120821)

* 8d150833b Use the impellerc GLES output flag when compiling shaders for Android (flutter/flutter#120647)

* c6b636fa5 [flutter_tools] Replace Future.catchError() with Future.then(onError: ...) (flutter/flutter#120637)

* 2b7d709fd Add `@widgetFactory` annotation (flutter/flutter#117455)

* e65dfba8e Add Linux unit tests to plugin template (flutter/flutter#120814)

* dccec41d5 5de007b90 Remove "bringup: true" from "Linux Fuchsia FEMU" (flutter/engine#39651) (flutter/flutter#120826)

* d6de6bc68 9f3b061b7 Roll buildroot to 64b0c3deecaff8e66c2deb74e2171e8297b2bfcd (flutter/engine#39653) (flutter/flutter#120830)

* da2508c9f bb1ff84b6 Add a white background to app anatomy diagram (flutter/engine#39638) (flutter/flutter#120832)

* 1f85497ef [flutter_tools] Add the NoProfile parameter to the PowerShell execution statement (flutter/flutter#120786)

* 4ad47fb47 Fix `StretchingOverscrollIndicator` not handling directional changes correctly (flutter/flutter#116548)

* 9a721c456 Update AndroidManifest.xml.tmpl (flutter/flutter#120527)

* c0b7d2ddd Roll Flutter Engine from bb1ff84b6c4f to 02a379db1d38 (4 revisions) (flutter/flutter#120845)

* a10e295a0 Added identical(a,b) short circuit to Material Library lerp methods (flutter/flutter#120829)

* efde35081 Roll Flutter Engine from 02a379db1d38 to a966cf878ffd (2 revisions) (flutter/flutter#120846)

* cc473e4f1 Roll Flutter Engine from a966cf878ffd to 3fc40ca5beb9 (3 revisions) (flutter/flutter#120850)

* d1252428c Roll Flutter Engine from 3fc40ca5beb9 to 9fa2a5c3cfbd (2 revisions) (flutter/flutter#120856)

* 22e17bb71 ea1d087c4 Roll Skia from b8b36146c7a0 to 7b3fb04bc3d4 (3 revisions) (flutter/engine#39673) (flutter/flutter#120860)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: text input Entering text in a text field or keyboard related problems f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Cupertino] CupertinoContextMenu doesn't match native contextMenu
6 participants