[ios_edit_menu]add native edit menu #50095

hellohuanlin · 2024-01-26T20:12:43Z

Support native edit menu on the engine side.

Design doc: https://docs.google.com/document/d/16-8kn58h_oD902e7vPSh6W20aHRBJKyNOdSe5rbAe_g/edit?resourcekey=0-gVdJ3fbOybV70ZKeHU7fkQ&tab=t.0

List which issues are fixed by this PR. You must list at least one issue.

flutter/flutter#103163

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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 and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

hellohuanlin · 2024-01-26T20:15:46Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

+  if (@available(iOS 16.0, *)) {
+    // TODO: get point and arrowDirection from args
+    CGPoint point = CGPointMake(100, 100);
+    UIEditMenuArrowDirection arrowDirection = UIEditMenuArrowDirectionUp;


@justinmc For M1, we just need a point and arrowDirection from the framework.

For point, I think both global or relative points are fine and I can do the conversion in the engine (not sure which is the convention)

For arrowDirection, up actually means the edit menu is below the text field (triangle pointing upwards)

I believe global coordinates is the convention. I just checked and the Scribble feature sends global Offsets in its method channel calls.

The way that arrow position works in the framework is that it tries to fit above the anchor point, but if it's too big, then it goes below. So for the framework to provide arrowDirection are we able to hardcode the height of the native menu in the framework?

Also, if you for example purposely position the menu too close to the top and use arrowDirection down, does it hang off the screen?

Also, if you for example purposely position the menu too close to the top and use arrowDirection down, does it hang off the screen?

When the menu is positioned exceeding the bound, then it will be capped within bound. For example, below I set y to be -1000:

The way that arrow position works in the framework is that it tries to fit above the anchor point, but if it's too big, then it goes below. So for the framework to provide arrowDirection are we able to hardcode the height of the native menu in the framework?

Interesting. Let me try to see if it is possible to do similar in the engine (e.g. get the size of the menu). If it's possible, then the framework can send 2 points (top and bottom), and engine can decide which one to use;
If not possible, then we will have to use a "magic number" (either engine side or framework side)

Sounds good. On the framework we typically already have both an anchorAbove and anchorBelow, so it would be easy to send both.

On the framework do you have the rectangle area? I found the new API that takes a rect and iOS will decide the arrow direction automatically: #50095 (comment)

justinmc

The API looks good and simple so far. I think the one main thing to make sure of is how the framework will decide the arrow direction (commented below). Mainly, how we will keep the behavior of making sure the menu doesn't hang off the screen.

justinmc · 2024-01-26T20:46:29Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

+  if (@available(iOS 16.0, *)) {
+    // TODO: get point and arrowDirection from args
+    CGPoint point = CGPointMake(100, 100);
+    UIEditMenuArrowDirection arrowDirection = UIEditMenuArrowDirectionUp;


I believe global coordinates is the convention. I just checked and the Scribble feature sends global Offsets in its method channel calls.

The way that arrow position works in the framework is that it tries to fit above the anchor point, but if it's too big, then it goes below. So for the framework to provide arrowDirection are we able to hardcode the height of the native menu in the framework?

Also, if you for example purposely position the menu too close to the top and use arrowDirection down, does it hang off the screen?

justinmc · 2024-01-26T20:47:25Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

  _keyboardViewContainer.frame = _keyboardRect;
 }

+- (void)showNativeEditMenu:(NSDictionary*)dictionary {


Nit: Maybe "showSystemEditMenu" to avoid saying "native"?

Or even maybe showEditMenu is ok.

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

justinmc · 2024-01-26T20:49:08Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

+- (UIMenu*)editMenuInteraction:(UIEditMenuInteraction*)interaction
+          menuForConfiguration:(UIEditMenuConfiguration*)configuration
+              suggestedActions:(NSArray<UIMenuElement*>*)suggestedActions API_AVAILABLE(ios(16.0)) {
+  return [UIMenu menuWithChildren:suggestedActions];


So in future milestones the framework could pass UIMenuElements like this?

Yes, the framework can send the items via showNativeEditMenu channel, and we can return it here.

Custom actions (e.g. send email) can be a bit more involved (e.g. the engine would need to call back to the framework).

hellohuanlin · 2024-01-26T22:38:23Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

+- (void)showNativeEditMenu:(NSDictionary*)dictionary {
+  if (@available(iOS 16.0, *)) {
+    // TODO: get targetRect from args
+    CGRect targetRect = CGRectMake(100, 100, 100, 100);


@justinmc I found an API that allows iOS to automatically decide the up/down arrow. Do you think you can provide a "target rect" from the framework side? The target rect is the rectangle that presents the menu.

Is that like a bounding box for the menu? Currently most menus use an anchorAbove and anchorBelow. One thing we could do is to create a full-width rectangle whose top and bottom are given by these two anchors. Or if we need something more specific, here is where we calculate the anchors. We should have most of the necessary metrics there, we just might need to rework the context menus to take rectangles.

It's more like a bounding box for the selected text area (not the bounding box of the edit menu). Apple just use the rect's top and bottom points for anchorAbove and anchorBelow for the edit menu.

I think the reason apple asks for the rect rather than the 2 points is for potential UI change that may show context menu on left or right, making it more future proof

Ok I see. Sounds like we are probably ok using the full-width solution mentioned above. If we really do need the bounding box of the selected text, though, we have it here and would just need to pipe it through.

Sounds like we are probably ok using the full-width solution mentioned above.

Ya a rect of any width will work.

we have it here and would just need to pipe it through.

If it's easy on your side, I prefer to take the whole rect, so we are feeding the right info into the API. But either way should work.

Sounds good, I'll play around with it when I work on the framework side.

hellohuanlin · 2024-01-30T20:44:19Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

+        [encodedTargetRect[@"x"] doubleValue], [encodedTargetRect[@"y"] doubleValue],
+        [encodedTargetRect[@"width"] doubleValue], [encodedTargetRect[@"height"] doubleValue]);
+    CGRect localTargetRect = [self.hostView convertRect:globalTargetRect toView:_activeView];
+    [_activeView showEditMenuWithTargetRect:localTargetRect];


@justinmc This PR should be something ready for you to pull and try out. I tested with this:

_channel.invokeMethod<void>( 'TextInput.showSystemContextMenu', <String, dynamic>{ // global coords 'target_rect': <String, dynamic>{ 'x': 0, 'y': 100, 'width': 100, 'height': 50, } }, );

note that i also changed the method channel name, since "edit" menu is an ios term.

hellohuanlin · 2024-02-05T19:17:19Z

hi @justinmc this is ready for review. could you take a look when you get time? Thanks!

justinmc

This seems to work great. I was finally able to get my engine compiling and try this out and it looks really nice with the native fade in/out animation and everything.

I guess this is tied to the currently active text input? I noticed that the buttons seem to work out of the box, and the menu doesn't show unless an input is focused. Is the native API tied to a UITextInput instance? I wonder if this prevents the use case of creating this menu for some reason unrelated to text input (in future milestones, I guess).

Also, I noticed that while the buttons did perform the correct action automatically, they didn't seem to be aware of the current selection. See the screenshot below where the top menu is native and the bottom one is Flutter:

Tomorrow I want to try sticking this into the real framework API and making sure that it works for a real world use case, I'll post back then.

justinmc · 2024-02-05T20:01:32Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm


+- (void)showEditMenu:(NSDictionary*)dictionary {
+  if (@available(iOS 16.0, *)) {
+    NSDictionary<NSString*, NSNumber*>* encodedTargetRect = dictionary[@"target_rect"];


Are these values typically camel case? I think I see others in this file that are camel case, but maybe we're not very standardized...

justinmc · 2024-02-05T23:48:28Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

  if (action == @selector(paste:)) {
    // Forbid pasting images, memojis, or other non-string content.
-    return [UIPasteboard generalPasteboard].string != nil;
+    // hasStrings does not require paste permission


Nit: Period at the end here.

hellohuanlin · 2024-02-06T01:13:26Z

I guess this is tied to the currently active text input?

Yes.

I noticed that the buttons seem to work out of the box, and the menu doesn't show unless an input is focused. Is the native API tied to a UITextInput instance?

Yes, the native API is tied to FlutterTextInputView, which is a UITextInput. In my PR I simply return the default suggestedActions, which contains actions based on current state of FlutterTextInputView.

I wonder if this prevents the use case of creating this menu for some reason unrelated to text input (in future milestones, I guess).

We should be able to add custom actions, by adding more items (rather than just returning the suggestedActions)

Also, I noticed that while the buttons did perform the correct action automatically, they didn't seem to be aware of the current selection. See the screenshot below where the top menu is native and the bottom one is Flutter

I think it's very likely because our canPerformAction's implementation (

engine/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

Lines 1169 to 1180 in 307471f

    
           - (BOOL)canPerformAction:(SEL)action withSender:(id)sender { 
        
             // When scribble is available, the FlutterTextInputView will display the native toolbar unless 
        
             // these text editing actions are disabled. 
        
             if ([self isScribbleAvailable] && sender == NULL) { 
        
               return NO; 
        
             } 
        
             if (action == @selector(paste:)) { 
        
               // Forbid pasting images, memojis, or other non-string content. 
        
               // hasStrings does not require paste permission 
        
               return [UIPasteboard.generalPasteboard hasStrings]; 
        
             } 
        
             return [super canPerformAction:action withSender:sender];

). We will need something like:

if (action == @selector(copy:)) {
  return [self textInRange:_selectedTextRange].length > 0
}

Let me quickly try it out.

hellohuanlin · 2024-02-06T01:44:29Z

Also, I noticed that while the buttons did perform the correct action automatically, they didn't seem to be aware of the current selection. See the screenshot below where the top menu is native and the bottom one is Flutter

@justinmc Updated the PR. This should fix the issue you have: https://github.com/flutter/engine/pull/50095/files#diff-4c7b102c0690b8ec5e2212b079f5d69fe3f816c84e47ce94bc7bc89312f39e40R1179-R1181

The overview of iOS logic is:

For milestone 1 (basic editing actions), we simply return suggestedActions in FlutterTextInputView::menuForConfiguration method. The suggestedActions is controlled by FlutterTextInputView::canPerformAction method. For example, if we return YES for FlutterTextInputView::copy: action and return NO for all other actions, then suggestedActions will only contain the copy item. Then after user tap on the button, the FlutterTextInputView::copy: method will be called.
For milestone 2 (including custom actions, and other default actions like "lookup"), we have to manually append the items in FlutterTextInputView::menuForConfiguration method. Note that we have to do it for default actions like "lookup", because it's a private API (prefixed by _).

justinmc

That seemed to work to get me the correct menu buttons, thanks!

I've opened a draft PR with the code I'm using to play around with this: flutter/flutter#143002. Just hacking it quickly and passing in the selection rect seems to work quite well:

It even does the thing where it flips upside down when near the top of the screen:

Looks really promising.

Implications of being tied to the text input connection

Because this menu is tied to the currently active text input connection in the engine, I'm going to say that it should be a part of EditableText, not AdaptiveTextSelectionToolbar on the framework side of things. The latter could be used by users for any sort of context menu, and they'll get unexpected behavior in that case, at least now for milestone 1.

In future milestones will it be possible to show a system menu for something other than text editing? Or actually, do we even need to? Say someone has built a drawing app, and long pressing on the drawing canvas shows a context menu with a "Paste" button. They want to handle the onPressed callback themselves, to do something like insert a text element into the drawing canvas with the contents of the clipboard. Is there no way around the clipboard access warning in that case, even in a native app?

When to enable this feature

Currently in milestone 1, this feature should be enabled by a boolean parameter that is disabled by default, right? Do we need to worry about what happens if someone enables it on an unsupported platform or even an unsupported version of iOS? The framework doesn't know the iOS version if I'm not mistaken.

Only other problem I'm seeing right now

The menu doesn't remove itself when I hot reload or even hot restart.

justinmc · 2024-02-08T00:32:29Z

I've got it working great on my end in my draft PR, including conditionally switching between system/Flutter menu based on supportsShowingSystemContextMenu 👍 . I want to play around with how the future milestones will work to make sure the API will support those well. I'll post back with more.

hellohuanlin · 2024-02-08T19:37:13Z

I want to play around with how the future milestones will work to make sure the API will support those well.

Sounds good. Let me know if you need anything from native side.

justinmc · 2024-02-08T23:55:54Z

Let me know if this sounds right.

On the framework side I think we can do this by creating a new widget that switches between the system menu and the normal old AdaptiveTextSelectionToolbar based on what is supported. Let's call it AdaptiveAndSystemTextSelectionToolbar (and CupertinoAdaptiveAndSystemTextSelectionToolbar for Cupertino).

By default, for now, we wouldn't do anything with these new widgets. If a user wants to use system menus, they can pass this widget to contextMenuBuilder.

return TextField(
  contextMenuBuilder: (BuildContext context, EditableTextState editableTextState) {
    return AdaptiveAndSystemTextSelectionToolbar(
      editableTextState: editableTextState,
    );
  },
);

If using only Cupertino:

return CupertinoTextField(
  contextMenuBuilder: (BuildContext context, EditableTextState editableTextState) {
    return CupertinoAdaptiveAndSystemTextSelectionToolbar(
      editableTextState: editableTextState,
    );
  },
);

In the future, if we want to enable system menus by default, we can put these widgets in _defaultContextMenuBuilder.

Milestone 1 - No customization

AdaptiveAndSystemTextSelectionToolbar creates the menu with the default buttons for text editing, both when native and when Flutter, with no ability to change anything. The only parameter is editableTextState, and it's required.

Milestone 2 - Custom buttons

In addition to taking a targetRect, showSystemContextMenu will also take a list of buttonItems. Something like this:

SystemButtonItem({
  int id,
  String label,
  SystemButtonItemType type, // Used to indicate the paste button.
  // Anything else for style, etc.?
})

In order to tell the framework that a context menu button was pressed, a new platform channel method could be created:

ContextMenu.systemContextMenuButtonTapped({
  int id,
})

AdaptiveAndSystemTextSelectionToolbar could optionally take a list of these button items. Passing nothing gives the original behavior of using only the default buttons.

Question: What if someone just wants to add a custom button to the default buttons?

Milestone 3 - Submenus

Add a children parameter of some sort to SystemButtonItem. The API of AdaptiveAndSystemTextSelectionToolbar stays the same.

hellohuanlin · 2024-02-12T19:00:30Z

By default, for now, we wouldn't do anything with these new widgets. If a user wants to use system menus, they can pass this widget to contextMenuBuilder.

It'd be great if we support secure paste out of box, without asking developers to migrate their code. The logic to enable it can be:

if (
   // 1. run time flag, true for iOS 16, false otherwise
  supportsShowingSystemContextMenu
   // 2. compile time flag, true for developers opt-in'ed
   && optInSystemContextMenuBeta 
   // 3. only support text input for now. But could support to non-text widgets in the future
   && text_input_service.hasTextConnection) 
{
  // use system context menu
} else {
  // use flutter context menu
}

Then after milestone 2 is done, we can remove optInSystemContextMenuBeta compile time flag so that its enabled by default.

What if someone just wants to add a custom button to the default buttons?

This should be doable. Developers can append items to the list of default items (from editableTextState.buttonItems). Something like this code sample.

justinmc

Another thing: The system context menu doesn't hide itself on a hot restart, even though the text field loses focus. Is there anything we can do about that? I don't think there's any way to be aware of that at the framework level... Maybe we could hack something if we had hideSystemContextMenu, like always calling that when a text input connection is opened. Or maybe it doesn't matter since it's only related to development and not a release build.

justinmc · 2024-02-22T20:57:26Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformPlugin.mm

  }
 }

+- (void)showSystemContextMenu:(NSDictionary*)args {


One possible problem I just thought of is the lack of a hideSystemContextMenu. Normally this is fine because the system context menu will hide itself when the text input connection is closed or when the user taps outside of the menu. However, it could be a problem if the user rebuilds their text field with a new context menu.

Is it possible to add a hideSystemContextMenu method, and do you think it's useful?

It should be easy to add. Let me try!

Just added ContextMenu.hideSystemContextMenu (no params). Feel free to play around.

Or maybe it doesn't matter since it's only related to development and not a release build.

If it's easy for you, I'm open to just fix it here. But if it's hard, we can also leave it, and I can create an issue to track that (no strong opinion)

Ok I'll play around with the hot restart thing and see if I can fix it, otherwise we'll open an issue.

Thanks for the quick turnaround on hideSystemContextMenu. I'll try it out and post back.

justinmc · 2024-02-23T00:43:11Z

@hellohuanlin One more thing that just came up. How does the keyboard shortcut cmd+v relate to secure paste? Do users of native apps see a warning when they paste that way, and what about Flutter apps?

hellohuanlin · 2024-02-23T04:59:04Z

@hellohuanlin One more thing that just came up. How does the keyboard shortcut cmd+v relate to secure paste? Do users of native apps see a warning when they paste that way, and what about Flutter apps?

I did a quick research on CMD+V, and wrote up a summary: flutter/flutter#143999

justinmc · 2024-02-23T19:34:21Z

Thanks for doing that!

justinmc · 2024-03-04T20:46:01Z

@hellohuanlin Is there a way to be informed of when the context menu is hidden? I see that if I tap anywhere on the screen, the system hides the context menu, and Flutter is unaware. This is probably not vitally important, but it would help with some edge cases.

hellohuanlin · 2024-03-05T04:02:06Z

@hellohuanlin Is there a way to be informed of when the context menu is hidden? I see that if I tap anywhere on the screen, the system hides the context menu, and Flutter is unaware. This is probably not vitally important, but it would help with some edge cases.

Yes, it should be easy to add. Let me know if (or when) you need it.

justinmc · 2024-03-05T19:46:22Z

@hellohuanlin Let's do it! Thank you helping me out with all of these changes.

I've written some exploratory code assuming that I can get called when the menu is hidden, and it looks good. It should make it easier to support the other milestones.

hellohuanlin · 2024-03-05T22:53:06Z

@justinmc Added the dismiss callback.

From somewhere near: https://github.com/flutter/flutter/blob/7a4c2465afe875355aaac3efee158084f13e6fc7/packages/flutter/lib/src/services/text_input.dart#L1916

Add a new case like:

      case 'TextInputClient.onDismissSystemContextMenu':
        print('TextInputClient.onDismissSystemContextMenu is called');
        print('client is');
        print(args[0]);

justinmc · 2024-03-06T00:24:39Z

Thank you for the quick turnaround! I'll try this out.

justinmc · 2024-03-07T21:36:36Z

@hellohuanlin I'm starting to feel good about this based on my work over in flutter/flutter#143002. I think one big question remains: Should the new platform channel calls be part of the text input channel, or should they be moved to their own new channel?

An argument in favor of keeping them part of text input is that currently, showing the menu requires an active text input connection. That's how it decides which buttons to show.

However, in future phases, we'd like to be able to show custom buttons in this menu. Users may want to show the menu in a non-text input related situation. For example, a drawing app may allow cut/copy/paste of drawing elements. If we want to support something like that, I think we'd have to move it off of the text input channel.

Do we ever want to support non-text-editing use cases? In the drawing app example, is secure paste still an issue when using the Flutter-rendered menu?

hellohuanlin · 2024-03-07T23:45:59Z

Should the new platform channel calls be part of the text input channel, or should they be moved to their own new channel?
Do we ever want to support non-text-editing use cases? In the drawing app example, is secure paste still an issue when using the Flutter-rendered menu?

Good point. I think it's a good idea to change. I can use platformChannel instead (https://github.com/flutter/engine/blob/90d1ff04ae021f90a7fb1ce5584b5f3daa2feb5b/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm#L619C3-L619C19). Let me know if you can access this channel in your code.

hellohuanlin · 2024-03-08T00:07:51Z

@justinmc I just updated to use platform channel rather than textInput channel. I also renamed the method to System. onDismissSystemContextMenu. Let me know if any problem using it.

justinmc · 2024-04-11T17:19:56Z

@hellohuanlin Can you fix the conflict here? I'm coming back to secure paste after finishing some other work finally.

hellohuanlin · 2024-04-11T18:03:44Z

@justinmc welcome back! Updated the branch.

justinmc

LGTM withe one last naming nit to consider.

Otherwise I have fully built the framework PR (flutter/flutter#143002) on top of this and banged on it and everything seems to work as expected. @hellohuanlin can you give the framework PR a last look before we commit to this engine interface and merge? I'm holding off on finishing the tests until this is merged because I'm struggling to run them locally with a local engine, so ignore the sloppy tests for now.

justinmc · 2024-04-12T17:43:15Z

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm


+- (void)flutterTextInputView:(FlutterTextInputView*)textInputView
+    willDismissEditMenuWithTextInputClient:(int)client {
+  [_platformChannel.get() invokeMethod:@"System.onDismissSystemContextMenu"


Should this be ContextMenuonDismissSystemContextMenu in order to align with ContextMenu.showSystemContextMenu etc.? Or is this the typical naming pattern?

Renamed to ContextMenu.onDismissSystemContextMenu

hellohuanlin · 2024-04-17T23:19:07Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformPlugin.mm

  } else if ([method isEqualToString:@"Share.invoke"]) {
    [self showShareViewController:args];
    result(nil);
+  } else if ([method isEqualToString:@"ContextMenu.showSystemContextMenu"]) {


@justinmc Wanna double check before landing it:

For non-text input, if developers setup contextMenuBuilder to use system menu, does it call this method? Does it crash or does it just NO-OP?

Currently it calls it, even if there is no active text input connection. The result I see is that nothing happens. I wonder if there's anything we can do to prevent that unnecessary call, let me look on the framework side...

To be clear, at least the only way to do this now is to directly use SystemContextMenuController. It's not currently possible to use SystemContextMenu without an EditableText.

I don't think there's a good way for the framework to know whether or not there is an active text input connection at that point (in SystemContextMenuController). Maybe should the ContextMenu.showSystemContextMenu call indicate an error in the case where it doesn't succeed in showing the menu?

I'm adding some docs to the framework PR about this.

I don't think there's a good way for the framework to know whether or not there is an active text input connection at that point (in SystemContextMenuController). Maybe should the ContextMenu.showSystemContextMenu call indicate an error in the case where it doesn't succeed in showing the menu?

I think I can add a warning on the engine side.

hellohuanlin · 2024-04-20T00:31:22Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformPlugin.mm

+    BOOL shownEditMenu = [textInputPlugin showEditMenu:args];
+    if (!shownEditMenu) {
+      FML_LOG(ERROR) << "Only text input supports system context menu for now. See "
+                        "https://github.com/flutter/flutter/issues/143033.";


@justinmc added this log if fails to show edit menu (e.g. when calling from non-text input).

Could you add this sentence if it's not too long?

"Ensure the system context menu is shown with an active text input connection."

That way it's more clear what someone needs to do to fix this, if it wasn't already obvious.

use targetRect API to auto determine the arrow direction parse global rects add unit tests add checks for copy/cut/delete use platform channel instead of text input channel, and use camel case add supportsShowingSystemContextMenu flag format add hide menu functionality add onDismiss callback use platform channel instead, and rename method fix conflict rename ContextMenu.onDismissSystemContextMenu add comment

hellohuanlin · 2024-04-22T19:11:16Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

 }

+- (BOOL)showEditMenu:(NSDictionary*)args API_AVAILABLE(ios(16.0)) {
+  if (!self.activeView.isFirstResponder) {


@justinmc this is where I determine the menu is triggered by text input widgets vs non-text input widgets.

I can also check _textInputClient != 0, but I have a feeling that there could be an edge case where _textInputClient is set, but it is not yet in editing state. So I feel checking the first responder is safer.

…147205) flutter/engine@62c9f17...f8e373d 2024-04-22 41930132+hellohuanlin@users.noreply.github.com [ios_edit_menu]add native edit menu (flutter/engine#50095) 2024-04-22 matanlurey@users.noreply.github.com Fail `run_impeller_golden_tests` if `LUCI_CONTEXT && !GOLDCTL` (flutter/engine#52300) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jsimmons@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Reverts: #143002 Initiated by: cbracken Reason for reverting: unresolved docs links. See failure here: https://ci.chromium.org/ui/p/flutter/builders/prod/Linux%20docs_test/16540/overview ``` dartdoc:stdout: Generating docs for package flutter... dartdoc:stderr: error: unresolved doc reference [TextInput.showSystemContextMenu] dartdoc:stderr: from widgets.MediaQueryData.supportsShowingSystemContextMenu: (file:///b/s/w/ir/x/w/flutter/packages/flutt Original PR Author: justinmc Reviewed By: {Renzo-Olivares, hellohuanlin} This change reverts the following previous change: In order to work around the fact that iOS 15 shows a notification on pressing Flutter's paste button (#103163), this PR allows showing the iOS system context menu in text fields. <img width="385" alt="Screenshot 2024-02-06 at 11 52 25 AM" src="https://github.com/flutter/flutter/assets/389558/d82e18ee-b8a3-4082-9225-cf47fa7f3674"> It is currently opt-in, which a user would typically do like this (also in example system_context_menu.0.dart): ```dart contextMenuBuilder: (BuildContext context, EditableTextState editableTextState) { // If supported, show the system context menu. if (SystemContextMenu.isSupported(context)) { return SystemContextMenu.editableText( editableTextState: editableTextState, ); } // Otherwise, show the flutter-rendered context menu for the current // platform. return AdaptiveTextSelectionToolbar.editableText( editableTextState: editableTextState, ); }, ``` Requires engine PR flutter/engine#50095. ## API changes ### SystemContextMenu A widget that shows the system context menu when built, and removes it when disposed. The main high-level way that most users would use this PR. Only works on later versions of iOS. ### SystemContextMenuController Used under the hood to hide and show a system context menu. There can only be one visible at a time. ### MediaQuery.supportsShowingSystemContextMenu Sent by the iOS embedder to tell the framework whether or not the platform supports showing the system context menu. That way the framework, or Flutter developers, can decide to show a different menu. ### `flutter/platform ContextMenu.showSystemContextMenu` Sent by the framework to show the menu at a given `targetRect`, which is the current selection rect. ### `flutter/platform ContextMenu.hideSystemContextMenu` Sent by the framework to hide the menu. Typically not needed, because the platform will hide the menu when the user taps outside of it and after the user presses a button, but it handles edge cases where the user programmatically rebuilds the context menu, for example. ### `flutter/platform System.onDismissSystemContextMenu` Sent by the iOS embedder to indicate that the system context menu has been hidden by the system, such as when the user taps outside of the menu. This is useful when there are multiple instances of SystemContextMenu, such as with multiple text fields.

matthew-carroll · 2024-05-26T08:32:34Z

@hellohuanlin @justinmc - Is there any place where I can see how to use this functionality? We're trying to integrate it into super_editor.

I checked the design doc, but I can't tell which aspects were actually chosen vs which aspects were rejected.

I checked the file changes in this PR, but it looks all engine-side.

I also noticed above that two weeks ago this work may have been reverted? If that's the case, what's the plan to get it updated and rolled back in?

hellohuanlin · 2024-05-28T16:46:50Z

@matthew-carroll the framework PR is here flutter/flutter#143002

justinmc · 2024-05-28T20:08:06Z

+1, ultimately it was relanded in flutter/flutter#148265. Let me know how it goes integrating into super_editor.

github-actions bot added the platform-ios label Jan 26, 2024

hellohuanlin commented Jan 26, 2024

View reviewed changes

justinmc reviewed Jan 26, 2024

View reviewed changes

hellohuanlin commented Jan 26, 2024

View reviewed changes

hellohuanlin force-pushed the secure_paste_native_edit_menu branch from 37af836 to 3ecc4d0 Compare January 30, 2024 20:37

hellohuanlin commented Jan 30, 2024

View reviewed changes

hellohuanlin requested a review from LongCatIsLooong January 31, 2024 22:44

hellohuanlin marked this pull request as ready for review January 31, 2024 22:45

hellohuanlin changed the title ~~[DRAFT][ios_edit_menu]add native edit menu~~ [ios_edit_menu]add native edit menu Jan 31, 2024

hellohuanlin requested a review from justinmc January 31, 2024 22:45

justinmc reviewed Feb 6, 2024

View reviewed changes

justinmc mentioned this pull request Feb 6, 2024

Native ios context menu flutter/flutter#143002

Merged

github-actions bot added the platform-web Code specifically for the web engine label Feb 7, 2024

justinmc reviewed Feb 22, 2024

View reviewed changes

hellohuanlin force-pushed the secure_paste_native_edit_menu branch from 981a814 to ab2b8cf Compare April 11, 2024 17:33

justinmc approved these changes Apr 12, 2024

View reviewed changes

hellohuanlin commented Apr 17, 2024

View reviewed changes

hellohuanlin force-pushed the secure_paste_native_edit_menu branch from 05a8270 to b3f9bdc Compare April 19, 2024 20:48

hellohuanlin commented Apr 20, 2024

View reviewed changes

hellohuanlin added 3 commits April 22, 2024 11:33

add warning if it's not from text-input

43ef18f

update wording

c73c2c7

hellohuanlin force-pushed the secure_paste_native_edit_menu branch from 55d7364 to c73c2c7 Compare April 22, 2024 18:49

hellohuanlin commented Apr 22, 2024

View reviewed changes

hellohuanlin added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 22, 2024

auto-submit bot merged commit f8e373d into flutter:main Apr 22, 2024

engine-flutter-autoroll mentioned this pull request Apr 23, 2024

Roll Flutter Engine from 62c9f17169cf to f8e373da5227 (2 revisions) flutter/flutter#147205

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 23, 2024

f8e373da5 [ios_edit_menu]add native edit menu (flutter/engine#50095)

00aef3c

auto-submit bot mentioned this pull request May 13, 2024

Reverts "Native ios context menu (#143002)" flutter/flutter#148237

Merged

jmagman mentioned this pull request Nov 21, 2024

iOS "Scribble" feature: assertion error when using SystemContextMenu flutter/flutter#159259

Closed

[ios_edit_menu]add native edit menu #50095

[ios_edit_menu]add native edit menu #50095

Uh oh!

Conversation

hellohuanlin commented Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

hellohuanlin Jan 26, 2024 • 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!

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!

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!

hellohuanlin Jan 29, 2024 • 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!

hellohuanlin Jan 30, 2024 • 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!

hellohuanlin Jan 30, 2024 • 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!

hellohuanlin commented Feb 5, 2024

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!

hellohuanlin commented Feb 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hellohuanlin commented Feb 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Implications of being tied to the text input connection

When to enable this feature

Only other problem I'm seeing right now

Uh oh!

justinmc commented Feb 8, 2024

hellohuanlin commented Jan 26, 2024 •

edited

Loading

hellohuanlin Jan 26, 2024 •

edited

Loading

hellohuanlin Jan 29, 2024 •

edited

Loading

hellohuanlin Jan 30, 2024 •

edited

Loading

hellohuanlin Jan 30, 2024 •

edited

Loading

hellohuanlin commented Feb 6, 2024 •

edited

Loading

hellohuanlin commented Feb 6, 2024 •

edited

Loading

hellohuanlin commented Feb 12, 2024 •

edited

Loading

justinmc left a comment •

edited

Loading

hellohuanlin Feb 22, 2024 •

edited

Loading