[HOLD for payment 2024-11-07] [$250] Web/Desktop - Emoji Picker - Nothing happens when user press "Enter" on emoji category icon

[lanitochka17]

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: 9.0.47-1
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5072091
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

1. Open Desktop App or go to staging.new.expensify.com
2. Login with any account
3. Go to any chat
4. Click Emoji icon in Composer
5. Use "Tab" key to select any emoji category icon
6. Press "Enter" key

Expected Result:

The emoji list scrolls to the selected category

Actual Result:

Nothing happens when user press "Enter" key on emoji category icon

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6630237_1728526095969.Web-Emoji-Picker-Category-Select-with-keys.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021846197861822341766
• Upwork Job ID: 1846197861822341766
• Last Price Increase: 2024-10-15
• Automatic offers:
• ahmedGaber93 | Reviewer | 104450544

Issue Owner

Current Issue Owner: @trjExpensify

[melvin-bot]

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

[lanitochka17]

@trjExpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[bernhardoj]

Proposal

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

Pressing Enter doesn't do anything on the emoji category button.

What is the root cause of that problem?

In the emoji picker menu code, we have a keydown listener which calls prevent default when we press ENTER.

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

Lines 142 to 148 in 8f5ba1e

	if (!isEnterWhileComposition(keyBoardEvent) && keyBoardEvent.key === CONST.KEYBOARD_SHORTCUTS.ENTER.shortcutKey) {
	// On web, avoid this Enter default input action; otherwise, it will add a new line in the subsequently focused composer.
	keyBoardEvent.preventDefault();
	// On mWeb, avoid propagating this Enter keystroke to Pressable child component; otherwise, it will trigger the onEmojiSelected callback again.
	keyBoardEvent.stopPropagation();
	return;
	}

Then, we also have an ENTER shortcut which also enables the prevent default.

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

Lines 165 to 188 in 8f5ba1e

	useKeyboardShortcut(
	CONST.KEYBOARD_SHORTCUTS.ENTER,
	(keyBoardEvent) => {
	if (!(keyBoardEvent instanceof KeyboardEvent) || isEnterWhileComposition(keyBoardEvent) || keyBoardEvent.key !== CONST.KEYBOARD_SHORTCUTS.ENTER.shortcutKey) {
	return;
	}
	// Select the currently highlighted emoji if enter is pressed
	let indexToSelect = focusedIndex;
	if (highlightFirstEmoji) {
	indexToSelect = 0;
	}
	const item = filteredEmojis.at(indexToSelect);
	if (indexToSelect === -1 || !item) {
	return;
	}
	if ('types' in item || 'name' in item) {
	const emoji = typeof preferredSkinTone === 'number' && preferredSkinTone !== -1 && item?.types?.at(preferredSkinTone) ? item.types.at(preferredSkinTone) : item.code;
	onEmojiSelected(emoji ?? '', item);
	}
	},
	{shouldPreventDefault: true, shouldStopPropagation: true},
	);

The code in the shortcut was previously in the keydown listener. So, the issue is, that we always prevent default when ENTER without checking whether there is any emoji currently being highlighted or not. So, pressing ENTER on the emoji category does nothing.

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

Remove the prevent default and stop propagation from the keydown listener,

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

Lines 142 to 148 in 8f5ba1e

	if (!isEnterWhileComposition(keyBoardEvent) && keyBoardEvent.key === CONST.KEYBOARD_SHORTCUTS.ENTER.shortcutKey) {
	// On web, avoid this Enter default input action; otherwise, it will add a new line in the subsequently focused composer.
	keyBoardEvent.preventDefault();
	// On mWeb, avoid propagating this Enter keystroke to Pressable child component; otherwise, it will trigger the onEmojiSelected callback again.
	keyBoardEvent.stopPropagation();
	return;
	}

and move it inside the ENTER shortcut and call it if there is a highlighted emoji, just like how it was before.

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

Lines 178 to 187 in 8f5ba1e

	const item = filteredEmojis.at(indexToSelect);
	if (indexToSelect === -1 || !item) {
	return;
	}
	if ('types' in item || 'name' in item) {
	const emoji = typeof preferredSkinTone === 'number' && preferredSkinTone !== -1 && item?.types?.at(preferredSkinTone) ? item.types.at(preferredSkinTone) : item.code;
	onEmojiSelected(emoji ?? '', item);
	}
	},
	{shouldPreventDefault: true, shouldStopPropagation: true},

And set the shouldPreventDefault to false and remove shouldStopPropagation.

After applying the solution, we still have another case to fix as mentioned here.
For that case, when we search for emoji, we will highlight the first emoji (setHighlightFirstEmoji).

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

Lines 117 to 128 in 96ffbfe

	if (normalizedSearchTerm === '') {
	// There are no headers when searching, so we need to re-make them sticky when there is no search term
	setFilteredEmojis(allEmojis);
	setHeaderIndices(headerRowIndices);
	setFocusedIndex(-1);
	setHighlightEmoji(false);
	return;
	}
	// Remove sticky header indices. There are no headers while searching and we don't want to make emojis sticky
	setFilteredEmojis(newFilteredEmojiList ?? []);
	setHeaderIndices([]);
	setHighlightFirstEmoji(true);

However, when clearing the search keyword, we don't reset the highlight state. So, when pressing enter, the index item will be 0. And because index 0 is a header, no emoji will be selected and prevent default is still called.

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

Lines 173 to 185 in 96ffbfe

	let indexToSelect = focusedIndex;
	if (highlightFirstEmoji) {
	indexToSelect = 0;
	}
	const item = filteredEmojis.at(indexToSelect);
	if (indexToSelect === -1 || !item) {
	return;
	}
	if ('types' in item || 'name' in item) {
	const emoji = typeof preferredSkinTone === 'number' && preferredSkinTone !== -1 && item?.types?.at(preferredSkinTone) ? item.types.at(preferredSkinTone) : item.code;
	onEmojiSelected(emoji ?? '', item);
	}

To fix it, we should reset the highlight first state back when clearing the search keyword here.

setHighlightFirstEmoji(false);

And, we can move the prevent default inside the if,

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

Lines 182 to 185 in 96ffbfe

	if ('types' in item || 'name' in item) {
	const emoji = typeof preferredSkinTone === 'number' && preferredSkinTone !== -1 && item?.types?.at(preferredSkinTone) ? item.types.at(preferredSkinTone) : item.code;
	onEmojiSelected(emoji ?? '', item);
	}

so we will only prevent the default when an emoji will be selected.

[trjExpensify]

Why was it moved in the first place?

[trjExpensify]

Looking at the regression test linked in the OP, it's a bit ambiguous that pressing enter there should jump to the emoji category:

1. Open the emoji picker
2. Press the down arrow key multiple times to reach the ""Animals & Nature"" category
3. Verify the highlighted emoji is visible on the screen with every key press.
4. Press the up arrow key multiple times to reach the top row of the emojis
5. Verify the highlighted emoji is visible on the screen with every key press.
6. Double click any emoji in the list
7. Verify that the emoji is inserted only once in the composer
8. Send the emoji
9. Hover over the emoji in the conversation history
10. Verify tooltip displays an enlarged emoji and the emoji name

[bernhardoj]

Why was it moved in the first place?

It's to fix this issue: #47272
The reason is explained here: #47272 (comment)

Basically, we have 2 ENTER shortcuts (Form and Emoji Picker) which are subscribed in a different way.

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[trjExpensify]

Hm okay, CC: @Krishna2323 @rayane-djouah for vis that the fix for that apparently broke this.

[trjExpensify]

@ahmedGaber93 can you review this proposal, please? Thanks!

[siddhakdak]

can i start work on this issue..

[siddhakdak]

/assign

[ahmedGaber93]

@siddhakdak you can read the guideline to know how you can start.
https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md
https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md#propose-a-solution-for-the-job

[ahmedGaber93]

@bernhardoj Thanks for the proposal
Your RCA seem correct, and the solution works good, but can you check this case before move forward.

1. Open the picker and type any letter, e.g. "s" then remove it (confirm the mouse pointer is out of the picker and not hover on any emoji while reproducing)
2. Try to reproduce the issue

20241015185548339.mp4

[bernhardoj]

Updated my proposal to fix that

[ahmedGaber93]

@bernhardoj's proposal LGTM!

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

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

Offer link
Upwork job

[bernhardoj]

PR is ready

cc: @ahmedGaber93

[aldo-expensify]

@trjExpensify , I think we should also include this in the scope of this issue: #50971 (comment), otherwise just fixing the ENTER in the category is a bit useless.

[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 9.0.55-10 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix pressing enter doesn't select the emoji category #50971

If no regressions arise, payment will be issued on 2024-11-07. 🎊

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

• @ahmedGaber93 requires payment automatic offer (Reviewer)
• @bernhardoj requires payment through NewDot Manual Requests

[melvin-bot]

@ahmedGaber93 @trjExpensify The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[ahmedGaber93]

BugZero Checklist:

• [Contributor] Classify the bug:

Bug classification

Source of bug:

• 1a. Result of the original design (eg. a case wasn't considered)
• 1b. Mistake during implementation
• 1c. Backend bug
• 1z. Other:

Where bug was reported:

• 2a. Reported on production
• 2b. Reported on staging (deploy blocker)
• 2c. Reported on a PR
• 2z. Other:

Who reported the bug:

• 3a. Expensify user
• 3b. Expensify employee
• 3c. Contributor
• 3d. QA
• 3z. Other:

• [Contributor] 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/47761/files#r1831768672

• [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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: N/A

• [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

Regression Test Proposal Template

• [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

  Link to issue:

Regression Test Proposal

Test:

1. Open any chat
2. Press the emoji picker
3. Type any character on the search and then remove it
4. Press TAB to focus on the emoji category
5. Press ENTER to select the category
6. Verify the emoji picker is scrolled to the pressed category
7. Press Downwards Arrow
8. Verify the first emoji on the selected category is focused

Do we agree 👍 or 👎