-
Notifications
You must be signed in to change notification settings - Fork 29.7k
[iOS][Secure Paste] Custom edit menu actions #171825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| - (void)flutterTextInputView:(FlutterTextInputView*)textInputView | ||
| performCustomAction:(NSString*)customAction | ||
| withClient:(int)client { | ||
| [self.textInputChannel invokeMethod:@"TextInputClient.performCustomAction" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you use platformChannel and ContextMenu prefix? (similar to willDismissEditMenuWithTextInputClient method in this file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reviewing! I've implemented all your suggestions, but I have a question about the method naming: The current method name is quite long. Would you prefer to keep it as is for clarity, or should we consider simplifying it? For example: performContextMenuCustomAction?
| performCustomAction:(NSString*)customAction | ||
| withClient:(int)client { | ||
| [self.textInputChannel invokeMethod:@"TextInputClient.performCustomAction" | ||
| arguments:@[ @(client), customAction ]]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the customAction the ID? if so you should call it customActionID
| if (customId && title) { | ||
| UIAction* action = [UIAction actionWithTitle:title | ||
| image:nil | ||
| identifier:customId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to set identifier.
| image:nil | ||
| identifier:customId | ||
| handler:^(__kindof UIAction* _Nonnull action) { | ||
| [self.textInputDelegate flutterTextInputView:self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to use weak self
|
The engine part is heading the right direction. Good work! |
a8fc6d0 to
1237713
Compare
justinmc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this approach looks solid! Most of my comments are just about how we organize the code.
I left one comment about adding an example to the examples directory. I think that will help us to visualize how this will work better, and it will allow you to remove your debug code for the vibrate button.
I also left a comment about the callback map not being cleared when the system hides the menu. I'll keep an eye out for other sources of leaks. We'll have to make sure that we cover use cases like showing one SystemContextMenu widget, then immediately showing another. I think hide might get called in that case so we'll be covered, but we should make sure.
| if (callback != null) { | ||
| callback(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also assert that the callback is not null so that developers can be aware that something is wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is testable or not, but it would be nice if we can test it.
| if (actionId == 'vibrate') { | ||
| debugPrint('[HARDCODED] Vibrate action triggered'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you write an example in the examples/api/lib directory that does something like this at the user level? Maybe a more complex version of this one: https://github.com/flutter/flutter/blob/master/examples/api/lib/widgets/system_context_menu/system_context_menu.0.dart
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe put your example on the items parameter of SystemContextMenu:
| final List<IOSSystemContextMenuItem> items; |
| firstArg['action'] as String, | ||
| firstArg['data'] == null ? <String, dynamic>{} : firstArg['data'] as Map<String, dynamic>, | ||
| ); | ||
| case 'TextInputClient.performCustomAction': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should go in services/binding.dart, where we handle onDismissSystemContextMenu.
| static SystemContextMenuController? _lastShown; | ||
|
|
||
| /// Map to store custom action callbacks, keyed by their unique identifiers. | ||
| static final Map<String, VoidCallback> _customActionCallbacks = <String, VoidCallback>{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the method channel call were handled in binding.dart, then you could handle this stuff via SystemContextMenuClient and SystemContextMenuController.
| } | ||
| _lastShown = null; | ||
| ServicesBinding.systemContextMenuClient = null; | ||
| _clearCustomActions(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should also get cleared when the platform hides the context menu (ContextMenu.onDismissSystemContextMenu).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how the platform hides it:
| case 'ContextMenu.onDismissSystemContextMenu': |
| @override | ||
| IOSSystemContextMenuItemData getData(WidgetsLocalizations localizations) { | ||
| final String callbackId = '${DateTime.now().millisecondsSinceEpoch}_${title.hashCode}'; | ||
| SystemContextMenuController.registerCustomAction(callbackId, onPressed); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a tricky side effect that developers probably won't expect when calling getData. See https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#getters-feel-faster-than-methods
Could you register these when the system context menu is shown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should hopefully sort itself out when you move the action map to SystemContextMenuController, as mentioned above. So I would do that first before trying to address this comment.
|
|
||
| @override | ||
| IOSSystemContextMenuItemData getData(WidgetsLocalizations localizations) { | ||
| final String callbackId = '${DateTime.now().millisecondsSinceEpoch}_${title.hashCode}'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just hashCode is probably a good key for this. The only possible collision would be for the exact same item with the exact same onPressed, say if you passed the same item into the items List twice. Even in that case, it would be fine to have only one entry in the _customActionCallbacks map and to call the same one for both.
Also IOSSystemContextMenuItemCustom is const, so this is equivalent to passing the same instance:
items: <IOSSystemContextMenuItem>[
IOSSystemContextMenuItemCustom(title: 'press me', onPressed: _myOnPressed),
IOSSystemContextMenuItemCustom(title: 'press me', onPressed: _myOnPressed),
],But even in that case, it's fine to have only one entry in _customActionCallbacks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Justin, I have made all the changes based on your comments and we discussed last time:
- Moved callbacks from static to instance-based
- Now routes through binding.dart
- getData() now only returns data, side-effect free
- Added proper cleanup when platform dismisses the menu
- Removed the hardcoded testa
- Added the tests and a working example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking forward to your feedback!
2d84e34 to
305c513
Compare
justinmc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good, thanks for doing that refactor. I think putting the callback map inside of SystemContextMenuController is much better now. Also the example is great.
| _controller.text = text.replaceRange( | ||
| selection.start, | ||
| selection.end, | ||
| '❤️', | ||
| ); | ||
| _controller.selection = TextSelection.collapsed( | ||
| offset: selection.start + 2, | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I would do this as a single update by setting _controller.value.
| expect(controller.text, ''); | ||
| expect(find.text('Text cleared'), findsOneWidget); | ||
| }, | ||
| skip: defaultTargetPlatform != TargetPlatform.iOS || kIsWeb, // [intended] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than skipping for the platform, use variant and TargetPlatformVariant.only. Skipping for kIsWeb is ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great example, thanks for adding it.
|
|
||
| ServicesBinding.systemContextMenuClient = this; | ||
|
|
||
| _customActionCallbacks.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you also might need to call this in handleSystemHide. Then I think we will be covered and this shouldn't leak.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also test this: Show the SystemContextMenu with a certain item, then show again with a new item. Then try sending a handleCustomContextMenuAction for the original item.
|
|
||
| for (final IOSSystemContextMenuItemData item in items) { | ||
| if (item is IOSSystemContextMenuItemDataCustom) { | ||
| _customActionCallbacks[item.callbackId] = item.onPressed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think, would it be worth it to assert that there are no duplicate item.callbackIds? I guess it's not a problem for us if there are, and maybe users have a real reason to do something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe assert if there are duplicate callbackIds AND the callbacks are not equivalent? That seems like it would very likely be a mistake on the part of the app developer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making a note that we talked this over offline and seemed to agree on the comment above this one.
| 'callbackId': callbackId, | ||
| 'title': title, | ||
| 'type': _jsonType, | ||
| 'id': callbackId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need both an id and a callbackId?
| return other is IOSSystemContextMenuItemDataCustom && | ||
| other.title == title && | ||
| other.callbackId == callbackId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onPressed should probably also be included here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also should include onPressed in hashCode (discussed offline).
| IOSSystemContextMenuItemData getData(WidgetsLocalizations localizations) { | ||
| return IOSSystemContextMenuItemDataCustom( | ||
| title: title, | ||
| callbackId: hashCode.toString(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of passing the callbackId in here, could IOSSystemContextMenuItemData generate it itself?
| final IOSSystemContextMenuItemCustom customItem1 = items[1] as IOSSystemContextMenuItemCustom; | ||
| final IOSSystemContextMenuItemCustom customItem2 = items[2] as IOSSystemContextMenuItemCustom; | ||
|
|
||
| controller2.handleCustomContextMenuAction(customItem1.hashCode.toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also test that a call from onPerformCustomAction works? You can simulate the platform message using TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger.handlePlatformMessage.
No need to use handlePlatformMessage every time, it's also fine to use handleCustomContextMenuAction directly like you are here. Just make sure that the onPerformCustomAction gets tested at least once too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also try the error state: send that message when there is no _systemContextMenuClient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the platform channel test as requested. Note: I had to use handlePlatformMessage for all tests (not just one) because SystemContextMenu doesn't expose its controller, it's created internally in
_SystemContextMenuState and not accessible from tests. Is this the correct approach, or did you have a different testing strategy in mind?
| /// {@tool dartpad} | ||
| /// This example shows how to add custom menu items to the iOS system context menu. | ||
| /// | ||
| /// ** See code in examples/api/lib/widgets/system_context_menu/system_context_menu.1.dart ** | ||
| /// {@end-tool} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it would be valuable to also add this example to SystemContextMenu.items, or is it too much?
| expect(customAction1Called, isTrue); | ||
| expect(customAction2Called, isFalse); | ||
|
|
||
| controller2.handleCustomContextMenuAction(customItem2.hashCode.toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another test idea: What happens when you call handleCustomContextMenuAction after the menu has been hidden?
What happens when it's called with a callbackId that doesn't exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably put those tests in system_context_menu_controller_test.dart.
|
Justin, Thanks for the meeting! I've made all the changes:
Looking forward to your feedback! |
justinmc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a question about whether the menu auto-closes when a button is pressed or not that came up in a few comments below.
| await tester.pumpAndSettle(); | ||
|
|
||
| if (defaultTargetPlatform == TargetPlatform.iOS) { | ||
| expect(find.byType(SystemContextMenu), findsOneWidget); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Can you also check: expect(find.byType(AdaptiveTextSelectionToolbar), findsNothing);?
| if (_systemContextMenuClient == null) { | ||
| assert( | ||
| false, | ||
| 'Platform sent onPerformCustomAction when no SystemContextMenuClient was registered.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Maybe add some actionable advice here like this? "ServicesBinding.systemContextMenuCilent shouldn't be cleared unless the menu is hidden."
I'm not sure how helpful that is for app developers, but this assert is such a weird case, not sure if there's a better option. Up to you if you want to include something like this or leave it as-is.
| // https://github.com/flutter/flutter/issues/103163 | ||
| /// An [IOSSystemContextMenuItemData] for custom action buttons defined by the developer. | ||
| /// | ||
| /// Must specify a [title] and [callbackId]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should say title and onPressed.
| final VoidCallback onPressed; | ||
|
|
||
| /// The unique identifier for this custom action. | ||
| String get callbackId => hashCode.toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: It would be possible to cache this to avoid recomputing it each time. Do you think it's worth it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, I think hashCode.toString() is basically free, and each instance's hashCode is already stable throughout its lifetime, not worth the extra code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on board with that 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Did you think about making callbackId an int instead of a String? I think a String makes sense to me, just wanted to make sure you thought through it.
| final String actionId = args[1] as String; | ||
| _systemContextMenuClient!.handleCustomContextMenuAction(actionId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I would call this callbackId here and in the parameter name of handleCustomContextMenuAction. The reason is just to keep the name always the same, because callbackId is used elsewhere in this PR.
| await tester.pumpAndSettle(); | ||
|
|
||
| expect(controller.text, ''); | ||
| expect(find.text('Text cleared'), findsOneWidget); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also check whether the SystemContextMenu is still visible or not? I assume the answer is no, it hides itself? If not the we should show how to do that in the example, but I think it does hide.
| (_) {}, | ||
| ); | ||
|
|
||
| expect(customActionCalled, isTrue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also expect SystemContextMenu is hidden here (assuming that's true).
| ); | ||
| expect(customAction2Called, isTrue); | ||
|
|
||
| state.hideToolbar(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have to manually call hide or does it automatically hide?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested on my iPhone - it does auto-close. But I think in tests maybe we need to fake it with hideToolbar().
| testWidgets( | ||
| 'can trigger custom menu action through platform channel message', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this test cover any code paths that the previous test does not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not you could consider deleting it, but it's up to you if this is valuable or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept it finally, I think tests mixed items scenario (Cut + custom), which is different from the pure custom test.
| }, | ||
| skip: kIsWeb, // [intended] | ||
| variant: TargetPlatformVariant.only(TargetPlatform.iOS), | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more test I think we should add for completeness: Pump an app with two TextFields, each with their own custom SystemContextMenu item. Open the context menu in one field, expect that only its custom button was shown, tap it, expect that its callback was called. Then do the same for the other field.
|
Justin, Thanks for the meeting! I've made all the changes:
Note: Device testing confirmed iOS menus do auto-close after custom actions. Tests simulate this with Looking forward to your feedback! |
justinmc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with nits 👍 . Great work!
| final VoidCallback onPressed; | ||
|
|
||
| /// The unique identifier for this custom action. | ||
| String get callbackId => hashCode.toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on board with that 👍
| // iOS system menus auto-close after custom actions on real devices, | ||
| // but in tests we need to manually call hideToolbar() to simulate this. | ||
| state.hideToolbar(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of calling hideToolbar would it be possible to send the ContextMenu.onDismissSystemContextMenu platform channel message in order to make this more realistic?
| expect(field2ActionCalled, isTrue); | ||
|
|
||
| state2.hideToolbar(); | ||
| await tester.pump(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Maybe do a final expect that SystemContextMenu is not found. Ending on a pump makes me wonder what was being tested there.
| final VoidCallback onPressed; | ||
|
|
||
| /// The unique identifier for this custom action. | ||
| String get callbackId => hashCode.toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Did you think about making callbackId an int instead of a String? I think a String makes sense to me, just wanted to make sure you thought through it.
| suggestedMenu:suggestedMenu]; | ||
| } | ||
| } else if ([type isEqualToString:@"custom"]) { | ||
| NSString* customId = encodedItem[@"id"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Have you thought about naming this parameter callbackId just so it has the same name as it does in the framework? I'm not sure which name is better, just wanted to call it out.
Add custom type support to systemContextMenuItemDataFromJson with fake callback
b35d962 to
8fb0e6d
Compare
This PR implements support for custom action items in the native edit menu on iOS, with changes to both framework and engine. This PR will be updated incrementally until the full feature is complete.
Phase 1: Add hardcoded custom menu item for iOS edit menu.
Phase 2: Add Framework API for custom iOS context menu items.
Phase 3: Add Optimization, Testing, and Documentation.
Part of #103163
Part of #140184
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
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.