[$1000] Chat - Copy-pasting emoji names with keyboard shortcut doesn't paste the right emoji name #24399

lanitochka17 · 2023-08-11T00:24:54Z

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

Action Performed:

Open an existing chat with a user
Send message
Apply emoji reaction on the sent message
Right-click on the emjoi you applied on the message
Triple click on the emoji name
Press 'Ctrl + C' to copy the name
Open emoji picker & paste the content to the search field

Expected Result:

The right emoji name should be pasted

Actual Result:

Wrong, long emoji name is pasted

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.53.1

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Screencast.from.2023-08-03.13-55-36.mp4

Recording.2898.mp4

Expensify/Expensify Issue URL:

Issue reported by: @Natnael-Guchima

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1691060569144289

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01de88e41de20d9ce9
Upwork Job ID: 1691744740321337344
Last Price Increase: 2023-08-23

melvin-bot · 2023-08-11T00:24:58Z

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

melvin-bot · 2023-08-11T00:25:00Z

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

jfquevedol2198 · 2023-08-11T01:40:12Z

Proposal

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

Chat - Copy-pasting emoji names with keyboard shortcut doesn't paste the right emoji name

What is the root cause of that problem?

Triple click selects avatar's svg style of nested reaction list

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

App/src/pages/home/report/ReactionList/BaseReactionList.js

Line 78 in c08d588

style={{maxWidth: variables.mobileResponsiveWidthBreakpoint}}

Add styles.userSelectNone in this line

style={{maxWidth: variables.mobileResponsiveWidthBreakpoint, ...styles.userSelectNone}}

Result

Screen.Recording.2023-08-11.at.9.43.20.AM.mov

What alternative solutions did you explore? (Optional)

N/A

bernhardoj · 2023-08-11T08:20:14Z

Proposal

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

When we triple-click the emoji code in the reaction list, the user's default avatar SVG style text also gets copied.

What is the root cause of that problem?

We can easily reproduce this issue by dragging the text selection to also include the avatar and copy it (ctrl/cmd+c), see the below video.

Screen.Recording.2023-08-11.at.15.36.51.mov

When we paste it, we will see the style text of the Avatar svg.

This happens because of our faulty selection logic. The selection is handled by SelectionScraper. When we are doing a selection copy (ctrl/cmd+c), SelectionScraper.getCurrentSelection will be called. To understand it better, we will use the example from the above video, that is selecting the user display name along with the default avatar.

First, it will get the HTML selection (getHTMLOfSelection). We can think of the HTML selection as selecting the HTML tag where our selection is present. In our case, it will select both avatar and display name elements. Nothing is wrong here.

Next, we parse the HTML (parseDocument) to be an object, so we can easily manipulate the HTML attributes, children, etc. The manipulation is done by replaceNodes, which will clear all attributes and only keep the dom data that is a type of text.

App/src/libs/SelectionScraper/index.js

Lines 107 to 112 in dd0d98f

    
           let data; 
        
           // Encoding HTML chars '< >' in the text, because any HTML will be removed in stripHTML method. 
        
           if (dom.type === 'text') { 
        
               data = Str.htmlEncode(dom.data); 
        
           }

The problem is, inline style text is also a type of text, thus the Avatar SVG inline style text content is also included.

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

The solution is to not include the style text content. There is no way to know whether the text is a CSS or a normal text, except from its parent. There are 3 ways to fix this (all in replaceNodes):

Ignore the data (text) if the parent is a style tag

if (dom.type === 'text' && dom.parent.type !== 'style') {
    data = Str.htmlEncode(dom.data);
}

(the final dom will include both style and its content element but without any data)

If the current element is a style tag, ignore its children

App/src/libs/SelectionScraper/index.js

Lines 131 to 133 in dd0d98f

    
           if (dom.children) { 
        
               domChildren = _.map(dom.children, (c) => replaceNodes(c, isChildOfEditorElement || !_.isEmpty(dom.attribs && dom.attribs[tagAttribute]))); 
        
           }

if (dom.children && dom.type !== 'style') {
    domChildren = _.map(dom.children, (c) => replaceNodes(...));
}

(the final dom will include style tag only without children)

If the current element is a style tag, return an empty object (at the 1st line of the function).

if (dom.type === 'style') return {}

(the final dom won't include style tag)

I prefer the 3rd option.

bfitzexpensify · 2023-08-16T09:32:21Z

Reproduced

melvin-bot · 2023-08-16T09:32:34Z

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

melvin-bot · 2023-08-16T09:32:37Z

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

melvin-bot · 2023-08-16T09:32:43Z

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

bfitzexpensify · 2023-08-22T12:25:51Z

@eVoloshchak couple of proposals ready for review here

studentofcoding · 2023-08-22T14:56:25Z

Proposal

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

Copy-pasting emoji names with keyboard shortcut doesn't paste the right emoji name

What is the root cause of that problem?

The function attempts to get the selected text using SelectionScraper.getCurrentSelection() on useCopySelectionHelper.js hooks, but it does not handle the case when an emoji is selected. Instead, it assumes that the selected text is always a non-emoji text.

This leads to incorrect behavior when copying and pasting emoji names, as the function does not properly handle the case when an emoji is selected and tries to convert it to an emoji name.

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

In this solution, we only need to use the getEmojiNameByUnicode function from EmojiUtils to get the emoji name by its Unicode on useCopySelectionHelper.js.

If an emoji name is found, we wrap it with colons : (which is the standard way to represent emojis in text), otherwise, we use the selected text as it is. This ensures that when an emoji is selected and copied, its name is copied instead of the emoji itself.

import {useEffect} from 'react';
import ExpensiMark from 'expensify-common/lib/ExpensiMark';
import CONST from '../CONST';
import KeyboardShortcut from '../libs/KeyboardShortcut';
import Clipboard from '../libs/Clipboard';
import * as EmojiUtils from '../libs/EmojiUtils'; // Add EmojiUtils
import SelectionScraper from '../libs/SelectionScraper';

function copySelectionToClipboard() {
    const selection = SelectionScraper.getCurrentSelection();
    if (!selection) {
        return;
    }
    const parser = new ExpensiMark();
    const selectedEmojiName = EmojiUtils. getEmojiNameByUnicode(selection); //handleEmojiUnicode
    const textToCopy = selectedEmojiName ? `:${selectedEmojiName}:` : parser.htmlToMarkdown(selection); //copied text
    if (!Clipboard.canSetHtml()) {
        Clipboard.setString(textToCopy); //set textToCopy
        return;
    }
    Clipboard.setHtml(selection, parser.htmlToText(selection));
}

export default function useCopySelectionHelper() {
    useEffect(() => {
        const copyShortcutConfig = CONST.KEYBOARD_SHORTCUTS.COPY;
        const unsubscribeCopyShortcut = KeyboardShortcut.subscribe(
            copyShortcutConfig.shortcutKey,
            copySelectionToClipboard,
            copyShortcutConfig.descriptionKey,
            copyShortcutConfig.modifiers,
            false,
        );

        return unsubscribeCopyShortcut;
    }, []);
}

What alternative solutions did you explore? (Optional)

None

Demo

Copy & Paste Just the Name (double-click)

Copy.Pasting.Emoji.Names.Working.mov

Copy & Paste the Name with Colons (triple-click)

Copy.Pasting.Names.Colons.Working.mov

cc: @eVoloshchak

joh42 · 2023-08-22T18:10:29Z

@studentofcoding you need to triple-click so that the colons on each side (and hidden text) are copied. Just copying the text is not broken

studentofcoding · 2023-08-23T01:55:24Z

Noted @joh42, the triple-click (or copy of all the colons) already covered in my solution too ;)

melvin-bot · 2023-08-23T11:35:09Z

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

bfitzexpensify · 2023-08-23T11:35:57Z

Heading ooo for a week - @sakluger, we're still in the proposal review stage here, so no action required from you at the moment. I can take this back over next week when I return.

melvin-bot · 2023-08-23T16:00:20Z

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

sakluger · 2023-08-29T02:04:10Z

@sobitneupane were you able to review proposals?

sobitneupane · 2023-08-29T08:57:54Z

Thanks for the proposal everyone.

Proposal from @bernhardoj looks good to me.

🎀 👀 🎀 C+ reviewed

melvin-bot · 2023-08-29T08:57:59Z

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

studentofcoding · 2023-08-29T09:01:52Z

Thanks for the proposal everyone.

Proposal from @bernhardoj looks good to me.

🎀 👀 🎀 C+ reviewed

Can you share why we have to do such a complicated changes where we only needed to include getEmojiNameByUnicode function like I Proposed ? Thanks

Copy.Pasting.Names.Colons.Working.mov

sobitneupane · 2023-08-29T12:24:09Z

@studentofcoding The issue is not confined to emoji. Please go through the RCA section of the proposal for other instances of the issue.

studentofcoding · 2023-08-29T16:14:55Z

As I recall, the expected behavior should be pasting the Emoji name with the : symbol

And for this could you share why we pursue A manual implementation of dom & nodes (that more like workaround) when we can simply use our EmojiUtils function instead?

Am I missing something @sobitneupane ?

sobitneupane · 2023-08-31T16:46:42Z

And for this could you share why we pursue A manual implementation of dom & nodes (that more like workaround) when we can simply use our EmojiUtils function instead?

@studentofcoding We always try to find the root cause and solve the issue there. Root Cause Analysis in this proposal looks promising. Please let us know if you find any issue with the solution.

studentofcoding · 2023-08-31T17:37:51Z

The thing is

The video demonstration is not on Emoji selection rather than a profile selection, when we only need to use EmojiUtils.getEmojiUnicode to handle the case, as introducing changes into SelectionScraper file might introduce regression & as we might want to keep the behavior on another component

Here is my RCA summary

The function attempts to get the selected text using SelectionScraper.getCurrentSelection() on useCopySelectionHelper.js hooks, but it does not handle the case when an emoji is selected. Instead, it assumes that the selected text is always a non-emoji text.`

But it's just my concern, and whoever gets selected, it's serve the same result

melvin-bot · 2023-09-01T10:26:29Z

@sakluger @sobitneupane @marcaaron @bfitzexpensify this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

marcaaron · 2023-09-01T22:01:43Z

@sakluger @bfitzexpensify this bug feels like a waste of time to me.

sakluger · 2023-09-01T23:14:51Z

@marcaaron I agree, I didn't look too closely at the issue when it was reassigned to me. I'm not sure if anyone actually would copy the emoji name from that menu in real life, and if they did, it would likely be someone who could figure out how to edit the name to work properly.

studentofcoding · 2023-09-02T03:10:44Z

Could you share more why this is a waste of time @marcaaron ?

Also given mind that the user can and possibly do this, that behaviour itself its making this bug valid, no @sakluger ?

Im just want to ask and clarify things as its not clear why its abruptly being closed where this is a valid bug

Thanks

bernhardoj · 2023-09-02T03:56:26Z

Agree with @studentofcoding. The bug itself is actually not specific to selecting the emoji code, but rather our SelectionScrapper logic includes text inside <style> tag into the selection. I see most of our SVG has <style> tag. I think it's low value but it can easily be reproduced by doing text selection.

marcaaron · 2023-09-05T22:06:20Z

I'd give the same reasoning as @sakluger. It's just not really something that a user seems like they would do. I don't think anyone has said it's "invalid" though. The complexity in fixing it don't align with the value we would get by fixing it.

studentofcoding · 2023-09-06T02:28:29Z

I'd give the same reasoning as @sakluger. It's just not really something that a user seems like they would do. I don't think anyone has said it's "invalid" though. The complexity in fixing it don't align with the value we would get by fixing it.

I beg to differ on the complexity in fixing it. In the end its just a simple fix, with using 'getEmojiNameByUnicode' from 'EmojiUtils'

With this in mind, did it really wasting the effort here?

bernhardoj · 2023-09-06T03:02:23Z

@marcaaron The solution is actually very simple, as simple as one line

if (dom.type === 'style') return {}

Even the chance of user copying the emoji name from the reaction list is small, I think the fix worths the value as this issue happen to all text selection.

spcheema · 2023-09-06T03:18:43Z

Apologies in advance if I'm not aligned with anyone's opinion.

I personally don't find this issue valuable to fix since it's not something a user will do in most cases.

I'd say if we keep on fixing such bugs saying simple or small then we will end up with not utilising available resources (C++ & internal team time, then testing team time and of course money is another player in the game).

Since we already reduced bounty price to make open source sustainable for long run.

studentofcoding · 2023-09-06T03:34:01Z

Apologies in advance if I'm not aligned with anyone's opinion.

I personally don't find this issue valuable to fix since it's not something a user will do in most cases.

I'd say if we keep on fixing such bugs saying simple or small then we will end up with not utilising available resources (C++ & internal team time, then testing team time and of course money is another player in the game).

Since we already reduced bounty price to make open source sustainable for long run.

Thats totally ok, and I understand you POV, especially for Issue after the price reduction is announced

But in this case, 1. Its not a part of price reduction, and 2. Its a still a valid bug, and The solution is simple. Its just dont fair for external contributor, to close and change the procedure middle way dont you think?

On another aspect, I think I and @bernhardoj will understand if this issue are decreased to half ($500) as we also have a mechanism for low-priority bug

studentofcoding · 2023-09-06T03:35:07Z

I'll wait for Internal Team to shed a light on this

spcheema · 2023-09-06T04:17:50Z

Well if I am in the same boat and also feel in the same way you guys feel. But it's just not worth it, that's the only view I am having here.

marcaaron · 2023-09-06T19:52:55Z

I'll defer to @sobitneupane as they should be managing this issue and selecting a proposal. The linked proposal has 3 different solutions. There is also various discussion happening and challenges to that proposal.

@sobitneupane Please be decisive, manage this conversation, and offer us a path forward. If the final proposal is clear and looks easy to implement then I will agree to reopen it and get it done.

sobitneupane · 2023-09-07T16:12:48Z

I agree with @marcaaron and @sakluger. I don't think the issue carries much value. The reproduction step is very specific and rare. So, my vote is on closing the issue.

lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 11, 2023

melvin-bot bot assigned bfitzexpensify Aug 11, 2023

melvin-bot bot added the Overdue label Aug 14, 2023

melvin-bot bot removed the Overdue label Aug 16, 2023

bfitzexpensify added the External Added to denote the issue can be worked on by a contributor label Aug 16, 2023

melvin-bot bot changed the title ~~Chat - Copy-pasting emoji names with keyboard shortcut doesn't paste the right emoji name~~ [$1000] Chat - Copy-pasting emoji names with keyboard shortcut doesn't paste the right emoji name Aug 16, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 16, 2023

melvin-bot bot assigned eVoloshchak Aug 16, 2023

melvin-bot bot added the Overdue label Aug 21, 2023

melvin-bot bot removed the Overdue label Aug 22, 2023

bfitzexpensify added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Aug 23, 2023

melvin-bot bot assigned sakluger Aug 23, 2023

Expensify deleted a comment from melvin-bot bot Aug 23, 2023

melvin-bot bot assigned marcaaron Aug 29, 2023

melvin-bot bot added the Engineering label Sep 1, 2023

sakluger closed this as completed Sep 1, 2023

This was referenced Sep 14, 2023

Ignore unselectable text from selection #27421

Merged

Web - Copied text from Invite screen contain names of the avatars when paste to Notepad #27723

Closed

This was referenced Oct 17, 2023

[$500] double click copy emoji text from emoji reaction popup copies extra text #29793

Closed

[HOLD for payment 2023-11-09] [$500] Chat - The HTML code of the avatar is copied when copying the login name #29921

Closed

situchan mentioned this issue Dec 5, 2023

[$500] Emojis - "No results found" message appears when select "Hand" from keyboard suggestion #31780

Closed

6 tasks

[$1000] Chat - Copy-pasting emoji names with keyboard shortcut doesn't paste the right emoji name #24399

[$1000] Chat - Copy-pasting emoji names with keyboard shortcut doesn't paste the right emoji name #24399

Comments

lanitochka17 commented Aug 11, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

melvin-bot bot commented Aug 11, 2023

melvin-bot bot commented Aug 11, 2023

Bug0 Triage Checklist (Main S/O)

jfquevedol2198 commented Aug 11, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

bernhardoj commented Aug 11, 2023

Proposal

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

What is the root cause of that problem?

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

bfitzexpensify commented Aug 16, 2023

melvin-bot bot commented Aug 16, 2023

melvin-bot bot commented Aug 16, 2023

melvin-bot bot commented Aug 16, 2023

bfitzexpensify commented Aug 22, 2023

studentofcoding commented Aug 22, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Demo

joh42 commented Aug 22, 2023

studentofcoding commented Aug 23, 2023 • edited Loading

melvin-bot bot commented Aug 23, 2023

bfitzexpensify commented Aug 23, 2023

melvin-bot bot commented Aug 23, 2023

sakluger commented Aug 29, 2023

sobitneupane commented Aug 29, 2023

melvin-bot bot commented Aug 29, 2023

studentofcoding commented Aug 29, 2023 • edited Loading

sobitneupane commented Aug 29, 2023 • edited Loading

studentofcoding commented Aug 29, 2023

sobitneupane commented Aug 31, 2023

studentofcoding commented Aug 31, 2023 • edited Loading

melvin-bot bot commented Sep 1, 2023

marcaaron commented Sep 1, 2023

sakluger commented Sep 1, 2023

studentofcoding commented Sep 2, 2023

bernhardoj commented Sep 2, 2023

marcaaron commented Sep 5, 2023

studentofcoding commented Sep 6, 2023

bernhardoj commented Sep 6, 2023

spcheema commented Sep 6, 2023 • edited Loading

studentofcoding commented Sep 6, 2023

studentofcoding commented Sep 6, 2023

spcheema commented Sep 6, 2023

marcaaron commented Sep 6, 2023

sobitneupane commented Sep 7, 2023 • edited Loading

lanitochka17 commented Aug 11, 2023 •

edited by melvin-bot bot

Loading

jfquevedol2198 commented Aug 11, 2023 •

edited

Loading

studentofcoding commented Aug 22, 2023 •

edited

Loading

studentofcoding commented Aug 23, 2023 •

edited

Loading

studentofcoding commented Aug 29, 2023 •

edited

Loading

sobitneupane commented Aug 29, 2023 •

edited

Loading

studentofcoding commented Aug 31, 2023 •

edited

Loading

spcheema commented Sep 6, 2023 •

edited

Loading

sobitneupane commented Sep 7, 2023 •

edited

Loading