engine icon indicating copy to clipboard operation
engine copied to clipboard

[ios_edit_menu]add native edit menu

Open hellohuanlin opened this issue 1 year ago • 18 comments

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.

https://github.com/flutter/flutter/issues/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

  • [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
  • [x] I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • [x] I listed at least one issue that this PR fixes in the description above.
  • [x] 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.
  • [x] I updated/added relevant documentation (doc comments with ///).
  • [x] I signed the CLA.
  • [x] All existing and new tests are passing.

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

hellohuanlin avatar Jan 26 '24 20:01 hellohuanlin

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

hellohuanlin avatar Feb 05 '24 19:02 hellohuanlin

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 (https://github.com/flutter/engine/blob/307471f972f4993055e531013a635f916762234c/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm#L1169-L1180). We will need something like:

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

Let me quickly try it out.

hellohuanlin avatar Feb 06 '24 01:02 hellohuanlin

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:

  1. 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.

  2. 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 _).

hellohuanlin avatar Feb 06 '24 01:02 hellohuanlin

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.

justinmc avatar Feb 08 '24 00:02 justinmc

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.

hellohuanlin avatar Feb 08 '24 19:02 hellohuanlin

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.

justinmc avatar Feb 08 '24 23:02 justinmc

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.

hellohuanlin avatar Feb 12 '24 19:02 hellohuanlin

@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?

justinmc avatar Feb 23 '24 00:02 justinmc

@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: https://github.com/flutter/flutter/issues/143999

hellohuanlin avatar Feb 23 '24 04:02 hellohuanlin

Thanks for doing that!

justinmc avatar Feb 23 '24 19:02 justinmc

@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.

justinmc avatar Mar 04 '24 20:03 justinmc

@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.

hellohuanlin avatar Mar 05 '24 04:03 hellohuanlin

@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.

justinmc avatar Mar 05 '24 19:03 justinmc

@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]);

hellohuanlin avatar Mar 05 '24 22:03 hellohuanlin

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

justinmc avatar Mar 06 '24 00:03 justinmc

@hellohuanlin I'm starting to feel good about this based on my work over in https://github.com/flutter/flutter/pull/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?

justinmc avatar Mar 07 '24 21:03 justinmc

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 avatar Mar 07 '24 23:03 hellohuanlin

@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.

hellohuanlin avatar Mar 08 '24 00:03 hellohuanlin

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

justinmc avatar Apr 11 '24 17:04 justinmc

@justinmc welcome back! Updated the branch.

hellohuanlin avatar Apr 11 '24 18:04 hellohuanlin

@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?

matthew-carroll avatar May 26 '24 08:05 matthew-carroll

@matthew-carroll the framework PR is here https://github.com/flutter/flutter/pull/143002

hellohuanlin avatar May 28 '24 16:05 hellohuanlin

+1, ultimately it was relanded in https://github.com/flutter/flutter/pull/148265. Let me know how it goes integrating into super_editor.

justinmc avatar May 28 '24 20:05 justinmc