[$1000] Misleading right-click on email linked to URL

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Open staging.new.expensify.com
2. Send an email linked to a URL (e.g. [test@google.com](google.com) )
3. Right click on the message

Expected Result :

Since the message is an email with a link, the message should be "Copy URL to Clipboard", and after selecting the menu, the URL is copied

Actual Result :

The message is "Copy Email to Clipboard", but after selecting the menu, the URL is copied - this is confusing for the user.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.13.3
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Email.URL.Right.Click.mp4

Recording.608.mp4

Expensify/Expensify Issue URL:
Issue reported by: @kerupuksambel
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1684029611911239

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0185cc75d081384f04
• Upwork Job ID: 1657974195145203712
• Last Price Increase: 2023-05-15

[melvin-bot]

Triggered auto assignment to @jliexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[allroundexperts]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Hyperlinked email still shows Copy email to clipboard menu.

What is the root cause of that problem?

The root cause is that we're just checking if the text matches the email pattern and ignoring what the href looks like here.

What changes do you think we should make in order to solve the problem?

We can replace this with:

ReportActionContextMenu.showContextMenu(
                    props.href.indexOf('mailto:') > -1 && Str.isValidEmailMarkdown(props.displayName)
                        ? ContextMenuActions.CONTEXT_MENU_TYPES.EMAIL
                        : ContextMenuActions.CONTEXT_MENU_TYPES.LINK,
                    event,
                    props.href,
                    lodashGet(linkRef, 'current'),
                );

What alternative solutions did you explore? (Optional)

We can also display both Copy email to clipboard and Copy url to clipboard in such cases by creating a new ContextMenuActions.CONTEXT_MENU_TYPES called ContextMenuActions.CONTEXT_MENU_TYPES.BOTH and calling it if href is a url and display text is an email.

[eh2077]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Right-click menu on email linked to URL shows Copy email to clipboard.

What is the root cause of that problem?

The root cause of this issue is that we determine the type of context menu by props.displayName of BaseAnchorForCommentsOnly here

App/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js

Line 53 in 2e73562

Str.isValidEmailMarkdown(props.displayName) ? ContextMenuActions.CONTEXT_MENU_TYPES.EMAIL : ContextMenuActions.CONTEXT_MENU_TYPES.LINK,

and props.displayName is the text of anchor tag see here

App/src/components/HTMLEngineProvider/HTMLRenderers/AnchorRenderer.js

Line 24 in 2e73562

const displayName = lodashGet(props.tnode, 'domNode.children[0].data', '');

which is buggy because

1. A normal link can also has an email address as the text, like [test@gmail.com](https://google.com)
2. An email can also has normal text as the text, like [this is an email](test@gmail.com)

A markdown link

[test@gmail.com](https://google.com)

is rendered as anchor tag

<a href="https://google.com" dir="auto" rel="noreferrer noopener" target="_blank" role="link" class="css-text-1rynq56" data-testid="a" style="color: rgb(90, 176, 255); font-size: 15px; text-align: left; font-family: ExpensifyNeue-Regular, &quot;Segoe UI Emoji&quot;, &quot;Noto Color Emoji&quot;; line-height: 20px; text-decoration-line: underline; text-decoration-color: rgb(90, 176, 255); user-select: text;">test@gmail.com</a>

So, the value of props.displayName for BaseAnchorForCommentsOnly is test@gmail.com and method Str.isValidEmailMarkdown returns true here

App/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js

Line 53 in 2e73562

Str.isValidEmailMarkdown(props.displayName) ? ContextMenuActions.CONTEXT_MENU_TYPES.EMAIL : ContextMenuActions.CONTEXT_MENU_TYPES.LINK,

That's why right-click menu on email linked to URL shows Copy email to clipboard.

What changes do you think we should make in order to solve the problem?

To fix this issue, we can check if the href property of anchor tag is started with mailto: to determine it's an email address. Note that we format anchor tag for email in ExpensiMark.js, see here and here.

We improve the checking condition here

App/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js

Line 53 in 2e73562

Str.isValidEmailMarkdown(props.displayName) ? ContextMenuActions.CONTEXT_MENU_TYPES.EMAIL : ContextMenuActions.CONTEXT_MENU_TYPES.LINK,

to

props.href.startsWith('mailto:') ? ContextMenuActions.CONTEXT_MENU_TYPES.EMAIL : ContextMenuActions.CONTEXT_MENU_TYPES.LINK,

Below are useful test cases to verify the solution

[test@gmail.com](https://google.com)

[this is an email](test@gmail.com)

[test@gmail.com](https://test.com/mailto:)

What alternative solutions did you explore? (Optional)

N/A

[jliexpensify]

This isn't typical behaviour I've seen, so asking here: https://expensify.slack.com/archives/C049HHMV9SM/p1684113182168159?thread_ts=1684029611.911239&cid=C049HHMV9SM

[melvin-bot]

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

[melvin-bot]

Current assignee @jliexpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @jasperhuangg (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[gabrialva]

Proposal
Please re-state the problem that we are trying to solve in this issue.
Right-click menu on email linked to URL shows Copy email to clipboard.

What is the root cause of that problem?
The root cause of this issue is that the markup-check module in App/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js

isValidEmailMarkdown function of node_modules/expensify-common/str.js was not written against all possible use cases.

This common function checks only if the props.displayName is either Email or Hyperlink, so needs to be improved so that it will display Copy URL to clipboard for email with hyperlinks messages.

What changes do you think we should make in order to solve the problem?
To fix this issue, we can give a minor tweak to the current code like this.

What alternative solutions did you explore? (Optional)
We can show two options like Copy URL to clipboard and Copy Email to clipboard at the same time when right-clicking.

[dukenv0307]

Proposal

Please re-state the problem that we are trying to solve in this issue.

The message is "Copy Email to Clipboard", but after selecting the menu, the URL is copied - this is confusing for the user.

What is the root cause of that problem?

In here

App/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js

Line 53 in f74a43c

Str.isValidEmailMarkdown(props.displayName) ? ContextMenuActions.CONTEXT_MENU_TYPES.EMAIL : ContextMenuActions.CONTEXT_MENU_TYPES.LINK,

, we're only checking if the displayName is an email, if yes we'll show Copy Email to Clipboard. In the case of a link with displayName as an email, it will show incorrectly.

What changes do you think we should make in order to solve the problem?

We need to, additionally check if the href of the link starts with mailto:, if yes then we know for sure it's an email.

In here

App/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js

Line 53 in f74a43c

Str.isValidEmailMarkdown(props.displayName) ? ContextMenuActions.CONTEXT_MENU_TYPES.EMAIL : ContextMenuActions.CONTEXT_MENU_TYPES.LINK,

, we can update to

props.href.startsWith('mailto:') && Str.isValidEmailMarkdown(props.displayName) ? ContextMenuActions.CONTEXT_MENU_TYPES.EMAIL : ContextMenuActions.CONTEXT_MENU_TYPES.LINK,

If we only check if the link contains mailto:, it will not work for normal link that has mailto: somewhere in the link param, for example.

We also shouldn't assume all anchors that starts with mailto: as emails, because if we add an email using link markdown like [email](test@gmail.com), it is still a link, not an email (as confirmed here) and when copying, the user will copy the mailto:... link signature.

What alternative solutions did you explore? (Optional)

If we want to be even stricter in the condition, we can also use the following:

Str.isValidEmailMarkdown(props.displayName) && props.href === `mailto:${props.displayName}`

to make sure it's the auto-email case and not a case of a link with email as href. For example, if we only check if the displayName starts with/contains mailto:, it will fail the following case:
[test@gmail.com](anotherEmail@gmail.com)
Where the user create a link, with displayName as an email and the href also an email. With the strict condition above, it will show correctly as "Copy URL to Clipboard"

[DanutGavrus]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Misleading right-click on email linked to URL.

What is the root cause of that problem?

BaseAnchorForCommentsOnly.js
51            onSecondaryInteraction={(event) => {
52               ReportActionContextMenu.showContextMenu(
53                    Str.isValidEmailMarkdown(props.displayName) ? ContextMenuActions.CONTEXT_MENU_TYPES.EMAIL : ContextMenuActions.CONTEXT_MENU_TYPES.LINK,
54                    event,
55                    props.href,
56                    lodashGet(linkRef, 'current'),
57                );
58            }}

1. props.displayName is a valid email so we pass the EMAIL type, thus the users sees "Copy email" message.
2. We pass props.href as the selection which is going to be copied to the clipboard, thus the user copies the URL.

What changes do you think we should make in order to solve the problem?

I was thinking of some changes to obtain the result in the following video:

18912.mp4

1. Add a new type in CONTEXT_MENU_TYPES such as LINKED_EMAIL: 'LINKED_EMAIL'
2. Replace what we have now in BaseAnchorForCommentsOnly with:

            onSecondaryInteraction={(event) => {
                const isValidEmail = Str.isValidEmailMarkdown(props.displayName);
                if (props.href.length > 0 && isValidEmail) {
                    ReportActionContextMenu.showContextMenu(
                        ContextMenuActions.CONTEXT_MENU_TYPES.LINKED_EMAIL,
                        event,
                        `${props.displayName}"${props.href}`,
                        lodashGet(linkRef, 'current'),
                    );
                } else {
                    ReportActionContextMenu.showContextMenu(
                        isValidEmail ? ContextMenuActions.CONTEXT_MENU_TYPES.EMAIL : ContextMenuActions.CONTEXT_MENU_TYPES.LINK,
                        event,
                        isValidEmail ? props.displayName : props.href,
                        lodashGet(linkRef, 'current'),
                    );
                }
            }}

3. Update the usages inside ContextMenuActions.js such as:

type === CONTEXT_MENU_TYPES.LINK || type === CONTEXT_MENU_TYPES.LINKED_EMAIL
...
type === CONTEXT_MENU_TYPES.EMAIL || type === CONTEXT_MENU_TYPES.LINKED_EMAIL

and use selection.split('"') before setting the string to the Clipboard. " is an invalid character both for emails and URLs, so if users enters any message with " inside it, the right click option in chat will be missing at all, the message appears just like any other text only message as:

If this feels unsafe(shouldn't be), we may adapt showContextMenu to receive an array instead of only a string for the selection.

What alternative solutions did you explore?

As the user may click the link and it auto opens in a new tab, maybe we should let the user copy the email address instead of the URL. Otherwise, he'll have to manually copy the email if he wants to send a message there. So, we may let him see "Copy email" and actually pass the email to be copied inside the Clipboard. For this, we may simply replace:

ReportActionContextMenu.showContextMenu(
53                    Str.isValidEmailMarkdown(props.displayName) ? ContextMenuActions.CONTEXT_MENU_TYPES.EMAIL : ContextMenuActions.CONTEXT_MENU_TYPES.LINK,
54                    event,
55                    props.href, // this
56                    Str.isValidEmailMarkdown(props.displayName) ? props.displayName : props.href, // with this
57                    lodashGet(linkRef, 'current'),
58                );

[jasperhuangg]

@fedirjh Have you had a chance to review these proposals? I like @allroundexperts proposal, but want a second opinion.

[fedirjh]

@jasperhuangg I am currently testing some higher-priority PRs. I will review this one by the end of the day.

[fedirjh]

Thank you, everyone, for the proposals. The proposals are quite similar and share the same solution therefore, I agree with @jasperhuangg, and we should proceed with @allroundexperts' proposal .

🎀 👀 🎀 C+ reviewed

[eh2077]

@fedirjh I think @allroundexperts 's proposal is not perfect enough to address test case like

[test@gmail.com](https://test.com/mailto:)

Can I have your feedback about my proposal?

[allroundexperts]

@fedirjh I think @allroundexperts 's proposal is not perfect enough to address test case like

[test@gmail.com](https://test.com/mailto:)

Can I have your feedback about my proposal?

@eh2077 I think the code in the proposal is meant to give an idea only (that is why it can contain pseudo code). The approach is similar in all other proposals IMO.

[dukenv0307]

@fedirjh @jasperhuangg I believe there're multiple cases that the selected proposal will fail at:

1. [test@gmail.com](https://test.com/mailto:)
2. [test@gmail.com](anotherEmail@gmail.com)
   (In both cases it will show "Copy email" here while the correct one to show is "Copy link", the alternative solution in my proposal works well in both cases)

Also I think the proposals are not the same solutions, I'd appreciate a more thorough reviews here. I believe for small issues, the completeness of the proposal matters.

If we ignore this and accept any proposal with a "generally looks ok" solution (with many failed cases), it will greatly discourage contributors from trying to come up with a completely bug-free and regression-free solutions, which is not a good outcome for the product as a whole I think.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.20-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: show correct copy to clipboard option for linked emails #19599

If no regressions arise, payment will be issued on 2023-06-07. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@allroundexperts / @fedirjh] The PR that introduced the bug has been identified. Link to the PR:
• [@allroundexperts / @fedirjh] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@allroundexperts / @fedirjh] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@allroundexperts / @fedirjh] Determine if we should create a regression test for this bug.
• [@allroundexperts / @fedirjh] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[jliexpensify]

Paying today. PR merged after 5 days, so no bonus.

[jliexpensify]

Everyone has been paid. @fedirjh and @allroundexperts - could you complete the checklist please?

[eh2077]

@jliexpensify Am I eligible for a report bonus for pointing out a related issue about copy email? It's been fixed, see PR comment #19599 (comment) and my proposal.

@fedirjh Would you like to share your thoughts on this? I think you would have full context about what I mentioned as you made a great summary of previous discussions here #18912 (comment)

cc @jasperhuangg

[jliexpensify]

@eh2077 is there a separate reporting GH that was raised that you can link me to?

[fedirjh]

• Link to the PR: Copy email url #8195
• Link to comment: Copy email url #8195 (comment)
• Link to discussion: https://expensify.slack.com/archives/C049HHMV9SM/p1686268547828009
• Regression test ✅

---

Regression Test Proposal

1. Open the App on the web.
2. Send a text message with email address linked to url [email@web.com](https://website.com)
3. Right click over the sent email in the message.
4. Verify that the context menu has an option that says Copy URL to clipboard.
5. Send another text message with text linked to email address [Email](mailto:email@web.com)
6. Right click over the sent email in the message.
7. Verify that the context menu has an option that says Copy Email to Clipboard

• Do we agree 👍 or 👎

[fedirjh]

Would you like to share your thoughts on this?

@jliexpensify I can confirm that @eh2077 made valuable contributions and bring the attention to another case (potential bug) which we ended up fixing in this issue.

[jliexpensify]

Was there a bug-reporting GH that you can link me to?

[eh2077]

@jliexpensify Am I eligible for a report bonus for pointing out a related issue about copy email? It's been fixed, see PR comment #19599 (comment) and my proposal.

@jliexpensify I didn’t file another bug report about it because I was thinking to fix it together in this issue at the beginning. I also mentioned in the slack thread when we first discussed reproduction of the bug, see https://expensify.slack.com/archives/C049HHMV9SM/p1684113923020359?thread_ts=1684029611.911239&cid=C049HHMV9SM.
I hope the links I included and @fedirjh 's last comment will be helpful for you to evaluate the case, thank you!

[jliexpensify]

Thanks for the context, I'm not sure of the process here so I'll surface this internally and get back to you.

[melvin-bot]

@allroundexperts @jliexpensify @jasperhuangg @fedirjh this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is today
• If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[melvin-bot]

Current assignees @allroundexperts and @fedirjh are eligible for the Internal assigner, not assigning anyone new.

[jasperhuangg]

@fedirjh what are the next steps required for us to close out this issue?

[fedirjh]

@jasperhuangg We are waiting for @jliexpensify's decision #18912 (comment) to close this issue. Could you please update the title as the checklist is already filled #18912 (comment).

[jliexpensify]

Apologies, I was OOO yesterday, checking the thread now.

[jliexpensify]

@eh2077 - unfortunately we won't be paying you a reporting bonus here: we'll only pay a bug reporter this bonus if they have initially reported it and it's on a GH. This is our process and it's not something we can deviate from, for fairness. We do appreciate your contributions here and that you've gone above and beyond to help us discover additional issues that were fixed.

In the future, I would encourage you to raise a separate bug report for a GH to be created, this will ensure you'll get the reporting bonus (if applicable).