[HOLD for payment 2023-12-15] [$500] Chat - A new line is created in the composer after emoji selection with Tab key #31210

kbecciv · 2023-11-10T20:30:01Z

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

Version Number: 1.3.98.0
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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

Navigate to staging.new.expensify.com
Go to any chat.
Click on the emoji picker.
Navigate to the emoji section with Tab key.
Press Enter.

Expected Result:

No new line is created in the composer after emoji selection.

Actual Result:

A new line is created in the composer after emoji selection.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6271766_1699647650902.20231111_031650.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~015985b1c6b3729714
Upwork Job ID: 1723075574402899968
Last Price Increase: 2023-11-24
Automatic offers:

ntdiary | Reviewer | 27901421
s-alves10 | Contributor | 27901425

melvin-bot · 2023-11-10T20:30:06Z

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

melvin-bot · 2023-11-10T20:30:07Z

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

melvin-bot · 2023-11-10T20:30:08Z

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

melvin-bot · 2023-11-10T20:30:12Z

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

s-alves10 · 2023-11-10T21:01:32Z

Proposal

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

A new line is created in the composer when pressing enter key after emoji is selected with Tab key in EmojiPicker

What is the root cause of that problem?

App/src/components/EmojiPicker/EmojiPickerMenu/index.js

Lines 306 to 314 in 3939bea

    
           if (!isEnterWhileComposition(keyBoardEvent) && keyBoardEvent.key === CONST.KEYBOARD_SHORTCUTS.ENTER.shortcutKey && highlightedIndex !== -1) { 
        
               const item = filteredEmojis[highlightedIndex]; 
        
               if (!item) { 
        
                   return; 
        
               } 
        
               const emoji = lodashGet(item, ['types', preferredSkinTone], item.code); 
        
               onEmojiSelected(emoji, item); 
        
               return; 
        
           }

As you can see in the above code segment, we don't prevent default action of 'Enter' key event

This is the root cause

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

We need to prevent default action of 'Enter' key

Add the following code above this line

        keyBoardEvent.preventDefault();

This works fine

Result

31210.mp4

What alternative solutions did you explore? (Optional)

bernhardoj · 2023-11-11T10:17:59Z

Proposal

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

Selecting an emoji using keyboard enter will add the emoji with a new line to the composer.

What is the root cause of that problem?

I don't know the exact root cause of it, but here is what I found.

This issue happens after the react-native-web upgrade. I guess there are some behavioral changes to it that make the new line added when we select an emoji with an ENTER key.

When we select an emoji, it will call replaceSelectionWithText.

App/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.js

Lines 319 to 330 in f0fb7a8

    
           const replaceSelectionWithText = useCallback( 
        
               (text, shouldAddTrailSpace = true) => { 
        
                   const updatedText = shouldAddTrailSpace ? `${text} ` : text; 
        
                   const selectionSpaceLength = shouldAddTrailSpace ? CONST.SPACE_LENGTH : 0; 
        
                   updateComment(ComposerUtils.insertText(commentRef.current, selection, updatedText)); 
        
                   setSelection((prevSelection) => ({ 
        
                       start: prevSelection.start + text.length + selectionSpaceLength, 
        
                       end: prevSelection.start + text.length + selectionSpaceLength, 
        
                   })); 
        
               }, 
        
               [selection, updateComment], 
        
           );

The function will replace the current selection with the text, in our case the emoji itself, with a trailing whitepsace added, no new line, and update it with updateComment.

I also thought that the new line was added because the ENTER key event is not consumed by any element, so the composer input consumes it. But the weird thing is, if I remove the updateComment from replaceSelectionWithText (so the emoji is not added to the composer), the new line is also not added.

There might be some more details to the issue, but this is what I only found.

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

I agree that the preventDefault do the work, but I just want to suggest that we can actually remove the ENTER key listener to select the emoji. Why?

Because pressing enter on the emoji item already triggers the emoji onPress callback that selects the emoji.

App/src/components/EmojiPicker/EmojiPickerMenu/index.js

Line 445 in f0fb7a8

onPress={(emoji) => onEmojiSelected(emoji, item)}

This way, the ENTER key will be consumed by the emoji item pressable.

currently, the emoji onPress is never called when we press enter because the keyDownHandler listener is called earlier than the onPress and the emoji popover is already closed

ntdiary · 2023-11-13T16:18:39Z

There might be some more details to the issue, but this is what I only found.

Agree, this behavior seems a bit weird, which requires some time for review.

ntdiary · 2023-11-14T13:59:07Z

App/src/components/EmojiPicker/EmojiPickerMenu/index.js

Lines 306 to 312 in 82ee577

    
           if (!isEnterWhileComposition(keyBoardEvent) && keyBoardEvent.key === CONST.KEYBOARD_SHORTCUTS.ENTER.shortcutKey && highlightedIndex !== -1) { 
        
               const item = filteredEmojis[highlightedIndex]; 
        
               if (!item) { 
        
                   return; 
        
               } 
        
               const emoji = lodashGet(item, ['types', preferredSkinTone], item.code); 
        
               onEmojiSelected(emoji, item);

The root cause for this issue is that we are calling input.focus in the keydown event handler. onEmojiSelected will close the emoji modal and return focus to the composer input, causing the target of the subsequent keyboard events to shift to the input element. As a result, the Enter key is received by the input element and fire the input event.

I think our code should lean towards standardization, so it would be better to remove the Enter key code rather than using preventDefault. But at the same time, we should also avoid a regression in #16065, user should also be able to select an emoji directly by pressing Enter in the search input of the emoji modal (@bernhardoj, your proposal should table care of this case).

a demo video:

target-shift.mp4

Additionally, there is a double-input problem with the same root cause, but I am not yet certain whether there is a related issue reported. : )

double-input.mp4

cc @kbecciv @anmurali

s-alves10 · 2023-11-14T14:13:13Z

@ntdiary

I think our code should lean towards standardization, so it would be better to remove the Enter key code rather than using preventDefault

I'm not sure why. Preventing default action is widely used to customize the default action of elements. And obviously, we should be able to select emoji by Enter key from the view of UX. If this function isn't needed, I don't think moving highlight by arrow keys isn't needed as well. IMO it looks like a tricky workaround to remove the Enter key code

Expecting your feedback on this

bernhardoj · 2023-11-14T14:46:00Z

user should also be able to select an emoji directly by pressing Enter in the search input of the emoji modal.

Ah, this is the case that would fail if we remove the Enter key listener because when we search, the first item is highlighted, but not focused.

ntdiary · 2023-11-14T15:46:24Z

@ntdiary

I think our code should lean towards standardization, so it would be better to remove the Enter key code rather than using preventDefault

I'm not sure why. Preventing default action is widely used to customize the default action of elements. And obviously, we should be able to select emoji by Enter key from the view of UX. If this function isn't needed, I don't think moving highlight by arrow keys isn't needed as well. IMO it looks like a tricky workaround to remove the Enter key code

Expecting your feedback on this

@s-alves10, eh, I didn't mean to remove the Enter feature. Instead, I'm suggesting fully leveraging React's native event prop to support the Enter feature.
While there are a few preventDefault in our code, in my personal experience, it serves as an alternative when standard support is lacking. If there is a better solution, we should also reduce our reliance on preventDefault.
Certainly, after another solution is refined, I will compare the two proposals again and provide my final recommendation. 🙂

melvin-bot · 2023-11-17T16:00:11Z

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

melvin-bot · 2023-11-20T13:03:21Z

@ntdiary, @anmurali Eep! 4 days overdue now. Issues have feelings too...

melvin-bot · 2023-11-22T12:59:38Z

@ntdiary, @anmurali Still overdue 6 days?! Let's take care of this!

melvin-bot · 2023-11-24T12:52:59Z

@ntdiary @anmurali this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot · 2023-11-24T13:00:23Z

@ntdiary, @anmurali Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

melvin-bot · 2023-11-24T16:00:18Z

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

tsa321 · 2023-11-26T13:34:19Z

Proposal

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

A new line is added if user open emoji picker and press tab to choose emoji then press enter to select/insert emoji in compose box.

What is the root cause of that problem?

App/src/components/EmojiPicker/EmojiPicker.js

Lines 97 to 108 in d985a0e

    
           const selectEmoji = (emoji, emojiObject) => { 
        
               // Prevent fast click / multiple emoji selection; 
        
               // The first click will hide the emoji picker by calling the hideEmojiPicker() function 
        
               if (!isEmojiPickerVisible) { 
        
                   return; 
        
               } 
        
               hideEmojiPicker(false); 
        
               if (_.isFunction(onEmojiSelected.current)) { 
        
                   onEmojiSelected.current(emoji, emojiObject); 
        
               } 
        
           };

When user select emoji (in this case by pressing enter) the hideEmojiPicker(false) will hide the emoji popover and will execute onModalHide:

App/src/pages/home/report/ReportActionCompose/ReportActionCompose.js

Lines 433 to 435 in d985a0e

    
           <EmojiPickerButton 
        
               isDisabled={isBlockedFromConcierge || disabled} 
        
               onModalHide={focus}

which is focus. This function will focus the composer.
When user press enter the modal will hide early and the composer will be focused and catch the enter keydown event and will add new line in composer.

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

We can move the hideEmojiPicker(false); below onEmojiSelected.current(emoji, emojiObject); to hide the popover later and to insert the emoji first. The code could be:

    const selectEmoji = (emoji, emojiObject) => {
        // Prevent fast click / multiple emoji selection;
        // The first click will hide the emoji picker by calling the hideEmojiPicker() function
        if (!isEmojiPickerVisible) {
            return;
        }

-       hideEmojiPicker(false);
        if (_.isFunction(onEmojiSelected.current)) {
            onEmojiSelected.current(emoji, emojiObject);
        }
+      hideEmojiPicker(false);
    };

melvin-bot · 2023-11-28T12:57:54Z

@ntdiary, @anmurali 12 days overdue now... This issue's end is nigh!

anmurali · 2023-11-29T04:39:42Z

@ntdiary have we reviewed the above proposal? Are you ready to pick one or are we still waiting for better proposals?

s-alves10 · 2023-12-01T01:05:48Z

@ntdiary

PR is ready for review #32324

melvin-bot · 2023-12-08T01:03:40Z

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

melvin-bot · 2023-12-08T01:03:45Z

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

fix: prevent default action of Enter key when selecting an emoji #32324

If no regressions arise, payment will be issued on 2023-12-15. 🎊

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

For reference, here are some details about the assignees on this issue:

@ntdiary requires payment automatic offer (Reviewer)
@s-alves10 requires payment automatic offer (Contributor)

melvin-bot · 2023-12-08T01:03:46Z

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:

[@ntdiary] The PR that introduced the bug has been identified. Link to the PR:
[@ntdiary] 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:
[@ntdiary] 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:
[@ntdiary] Determine if we should create a regression test for this bug.
[@ntdiary] 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.
[@anmurali] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

melvin-bot · 2023-12-18T00:38:26Z

@flodnv, @ntdiary, @anmurali, @s-alves10 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

ntdiary · 2023-12-18T11:18:41Z

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:

[@ntdiary] The PR that introduced the bug has been identified. Link to the PR: Upgrade to react-native-web v0.19.9 #24482

[@ntdiary] 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: https://github.com/Expensify/App/pull/24482/files#r1429943408

[@ntdiary] 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: No need

[@ntdiary] Determine if we should create a regression test for this bug. No need

[@ntdiary] 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. No need

[@anmurali] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

It's just a edge case and a minor fix, we've already added comments to the code, so I don't think we need to create a regression test. :)

flodnv · 2023-12-20T16:59:06Z

Agreed 👍

@anmurali please issue payments 🙇

melvin-bot · 2023-12-26T11:11:42Z

@flodnv, @ntdiary, @anmurali, @s-alves10 Eep! 4 days overdue now. Issues have feelings too...

melvin-bot · 2023-12-28T11:14:27Z

@flodnv, @ntdiary, @anmurali, @s-alves10 Still overdue 6 days?! Let's take care of this!

melvin-bot · 2024-01-01T11:20:15Z

@flodnv, @ntdiary, @anmurali, @s-alves10 10 days overdue. I'm getting more depressed than Marvin.

melvin-bot · 2024-01-02T17:15:06Z

@flodnv, @ntdiary, @anmurali, @s-alves10 12 days overdue. Walking. Toward. The. Light...

anmurali · 2024-01-03T00:06:20Z

Paid.

kbecciv added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 10, 2023

melvin-bot bot assigned anmurali Nov 10, 2023

melvin-bot bot changed the title ~~Chat - A new line is created in the composer after emoji selection with Tab key~~ [$500] Chat - A new line is created in the composer after emoji selection with Tab key Nov 10, 2023

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

melvin-bot bot assigned ntdiary Nov 10, 2023

melvin-bot bot added the Overdue label Nov 13, 2023

melvin-bot bot removed the Overdue label Nov 13, 2023

melvin-bot bot added the Overdue label Nov 17, 2023

melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Dec 1, 2023

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Dec 8, 2023

melvin-bot bot changed the title ~~[$500] Chat - A new line is created in the composer after emoji selection with Tab key~~ [HOLD for payment 2023-12-15] [$500] Chat - A new line is created in the composer after emoji selection with Tab key Dec 8, 2023

melvin-bot bot removed the Reviewing Has a PR in review label Dec 8, 2023

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Dec 14, 2023

melvin-bot bot added the Overdue label Dec 18, 2023

ntdiary mentioned this issue Dec 18, 2023

Upgrade to react-native-web v0.19.9 #24482

Merged

59 tasks

melvin-bot bot added Overdue and removed Overdue labels Dec 18, 2023

melvin-bot bot added Overdue and removed Overdue labels Dec 20, 2023

anmurali closed this as completed Jan 3, 2024

melvin-bot bot removed the Overdue label Jan 3, 2024

bernhardoj mentioned this issue Jan 9, 2024

[HOLD for payment 2024-01-31] [$500] Web - Compose box - The compose box displays duplicate characters upon pressing a single key #33710

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD for payment 2023-12-15] [$500] Chat - A new line is created in the composer after emoji selection with Tab key #31210

[HOLD for payment 2023-12-15] [$500] Chat - A new line is created in the composer after emoji selection with Tab key #31210

kbecciv commented Nov 10, 2023 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Nov 10, 2023

melvin-bot bot commented Nov 10, 2023

melvin-bot bot commented Nov 10, 2023

melvin-bot bot commented Nov 10, 2023

s-alves10 commented Nov 10, 2023 •

edited

Loading

bernhardoj commented Nov 11, 2023

ntdiary commented Nov 13, 2023

ntdiary commented Nov 14, 2023 •

edited

Loading

s-alves10 commented Nov 14, 2023 •

edited

Loading

bernhardoj commented Nov 14, 2023

ntdiary commented Nov 14, 2023

melvin-bot bot commented Nov 17, 2023

melvin-bot bot commented Nov 20, 2023

melvin-bot bot commented Nov 22, 2023

melvin-bot bot commented Nov 24, 2023

melvin-bot bot commented Nov 24, 2023

melvin-bot bot commented Nov 24, 2023

tsa321 commented Nov 26, 2023

melvin-bot bot commented Nov 28, 2023

anmurali commented Nov 29, 2023

s-alves10 commented Dec 1, 2023

melvin-bot bot commented Dec 8, 2023

melvin-bot bot commented Dec 8, 2023 •

edited by anmurali

Loading

melvin-bot bot commented Dec 8, 2023

melvin-bot bot commented Dec 18, 2023

ntdiary commented Dec 18, 2023

flodnv commented Dec 20, 2023

melvin-bot bot commented Dec 26, 2023

melvin-bot bot commented Dec 28, 2023

melvin-bot bot commented Jan 1, 2024

melvin-bot bot commented Jan 2, 2024

anmurali commented Jan 3, 2024

[HOLD for payment 2023-12-15] [$500] Chat - A new line is created in the composer after emoji selection with Tab key #31210

[HOLD for payment 2023-12-15] [$500] Chat - A new line is created in the composer after emoji selection with Tab key #31210

Comments

kbecciv commented Nov 10, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Nov 10, 2023

melvin-bot bot commented Nov 10, 2023

melvin-bot bot commented Nov 10, 2023

Bug0 Triage Checklist (Main S/O)

melvin-bot bot commented Nov 10, 2023

s-alves10 commented Nov 10, 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 Nov 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?

ntdiary commented Nov 13, 2023

ntdiary commented Nov 14, 2023 • edited Loading

s-alves10 commented Nov 14, 2023 • edited Loading

bernhardoj commented Nov 14, 2023

ntdiary commented Nov 14, 2023

melvin-bot bot commented Nov 17, 2023

melvin-bot bot commented Nov 20, 2023

melvin-bot bot commented Nov 22, 2023

melvin-bot bot commented Nov 24, 2023

melvin-bot bot commented Nov 24, 2023

melvin-bot bot commented Nov 24, 2023

tsa321 commented Nov 26, 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?

melvin-bot bot commented Nov 28, 2023

anmurali commented Nov 29, 2023

s-alves10 commented Dec 1, 2023

melvin-bot bot commented Dec 8, 2023

melvin-bot bot commented Dec 8, 2023 • edited by anmurali Loading

melvin-bot bot commented Dec 8, 2023

melvin-bot bot commented Dec 18, 2023

ntdiary commented Dec 18, 2023

flodnv commented Dec 20, 2023

melvin-bot bot commented Dec 26, 2023

melvin-bot bot commented Dec 28, 2023

melvin-bot bot commented Jan 1, 2024

melvin-bot bot commented Jan 2, 2024

anmurali commented Jan 3, 2024

kbecciv commented Nov 10, 2023 •

edited by melvin-bot bot

Loading

s-alves10 commented Nov 10, 2023 •

edited

Loading

ntdiary commented Nov 14, 2023 •

edited

Loading

s-alves10 commented Nov 14, 2023 •

edited

Loading

melvin-bot bot commented Dec 8, 2023 •

edited by anmurali

Loading