Add image keyboard support on Android #27763

tneotia · 2021-07-28T15:15:19Z

This PR adds support for image keyboard support on Android, utilizing the commitContent function.

Partially addresses flutter/flutter#20796. Associated Flutter PR: flutter/flutter#87203.

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 Hixie said the PR is test-exempt.
I updated/added relevant documentation (doc comments with ///).
I signed the [CLA].
All existing and new tests are passing.

~~I'm not super sure how I would be able to write a test for commitContent, if anyone has any input that would be much appreciated.~~

flutter-dashboard · 2021-07-28T15:15:21Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

google-cla · 2021-07-28T15:15:52Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

tneotia · 2021-07-28T16:00:41Z

@googlebot I fixed it

google-cla · 2021-07-28T16:01:10Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

gspencergoog · 2021-07-28T16:10:02Z

@tneotia, I think you need to rebase so that only your changes appear in the diffs from the master branch. The CLA bot thinks that all of the refs you have merged are part of the PR, even the ones you didn't author.

google-cla · 2021-07-28T16:26:16Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

zlshames · 2021-07-28T16:28:32Z

@googlebot I consent.

tneotia · 2021-07-28T16:28:52Z

@tneotia, I think you need to rebase so that only your changes appear in the diffs from the master branch. The CLA bot thinks that all of the refs you have merged are part of the PR, even the ones you didn't author.

I see that... Not super well-versed in git and I can't figure out how to do this. I ended up adding commits rather than removing :( What I tried is rebasing the engine/flutter/src directory onto master but it didn't seem to work.

google-cla · 2021-07-28T16:29:02Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

gspencergoog · 2021-07-28T16:35:34Z

The way I do this is to git fetch upstream followed by (from your work branch) git rebase upstream/master, and then resolve and conflicts and git push again.

If that doesn't work, you can also try squashing the commits down to one commit with (from your work branch) git reset --soft master followed by git commit -m "commit message".

gspencergoog · 2021-07-28T16:38:02Z

And be sure to be doing this from the engine/src/flutter directory, not the one above it (which has its own git repository), in your gif-insertion-pr branch.

google-cla · 2021-07-28T17:02:41Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

tneotia · 2021-07-28T17:25:07Z

Thanks @gspencergoog I think I got it to work :)

gspencergoog · 2021-07-28T17:28:01Z

Yes, that looks much better, and the CLA bot should be happy now.

zlshames · 2021-07-28T17:39:27Z

Thanks @gspencergoog I think I got it to work :)

Thanks for this! And thank you @gspencergoog for all the help!

justinmc

Thanks so much for taking on this feature, it's not a small task but it's something Flutter really needs.

First of all we need to get some consensus on the overall approach you've taken here and make sure it's the best one. I see two questions we need to answer:

What if the Flutter developer doesn't want to support images in their field? It looks to me like Flutter developers might also want to keep the current behavior, where images are explicitly disallowed, but that might not be possible if we hardcode the mime types like this. I left a comment below about this with a screenshot.
When we add iOS support for this feature, will the approach here be compatible? What about pasting an image, or is that totally different? Would you be able to take a brief look into these to make sure this approach would be more or less compatible?

Overall I really like this approach of just giving developers an onContentCommitted callback and letting them handle images however they want. If we support this kind of thing out of the box in the future (like maybe a RichTextField or something), then we can easily include image support. I'm looking forward to getting these two PRs merged.

justinmc · 2021-07-29T18:02:41Z

shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java

@@ -330,6 +331,17 @@ public InputConnection createInputConnection(
    }
    outAttrs.imeOptions |= enterAction;

+    String[] imgTypeString = new String[] {


Nit: Should this be declared outside the method as a constant?

This nit may be obsolete actually, see my comment below.

justinmc · 2021-07-29T18:18:09Z

shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java

+      "image/jpeg",
+      "image/webp"
+    };
+    EditorInfoCompat.setContentMimeTypes(outAttrs, imgTypeString);


I noticed that on Android, if try I use Gboard to insert a gif into a field that doesn't support images, it gives me a message saying, "Google does not support image insertion here". Is that message based on this mime type list? If so, maybe we need a way for Flutter developers to specify that a field does not support images.

I also tried this on a Flutter app (before your changes) and I get the same message. We might want to include a way to support this original behavior. The first thing that comes to mind is adding a contentMimeTypes parameter to EditableText and passing it through to here, would that work?

As far as I know that message is only based on whether the contentCommit hook is overriden in the InputClient. It could be based off the mimeType list, in which case I agree finding a way to pass those to there would be a good idea.

@justinmc so apparently the mime types do nothing in @zlshames's testing... how should we approach this then? Should we just remove this code entirely?

I don't really know why it doesn't affect the content insertion, it seems to me that we have everything set up correctly. If we indeed haven't made any mistakes, imho the best option would be to let the dev handle their desired mimeTypes inside onContentCommit and choose how to notify their users that the content they are inserting is unsupported. Thinking on the iOS side as well, I am not 100% sure you can control the mimeType of items being pasted into an editable text field - so it may be better to do it this way for the sake of consistency.

We could make the default behavior of onContentCommit show a snackbar saying "Image insertion isn't supported here" to replicate the original behavior in case devs don't want to support image insertion at all.

tneotia · 2021-07-29T19:23:29Z

2. When we add iOS support for this feature, will the approach here be compatible? What about pasting an image, or is that totally different? Would you be able to take a brief look into these to make sure this approach would be more or less compatible?

I've tried to find documentation for a similar API to contentCommit on iOS and I haven't been able to find anything at all - aside from some old threads from iOS 10 when apps were using copy/paste as a primitive workaround for inserting image content. I guess that is the only way to do it on iOS.

I'd say you could probably still get the image data and send that to the TextInput contentCommit callback when you detect pasted content. Technically the CommittedContent class (which is the outward facing API) only needs mimeType, uri, and data, which should be easily accessible from the pasted content details. Again, I've never worked with Swift so I'm not 100% sure but I think this approach should be compatible when someone adds support for iOS.

zlshames · 2021-07-29T19:53:04Z

FYI, I'm pretty sure our solution works for pasting via GBoard. However, I dont think it works with Samsung keyboard. Not entirely sure why because I don't know the specifics of how that keyboard commits data

Content Commit Mimes & v2.8.1

zlshames · 2022-01-28T20:03:18Z

@justinmc Hey, I think this should be good to go. I already had tagged it, just mis-versioned it in my fork. But you don't have to worry about that. I merged my content commit mime type changes, which is pretty straight forward. I replaced the static list with a dynamic value passed from the configuration from the method channel.

Please let me know if anyone finds anything that I need to change

Edit: I have hit up a contributor to accept the CLA, waiting for that.

ryanawhelan · 2022-01-28T20:26:51Z

@googlebot i consent

Adds additional parameter to TextInputChannel.Configuration

justinmc

@tneotia Sorry I lost track of this PR for awhile. Can you take a look at the failures? They look legitimate. I'll take a look at the framework side right now.

FYI @matthew-carroll you may be interested in this change and the corresponding framework PR (flutter/flutter#87203).

justinmc · 2022-04-13T20:56:50Z

shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java

@@ -473,6 +482,65 @@ public boolean performEditorAction(int actionCode) {
    return true;
  }

+  @Override
+  public boolean commitContent(InputContentInfo inputContentInfo, int flags, Bundle opts) {


A good point was recently brought up by @matthew-carroll about how APIs like this should follow Flutter's declarative pattern instead of being imperative. So instead of telling the client to commit the content, informing the client that the user committed content (contentCommitted maybe?). Though, it seems that adjacent API methods already have an imperative pattern, so maybe now is not the time to change that.

matthew-carroll · 2022-04-13T21:24:21Z

shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java

+    if (BuildCompat.isAtLeastNMR1()
+        && (flags & InputConnectionCompat.INPUT_CONTENT_GRANT_READ_URI_PERMISSION) != 0) {
+      try {
+        inputContentInfo.requestPermission();


Does this request show a dialog for permissions? If so, I would recommend keeping all UI-related behavior out of this class. Consider making it the clients responsibility to obtain the appropriate permissions before running the code that requires the permissions. You can throw an exception if that permission isn't granted.

It does not appear to show a dialog for any permissions. I think its more of asking the system to have temporary access to the committed files, if the app isn't given access already.

https://developer.android.com/reference/android/view/inputmethod/InputContentInfo#requestPermission()

Have you tried running the code without the permissions to see what happens? If no dialog is shown, then who are you "requesting permission" from? Typically a "request" is something that can be denied, so this seems strange.

…n-pr

chinmaygarde · 2022-05-05T20:28:37Z

This PR has the WIP tag but I see recent updates? Is this ready for a review?

tneotia · 2022-05-07T18:22:41Z

@chinmaygarde yes, this should be ready for review I think

chinmaygarde · 2022-05-26T20:14:14Z

This definitely needs a rebase. cc @justinmc Are you able to review this?

tneotia · 2022-05-27T14:17:58Z

Actually @chinmaygarde , we had a small discussion in the PR in the Flutter SDK repo, and we are thinking about decoupling this API from the TextInputClient. I'd re-mark this as a WIP because I think a some of the engine code will also have to change.

chinmaygarde · 2022-07-21T20:28:20Z

Closing this as stale. Please re-open once progress can be made.

justinmc · 2022-08-19T17:40:11Z

After catching up on the situation here, I believe this PR can be reopened without major changes, as explained in the framework PR.

tneotia · 2022-08-19T21:57:27Z

Should we rebase and re-PR? Or is there a way to reopen this one?

flutter-dashboard bot added platform-android needs tests labels Jul 28, 2021

google-cla bot added the cla: no label Jul 28, 2021

tneotia mentioned this pull request Jul 28, 2021

Add image keyboard support on Android flutter/flutter#87203

Closed

8 tasks

Android GIF/Image Insertion

54a9657

tneotia force-pushed the tanay/gif-insertion-pr branch from b4d8cc5 to 54a9657 Compare July 28, 2021 17:22

google-cla bot added cla: yes and removed cla: no labels Jul 28, 2021

Remove debug logs

5f0582e

justinmc requested review from justinmc, Renzo-Olivares and GaryQian July 29, 2021 17:52

justinmc reviewed Jul 29, 2021

View reviewed changes

zlshames and others added 3 commits January 28, 2022 14:57

2.8.1 content commit merges & comment fixes

3cead60

Java refactor

4f735ff

Merge pull request #5 from BlueBubblesApp/zach/gif-insertion-pr

76f5faf

Content Commit Mimes & v2.8.1

google-cla bot added cla: no and removed cla: yes labels Jan 28, 2022

google-cla bot added cla: yes and removed cla: no labels Jan 28, 2022

zlshames and others added 2 commits January 28, 2022 16:27

Adds additional parameter to TextInputChannel.Configuration

5d6d517

Merge pull request #6 from BlueBubblesApp/zach/gif-insertion-pr

6004596

Adds additional parameter to TextInputChannel.Configuration

justinmc reviewed Apr 13, 2022

View reviewed changes

matthew-carroll reviewed Apr 13, 2022

View reviewed changes

tneotia added 4 commits April 13, 2022 17:28

Fix build failures

0224531

Merge remote-tracking branch 'flutter/master' into tanay/gif-insertio…

f1d7ab0

…n-pr

Remove duplicate function

41fe8f0

Fix missing imports

4105ebc

chinmaygarde removed the Work in progress (WIP) Not ready (yet) for review! label May 19, 2022

zanderso added the Work in progress (WIP) Not ready (yet) for review! label Jun 2, 2022

chinmaygarde closed this Jul 21, 2022

This was referenced Aug 23, 2022

Add support for image insertion on Android flutter/flutter#110052

Merged

Add support for image insertion on Android #35619

Merged

Add image keyboard support on Android #27763

Add image keyboard support on Android #27763

Conversation

tneotia commented Jul 28, 2021 • edited Loading

Pre-launch Checklist

flutter-dashboard bot commented Jul 28, 2021

google-cla bot commented Jul 28, 2021

tneotia commented Jul 28, 2021

google-cla bot commented Jul 28, 2021

gspencergoog commented Jul 28, 2021

google-cla bot commented Jul 28, 2021

zlshames commented Jul 28, 2021

tneotia commented Jul 28, 2021

google-cla bot commented Jul 28, 2021

gspencergoog commented Jul 28, 2021 • edited Loading

gspencergoog commented Jul 28, 2021

google-cla bot commented Jul 28, 2021

tneotia commented Jul 28, 2021

gspencergoog commented Jul 28, 2021

zlshames commented Jul 28, 2021

justinmc left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tneotia Jul 29, 2021 • edited Loading

Choose a reason for hiding this comment

tneotia commented Jul 29, 2021

zlshames commented Jul 29, 2021

zlshames commented Jan 28, 2022 • edited Loading

ryanawhelan commented Jan 28, 2022

justinmc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinmaygarde commented May 5, 2022

tneotia commented May 7, 2022

chinmaygarde commented May 26, 2022

tneotia commented May 27, 2022

chinmaygarde commented Jul 21, 2022

justinmc commented Aug 19, 2022

tneotia commented Aug 19, 2022

tneotia commented Jul 28, 2021 •

edited

Loading

gspencergoog commented Jul 28, 2021 •

edited

Loading

justinmc left a comment •

edited

Loading

tneotia Jul 29, 2021 •

edited

Loading

zlshames commented Jan 28, 2022 •

edited

Loading