[HOLD for payment 2023-10-30] [HOLD for payment 2023-10-27] [$250] Chat - First row emojis are skipped

[kbecciv]

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.87.8
Reproducible in staging?: y
Reproducible in production?: n
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:@s-alves10
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1697753213411739

Action Performed:

1. Open ND and log in with any account
2. Open any chat and tap emoji button in the main composer to open EmojiPicker
3. Type any characters in search input(for example, he)
4. Press arrow down key several times and try to highlight first row emojis by pressing arrow down or arrow left keys
5. Verify that first row emojis are skipped

Expected Result:

First row emojis can be highlighted by arrow keys

Actual Result:

First row emojis are skipped

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

Android: Native

Android: mWeb Chrome

android_chrome.1.mp4

iOS: Native

iOS: mWeb Safari

ios_safari.1.mp4

MacOS: Chrome / Safari

mac_chrome.mp4

Recording.5093.mp4

MacOS: Desktop

mac_desktop.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01ed51d25e4548ec9f
• Upwork Job ID: 1715386754082848768
• Last Price Increase: 2023-10-20
• Automatic offers:
• fedirjh | Reviewer | 27296434
• s-alves10 | Contributor | 27296440

[OSBotify]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

Triggered auto assignment to @AndrewGable (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[s-alves10]

Proposal

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

Can't highlight first row emojis by arrow keys when searching emojis in Emoji Picker

What is the root cause of that problem?

When moving highlight by arrow keys, we check the bound by the following code

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

Line 257 in 18acd82

() => highlightedIndex - 1 < firstNonHeaderIndex.current,

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

Line 271 in 18acd82

() => highlightedIndex - CONST.EMOJI_NUM_PER_ROW < firstNonHeaderIndex.current,

When searching emojis, firstNonHeaderIndex.current should be 0 because there are no spaces or headers.
But this value is updated by

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

Lines 364 to 379 in 18acd82

	useEffect(() => {
	// This callback prop is used by the parent component using the constructor to
	// get a ref to the inner textInput element e.g. if we do
	// <constructor ref={el => this.textInput = el} /> this will not
	// return a ref to the component, but rather the HTML element by default
	if (shouldFocusInputOnScreenFocus && forwardedRef && _.isFunction(forwardedRef)) {
	forwardedRef(searchInputRef.current);
	}
	setupEventHandlers();
	updateFirstNonHeaderIndex(emojis.current);
	return () => {
	cleanupEventHandlers();
	};
	}, [forwardedRef, shouldFocusInputOnScreenFocus, cleanupEventHandlers, setupEventHandlers]);

every time its dependency changes

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

Line 374 in 18acd82

updateFirstNonHeaderIndex(emojis.current);

That is, updateFirstNonHeaderIndex function is called with the entire emojis every time highlight index is changed. So the value of firstNonHeaderIndex.current becomes 8
This is the root cause

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

updateFirstNonHeaderIndex function should be called with the entire emojis only on mount and when search input is empty.
We do this when search input is empty here

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

Line 180 in 18acd82

updateFirstNonHeaderIndex(emojis.current);

Remove this line and call the function on mount

    useEffect(() => {
        updateFirstNonHeaderIndex(emojis.current);
    }, []);

This works as expected

What alternative solutions did you explore? (Optional)

[kbecciv]

Proposal by:@s-alves10
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1697753213411739

Proposal

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

Can't highlight first row emojis by arrow keys when searching emojis in Emoji Picker

What is the root cause of that problem?

When moving highlight by arrow keys, we check the bound by the following code

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

Line 257 in 18acd82

() => highlightedIndex - 1 < firstNonHeaderIndex.current,

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

Line 271 in 18acd82

() => highlightedIndex - CONST.EMOJI_NUM_PER_ROW < firstNonHeaderIndex.current,

When searching emojis, firstNonHeaderIndex.current should be 0 because there are no spaces or headers.
But this value is updated by

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

Lines 364 to 379 in 18acd82

	useEffect(() => {
	// This callback prop is used by the parent component using the constructor to
	// get a ref to the inner textInput element e.g. if we do
	// <constructor ref={el => this.textInput = el} /> this will not
	// return a ref to the component, but rather the HTML element by default
	if (shouldFocusInputOnScreenFocus && forwardedRef && _.isFunction(forwardedRef)) {
	forwardedRef(searchInputRef.current);
	}
	setupEventHandlers();
	updateFirstNonHeaderIndex(emojis.current);
	return () => {
	cleanupEventHandlers();
	};
	}, [forwardedRef, shouldFocusInputOnScreenFocus, cleanupEventHandlers, setupEventHandlers]);

every time its dependency changes

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

Line 374 in 18acd82

updateFirstNonHeaderIndex(emojis.current);

That is, updateFirstNonHeaderIndex function is called with the entire emojis every time highlight index is changed. So the value of firstNonHeaderIndex.current becomes 8
This is the root cause

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

updateFirstNonHeaderIndex function should be called with the entire emojis only on mount and when search input is empty.
We do this when search input is empty here

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

Line 180 in 18acd82

updateFirstNonHeaderIndex(emojis.current);

Remove this line and call the function on mount

    useEffect(() => {
        updateFirstNonHeaderIndex(emojis.current);
    }, []);

This works as expected

What alternative solutions did you explore? (Optional)

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

Upwork job price has been updated to $250

[melvin-bot]

📣 @fedirjh 🎉 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

[melvin-bot]

📣 @s-alves10 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[mountiny]

Lowering the amount since this is lower value bug and accessibility feature as well which we dont technically support yet

[ghost]

Dibs

[s-alves10]

@fedirjh

PR is ready for review #30092

[situchan]

@s-alves10 what's offending PR?

[s-alves10]

Looks like #29241 is related

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

• fix: update first emoji index on mount #30092

If no regressions arise, payment will be issued on 2023-10-30. 🎊

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:

• @fedirjh requires payment offer (Reviewer)
• @s-alves10 requires payment offer (Contributor)

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

[fedirjh]

BugZero Checklist:

• Link to the PR: Refactor: Migrate EmojiPickerMenu Class component to Functional Component #29241
• Link to comment: Refactor: Migrate EmojiPickerMenu Class component to Functional Component #29241 (comment)
• Link to discussion: N/A I don't think there is something we can add to the checklist to make it easier to catch this bug.
• Determine if we should create a regression test for this bug: N/A this is a minor accessibility issue.

[melvin-bot]

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

[melvin-bot]

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

[AndrewGable]

@joekaufmanexpensify - Can you pay out? Thanks

[joekaufmanexpensify]

Sure!

[joekaufmanexpensify]

This PR qualified for a speed bonus, this means we need to make the following payments:

• @s-alves10 - $425 ($250 for PR + $125 speed bonus + $50 for reporting the bug). Paid via Upwork.
• @fedirjh - $375 ($250 for C+ review + $125 speed bonus). Paid via Upwork.

[joekaufmanexpensify]

@s-alves10 $425 sent and contract ended!

[joekaufmanexpensify]

@fedirjh $375 sent and contract ended!

[joekaufmanexpensify]

Upwork job closed.

[joekaufmanexpensify]

Closing as this is all set. Thanks everyone!