[ios][secure_paste]Show context items based on items sent from framework #56362

hellohuanlin · 2024-11-04T22:03:07Z

This PR shows the context menu based on what's received from the framework via method channel. There are 2 cases:

1. For basic editing actions (e.g. copy/paste):

We perform a DFS search inside suggestedActions, so that we don't have to specify things like keyboard shortcut.

2. For additional actions (e.g. look up, share, search web):

We manually create UICommand because (1) some actions are not provided by suggestedActions (2) these are simply UICommand (rather than UIKeyCommand), so we just need title and action (3) the actions are private APIs. However this would require framework to send the item title.

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

Second milestone for 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-11-04T22:08:17Z

@justinmc This PR is not complete, but it should be good enough to start the framework side on top of it.

Example usage:

SystemChannels.platform.invokeMethod<void>(
      'ContextMenu.showSystemContextMenu',
      <String, dynamic>{
        'targetRect': <String, dynamic>{
          'x': 100,
          'y': 100,
          'width': 100,
          'height': 100,
        },
        'items': <dynamic>[
          <String, dynamic>{
            'type': 'default',
            'action': 'paste',
          },
          <String, dynamic>{
            'type': 'default',
            'action': 'cut',
          },
          <String, dynamic>{
            'type': 'default',
            'action': 'copy',
          },
          <String, dynamic>{
            'type': 'default',
            'action': 'selectAll',
          },
          <String, dynamic>{
            'type': 'default',
            'title': 'Look Up',
            'action': 'lookUp',
          },
          <String, dynamic>{
            'type': 'default',
            'title': "Search Web",
            'action': 'searchWeb',
          },
          <String, dynamic>{
            'type': 'default',
            'title': "Share",
            'action': 'share',
          },
        ],
      }
    );

Also notice that I added title field for the additional actions. You can decide either add title to all actions for consistency, or just the additional actions.

hellohuanlin · 2024-11-08T19:13:54Z

@justinmc just added implementation for additional items (look up, search web, etc), and can confirm they worked.

justinmc · 2024-11-09T00:00:47Z

Cool I'm excited about this! I'll be OOO on Monday but I'll start playing around with this on Tuesday.

justinmc · 2024-11-14T00:32:08Z

Still planning to compile this and try talking to it from the framework by the end of the week.

justinmc

Ok I was able to get this to compile and play around with it from the framework, and it works great. I still need to write some real framework code to interact with this and double check that it's what we want, but everything seems good.

One thought: Should there be some error checking to make sure that the items that are being passed are well-formatted? For example, if I pass this, it silently just doesn't show the button:

<String, dynamic>{
  'type': 'default',
  'action': 'share',
},

justinmc · 2024-11-16T00:18:43Z

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

+// DFS algorithm to search a UICommand from the menu tree.
+- (UICommand*)searchCommandWithSelector:(SEL)selector
+                                element:(UIMenuElement*)element API_AVAILABLE(ios(16.0)) {


Is this searching through the default menu (suggestedActions) trying to find the item given in the call to showSystemContextMenu? Won't all of the ones that we currently support never be nested? Or maybe I just don't understand how suggestedActions is structured.

Actually all of them are nested at least 1 level deep in my observation - Apple uses UIMenuOptionsDisplayInline to make the items appear on the top level. And since this is not an API contract, we have to do a tree search here.

Got it, thanks for the explanation!

hellohuanlin · 2024-11-16T02:07:59Z

@justinmc Added the check for missing title, for example:

Missing title for context menu item action "share".

justinmc · 2024-11-18T18:49:59Z

Ok I tried sending a mal-formatted item and indeed I got the error message 👍 . So with this approach, it will log an error but the call to invokeMethod will complete successfully and the menu will be shown minus the malformatted item. Is that the right approach or should invokeMethod fail? With the way it is now, would a Flutter app developer be able to be notified that this error had occurred on a user device?

hellohuanlin · 2024-11-18T18:57:43Z

Is that the right approach or should invokeMethod fail? With the way it is now, would a Flutter app developer be able to be notified that this error had occurred on a user device?

My understanding is that flutter app developers won't be able to send invalid format from framework to engine, because the menu created using our dart API will be correctly translated into the right format in the framework.

The error message is there just so that the framework contributors (i.e. you and future others) know what's going on during development phase, but the error shouldn't happen to flutter app developers.

justinmc · 2024-11-19T21:33:52Z

A Flutter app developer can always send whatever crazy json they want over the platform channels if they really want to:

    SystemChannels.platform.invokeMethod<Map<String, dynamic>>(
      'ContextMenu.showSystemContextMenu',
      <String, dynamic>{
        'targetRect': myTargetRect,
        'items': myCrazyItems,
        'something else that doesnt make sense': 42,
    );

But you're right that normally a user would never mess with this low level API, and I'm writing a higher-level API that should make a mal-formatted request impossible at that level.

I guess this is kind of splitting hairs. Either way is fine. In the long term hopefully we will be using FFI with compile time checking of these calls.

justinmc · 2024-11-19T21:37:41Z

What do you think about changing the name of the "default" type? It's a keyword in Dart so I have to work around it:

enum SystemContextMenuType {
  custom,
  defaultType; // This can't be `default`, so I'm using `defaultType`.

  String get json => switch (this) {
    SystemContextMenuType.defaultType => 'default',
    _ => name,
  };
}

But if we renamed it then it could be simpler:

enum SystemContextMenuType {
  custom,
  builtIn; // Or some better name... `platform`? `platformDefault`?
}

hellohuanlin · 2024-11-19T23:29:11Z

What do you think about changing the name of the "default" type? It's a keyword in Dart so I have to work around it:

I'm ok with either. How about let's do builtIn? Because with defaultType we would have to rename custom to customType as well to make them consistent.

hellohuanlin · 2024-11-19T23:40:54Z

Updated from default to builtIn

justinmc · 2024-11-21T00:17:29Z

Thank you! I like builtIn.

justinmc · 2024-12-02T19:37:16Z

@hellohuanlin If I pass a title for cut, copy, paste, or select all, it silently ignores the given title and uses the default title ("Cut" etc.). Is that intended/required? I guess it will use the locale of the user's device to get the string in the correct language?

I guess this is just a result of the two different types of actions that you mention in the PR description, but I want to make sure it's clear to app developers.

hellohuanlin · 2024-12-02T19:49:05Z

@hellohuanlin If I pass a title for cut, copy, paste, or select all, it silently ignores the given title and uses the default title ("Cut" etc.). Is that intended/required? I guess it will use the locale of the user's device to get the string in the correct language?

Yes it's intended that I use the title from the system itself, and it is localized.

I can also used the title passed from framework (except for the "paste" item which i cannot change), if you think it's necessary?

I guess this is just a result of the two different types of actions that you mention in the PR description, but I want to make sure it's clear to app developers.

To app developers, do they have to pass in the title? or do they simply pass the ContextMenuButtonType?

justinmc · 2024-12-02T22:26:59Z

Thanks for the explanation. I guess let's keep it as-is. I'll enforce on the framework side that no title can be given for cut/copy/paste/selectall, and only the type is needed.

justinmc · 2024-12-10T00:39:11Z

@hellohuanlin Could we combine the type and action fields, or is there a reason that they're separate? Like we could only have one field that includes all action values plus the value custom (there would be no builtIn value). We would still be able to distinguish everything that we can now (at least as far as the framework side is concerned).

I mention this because I noticed that in the framework I can just infer type from action without needing to store both. This is similar to the existing ContextMenuButtonItemType values.

hellohuanlin · 2024-12-10T00:47:38Z

@justinmc no specific reason. I can do that if you like?

(deleted internal link)

hellohuanlin · 2024-12-10T01:27:23Z

@justinmc The new design is indeed simpler! i've just updated the PR.

justinmc · 2024-12-10T21:07:59Z

Thank you! I've updated my framework PR to work with it and it looks good.

justinmc · 2024-12-10T23:11:46Z

Are these rules correct for all of the types that we have now?

copy, cut, paste, selectAll
- Must not provide title or onPressed.
share, searchWeb, lookUp
- Must provide title, must not provide onPressed.
custom
- Must provide title and onPressed.

Edit: Updated my understanding of share, searchWeb, lookUp.

hellohuanlin · 2024-12-10T23:40:35Z

Are these rules correct for all of the types that we have now?

copy, cut, paste, selectAll

Must not provide title or onPressed.

share, searchWeb, lookUp

Must provide title, must not provide onPressed.

custom

Must provide title and onPressed.

Edit: Updated my understanding of share, searchWeb, lookUp.

By "not providing title", do you mean framework not providing title to the engine? or do you mean app developers not providing title to the framework?

If you are talking about framework <-> engine communication, yes you are right.

justinmc · 2024-12-11T18:52:54Z

Great, thanks. Yes, this was about framework/engine communication. In the SystemContextMenu widget I will populate titles with default localized strings where I can, so the requirements are slightly different.

justinmc · 2025-01-02T23:44:40Z

@hellohuanlin Give the repo merge, I think these changes now need to happen in flutter/flutter right? Any idea on the best way to do that? Maybe you could apply the diff to my branch and then push a commit to my branch.

hellohuanlin · 2025-01-03T01:21:13Z

@hellohuanlin Give the repo merge, I think these changes now need to happen in flutter/flutter right? Any idea on the best way to do that? Maybe you could apply the diff to my branch and then push a commit to my branch.

It seems there are a few options:

As you said, push to your branch. Though in this setup I would be approving my own code during PR review, which is a bit strange :)
I push the engine change to the flutter/flutter first, then you rebase on top of it.
Keep working on the current setup, then pick either (1) or (2) when we are done.

I am leaning towards either (2) or (3 then 2). No strong opinion against (1) though. Let me know.

justinmc · 2025-01-03T17:21:18Z

Oh right I guess I can still build this branch from my separate local engine repo checkout in order to do local development, so let's go with 3 for now.

For 2, do you mean that you would open a separate PR in flutter/flutter?

hellohuanlin · 2025-01-03T17:34:50Z

Oh right I guess I can still build this branch from my separate local engine repo checkout in order to do local development, so let's go with 3 for now.

Sounds good.

For 2, do you mean that you would open a separate PR in flutter/flutter?

Yep.

justinmc · 2025-01-03T18:33:25Z

Could you actually update this branch with main? It seems to be incompatible with flutter/flutter's master branch.

hellohuanlin · 2025-01-03T18:46:12Z

Could you actually update this branch with main? It seems to be incompatible with flutter/flutter's master branch.

I'm not surprised that it's incompatible with the latest master. Do you have an old branch on flutter/flutter? Or if you want I can also put up a PR on flutter/flutter.

justinmc · 2025-01-03T19:01:59Z

I think I need you to open a PR on flutter/flutter. I tried updating this branch locally and it still wasn't able to run after rebuilding the engine. My flutter/flutter branch is up to date as of yesterday. Thank you!

hellohuanlin · 2025-01-03T19:29:09Z

I think I need you to open a PR on flutter/flutter. I tried updating this branch locally and it still wasn't able to run after rebuilding the engine. My flutter/flutter branch is up to date as of yesterday. Thank you!

Sounds good.

justinmc · 2025-01-06T21:57:06Z

I think this PR can be closed in favor of flutter/flutter#161103.

I was able to get these changes working in the monorepo by merging that branch with my PR flutter/flutter#159013. I had some struggles building the engine in the monorepo but I updated the docs in flutter/flutter#161184 with what I found to work.

jmagman · 2025-01-07T06:21:26Z

I think this PR can be closed in favor of flutter/flutter#161103.

Thank you @justinmc and @hellohuanlin! Closing...

github-actions bot added the platform-ios label Nov 4, 2024

[ios]allow reordering and more items in native context menu

bb224ce

hellohuanlin force-pushed the secure_paste_m2 branch from 2593238 to bb224ce Compare November 4, 2024 22:04

hellohuanlin requested a review from justinmc November 8, 2024 19:14

some additional actions

edb9a0b

hellohuanlin force-pushed the secure_paste_m2 branch from 86350c4 to edb9a0b Compare November 9, 2024 00:44

hellohuanlin added 2 commits November 8, 2024 16:45

WIP some tests, need more testing for DFS

37a3e12

format

955ed3d

hellohuanlin changed the title ~~[ios][secure_paste]Show context items based on framework's data~~ [ios][secure_paste]Show context items based on items sent from framework Nov 10, 2024

more tests for DFS

6821d62

justinmc reviewed Nov 16, 2024

View reviewed changes

justinmc mentioned this pull request Nov 16, 2024

Secure paste milestone 2 flutter/flutter#159013

Merged

print out error message if no item title provided

ba7d03a

justinmc mentioned this pull request Nov 18, 2024

Flutter should support iOS 15's Secure Paste feature flutter/flutter#103163

Closed

rename default to builtIn

812a5e2

simplified api with combined type and action

e8bbf97

hellohuanlin mentioned this pull request Jan 3, 2025

[ios][secure_paste]show menu item based on info sent from framework flutter/flutter#161103

Merged

9 tasks

jmagman closed this Jan 7, 2025

[ios][secure_paste]Show context items based on items sent from framework #56362

[ios][secure_paste]Show context items based on items sent from framework #56362

Uh oh!

Conversation

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

Uh oh!

Pre-launch Checklist

Uh oh!

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

Uh oh!

Uh oh!

hellohuanlin commented Nov 8, 2024

Uh oh!

justinmc commented Nov 9, 2024

Uh oh!

justinmc commented Nov 14, 2024

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc Nov 16, 2024

Choose a reason for hiding this comment

Uh oh!

hellohuanlin Nov 16, 2024

Choose a reason for hiding this comment

Uh oh!

justinmc Nov 18, 2024

Choose a reason for hiding this comment

Uh oh!

hellohuanlin commented Nov 16, 2024

Uh oh!

justinmc commented Nov 18, 2024

Uh oh!

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

Uh oh!

Uh oh!

justinmc commented Nov 19, 2024

Uh oh!

justinmc commented Nov 19, 2024

Uh oh!

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

Uh oh!

Uh oh!

hellohuanlin commented Nov 19, 2024

Uh oh!

justinmc commented Nov 21, 2024

Uh oh!

justinmc commented Dec 2, 2024

Uh oh!

hellohuanlin commented Dec 2, 2024

Uh oh!

justinmc commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmc commented Dec 10, 2024

Uh oh!

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

Uh oh!

Uh oh!

hellohuanlin commented Dec 10, 2024

Uh oh!

justinmc commented Dec 10, 2024

Uh oh!

justinmc commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

justinmc commented Dec 11, 2024

Uh oh!

justinmc commented Jan 2, 2025

Uh oh!

hellohuanlin commented Jan 3, 2025

Uh oh!

justinmc commented Jan 3, 2025

Uh oh!

hellohuanlin commented Jan 3, 2025

Uh oh!

justinmc commented Jan 3, 2025

hellohuanlin commented Nov 4, 2024 •

edited

Loading

hellohuanlin commented Nov 4, 2024 •

edited

Loading

hellohuanlin commented Nov 18, 2024 •

edited

Loading

hellohuanlin commented Nov 19, 2024 •

edited

Loading

justinmc commented Dec 2, 2024 •

edited

Loading

hellohuanlin commented Dec 10, 2024 •

edited

Loading

justinmc commented Dec 10, 2024 •

edited

Loading

hellohuanlin commented Dec 10, 2024 •

edited

Loading