-
Notifications
You must be signed in to change notification settings - Fork 3.5k
fix: move truncation of HTML paste into parser #61322
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
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
|
|
@mananjadhav 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] |
|
Waiting on high traffic account before I can complete the author checklist. |
|
@mananjadhav Not sure how to handle the HybridApp warning. |
|
@yellowtailfan I think the best way would be to trigger the adhoc build. The reason being we'll be able to test it like an end user and also large content. @Gonals Can you please trigger adhoc build for this one. |
|
Tested on high-traffic account and staging API. Author checklist completed. ✅ |
|
Thanks for constant communication on this one @yellowtailfan. I'll get to it today. |
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.
Code change is fine. I don't have a Windows machine to test right now. Can we have an adhoc build @Gonals? I can then try to test this out.
|
🚧 @Gonals 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! 🧪🧪 |
|
@mananjadhav It looks like a lot of the automated checks failed, but in parts of the code that this PR doesn't change. I guess I forked off I got a message with QR codes for an ad hoc build thanks @Gonals. @mananjadhav should I test the ad hoc build, or you, or both of us? |
|
You can test the adhoc build, I'll also be testing by Friday-Saturday. Meanwhile can you sync with the latest main? |
|
@yellowtailfan can we still fix these so that we can unblock the PR? Generally we try to fix the lint issues despite not a part of the PR. These are split amongst different PRs. |
|
@mananjadhav Thanks for your guidance. I have fixed the lint issues. Some of them were also fixed upstream so I merged the fixes. The lint fixes were:
For For the deprecated function, it was actually only used in the To test the lint changes this is what I did:
I wasn't able to reach all of the affected functions in my app, e.g. I couldn't figure out how to access tasks. However, the ones I could reach all worked fine. |
|
A couple more lints. I can look at those tomorrow if you like. |
|
@yellowtailfan Can you merge the latest main? @Gonals Should we rollback and skip the lint fixes? I can see several fixes which are not in the test path and could affect. If not can we atleast skip these new lint errors for this PR scope. |
|
Quick bump @Gonals |
|
We managed to get past something this by coordinating merge/push times in this other PR |
|
The lint issues may be caused by a bug described in #62656. |
|
@mananjadhav we need to update the e-common version to the latest to solve an error on main. have you tested and approved this PR? I am going to merge the new version bump, then we won't need to do the same here in this PR. In case, you find any issues here, you will have to update the e-common again and version on this PR. Thus I can safely merge my PR. |
|
@parasharrajat Thanks for letting me know about the e-common version bump, I will resolve and test on the next merge. Please let me know which PR you are merging. @Gonals Can you please trigger the workflows on this PR at 10 am EDT tomorrow (Wed)? Before then I propose to do these steps:
|
|
@yellowtailfan That PR is merged. During the conflict resolution, take the most recent version. #62873 |
|
OK I've synchronised with upstream main and reverted my lint fixes on unrelated files. I tested my fix still works. I will resync shortly before 10 am EDT. |
|
@Gonals I just did a sync and If I don't hear from you in the next hour, I will do another sync just before 10 am EDT. |
|
✋ 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/Gonals in version: 9.1.54-0 🚀
|
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.1.54-7 🚀
|
Explanation of Change
Currently when a long HTML string is pasted into the App, the string is truncated before any parsing. However, some clipboard sources such as Microsoft Word create large headers that are bigger than the typical
maxLengthof 10K. This means the body element of the HTML is truncated and the HTML to Markdown parser attempt to parse the header producing nonsense codes.This change moves the long string truncation into the Parser. The truncation happens after the body section is extracted but before any other parsing to preserve performance for long strings. The fix depends on Expensify/expensify-common#852.
The result is that HTML pastes are more reliable and preserve formatting such as bold and italics.
Multiple combinations of app platform + app browser host + clipboard source app were tested before and after the patch. In the combination reported (Windows), the patch fixes the reported issue. In other combinations, the behaviour was different from the reported issue (some cases were correct, others were not), and this patch makes no changes to that behaviour.
Fixed Issues
$ #57241
PROPOSAL: #57241 (comment)
Tests
Note that the bug was reported on Windows and the fix also applies to Windows. See the test results table below for the behaviour before and after this PR for all platforms.
Test: text with bold and italic formatting copied in Word (or equivalent) and pasted into Expensify
Browsers used as an app host:
Medium and large pasted text was truncated by the Expensify app code (as expected) unless otherwise noted.
❌SM: Code (FF)
❌L: 2 sec lag then code (FF)
✅SM: Formatted (FF)
✅L: Formatted + 2 sec lag (FF)
✅SM: Formatted (SF)
❌L: 2 sec lag then blank line (SF)
✅SM: Formatted (SF)
❌L: 5 sec lag then blank line (SF)
❌L: 2 sec lag then image attachment
❌L: 2 sec lag then image attachment
ML: Word (macOS)
❌L: 2 sec lag then blank line
❌L: 2 sec lag then blank line
Chrome
Chrome
Image copying was also tested on Windows with the Web version of Expensify:
This happens both on the original and patched code.
The same test with Chrome as the source, and copying a mix of formatted text and an image produces a correctly formatted chat message including the image, both on the original and patched code.
There was an alert related to the
canBeMissingparam foruseOnyxbut it occurred both in the main branch and patched code, so I assume it is an unrelated and unfixed issue on main.Offline tests
Tested patched code works as expected on reported steps when browser is offline (Web on Windows).
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
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
native-andoid-patched.webm
Android: mWeb Chrome
mweb-android-chrome-patched.webm
iOS: Native
native-ios-patched.mp4
iOS: mWeb Safari
mweb-ios-safari-patched.mp4
MacOS: Chrome / Safari
web-mac-safari-patched.mp4
MacOS: Desktop
desktop-mac-safari-patched.mp4
Windows: Chrome
web-win-chrome-patched.mp4