[$1000] ESC key focuses the input field in emoji picker before closing it

[kavimuru]

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:

1. open emoji picker
2. remove focus by clicking outside
3. now press ESC

Expected results:

should close the emoji picker

Actual results:

re-focuses the search field before closing it

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

bandicam.2023-05-06.21-36-37-911.mp4

Recording.521.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01cd478505a855a06d
• Upwork Job ID: 1655819951666835456
• Last Price Increase: 2023-05-09

[melvin-bot]

Triggered auto assignment to @michaelhaxhiu (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

[huzaifa-99]

Proposal

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

We want to prevent the search field getting focused in emoji picker when its already not focused and escape is pressed.

What is the root cause of that problem?

We are focusing the search input here as a default condition

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

Lines 182 to 184 in ab9171f

	if (this.searchInput && !this.searchInput.isFocused()) {
	this.setState({selectTextOnFocus: false});
	this.searchInput.focus();

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

We should prevent the input getting focused when escape is pressed (and its already not focused) by adding an additional condition keyBoardEvent.key !== 'Escape' here

if (this.searchInput && !this.searchInput.isFocused() && keyBoardEvent.key !== 'Escape') {
    this.setState({selectTextOnFocus: false});
    this.searchInput.focus();

    // Re-enable selection on the searchInput
    this.setState({selectTextOnFocus: true});
}

What alternative solutions did you explore? (Optional)

N/A

[dukenv0307]

Proposal

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

ESC key focuses the input field in emoji picker before closing it

What is the root cause of that problem?

We allow typing in the search box if any key is pressed. But we don't prevent some special keys like command, esc, option, control,...

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

Lines 181 to 190 in ab9171f

	// We allow typing in the search box if any key is pressed apart from Arrow keys.
	if (this.searchInput && !this.searchInput.isFocused()) {
	this.setState({selectTextOnFocus: false});
	this.searchInput.focus();
	// Re-enable selection on the searchInput
	this.setState({selectTextOnFocus: true});
	}
	};

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

We should check if the pressed key is a special key (length > 1) or a key is pressed in combination with Meta, Control, Alt. we will prevent focusing the input like this

             if (this.searchInput.isFocused()) {
                this.setState({selectTextOnFocus: true});
                return;
            }
            // If the key pressed is non-character keys like Enter, Shift, ... do not focus
            if (keyBoardEvent.key.length > 1) {
                this.setState({selectTextOnFocus: false});
                return;
            }

            // If a key is pressed in combination with Meta, Control or Alt do not focus
            if ((keyBoardEvent.metaKey || keyBoardEvent.ctrlKey || keyBoardEvent.altKey)) {
                this.setState({selectTextOnFocus: false});
                return;
            }

            // We allow typing in the search box if any key is pressed apart from Arrow keys.
        
               if (this.searchInput) {
                this.setState({selectTextOnFocus: false});
                this.searchInput.focus();

                // Re-enable selection on the searchInput
                this.setState({selectTextOnFocus: true});
            }
        

What alternative solutions did you explore? (Optional)

[melvin-bot]

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

[michaelhaxhiu]

Re-assigning because I'm headed OOO this week (starting this evening) and need to clear my plate.

[kadiealexander]

Reproduced on the desktop web app:

2023-05-09_18-18-29.mp4

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[dukenv0307]

PROPOSAL

updated

[melvin-bot]

📣 @qsstudioprince! 📣
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. 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.
2. 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.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[qsstudioprince]

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/~014fb8412d7c83935a

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[qsstudioprince]

Proposal

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

Pressing the ESC key in the Emoji menu (when search bar is not focused) focuses the search bar before closing.

What is the root cause of that problem?

The final if statement in keyDownHandler, triggers the search input to focus as the if statement only checks if search input is not focused.

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

When the ESC key is pressed, it should simply execute the default behaviour and close the menu without having to execute any of the actions inside keyDownHandler.
We should add a if statement at the beginning of the handler to check if ESC is pressed and return from the function without executing any of the other if statements.
this.keyDownHandler = (keyBoardEvent) => { if (keyBoardEvent.key === 'Escape') { return; } .... }

What alternative solutions did you explore? (Optional)

I explored the other proposals mentioned above which works as well. However, I feel there is no need to go through all the if statements if ESC is pressed.

[thesahindia]

I am not sure about the expected behaviour, so I will ask about it on Slack after completing other tasks.

[chiragxarora]

I'm very sure this is introduced with a recent merge since at the time of reporting, it was only present in staging and not in production. I mentioned it in the bug report too but I think that was missed by the OP while creating issue.
original bug report

Can also be seen in the video attached in bug report by me

[thesahindia]

I'm very sure this is introduced with a recent merge since at the time of reporting, it was only present in staging and not in production. I mentioned it in the bug report too but I think that was missed by the OP while creating issue. original bug report

Can also be seen in the video attached in bug report by me

Thanks for pointing that out! Before moving forward, we will have to find the PR that caused this (to prevent introducing any regressions)

[dukenv0307]

@thesahindia This PR causes this bug.
Before: We always subscribe the ESC key to close modal

So that, the modal is closed before the key down listener is triggered here

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

Lines 181 to 190 in ab9171f

	// We allow typing in the search box if any key is pressed apart from Arrow keys.
	if (this.searchInput && !this.searchInput.isFocused()) {
	this.setState({selectTextOnFocus: false});
	this.searchInput.focus();
	// Re-enable selection on the searchInput
	this.setState({selectTextOnFocus: true});
	}
	};

--> this bug doesn't happen

Now: After This PR is merged, we only subscribe ESC key when this.props.isShortcutsModalOpen === true. So in this bug, the app doesn't subscribe ESC key and then this code is triggered before the modal closed

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

Lines 181 to 190 in ab9171f

	// We allow typing in the search box if any key is pressed apart from Arrow keys.
	if (this.searchInput && !this.searchInput.isFocused()) {
	this.setState({selectTextOnFocus: false});
	this.searchInput.focus();
	// Re-enable selection on the searchInput
	this.setState({selectTextOnFocus: true});
	}
	};

[dukenv0307]

So IMO, the RCA is we don't prevent subscribing the special key in the emoji search input (It's unnecessary)

In the main composer, we also don't focus if a key is pressed in combination with Meta, Control or Alt or If the key pressed is non-character keys like Enter, Shift (see this PR)

[techievivek]

I am not sure if this is worth fixing because it seems to close the emoji picker just fine.

[dukenv0307]

@techievivek Could you see my proposal? I think we still should prevent focus to search input when the user type Enter, Esc, Cmd, Shift Key,... or a key is pressed in combination with Meta, Control or Alt like in the main composer in this PR
cc @thesahindia

[chiragxarora]

I am not sure if this is worth fixing because it seems to close the emoji picker just fine.

I'm not sure if this is the right reason to close the issue since its FOCUSING search composer when user intends to CLOSE the picker which is quite contradictory in itself. Also, that brief flash doesn't look user friendly at all and lastly, its a something we did not intend and is found in regression
cc @thesahindia

[dukenv0307]

@techievivek I just created new bug on Slack to catch this bug