[migbot]

Summary

There is currently no way to handle pasting an image from the clipboard into TextInput. This PR introduces an onPaste prop for the TextInput which will receive the content pasted (Base64 encoded string if pasting an image) and the MIME type of the content.

Changelog

[Android] [iOS] [Added] - Add onPaste prop to TextInput

Test Plan

• Updated the TextInput Jest test to verify that onPaste callback is called with text and mimeType arguments
• Updated the Android TextInput integration test to paste some test plain text and an image and verify that the expected values for text and MIME type are received in the onPaste callback

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[analysis-bot]

Code analysis results:

• eslint found some issues.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[elicwhite]

While this seems reasonable at first glance, I want to make sure that such an API is compatible with more than just Android. Even if we don't implement it right now, can you do some investigation as to whether an API like this would be implementable on iOS?

[migbot]

@TheSavior For iOS the changes would be added here https://github.com/migbot/react-native/blob/support-text-input-on-paste/Libraries/Text/TextInput/Singleline/RCTUITextField.m#L135 and here https://github.com/migbot/react-native/blob/support-text-input-on-paste/Libraries/Text/TextInput/Multiline/RCTUITextView.m#L144. Something like the following:

- (void)paste:(id)sender
{
  UIPasteboard *pb = [UIPasteboard generalPasteboard];
  if (pb.string) {
    // Set MIME type then send event
  } else if (pb.image) {
    // Set MIME type then send event
  } else if (pb.URL) {
    // Set MIME type then send event
  }
  
  [super paste:sender];
  _textWasPasted = YES;
}

I'm not too familiar with the iOS code so this will take me a bit longer to get right. Should I make the changes as part of this PR or submit another once the changes are ready?

[migbot]

@TheSavior After some more investigation, I found that the UIImage instance from the iOS pasteboard does not contain the source path of the image and so will not work with onPaste as it is currently implemented. I'll continue looking into what my other options are.

[migbot]

@TheSavior For iOS, the only option is to return the MIME type along with the Base64 encoded image data. For consistency in the API I will update the Android code to do the same. How does that sound to you?

[migbot]

I've gone ahead and made the changes required to return the Base64 encoded image for both Android and iOS. I'm a bit lost as to how to go about writing unit/integration tests for iOS. Can I get some guidance?

[janicduplessis]

@migbot Can we save the image to a temporary file and return the uri of that file? Not sure if we do this anywhere in the codebase but somethig like https://github.com/bamlab/react-native-image-resizer/blob/master/ios/RCTImageResizer/RCTImageResizer.m#L40 could work.

[migbot]

@janicduplessis I thought about that but wasn't sure if duplicating the file, even if temporarily, was good practice. I can go ahead and do that, however, deleting the file would be the responsibility of the app unless it is OK to leave that to the OS. What do you think?

[migbot]

@janicduplessis @TheSavior What do you guys think about my comment above?

[migbot]

@cpojer I've got a couple of things left to do:

1. I'm having trouble figuring out how to write iOS integration tests like the ones I have for Android here. Could you provide some guidance, please?
2. I found that !backedTextInputView.textWasPasted is returning NO on the first press of Paste when pasting an image on a multiline TextInput. Another press returns YES. I'll do some more investigation here.

[janicduplessis]

Awesome work! I’ll have a closer look tomorrow.

Sadly I don’t think we have any test for textinput on iOS so there isn’t really a way to test this without adding a lot of infra.

[migbot]

2. I found that !backedTextInputView.textWasPasted is returning NO on the first press of Paste when pasting an image on a multiline TextInput. Another press returns YES. I'll do some more investigation here.

OK this is fixed now. Regarding the detox end to end test, I'm not sure why it's failing as it passed locally for me.

[ericlewis]

@migbot the switch detox tests are flaky, nothing to worry about.

_{Sent with GitHawk}

[migbot]

How does everyone feel about the changes?

[shergin]

This is truly great work!

I believe, the first thing that we have to agree on is: do we really need this functiononality in the core library or not. I don't have a strong opinion here; I am okay with that but kinda doubt. The biggest problem with it is that TextInput is already extremely complex component and adding more stuff to it does not really make things nicer. If any other kind of solution that does not require touching TextInput can serve the needs of products, I would strongly prefer that.

I am not sure about Android, but I know that on iOS paste command is propagated/bubbles, so it's possible to create a component which will hear and proxy to JS this event.

Then API, do we really need an additional event for paste? How exactly it helps to product code to figure out what a user is doing (e.g. where exactly (the cursor position) the passting happened)? Should it be just an additional flag in onChange event with semantic "this change happened because of paste"?

Then, especially related to iOS implementation, I have a feeling that such kinda events/flags should be implemented some where inside adapters classes. Not sure how exactly it can be done thought.

If we have some consenes relate to passing images, we can just decouple that feature from this PR and evolve/land the text version first.

[migbot]

Thanks @shergin!

What would be the process to determine if this should be part of the core library? From the conversation here since I submitted the PR this was not a concern. I think that determination should be made first before I continue with further changes, if needed.

[migbot]

Thanks @shergin!

What would be the process to determine if this should be part of the core library? From the conversation here since I submitted the PR this was not a concern. I think that determination should be made first before I continue with further changes, if needed.

@shergin Thoughts on my comment above?

[shergin]

@migbot The general rule is "If there is a possibility to implement something outside of the core without significant drawbacks, we should not merge that into core".
So, to merge this into the core, somebody should prove that implementing that outside of the core is a really bad idea.

[shergin]

I am really sorry for closing this PR but see my last comment (we need a proof that margin that into TextInput is the only way deliver this feature).

As I mentioned here and in other PR, TextInput is by far the most complex component in the whole RN ecosystem. Adding more functionality into in introduce even more extreme maintenance costs (which are already HUGE).

[migbot]

@shergin Understood and no prob!

[migbot]

Hi @shergin, we have a contributor that added a paste listener to an extension of ReactEditText, however, to make use of it on the JS side he is extending TextInput and overriding its _renderAndroid method in order to add the onPaste prop. This requires that we keep an eye on TextInput for each new RN release and apply any changes to stay in sync. Do you or anyone else on the RN team have any suggestions on how to better extend TextInput?