fix: binary content appears in composer when pasting image #60501
Conversation
|
@allgandalf Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
|
|
Hey @s77rt, I created the PR with changes proposed on my proposal. Let's discuss what we were talking about. |
patches/react-native+0.77.1+025+fix-android-textinput-onPaste-binary.patch
Outdated
Show resolved
Hide resolved
This reverts commit fac5b14.
|
I removed the undesired diffs. Is it looking good now, @s77rt? |
|
Yes looks good. You can try confirm if it's working by removing node_modules/react-native and then run npm install, you should be able to see a message if the patch was applied correctly or not |
Yes, I've tried it and the patch applied successfully. Thanks for helping me with this, I learned something new :) |
|
@rohit9625 Can you try paste images into TextInputs that do not use markdown |
|
Any refs to such text inputs? Are you facing some issues? |
|
This is happening on main when pasting image Search Field: |
|
Those TextInputs do not use the onPaste event and thus as per the implemented solution it won't return |
Got it. So I should be re-writing the method to return regardless of the |
|
Hi @s77rt, what do you think about this variant? public boolean onTextContextMenuItem(int id) {
if (id == android.R.id.paste || id == android.R.id.pasteAsPlainText) {
id = android.R.id.pasteAsPlainText;
ClipboardManager clipboardManager =
(ClipboardManager) getContext().getSystemService(Context.CLIPBOARD_SERVICE);
ClipData clipData = clipboardManager.getPrimaryClip();
String type = null;
String data = null;
if (clipData.getDescription().hasMimeType(ClipDescription.MIMETYPE_TEXT_PLAIN)) {
type = ClipDescription.MIMETYPE_TEXT_PLAIN;
data = clipData.getItemAt(0).getText().toString();
} else {
Uri itemUri = clipData.getItemAt(0).getUri();
if (itemUri != null) {
ContentResolver cr = getReactContext(this).getContentResolver();
type = cr.getType(itemUri);
data = itemUri.toString();
}
}
if (type != null && data != null && mPasteWatcher != null) {
mPasteWatcher.onPaste(type, data);
}
return true
}
return super.onTextContextMenuItem(id);
}Or this one? It only returns true if clipboard content has URI that might point to any binary data? public boolean onTextContextMenuItem(int id) {
if (id == android.R.id.paste || id == android.R.id.pasteAsPlainText) {
id = android.R.id.pasteAsPlainText;
ClipboardManager clipboardManager =
(ClipboardManager) getContext().getSystemService(Context.CLIPBOARD_SERVICE);
ContentResolver cr = getReactContext(this).getContentResolver();
ClipData clipData = clipboardManager.getPrimaryClip();
if (clipData != null) {
ClipData.Item item = clipData.getItemAt(0);
Uri uri = item.getUri();
String type = null;
String data = null;
if (uri != null) {
type = cr.getType(uri);
data = itemUri.toString();
if (mPasteWatcher != null) {
mPasteWatcher.onPaste(type, data);
}
return true;
}
if (clipData.getDescription().hasMimeType(ClipDescription.MIMETYPE_TEXT_PLAIN)) {
type = ClipDescription.MIMETYPE_TEXT_PLAIN;
data = item.getText().toString();
if (mPasteWatcher != null) {
mPasteWatcher.onPaste(type, data);
}
}
}
}
return super.onTextContextMenuItem(id);
} |
|
i think the second one is correct. The custom handling is only for files just to prevent raw content pasting. You should also state that in a comment as it may not be clear |
Okay, let's go with the second one and I'll also write appropriate comments. Thanks once again :) |
|
@rohit9625 is this PR ready for final review? I'll ask for adhoc builds after you confirm |
|
🚧 @Beamanator has triggered a test app build. You can view the workflow run here. |
|
Hey @allgandalf, I am pushing the changes within 20 mins. What is this workflow running? |
|
I saw a 👍 to my message earlier from you so i asked for a test build, if you are still pushing changes, lets build again after you're done |
Edit: I wanted to test the pasting image in both markdown enabled and disabled text inputs but the build was falling which happens very often on my system. Now, it's resolved and will be update you here shortly :) |
This comment has been minimized.
This comment has been minimized.
Yeah, I misread the message. I thought you ask me let you know when the PR is ready. My bad :( |
|
Hi @allgandalf, I pushed the recent changes and tested for both type of text fields. It's working as expected After_onPaste_Patch_Fix.mp4 |
|
Will wait for the tests to pass and ask for adhoc again |
|
🚧 @Beamanator has triggered a test app build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
@rohit9625 can you please check on the ^ android build as well |
|
It's working as expected, @allgandalf Expensify_Adhoc_Build_Test.mp4 |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2025-04-24.at.2.15.45.PM.movAndroid: mWeb ChromeScreen.Recording.2025-04-24.at.2.28.22.PM.moviOS: NativeScreen.Recording.2025-04-24.at.12.33.03.PM.moviOS: mWeb SafariScreen.Recording.2025-04-24.at.12.34.12.PM.movMacOS: Chrome / SafariScreen.Recording.2025-04-24.at.11.53.02.AM.movMacOS: DesktopScreen.Recording.2025-04-24.at.11.50.10.AM.mov |
|
@rohit9625 can you please update the testing steps to specify that we need a samsung android device to test this? |
|
I added that as a prerequisite. Is that good @allgandalf? |
|
YEssssssssssss! @rohit9625 can you please do that :)) |
|
Merging but let's not forget! 🙏 |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Well, the PR which added the |
|
The upstream PR is stale as there is no movement on the proposal react-native-community/discussions-and-proposals#804 |
No problem, you did a great job in that PR. They'll definitely respond :) |
|
🚀 Deployed to staging by https://github.com/Beamanator in version: 9.1.33-0 🚀
|
|
🚀 Deployed to staging by https://github.com/Beamanator in version: 9.1.35-1 🚀
|
|
🚀 Deployed to staging by https://github.com/Beamanator in version: 9.1.36-3 🚀
|
|
🚀 Deployed to staging by https://github.com/Beamanator in version: 9.1.37-1 🚀
|
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.1.37-3 🚀
|
Explanation of Change
In the previous patch where
onPastefunctionality was added, inside the override ofonTextContextMenuItem(int id)we are calling the defaultsuper.onTextContextMenuItem(id)even when the clipboard contains binary content of the image.I added the code to return true from the
onTextContextMenuItem(int id)when pasting files because returningtrueindicates that the paste functionality is handled by our code.Fixed Issues
$ #55304
PROPOSAL: #55304 (comment)
Tests
Prerequisites: You must have a Samsung Device to repro this issue
Offline tests
Same as normal Tests steps
QA Steps
Prerequisites: You must have a Samsung Device to repro this issue
Perform the same steps for Standard App as the issue is reproducible on Android only.
Note: For other targets, just try copy pasting
images/filesin the composer.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
After_React_Native_Patch.mp4
Android: mWeb Chrome
Android_Chrome.mp4
iOS: Native
iOS_Native.mov
iOS: mWeb Safari
iOS_Safari.mov
MacOS: Chrome / Safari
Mac_Safari.mov
MacOS: Desktop
Mac_Desktop.mov