-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix: pasting from wps office crashes app #64457
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
|
|
|
Android build kept failing for me. Asked on Slack here. |
|
@jayeshmangwani 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] |
|
I'm still stuck with Android build #64457 (comment). @cead22 Can you help to trigger adhoc builds for this PR? |
|
🚧 @cead22 has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
@mkzie2 it looks like that error is known (https://expensify.slack.com/archives/C01GTK53T8Q/p1750696354220979) and it'll be fixed soon. Can you merge main in the mean time? |
|
This is not done. We just upgraded to RN 0.79.2. I need to verify the patches. |
|
@cead22 Can you please re-trigger the adhoc builds? Thanks! |
|
@cead22 @jayeshmangwani I think we can ignore the @mateuuszzzzz Hi, I already included the description of the modified patch in PR description. Please help me to include it in your clean-up PR in case my PR got merged first. Thanks! |
|
🚧 @cead22 has triggered a test Expensify/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! 🧪🧪
|
|
The builds worked well. I added the recordings. @jayeshmangwani You can start your review now. Note that pasting image from WPS Office is not supported on native apps yet (while Google Docs does). |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid.mp4Android: mWeb Chromemweb-chrome.mp4iOS: HybridAppiOS.moviOS: mWeb Safarimweb-safari.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
|
@mkzie2 There are a few inconsistencies when copying images from WPS Office. This might be a separate issue from the crash, but I just want to confirm before approving this PR. Copying and pasting images from WPS behaves differently on Android and mobile web (Chrome). wps-android.mp4 |
|
@cead22 Tested the build on Android - it's working well when copying both image and text; no crashes observed. Please refer to comment #64457 (comment) for details on the inconsistency. There are some issues when copying from WPS Office - specifically, images copied from WPS don't paste properly into the Android composer. I also tested on mobile Chrome (web), and it throws an error after copying from WPS, so it's not working there either (production version). That said, these issues appear to be unrelated to this PR, which specifically addresses the crash. I’ll report those bugs separately, but I'm noting them here for visibility. |
| + data = itemUri.toString(); | ||
| + if (mPasteWatcher != null) { | ||
| + mPasteWatcher.onPaste(type, data); | ||
| + if (type != null && !type.equals(ClipDescription.MIMETYPE_TEXT_PLAIN)) { |
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.
The indentation in this patch seems off, does it matter?
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.
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.
Oh yeah the green and red background in the GH diff is confusing on these patches
|
The validate patches GHA is still failing |
|
Any updates? |
|
@cead22 The validate patches GHA failure is expected for now. See #64457 (comment). @mateuuszzzzz Can you confirm that the |
Should we disable it for now until it's fixed, instead of ignoring it in every PR? |
|
The check would only be triggered on changed files which rarely happens given it's related to 3rd-party lib patches. Maybe @roryabraham could suggest a better way here. TL;DR I updated a react-native patch and the Validate Patches action failed because it's missing |
|
Ah, yes I agree that we can ignore the failing I also brought this up in slack to get #63324 finished soon.
@cead22 for context, the check is only non-trivial to fix if we modify patches of certain dependencies that have multiple patches, such as react-native, react-native-web, and react-native-reanimated. So I think it's valuable to keep it enabled in the meantime, because if someone adds a new patch for a currently-unpatched dependency, we'd enforce that the required documentation is collected. |
|
@roryabraham thanks for the context, and @mkzie2 @jayeshmangwani sorry for the delay in the review. Can you merge main to resolve the conflicts, and I'll approve and merge? Thanks |
|
FWIW, #63324 is merged now, so after merging main and resolving conflicts, |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/cead22 in version: 9.1.83-0 🚀
|
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.1.83-5 🚀
|

Explanation of Change
This PR fixes:
details.mdFixed Issues
$ #63191
PROPOSAL: #63191 (comment)
Tests
Offline tests
None
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
Google Docs text & image:
Screenrecording_20250625_124519.mp4
Screenrecording_20250625_124448.mp4
WPS text:
Screenrecording_20250625_124341.mp4
Android: mWeb Chrome
Screenrecording_20250625_125507.mp4
iOS: Native
RPReplay_Final1750831938.mov
iOS: mWeb Safari
RPReplay_Final1750831521.mov
RPReplay_Final1750831653.mov
RPReplay_Final1750831809.mov
MacOS: Chrome / Safari
Untitled.mov
Screen.Recording.2025-06-25.at.13.44.07.mov
Screen.Recording.2025-06-25.at.13.50.12-compressed.mov
Screen.Recording.2025-06-25.at.13.49.17-compressed.mov
MacOS: Desktop
Screen.Recording.2025-06-25.at.13.46.27-compressed.mov
Screen.Recording.2025-06-25.at.13.50.34-compressed.mov