-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[$250] Web - Chat - Pasted message not always displays no hyperlink format when paste as plain text #53718
Comments
Triggered auto assignment to @alexpensify ( |
Edited by proposal-police: This proposal was edited at 2024-12-06 21:28:37 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.Web - Chat - Pasted message not always displays no hyperlink format when paste as plain text What is the root cause of that problem?We are setting the plain text to a markdown value if it is anchor tag and plain text value if it is not
but anchor regex test passes intermittently for the same value as explained here because:
What changes do you think we should make in order to solve the problem?This isAnchorRegex check is introduced in #42147 to achieve copy paste consistency across web and native platforms but to achieve it the correct way to do it is to copy the mardown content to the plain clipboard here App/src/pages/home/report/ContextMenu/ContextMenuActions.tsx Lines 47 to 52 in 476f397
then convert from markdown > text here for web App/src/hooks/useHtmlPaste/index.ts Lines 93 to 98 in 476f397
What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?We can make a unit test for What alternative solutions did you explore? (Optional) |
Job added to Upwork: https://www.upwork.com/jobs/~021866332344495527627 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @Pujan92 ( |
@Pujan92 - Can you please review and confirm if one of these proposals will fix the issue? Thanks! |
Please re-state the problem that we are trying to solve in this issue.Web - Chat - Pasted message not always displays no hyperlink format when paste as plain text What is the root cause of that problem?ContextMenuActions.tsx (lines 45–55) The root cause is that the plain text fallback wasn't being reliably set as the primary clipboard content before the HTML, leading to inconsistent behavior during the first "Paste as plain text" operation. What changes do you think we should make in order to solve the problem?After: const plainText = isAnchorTag ? Parser.htmlToMarkdown(content) : Parser.htmlToText(content);
// First, set the plain text to the clipboard to ensure it is prioritized
Clipboard.setString(plainText);
// Then, set the HTML content with plain text fallback
Clipboard.setHtml(content, plainText); What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?To prevent reintroducing this issue in the future, automated tests should cover a range of scenarios to verify clipboard behavior and ensure consistency across environments. What alternative solutions did you explore? (Optional) |
📣 @AK-web! 📣
|
Edited by proposal-police: This proposal was edited at 2024-12-10 15:20:03 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.Chat - Pasted message is not always displayed without hyperlink format when paste as plain text What is the root cause of that problem?There are two problems here
App/src/pages/home/report/ContextMenu/ContextMenuActions.tsx Lines 50 to 52 in df7ab48
App/src/hooks/useHtmlPaste/index.ts Lines 79 to 81 in 9fe5511
What changes do you think we should make in order to solve the problem?We should create a new instance of RegExp. The updated code in
To handle root cause (2), we should check and convert the markdown to plain text in App/src/hooks/useHtmlPaste/index.ts Line 81 in 9fe5511
What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?None What alternative solutions did you explore? (Optional) |
@alexpensify, @Pujan92 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
Thanks for the proposals. @FitseTLT and @prakashbask's proposal of regex new instance won't solve the problem as it will only make it consistent(isAnchorTag gets true) and paste will be always the link format. @AK-web's RCA and solution don't look correct to me.
I agree with the @prakashbask RCA but why do we need the text extraction within the useHTMLPaste? Isn't the right plainText should be passed from ContextMenuActions |
@Pujan92 You have misunderstood the problem. The problem is that isAnchorTag is being inconsistent and the solution is to make it consistent. You have to first understand the purposes of |
@FitseTLT The problem isn't only making |
As a hint, I would like to suggest checking whether #40564 is an actual issue or correct behavior. |
That's a nice idea. My first proposal was like your suggestions but after digging deep I found out the PR and that's why I proposed the fix for the RCA of the inconsistency. In case your expectation is accepted I have Updated to incorporate it to my alt solution Ok @Pujan92 Let's ask for the expected behaviour 👍 |
@alexpensify Could you plz confirm the expected behavior for this issue as @FitseTLT is requesting? To me, it looks correct. |
@Pujan92 @alexpensify We will need to club this 53832 issue also to discuss the expected behaviour |
@alexpensify To make it clear for you. The problem here is inconsistency in the paste behaviour as mentioned in the Actual Result
We have no doubt on fixing the inconsistency. What we want from you is to confirm on the behaviour of paste as a plain text for links. what should be pasted? In all other cases for past as plain we paste the text without the formating, for instance, if it was a bold text markdown when we paste as plain we paste only the raw text without formatting but the problem is they have made an exception for anchor links in this PR to fix an issue That change made links to be pasted with the link for both normal paste and paste as plain text options. WDYT is the expected behaviour? Should we revert the change by that PR or fix the inconsistency based on the expectations from that PR. |
Agree @prakashbask, both can be clubbed. The only platform that differs is Android as it doesn't support HTML paste and we need to consider it a special case for deriving plainText. cc @ZhenjaHorbach
I might be wrong here @FitseTLT, I think that issue needs to be generic to render all markdowns on Android instead of plain text. |
There isn't really anything for me to review, we need to wait for @Pujan92 @Skalakid and @MichaelBuhler in this comment:
|
Hello, in my opinion, every time a user copies, for example, the Since Android doesn't support pasting HTML, and there were some changes introduced in this issue, That's how I view it, but it would be better to ask internals from Expensify and the design team about it |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
Thanks @Skalakid. I believe you are suggesting applying the markdown in the text/plain for the android to make it consistent across platforms. I will ask the same in the slack also to get more inputs. |
On re-review, I see that it makes sense to provide markdown Text as a second argument to @jasperhuangg In this case, I would suggest assigning the issue to @prakashbask to raise a PR and splitting the bounty. Let me know your thoughts too. |
@Pujan92 @jasperhuangg My proposal for the issue is different and changes will be required in ExpensiMark library to support markdown -> text instead of markdown -> html -> text using existing Parser calls as proposed by @FitseTLT in App/src/hooks/useHtmlPaste/index.ts Line 81 in 9fe5511
Moreover, I'm uncertain whether the ExpensiMark library team will approve and implement mardown -> text proposal. This needs to clarified before PR can be raised. |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@prakashbask We are the ExpensiMark library team 😆 Your solution looks good to me, I think we can move forward with implementing it and splitting the bounty on this issue. |
📣 @prakashbask 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
@jasperhuangg My understanding on the scope of work is as below. Please confirm |
@Skalakid Is this(utility to convert markdown to plain text) something you can help with? |
@Pujan92 @prakashbask I'm sorry but currently I don't have a bandwidth to do that. However, there were plenty of similar issues that required changes both in the E/App and |
@Skalakid @Pujan92 I did some analysis on the approach to implement the changes required for Option 1: Option 2: |
@prakashbask, what is the real difference between a If we don't want to do that, we can workaround this and parse the markdown to HTML and then to text like @FitseTLT proposed |
@Skalakid I was also looking at following scenarios where markdown syntax will be removed when it is converted to plain text
The workaround of markdown -> HTML -> text can be implemented within E/App without making any changes to ExpensiMark. I will proceed with this approach for the PR. |
@prakashbask Right, there are more cases like that. So to not add another layer of complexity to ExpensiMark I prefer a Also, it was originally @FitseTLT's idea in this proposal, @alexpensify @Pujan92 how will we handler this situation? Please take this into account when setting up the payments |
@alexpensify @Pujan92 Let @FitseTLT raise the PR for this issue as he has proposed the markdown -> HTML -> text solution. The offer sent to me on upwork can be withdrawn. |
@Skalakid Agree and it makes sense.
@prakashbask You can raise a PR as we are considering both the proposals for the issue and also mentioned splitting the bounty here. Hope @FitseTLT should be fine with it. cc: @jasperhuangg |
PR #55932 |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: undefined
Reproducible in staging?: Yes
Reproducible in production?: Yes
Issue reported by: Applause Internal Team
Action Performed:
Expected Result:
When choosing "Paste as plain text", the message is pasted without any hyperlink format
Actual Result:
The first time paste as plain text, the message is pasted with hyperlink format, the second time it is pasted without hyperlink format as expected
Workaround:
Unknown
Platforms:
Screenshots/Videos
Bug6679622_1732842325268.Recording__740_1_.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: