iOS 13 Context Menu by justinmc · Pull Request #37778 · flutter/flutter

justinmc · 2019-08-07T15:00:31Z

Description

This PR implements the ContextMenu widget.

ContextMenu(
  child: FittedBox(
    fit: BoxFit.cover,
    child: Image.asset('images/my_image.jpg'),
  ),
  actions: <ContextMenuSheetAction>[
    ContextMenuSheetAction(
      child: const Text('My action'),
      onPressed: () { ... },
    ),
    // More ContextMenuSheetActions...
  ],
);

native	Flutter

Related Issues

Closes #34728

Tests

ContextMenuSheetAction
- tapping
- hold to change color
ContextMenu
- default state without being interacted with
- opening
- all different ways to close

fluttergithubbot · 2019-08-07T15:00:33Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. While there are exceptions to this rule, if this patch modifies code it is probably not an exception.

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

…ntextMenuAction

justinmc · 2019-10-22T18:18:17Z

@HansMuller @LongCatIsLooong Ready for re-review. I've renamed to CupertinoContextMenu, changed preview to previewBuilder, and made a bunch of smaller requested changes.

One question I had: Is there a callback type that I can reuse instead of defining the PreviewBuilder and _PreviewBuilderChildless types ?

HansMuller

This looks good; just some small feedback plus: need to document/test ModalRoute.filter

HansMuller · 2019-10-22T18:22:07Z

packages/flutter/lib/src/cupertino/context_menu.dart

+/// A function that produces the preview when the CupertinoContextMenu is open.
+///
+/// Called every time the animation value changes.
+typedef PreviewBuilder = Widget Function(


This should have a name that's a little more specific because inadvertant collisions. Maybe ContextMenuPreviewBuilder

HansMuller · 2019-10-22T23:12:43Z

packages/flutter/lib/src/cupertino/context_menu.dart

+/// Widget build(BuildContext context) {
+///   return Scaffold(
+///     key: scaffoldKey,
+///     body: Center(


My mistake, what you had is fine.

HansMuller · 2019-10-22T23:23:15Z

packages/flutter/lib/src/cupertino/context_menu.dart

+  /// The widget that can be "opened" with the [CupertinoContextMenu].
+  ///
+  /// When the [CupertinoContextMenu] is long-pressed, the menu will open and
+  /// this widget (or the widget returned by [previewBuilder], if provided) will


I think this may be revealing a detail that's not important: that the child/previewBuilder will be moved to the overlay. Can we explain what it means to "open" the menu just in terms of the route we're going to push? That approach jibes with using Navigator.pop() to close the menu.

HansMuller · 2019-10-22T23:24:40Z

packages/flutter/lib/src/cupertino/context_menu.dart

+
+  /// The actions that are shown in the menu.
+  ///
+  /// This parameter cannot be null, and items in the List must be


can't be null or empty

HansMuller · 2019-10-22T23:26:32Z

packages/flutter/lib/src/cupertino/context_menu.dart

+  /// sharp corners when in the page.
+  ///
+  /// In addition to the current [BuildContext], the function is also called
+  /// with an [Animation] and the [child]. The animation is used internally to


instead of "is used internally", you could explain that the animation goes from 0-1 when the menu opens, 1-0 when it closes.

HansMuller · 2019-10-22T23:26:57Z

packages/flutter/lib/src/cupertino/context_menu.dart

+  ///       // it's closed. It uses the given animation to animate the corners
+  ///       // in sync with the opening animation.
+  ///       child: ClipRRect(
+  ///         borderRadius: BorderRadius.circular(64.0 * animation.value),


HansMuller · 2019-10-22T23:27:50Z

packages/flutter/lib/src/cupertino/context_menu.dart

+  // child in the screen.
+  _ContextMenuOrientation get _contextMenuOrientation {
+    final Rect childRect = _getRect(_childGlobalKey);
+    final double screenWidth = MediaQuery.of(context).size.width;


What's the answer here?

HansMuller · 2019-10-22T23:31:19Z

packages/flutter/lib/src/cupertino/context_menu.dart

+
+// A floating copy of the CupertinoContextMenu's child.
+//
+// When the child is pressed, but before the CupertinoContextMenu opens, it does


Thanks for the explanation!

HansMuller · 2019-10-22T23:37:07Z

packages/flutter/lib/src/widgets/routes.dart

+  }) : _filter = filter,
+       super(settings: settings);
+
+  final ui.ImageFilter _filter;


Doc needed

This new route feature should also get its own test.

I added a test to check that CupertinoContextMenu is able to add a BackdropFilter using this filter param, which was easiest since ModalRoute is abstract. Let me know if you had something else in mind.

LongCatIsLooong · 2019-10-22T22:21:38Z

packages/flutter/lib/src/cupertino/context_menu.dart

+  /// Below is an example of using `previewBuilder` to show an image tile that's
+  /// similar to each tile in the iOS iPhoto app's context menu. Several of
+  /// these could be used in a GridView for a similar effect.
+


Suggested change

///

LongCatIsLooong · 2019-10-22T22:32:11Z

packages/flutter/lib/src/cupertino/context_menu.dart

+
+class _CupertinoContextMenuState extends State<CupertinoContextMenu> with TickerProviderStateMixin {
+  final GlobalKey _childGlobalKey = GlobalKey();
+  double _childOpacity = 1.0;


I wonder if we can use an Offstage and a TickerMode instead, like the Hero widget does?

I'm trying this out.

Actually @goderbauer I got this code from you awhile back. I was asking about animating to a final laid out state, and you suggested rendering the first frame offstage and sent me this gist: https://gist.github.com/goderbauer/b136e871fac33960dbd539ea1930ff97

Any idea if there is an advantage to Offstage and TickerMode instead?

Ah I think it's because PopupRoute is already using Offstage internally, and so this widget is just managing the offstage variable.

flutter/packages/flutter/lib/src/widgets/routes.dart

Line 663 in ed36fe5

child: Offstage(

What I originally meant was to use Offstage + TickerMode to hide the original child instead of Opacity. Should we stop the animation in the original child when it's hidden, by adding a TickerMode parent? As for Offstage vs Opacity they seem interchangeable?

Oh I see, sorry! That makes more sense and is probably more performant. I'll change it.

Actually after changing it something broke. I was getting the size of the Opacity widget using a GlobalKey, and I couldn't do that with an Offstage widget (even when it was onstage). It gave me weird values and broke the tests.

My compromise is to use a TickerMode and an Opacity widget together, but let me know if you know why getting the size wasn't working.

LongCatIsLooong · 2019-10-22T22:36:18Z

packages/flutter/lib/src/cupertino/context_menu.dart

+  ///
+  /// This parameter cannot be null, and items in the List must be
+  /// [CupertinoContextMenuAction]s.
+  final List<CupertinoContextMenuAction> actions;


Might have overlooked something, but CupertinoContextMenu's implementation does not seem to make use of the fact that actions are CupertinoContextMenuActions. Is it possible to relax the type constraint to List<Widget>? According to the sketch file the actions can be grouped, and group separators look quite different from regular ones, so I wonder if we can make this a list of Widgets.

Yeah there is nothing preventing us from using List<Widget> here. I did it just to encourage users to always use CupertinoContextMenuActions, but I also didn't realize there were group separators. Does Flutter usually allow any type of widget when it can?

There's an issue for this: #12078. Also I tried adding a context menu to a UISegmentedControl, and it turns out other than group separators there can also be headers (similar to action sheet headers)

If that's more in line with how we usually do it then changing it sounds good to me.

Oh sorry I posted my last comment before Github updated to show yours. That issue definitely confirms that this should be agnostic so I'm changing it now 👍

LongCatIsLooong · 2019-10-22T23:14:47Z

packages/flutter/lib/src/cupertino/context_menu.dart

+      flex: 2,
+      child: IntrinsicHeight(
+        child: ClipRRect(
+          borderRadius: BorderRadius.circular(16.0),


Per xcode the value is 13.

codecov · 2019-10-23T23:59:52Z

Codecov Report

Merging #37778 into master will decrease coverage by 0.14%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #37778      +/-   ##
==========================================
- Coverage   61.77%   61.62%   -0.15%     
==========================================
  Files         199      195       -4     
  Lines       19276    19016     -260     
==========================================
- Hits        11907    11718     -189     
+ Misses       7369     7298      -71

Flag	Coverage Δ
#flutter_tool	`61.62% <0%> (-0.15%)`	⬇️

Impacted Files	Coverage Δ
...tter_tools/lib/src/build_system/targets/linux.dart	`56.71% <0%> (-32.84%)`	⬇️
...tools/lib/src/fuchsia/fuchsia_kernel_compiler.dart	`7.69% <0%> (-17.89%)`	⬇️
...ages/flutter_tools/lib/src/commands/build_aot.dart	`39.79% <0%> (-14.28%)`	⬇️
...lutter_tools/lib/src/android/android_workflow.dart	`63.92% <0%> (-9.13%)`	⬇️
...flutter_tools/lib/src/android/android_builder.dart	`75% <0%> (-8.34%)`	⬇️
...ckages/flutter_tools/lib/src/base/async_guard.dart	`88.88% <0%> (-5.56%)`	⬇️
packages/flutter_tools/lib/src/android/gradle.dart	`79.73% <0%> (-3.1%)`	⬇️
...ges/flutter_tools/lib/src/build_runner/web_fs.dart	`34.71% <0%> (-2.44%)`	⬇️
.../flutter_tools/lib/src/commands/build_fuchsia.dart	`81.81% <0%> (-2.19%)`	⬇️
packages/flutter_tools/lib/src/ios/xcodeproj.dart	`89.71% <0%> (-2.09%)`	⬇️
... and 55 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf45897...4a0f41c. Read the comment docs.

HansMuller · 2019-10-26T00:36:44Z

packages/flutter/lib/src/cupertino/context_menu.dart

+  final Widget child;
+
+  /// The actions that are shown in the menu.
+  ///


This would be a good place to say that typically the actions are [CupertinoContextMenuAction]s.

HansMuller

LGTM

justinmc · 2019-11-04T18:11:11Z

Closing in favor of: #43918

justinmc added 2 commits August 5, 2019 15:29

ContextMenu widget that just does the bounce longpress

2c7f318

Possible way to do add background blur to existing modal code

4bd6692

justinmc added framework flutter/packages/flutter repository. See also f: labels. f: cupertino flutter/packages/flutter/cupertino repository work in progress; do not review labels Aug 7, 2019

justinmc self-assigned this Aug 7, 2019

googlebot added the cla: yes label Aug 7, 2019

An option for adding widget to popup as an image

1ac872c

justinmc force-pushed the ios13-context-menus branch from 4a9fef5 to 346bd20 Compare August 7, 2019 22:02

Children in and out of popup line up perfectly

2cc3813

justinmc force-pushed the ios13-context-menus branch from 346bd20 to 2cc3813 Compare August 7, 2019 22:03

Animate child to final open position, hard coded for now

5d534ee

justinmc force-pushed the ios13-context-menus branch from 0afdd9d to 5d534ee Compare August 8, 2019 23:10

justinmc added 4 commits August 12, 2019 09:28

Animation fully reverses itself at the right time

c8f7520

Mask works

8080ee3

Cleanup extra widget

577fc7d

Fix analyze issues

96e9f0c

justinmc force-pushed the ios13-context-menus branch from d1abc7b to 96e9f0c Compare August 12, 2019 22:21

Open contextmenu layout for child and menu

4f75892

justinmc force-pushed the ios13-context-menus branch from 88e574e to 4f75892 Compare August 13, 2019 17:31

justinmc added 2 commits August 13, 2019 15:07

Properly lined up animations, and correct final positions

336101b

Menu looks and behaves like a menu.

45fd739

justinmc force-pushed the ios13-context-menus branch from 867363b to 45fd739 Compare August 14, 2019 16:52

justinmc added 2 commits August 14, 2019 11:26

Tap event on the child itself exposed to consumer

a6276cd

Fit all sized children into all screen sizes.

d5ddf75

justinmc force-pushed the ios13-context-menus branch from 171b77a to d5ddf75 Compare August 15, 2019 16:56

justinmc added 3 commits August 15, 2019 10:35

Horizontal padding

07822d7

Menu action border, and color tweaks

0013f66

License header for new test file

d49de11

justinmc added 2 commits October 22, 2019 10:41

preview => previewBuilder

e386268

Use addPostFrameCallback instead of Future.delayed, to be more explicit

904447f

justinmc force-pushed the ios13-context-menus branch from 3c16d3d to 904447f Compare October 22, 2019 17:44

ContextMenu => CupertinoContextMenu, ContextMenuAction => CupertinoCo…

0421d6a

…ntextMenuAction

Bracket link to itself

552da5d

HansMuller reviewed Oct 22, 2019

View reviewed changes

justinmc added 2 commits October 23, 2019 10:26

Code review, docs and test for filter, fix test naming

e69b593

Fix naming in CupertinoContextMenuAction test

70bf037

LongCatIsLooong reviewed Oct 23, 2019

View reviewed changes

justinmc added 2 commits October 23, 2019 14:41

Code review docs fix and ios border radius value

30ee2af

List<CupertinoContextMenuAction> => List<Widget>

af62ede

justinmc added 2 commits October 25, 2019 08:57

Remove unused import

c3c4f75

kOpenScale was measured to be 1.1 in Xcode.

aa287e4

HansMuller reviewed Oct 26, 2019

View reviewed changes

HansMuller approved these changes Oct 26, 2019

View reviewed changes

justinmc added 6 commits October 28, 2019 09:10

Docs addition

941d7e0

Update kopenscale in tests too, whoops

28ff626

Merge branch 'master' into ios13-context-menus

a1eb37a

Merge branch 'master' into ios13-context-menus

e3f1a1b

Use Offstage and TickerMode instead of Opacity

08edf10

Merge branch 'master' into ios13-context-menus

aa44fc1

justinmc mentioned this pull request Oct 31, 2019

CupertinoContextMenu (iOS 13) #43918

Merged

Merge branch 'master' into ios13-context-menus

4a0f41c

justinmc closed this Nov 4, 2019

justinmc deleted the ios13-context-menus branch December 4, 2019 00:44

justinmc mentioned this pull request Sep 3, 2020

Support for sheet presentation style #42560

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2021

Conversation

justinmc commented Aug 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Uh oh!

fluttergithubbot commented Aug 7, 2019

Uh oh!

justinmc commented Oct 22, 2019

Uh oh!

HansMuller 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Oct 30, 2019 • 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!

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!

codecov bot commented Oct 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc commented Nov 4, 2019

Uh oh!

Reviewers

Assignees

Labels

justinmc commented Aug 7, 2019 •

edited

Loading

LongCatIsLooong Oct 30, 2019 •

edited

Loading

codecov bot commented Oct 23, 2019 •

edited

Loading