Skip to content
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

Open
2 of 8 tasks
IuliiaHerets opened this issue Dec 6, 2024 · 57 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Dec 6, 2024

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:

  1. Open https://staging.new.expensify.com/
  2. At any chat, type hello and send message
  3. Click three dots of message action menu
  4. Click "Copy to clipboard"
  5. At compose box, right click mouse and select "Paste as plain text"
  6. Send message
  7. Repeat 3 times from step 2 to step 6

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:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6679622_1732842325268.Recording__740_1_.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021866332344495527627
  • Upwork Job ID: 1866332344495527627
  • Last Price Increase: 2025-01-14
  • Automatic offers:
    • prakashbask | Contributor | 105699511
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 6, 2024
Copy link

melvin-bot bot commented Dec 6, 2024

Triggered auto assignment to @alexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@FitseTLT
Copy link
Contributor

FitseTLT commented Dec 6, 2024

Edited by proposal-police: This proposal was edited at 2024-12-06 21:28:37 UTC.

Proposal

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?

We are setting the plain text to a markdown value if it is anchor tag and plain text value if it is not

const plainText = isAnchorTag ? Parser.htmlToMarkdown(content) : Parser.htmlToText(content);

but anchor regex test passes intermittently for the same value as explained here because:

When the RegExp test method is run with global flag (/g), the regex keeps internally the state of the search. Therefore at each invocation the regular exception will be run from the last index that was found previously.

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

} else {
const anchorRegex = CONST.REGEX_LINK_IN_ANCHOR;
const isAnchorTag = anchorRegex.test(content);
const plainText = isAnchorTag ? Parser.htmlToMarkdown(content) : Parser.htmlToText(content);
Clipboard.setHtml(content, plainText);
}

        Clipboard.setHtml(content, Parser.htmlToMarkdown(content));

then convert from markdown > text here for web

const handlePastePlainText = useCallback(
(event: ClipboardEvent) => {
const plainText = event.clipboardData?.getData('text/plain');
if (plainText) {
paste(plainText);
}

const markdownText = event.clipboardData?.getData('text/plain');
            if (markdownText) {
                const pastedHTML = Parser.replace(markdownText);
                paste(Parser.htmlToText(pastedHTML));
            }

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

We can make a unit test for setClipboardMessage passing the same anchor link content (mocking canSetHtml to return true) and asserting the text argument it is passing to setHtml is the same (markdown not plain text version) for consecutive calls of the function.

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added the Overdue label Dec 9, 2024
@alexpensify alexpensify added the External Added to denote the issue can be worked on by a contributor label Dec 10, 2024
@melvin-bot melvin-bot bot changed the title Web - Chat - Pasted message not always displays no hyperlink format when paste as plain text [$250] Web - Chat - Pasted message not always displays no hyperlink format when paste as plain text Dec 10, 2024
Copy link

melvin-bot bot commented Dec 10, 2024

Job added to Upwork: https://www.upwork.com/jobs/~021866332344495527627

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 10, 2024
Copy link

melvin-bot bot commented Dec 10, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Pujan92 (External)

@melvin-bot melvin-bot bot removed the Overdue label Dec 10, 2024
@alexpensify
Copy link
Contributor

@Pujan92 - Can you please review and confirm if one of these proposals will fix the issue? Thanks!

@AK-web
Copy link

AK-web commented Dec 10, 2024

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.
Plain Text Fallback: Ensure that if Clipboard.canSetHtml() returns false, the plain text content is set correctly.
HTML Content and Plain Text: Verify that when Clipboard.canSetHtml() is true, both HTML and plain text versions of the content are set as expected.

What alternative solutions did you explore? (Optional)

Copy link

melvin-bot bot commented Dec 10, 2024

📣 @AK-web! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@prakashbask
Copy link
Contributor

prakashbask commented Dec 10, 2024

Edited by proposal-police: This proposal was edited at 2024-12-10 15:20:03 UTC.

Proposal

Please 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

  1. During copy to clipboard, we have inconsistent behavior observed when using the regex CONST.REGEX_LINK_IN_ANCHOR as it has global flag ('g') which is stateful and the regex lastIndex is not reset to zero before using it again. So alternatively, markdown and plain text gets copied. In the condition for isAnchorTag, only markdown should be copied to clipboard

const anchorRegex = CONST.REGEX_LINK_IN_ANCHOR;
const isAnchorTag = anchorRegex.test(content);
const plainText = isAnchorTag ? Parser.htmlToMarkdown(content) : Parser.htmlToText(content);

  1. If we solve root cause (1), since markdown will be copied to clipboard as plain text, in useHTMLPaste hook when handlePastePlainText is called, we don't have the text extraction of the markdown so instead of "hello" we will be pasting [hello](<link>) which is not the expected behaviour

const plainText = event.clipboardData?.getData('text/plain');
if (plainText) {
paste(plainText);

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

const anchorRegex = CONST.REGEX_LINK_IN_ANCHOR;
will be

const anchorRegex = new RegExp(CONST.REGEX_LINK_IN_ANCHOR);

To handle root cause (2), we should check and convert the markdown to plain text in

paste(plainText);

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)

Copy link

melvin-bot bot commented Dec 13, 2024

@alexpensify, @Pujan92 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added the Overdue label Dec 13, 2024
@Pujan92
Copy link
Contributor

Pujan92 commented Dec 13, 2024

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.

  1. If we solve root cause (1), since markdown will be copied to clipboard as plain text, in useHTMLPaste hook when handlePastePlainText is called, we don't have the text extraction of the markdown so instead of "hello" we will be pasting hello which is not the expected behaviour

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 Clipboard.setHtml?

@melvin-bot melvin-bot bot removed the Overdue label Dec 13, 2024
@FitseTLT
Copy link
Contributor

FitseTLT commented Dec 13, 2024

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.

@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 isAnchorTag check please refer to #42147 for more context where they have implemented to make the copy of anchor to be always the markdown text for both plain and HTML clipboard. The purpose is to make it consistent whether anchor link are copied to clipboard from web or native. 👍

@Pujan92
Copy link
Contributor

Pujan92 commented Dec 13, 2024

@FitseTLT The problem isn't only making isAnchorTag consistent, it also should paste correct text without the link when the user chooses "Paste as plain text".

@FitseTLT
Copy link
Contributor

@FitseTLT The problem isn't only making isAnchorTag consistent, it also should paste correct text without the link when the user chooses "Paste as plain text".

We will cause regression on #42147 please read the PR for more context.

@Pujan92
Copy link
Contributor

Pujan92 commented Dec 13, 2024

You have to first understand the purposes of isAnchorTag check please refer to #42147 for more context where they have implemented to make the copy of anchor to be always the markdown text for both plain and HTML clipboard.

As a hint, I would like to suggest checking whether #40564 is an actual issue or correct behavior.

@FitseTLT
Copy link
Contributor

You have to first understand the purposes of isAnchorTag check please refer to #42147 for more context where they have implemented to make the copy of anchor to be always the markdown text for both plain and HTML clipboard.

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 👍

@Pujan92
Copy link
Contributor

Pujan92 commented Dec 13, 2024

When choosing "Paste as plain text", the message is pasted without any hyperlink format

@alexpensify Could you plz confirm the expected behavior for this issue as @FitseTLT is requesting? To me, it looks correct.

@prakashbask
Copy link
Contributor

When choosing "Paste as plain text", the message is pasted without any hyperlink format

@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

@FitseTLT
Copy link
Contributor

@alexpensify To make it clear for you. The problem here is inconsistency in the paste behaviour as mentioned in the Actual Result

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

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?
A. plain text
B. link

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.

@Pujan92
Copy link
Contributor

Pujan92 commented Dec 13, 2024

@Pujan92 @alexpensify We will need to club this 53832 issue also to discuss the expected behaviour

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

As a hint, I would like to suggest checking whether #40564 is an actual issue or correct behavior.

I might be wrong here @FitseTLT, I think that issue needs to be generic to render all markdowns on Android instead of plain text.

@melvin-bot melvin-bot bot removed the Overdue label Jan 4, 2025
@jasperhuangg
Copy link
Contributor

There isn't really anything for me to review, we need to wait for @Pujan92 @Skalakid and @MichaelBuhler in this comment:

Setting the plainText as htmlText in the clipboard seems logically wrong to me, but this only looks like the option(setting markdown text in text/plain option) for android. As I am not sure, let me ask for inputs from react-native-live-markdown team.

Issue: When we paste the clipboard content in android which is copied from the other web platform, markdown isn't rendered as text/html type isn't supported in android.

@Skalakid @BartoszGrajdek Could you plz give a look and provide your suggestion?

@Skalakid
Copy link
Contributor

Skalakid commented Jan 7, 2025

Hello, in my opinion, every time a user copies, for example, the google.com link from the message and intentionally chooses the Paste as plain text option, only google.com text should be pasted into the input. In other cases [google.com](https://google.com) should be inserted.

Since Android doesn't support pasting HTML, and there were some changes introduced in this issue, [google.com](https://google.com) should always be an expected result of pasting there. To make it consistent among native devices we can do the similar thing on iOS.

That's how I view it, but it would be better to ask internals from Expensify and the design team about it

Copy link

melvin-bot bot commented Jan 7, 2025

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@Pujan92
Copy link
Contributor

Pujan92 commented Jan 9, 2025

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.

@Pujan92
Copy link
Contributor

Pujan92 commented Jan 13, 2025

On re-review, I see that it makes sense to provide markdown Text as a second argument to setHtml function which will be used for the native. For plain text in a browser, it should be converted from the markdown text. Proposals from @FitseTLT and @prakashbask(considering a proposal in other issue) look similar and LGTM.

@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.

@prakashbask
Copy link
Contributor

@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

paste(plainText);

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.

Copy link

melvin-bot bot commented Jan 14, 2025

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@jasperhuangg
Copy link
Contributor

@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.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 14, 2025
Copy link

melvin-bot bot commented Jan 14, 2025

📣 @prakashbask 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@prakashbask
Copy link
Contributor

@jasperhuangg My understanding on the scope of work is as below. Please confirm
expensify-common -> ExpensiMark library: To be implemented by ExpensiMark team
Expensify App: To be implemented by me once the ExpensiMark team publishes the required changes

@melvin-bot melvin-bot bot added the Overdue label Jan 23, 2025
@Pujan92
Copy link
Contributor

Pujan92 commented Jan 23, 2025

Please confirm expensify-common -> ExpensiMark library: To be implemented by ExpensiMark team

@Skalakid Is this(utility to convert markdown to plain text) something you can help with?

@melvin-bot melvin-bot bot removed the Overdue label Jan 23, 2025
@Skalakid
Copy link
Contributor

@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 expensify-common library and always the contributor whose proposal was chosen, solved the issue in both repositories. The ExpensiMark parser is also a part of the Expensify open source and every one can contribute to it https://github.com/Expensify/expensify-common. So @prakashbask feel free to push the changes to that repo, I can help with a review ;)

@prakashbask
Copy link
Contributor

@Skalakid @Pujan92 I did some analysis on the approach to implement the changes required for markdownToText feature in ExpensiMark parser. Please confirm on the options before I can raise a PR.

Option 1:
Use the existing markdown rules/regex and add a new replacementText function.
markdownToText function will process the input markdown string and call replacementText to covert into plain text. This requires a change in type CommonRule

Option 2:
Simplified version of markdownToTextRules similar to htmlToTextRules
markdownToText function will process the input markdown string and call replacement to covert into plain text

@Skalakid
Copy link
Contributor

@prakashbask, what is the real difference between a markdown text and a result of markdownToText? I think only hyperlinks and inline attachments are parsed differently. Because of that, I think we can just add two small rules and a markdownToText function.

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

@prakashbask
Copy link
Contributor

@Skalakid I was also looking at following scenarios where markdown syntax will be removed when it is converted to plain text

Markdown                Plain Text
# Heading                Heading
_italic_                 italic
*bold*                   bold

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.

@Skalakid
Copy link
Contributor

@prakashbask Right, there are more cases like that. So to not add another layer of complexity to ExpensiMark I prefer a markdown -> HTML -> text solution. @Pujan92 what do you think about it? In my opinion there is no sense of adding another set of rules that will be hard to maintain.

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

@prakashbask
Copy link
Contributor

@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.

@Pujan92
Copy link
Contributor

Pujan92 commented Jan 28, 2025

So to not add another layer of complexity to ExpensiMark I prefer a markdown -> HTML -> text solution. @Pujan92 what do you think about it? In my opinion there is no sense of adding another set of rules that will be hard to maintain.

@Skalakid Agree and it makes sense.

@alexpensify @Pujan92 Let @FitseTLT raise the PR for this issue as he has proposed the markdown -> HTML -> text solution.

@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

@prakashbask
Copy link
Contributor

PR #55932

Copy link

melvin-bot bot commented Feb 6, 2025

⚠️ Looks like this issue was linked to a Deploy Blocker here

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
Status: LOW
Development

No branches or pull requests

8 participants