Mac context menu by justinmc · Pull Request #73882 · flutter/flutter

justinmc · 2021-01-13T19:34:31Z

A minimal Mac context menu.

Native is left, Flutter is right.

Flutter light mode	Flutter dark mode

Tests

Tested that the Mac text selection toolbar shows and works correctly on all text editing widgets (I couldn't just test EditableText because it doesn't set up the text selection controls itself).
Tested Mac mouse selection behavior.
Stopped testing force press on Mac.

Breaking change

No.

Known limitations

I'm tracking all of these in #74255.

We can't draw outside of the current window yet, so this menu moves to always stay inside.
The menu items are the same as on mobile. None of the myriad native desktop buttons will appear. ~~However, it is possible to customize the buttons with a little work (similar to how it is on Android/iOS now).~~ Disallowing customization until the menu is done in a more robust way.
There is no flash animation when a button is clicked.
There is no built-in support for submenus.
The menu's hover effect doesn't happen unless the window is focused.
Only right clicking on a text field or SelectableText will show the menu, not just anywhere. Same for trying to close the menu by clicking elsewhere.
Menu items that are wider than the default width are ellided, but on native the menu grows to accommodate them.

…ST file

…ehavior

…position

justinmc · 2021-01-14T01:50:37Z

packages/flutter/lib/src/rendering/editable.dart

  Offset? _lastTapDownPosition;

+  /// The position of the most recent tap down event on this text input.
+  Offset? get lastTapDownPosition => _lastTapDownPosition;


I had to expose this in order to position the menu at the tap position (on mobile, it's positioned centered on the selection). Any objections to this or better ideas?

Ideally I'll be able to clean up RenderEditable stuff like this in my refactor later...

Probably OK in the short term. We should really be using the existing menu system to show desktop menus.

justinmc · 2021-01-14T02:15:14Z

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

+const double _kToolbarWidth = 222.0;
+const Color _kToolbarBorderColor = Color(0xFF505152);
+const Radius _kToolbarBorderRadius = Radius.circular(4.0);
+const Color _kToolbarBackgroundColor = Color(0xFF2D2E31);


How do I get this from the theme so that it depends on light/dark mode?

CupertinoDynamicColor values in CupertinoColors resolve to different color values, depending on the theme's CupertinoTheme's brightness. CupertinoDynamicColor.resolve(CupertinoColors.systemBlue, context).

justinmc · 2021-01-14T02:16:36Z

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

+        child: CupertinoButton(
+          alignment: Alignment.centerLeft,
+          borderRadius: null,
+          color: _isHovered ? CupertinoTheme.of(context).primaryColor : null,
+          minSize: 0.0,
+          onPressed: widget.onPressed,
+          padding: _kToolbarButtonPadding,
+          pressedOpacity: 0.7,
+          child: widget.child,
+        ),


I worry that the text may not be vertically centered in these buttons.

It's not, although we could improve the situation a little by tweaking the padding for now. Part of the root problem is #72521 (comment)

I've improved it by a pixel or two with padding and it looks noticeably better.

LefebvreIlyas · 2021-01-14T14:04:53Z

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

+
+// These values were measured from a screenshot of TextEdit on MacOS 10.15.7 on
+// a Macbook Pro.
+const double _kToolbarWidth = 222.0;


Hello Justin !

It seems that the width is not fixed and depends on the widest menu item according to Apple documentation.

Good point, thanks! I'll make that a minimum width.

This is not as easy as I thought. I think it might need a custom layout widget. The parent needs to size itself to the biggest intrinsic width of its children, and the children then need to all match that.

I've added this to the "Known limitations" above since this PR is somewhat rushed. Given more time this is definitely doable, though.

Similar to #72023.

Okok perfect!

justinmc · 2021-01-14T19:47:34Z

packages/flutter/lib/src/material/text_selection_toolbar.dart

-///   * [TextSelectionToolbar.toolbarBuilder], which is of this type.
-///   * [CupertinoTextSelectionToolbar.toolbarBuilder], which is similar, but
-///     for a Cupertino-style toolbar.
-typedef ToolbarBuilder = Widget Function(BuildContext context, Widget child);


Moved to widgets so it can be shared.

…w fixes.

…ow allows Mac taps still

justinmc · 2021-01-15T21:38:43Z

The Google testing failure is just because #73900, which this PR depends on, has not yet been rolled into google3.

…ontext menu

fluttergithubbot · 2021-01-19T19:19:04Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

justinmc · 2021-01-20T21:17:53Z

I'm going to merge this even though Google tests are failing because:

This needs to make it into the next cutoff.
The failing tests seem to all be because Added CupertinoButton alignment property #73900 hasn't been rolled yet, otherwise they should pass.
I've manually run the Google tests using master and they seem to pass: https://critique-ng.corp.google.com/cl/352837311

CC @HansMuller

creativecreatorormaybenot · 2021-02-02T18:55:04Z

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

    List<TextSelectionPoint> endpoints,
    TextSelectionDelegate delegate,
    ClipboardStatusNotifier clipboardStatus,
+    Offset? lastSecondaryTapDownPosition,


@justinmc This is a breaking change.

What is the suggested migration?

(this breaks flutter_math e.g. - I will link the PR)

Ah sorry about that. The migration should be to pass renderObject.lastSecondaryTapDownPosition. See https://github.com/flutter/flutter/pull/73882/files#diff-6f186b82dcb3d864ea23246744ea7a67bcb64d369e0da5495cd0bd0e038f9bd9R600

Alright, thanks ❤️

All desktop context menu work in one commit due to weird branching

c31cd9c

justinmc self-assigned this Jan 13, 2021

flutter-dashboard bot added f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jan 13, 2021

google-cla bot added the cla: yes label Jan 13, 2021

justinmc added 13 commits January 13, 2021 12:05

Split out button and basic toolbar

5b35b1e

Use shared LayoutDelegate for position, and move Content to desktop T…

b10080c

…ST file

No more overflow logic since this menu doesnt have special overflow b…

1b6601a

…ehavior

Naively position the menu at the last tap down position

667737d

Move the menu onscreen if it is hanging off

7695fdb

Working toolbar builder

cbc6d55

iOS=>Mac in docs and remove constants

a7d9bbf

Delete handles

2ed0b58

Fixed todos in button and added docs

8cd6348

Merge branch 'master' into desktop-context-menu

9ba4abd

License headers

4301e61

Code cleanup

d7b2d99

When tapping on selection, the menu fades out and back in at the new …

448aa1d

…position

justinmc commented Jan 14, 2021

View reviewed changes

Move lastTapWasOnSelection so I can make it private

e7b35d1

justinmc requested a review from HansMuller January 14, 2021 02:01

Analyzer fixes

343c2ec

justinmc force-pushed the desktop-context-menu branch from a3b723d to 343c2ec Compare January 14, 2021 02:02

Color from theme in button

51c86bb

justinmc commented Jan 14, 2021

View reviewed changes

LefebvreIlyas reviewed Jan 14, 2021

View reviewed changes

Fix docs macro reference

06017ac

justinmc commented Jan 14, 2021

View reviewed changes

justinmc added 3 commits January 15, 2021 11:02

Simplify clipboard status listening logic, and other small code revie…

ddfc82a

…w fixes.

Consistent lastSecondaryTapDown name

1982dee

Fix clipboardstatus in tests, and re-add Mac tap tests since the PR n…

d318d0d

…ow allows Mac taps still

justinmc added 5 commits January 15, 2021 15:11

WIP Simple test for all high level text widgets that can have a Mac c…

c66defc

…ontext menu

Correct light mode border color

3c2de78

Mac menu test for CupertinoTextField and SelectableText

dce1305

Fix references to text.dart (consolidated to editable_text_utils.dart)

e437a2c

fix clipboard in textformfield test

077dab3

justinmc marked this pull request as ready for review January 16, 2021 08:09

Dont test the desktop text selection toolbar on web

1dbb384

justinmc force-pushed the desktop-context-menu branch from 43eebd8 to 1dbb384 Compare January 19, 2021 17:09

justinmc mentioned this pull request Jan 19, 2021

Proposal: Simple custom text selection toolbars #73574

Closed

Merge branch 'master' into desktop-context-menu

f7f6e03

justinmc added the waiting for tree to go green label Jan 19, 2021

fluttergithubbot removed the waiting for tree to go green label Jan 19, 2021

This was referenced Jan 19, 2021

☂️ Proposal: desktop context menu fidelity #74255

Open

Material Desktop Context Menu #74286

Merged

justinmc merged commit 24e195d into flutter:master Jan 20, 2021

justinmc deleted the desktop-context-menu branch January 20, 2021 21:18

justinmc mentioned this pull request Jan 20, 2021

Should implement a context menu widget #31955

Closed

dvd-bnc mentioned this pull request Jan 27, 2021

Do not select text on secondary tap on desktop #74791

Closed

creativecreatorormaybenot reviewed Feb 2, 2021

View reviewed changes

creativecreatorormaybenot mentioned this pull request Feb 2, 2021

Fix breaking buildToolbar change znjameswu/flutter_math#33

Closed

justinmc added the a: text input Entering text in a text field or keyboard related problems label Feb 23, 2021

bradcypert mentioned this pull request Mar 4, 2021

Fix selection issues with Flutter 2.0 memspace/zefyr#488

Closed

lesnitsky mentioned this pull request Sep 28, 2021

Menu macosui/macos_ui#89

Closed

Conversation

justinmc commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tests

Breaking change

Known limitations

Uh oh!

justinmc Jan 14, 2021 • 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!

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!

justinmc commented Jan 15, 2021

Uh oh!

fluttergithubbot commented Jan 19, 2021

Uh oh!

justinmc commented Jan 20, 2021

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

justinmc commented Jan 13, 2021 •

edited

Loading

justinmc Jan 14, 2021 •

edited

Loading