[HOLD for payment 2024-02-22][$500] The back and forward keys are not working properly for navigating back to a characters

[m-natarajan]

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.4.38-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:

1. Open the emoji picker
2. Enter characters in the search field
3. Use the back and forward keys

Expected Result:

The back and forward keys should navigate characters entered in the emoji picker's search field

Actual Result:

The back and forward keys are not working properly for navigating back to a character while in use

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

Bug6370743_1707330233368.2024-02-07_22-24-58.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a4bbd031958bf120
• Upwork Job ID: 1755301304657534976
• Last Price Increase: 2024-02-07
• Automatic offers:
• jjcoffee | Reviewer | 0
• Krishna2323 | Contributor | 0

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[ZhenjaHorbach]

Proposal

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

The back and forward keys are not working properly for navigating back to a characters

What is the root cause of that problem?

The main problem with issue is that on EmojiPickerMenu screen we are using useArrowKeyFocusManager which is disabled using arrow keys when index of element is -1
And when input is active,useArrowKeyFocusManager does not allow us to interact with the input

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

To fix this issue we can update useArrowKeyFocusManager and add new param enabledKeyboards and pass isActive param
And when we have focused input we will have only enabled the up and down button in ArrowKeyFocusManager because enabledKeyboards: [CONST.KEYBOARD_SHORTCUTS.ARROW_UP, CONST.KEYBOARD_SHORTCUTS.ARROW_DOWN]
From the other side left and right will be disabled for the emoji list
As a result, events will work as intended

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

Lines 103 to 111 in 82fa188

	const [focusedIndex, setFocusedIndex] = useArrowKeyFocusManager({
	maxIndex: filteredEmojis.length - 1,
	// Spacers indexes need to be disabled so that the arrow keys don't focus them. All headers are hidden when list is filtered
	disabledIndexes,
	itemsPerRow: CONST.EMOJI_NUM_PER_ROW,
	initialFocusedIndex: -1,
	disableCyclicTraversal: true,
	onFocusedIndexChange,
	});

    const [focusedIndex, setFocusedIndex] = useArrowKeyFocusManager({
        maxIndex: filteredEmojis.length - 1,
        // Spacers indexes need to be disabled so that the arrow keys don't focus them. All headers are hidden when list is filtered
        disabledIndexes,
        itemsPerRow: CONST.EMOJI_NUM_PER_ROW,
        initialFocusedIndex: -1,
        disableCyclicTraversal: true,
        onFocusedIndexChange,
        enabledKeyboards: [CONST.KEYBOARD_SHORTCUTS.ARROW_UP, CONST.KEYBOARD_SHORTCUTS.ARROW_DOWN],
        isActive: !isFocused,
    });

And then we need update useArrowKeyFocusManager

Add enabledKeyboards = CONST.EMPTY_ARRAY in params

Update arrowConfig

const arrowConfig = useCallback(
    (isAlwaysActive?: boolean) => ({
        excludedNodes: shouldExcludeTextAreaNodes ? ['TEXTAREA'] : [],
        isActive: isAlwaysActive || isActive,
    }),
    [isActive, shouldExcludeTextAreaNodes],
);

App/src/hooks/useArrowKeyFocusManager.ts

Lines 47 to 53 in 61287bc

	const arrowConfig = useMemo(
	() => ({
	excludedNodes: shouldExcludeTextAreaNodes ? ['TEXTAREA'] : [],
	isActive,
	}),
	[isActive, shouldExcludeTextAreaNodes],
	);

And update all useKeyboardShortcut

For example CONST.KEYBOARD_SHORTCUTS.ARROW_UP

  useKeyboardShortcut(
    CONST.KEYBOARD_SHORTCUTS.ARROW_UP,
    arrowUpCallback,
    arrowConfig(enabledKeyboards.includes(CONST.KEYBOARD_SHORTCUTS.ARROW_UP)),
);


https://github.com/Expensify/App/blob/61287bc4e41c39f25a433bfbe2ba063d904dd111/src/hooks/useArrowKeyFocusManager.ts#L87

What alternative solutions did you explore? (Optional)

NA

[Krishna2323]

Proposal

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

The back and forward keys are not working properly for navigating back to a characters

What is the root cause of that problem?

Inside EmojiPickerMenuWithRef we use useArrowKeyFocusManager which catches the arrow key presses and focuses on the emojis instead of changing the input selection.

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

We need to disable the horizontal key presses when input is focused and we have some text in input. This way we can still use left/right arrow keys if the input is empty.

Steps to solve the bug:

1. Introduce a state for storing input text.

const [inputText, setInputText] = useState('');

2. Introduce a new parameter inside useArrowKeyFocusManager called shouldDisableHorizontalKeys and set initial value to false.

disableHorizontalKeys?:boolean;
disableHorizontalKeys = false,

3. Create a new config for horizontal arrow key presses, arrowConfigHorizontalKeys.

    const arrowConfigHorizontal = useMemo(
        () => ({
            excludedNodes: shouldExcludeTextAreaNodes ? ['TEXTAREA'] : [],
            isActive: isActive && !disableHorizontalKeys,
        }),
        [isActive, shouldExcludeTextAreaNodes, disableHorizontalKeys],
    );

4. Use arrowConfigHorizontalKeys for ARROW_LEFT & ARROW_RIGHT.

useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ARROW_LEFT, arrowLeftCallback, arrowConfigHorizontal);
useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ARROW_RIGHT, arrowRightCallback, arrowConfigHorizontal);

5. From EmojiPickerMenu pass disableHorizontalKeys: isFocused && inputText.length to useArrowKeyFocusManager.

disableHorizontalKeys: isFocused && inputText.length,

Result

arrow_keys.mp4

[jjcoffee]

Reviewing tomorrow!

[jjcoffee]

While both proposals are similar, I prefer @Krishna2323's proposal as it offers a much clearer solution.

One thing though, I think we're not so bothered about the right arrow key navigating from the text input to the first emoji, so we may be able to just leave out the useState parts of the proposal that enable that when the text input is empty. Apart from reducing reliance on state, this also ensures consistent behaviour since otherwise you'd be able to navigate to the emojis using the right arrow only when the input is empty, which could be a little confusing.

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[amyevans]

Assigning @Krishna2323, and +1 to @jjcoffee's note in #36059 (comment)

[melvin-bot]

📣 @jjcoffee 🎉 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]

📣 @Krishna2323 🎉 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 📖

[Krishna2323]

@jjcoffee PR ready for review :)

[Krishna2323]

PR was deployed to production on 15th Feb, this should be ready for payments tomorrow, I guess.

cc: @miljakljajic

[jjcoffee]

• The PR that introduced the bug has been identified. Link to the PR: feat: implement useArrowKeyFocusManager in EmojiPickerMenu #34581
• 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: feat: implement useArrowKeyFocusManager in EmojiPickerMenu #34581 (comment)
• 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: N/A - just a missed test case
• Determine if we should create a regression test for this bug. Yes
• 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.

Regression Test Proposal

1. Open the emoji picker
2. Enter characters in the search field
3. Type some text in the search input field
4. Verify that you can use the left/right arrow keys to move the text cursor
5. While still focused on the search input use the down key & verify that the focus moves from input to emoji
6. Verify that you can use left/right, up/down keys to navigate on the emojis

Do we agree 👍 or 👎

[melvin-bot]

@amyevans, @jjcoffee, @miljakljajic, @Krishna2323 Eep! 4 days overdue now. Issues have feelings too...

[Krishna2323]

@miljakljajic can you pls start the payment process?

[jjcoffee]

@miljakljajic Friendly bump for payment 🙇

[miljakljajic]

Paid - so sorry for the hold up