[ios_edit_menu]add native edit menu
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.
hi @justinmc this is ready for review. could you take a look when you get time? Thanks!
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.
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
suggestedActionsinFlutterTextInputView::menuForConfigurationmethod. ThesuggestedActionsis controlled byFlutterTextInputView::canPerformActionmethod. For example, if we return YES forFlutterTextInputView::copy:action and return NO for all other actions, thensuggestedActionswill only contain thecopyitem. Then after user tap on the button, theFlutterTextInputView::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::menuForConfigurationmethod. Note that we have to do it for default actions like "lookup", because it's a private API (prefixed by_).
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.
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.
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.
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 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 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
Thanks for doing that!
@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 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 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 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]);
Thank you for the quick turnaround! I'll try this out.
@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?
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.
@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 Can you fix the conflict here? I'm coming back to secure paste after finishing some other work finally.
@justinmc welcome back! Updated the branch.
@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 the framework PR is here https://github.com/flutter/flutter/pull/143002
+1, ultimately it was relanded in https://github.com/flutter/flutter/pull/148265. Let me know how it goes integrating into super_editor.