[HOLD for payment 2024-04-05] HIGH: [Mentions] [$500] Pressingrightarrow key does nothing when mentioning and keeping the cursor after @ in the compose box

[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.50-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
Expensify/Expensify Issue URL:
Issue reported by: @quinthar
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1710108211692399

Action Performed:

1. Open any chat
2. in the composer type a message like Hello @applausetester+mn03@applause.expensifail.com
3. Then move the cursor to the left end
4. Keep pressing "right arrow" key until you reach @
5. After that press "right arrow" again

Expected Result:

Should be able to move to next character

Actual Result:

Pressing "right arrow" does nothing and it stucks there

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

Recording.2835.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01e5a6db314206cef2
• Upwork Job ID: 1767261656990289920
• Last Price Increase: 2024-03-11

Issue Owner

Current Issue Owner: @bfitzexpensify

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[Krishna2323]

Proposal

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

Pressingrightarrow key does nothing when mentioning and keeping the cursor after @ in the compose box

What is the root cause of that problem?

We are using useArrowKeyFocusManager which captures left/right arrow key presses instead of the composer.

App/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx

Line 53 in f4c7378

const [highlightedMentionIndex, setHighlightedMentionIndex] = useArrowKeyFocusManager({

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

Pass disableHorizontalKeys as true to useArrowKeyFocusManager

    const [highlightedMentionIndex, setHighlightedMentionIndex] = useArrowKeyFocusManager({
        isActive: isMentionSuggestionsMenuVisible,
        maxIndex: suggestionValues.suggestedMentions.length - 1,
        shouldExcludeTextAreaNodes: false,
        disableHorizontalKeys: true,
    });

We need to do the same for the emoji suggestions also.

App/src/pages/home/report/ReportActionCompose/SuggestionEmoji.tsx

Line 67 in f4c7378

const [highlightedEmojiIndex, setHighlightedEmojiIndex] = useArrowKeyFocusManager({

Result

[allgandalf]

Proposal

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

Pressing right arrow key does nothing when mentioning and keeping the cursor after @ in the compose box

What is the root cause of that problem?

We actually have a very confusing condition in useArrowKeyFocusManager:
First we set disableHorizontalKeys to false

App/src/hooks/useArrowKeyFocusManager.ts

Line 46 in 9447937

disableHorizontalKeys = false,

Then in horizontalArrowConfig we negate this value:

App/src/hooks/useArrowKeyFocusManager.ts

Lines 58 to 61 in 9447937

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

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

To avoid confusion and have literal meaning of the variable i propose that we set the default value of disableHorizontalKeys= true (This will actually help us in places where we don't have any horizontal elements).

Then update the horizontalArrowConfig as:

    const horizontalArrowConfig = useMemo(
        () => ({
            excludedNodes: shouldExcludeTextAreaNodes ? ['TEXTAREA'] : [],
            isActive: isActive && disableHorizontalKeys,

With the we have the understanding of variable name a lot clear.

Now in SuggestionMention simply pass disableHorizontalKeys=false and we're good to go :)

What alternative solutions did you explore? (Optional)

N/A

[bernhardoj]

Proposal

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

Can't use left/right arrow key to change the selection position when a mention suggestion appears.

What is the root cause of that problem?

In the suggestion component, we use an arrow key manager hook to manage the arrow key, including left and right arrow key.

App/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx

Lines 53 to 57 in b8590c9

	const [highlightedMentionIndex, setHighlightedMentionIndex] = useArrowKeyFocusManager({
	isActive: isMentionSuggestionsMenuVisible,
	maxIndex: suggestionValues.suggestedMentions.length - 1,
	shouldExcludeTextAreaNodes: false,
	});

It will be active when the suggestion is visible. So, when the suggestion is visible all arrow keys are captured by the hook (keyboard shortcut).

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

We already have a way to disable the horizontal key with disableHorizontalKeys prop,

App/src/hooks/useArrowKeyFocusManager.ts

Lines 58 to 61 in b8590c9

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

but instead of disabling it case by case, we should disable it whenever the component doesn't support horizontal arrow navigation, with the help of allowHorizontalArrowKeys.

isActive: isActive && allowHorizontalArrowKeys && !disableHorizontalKeys

App/src/hooks/useArrowKeyFocusManager.ts

Line 48 in b8590c9

const allowHorizontalArrowKeys = !!itemsPerRow;

allowHorizontalArrowKeys is currently true only for emoji picker

[bfitzexpensify]

Few proposals ready for review @eh2077

[eh2077]

@bfitzexpensify Yeah, reviewing proposals

[eh2077]

Thank you all for your proposals!

While all proposals are similar, I'm inclined to go with @Krishna2323 proposal as it first identified the root cause and provided the solution.

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[tgolen]

I had a look at all the proposals, and I'm not convinced yet that any of them is a good 100% solution. I'd like to ask a couple of questions to make sure I am understanding it correctly first:

• How many places is the arrow key manager used and what are their prop settings for the horizontal keys?
• What's the most correct default state for disabling horizontal keys (ie. the majority case)?
• Why is there both allowHorizontalArrowKeys and disableHorizontalKeys? What is the difference? Can this be consolidated into a single setting?

I think some of the proposals touched on this being a bit confusing (and I agree), so let's take the time to really clean this up and make it clear.

[bernhardoj]

How many places is the arrow key manager used and what are their prop settings for the horizontal keys?

Popover menu, emoji picker, attachment picker menu, report context menu, emoji & mention suggestion. Only emoji picker that uses disableHorizontalKeys

What's the most correct default state for disabling horizontal keys (ie. the majority case)?

I would say we should disable it if allowHorizontalArrowKeys is false which is what I propose. Currently, every component mentioned above is subscribed to the left and right arrow keys even though they don't support horizontal navigation. The keyboard shortcut callback already does nothing if allowHorizontalArrowKeys is false.

App/src/hooks/useArrowKeyFocusManager.ts

Lines 171 to 174 in 92ad1cd

	const arrowRightCallback = useCallback(() => {
	if (maxIndex < 0 || !allowHorizontalArrowKeys) {
	return;
	}

Why is there both allowHorizontalArrowKeys and disableHorizontalKeys? What is the difference? Can this be consolidated into a single setting?

disableHorizontalKeys is to manually (forcefully) disable the horizontal navigation from outside of the hook, for example in the emoji picker when the emoji text input is focused.

I think we can consolidate it by having a single prop called allowHorizontalArrowKeys with the default value of false. The horizontal arrow config isActive will be true if allowHorizontalArrowKeys is true.

Then in the emoji picker, we will set allowHorizontalArrowKeys to true if the text input is not focused.

allowHorizontalArrowKeys: !isFocused

If other component needs the horizontal navigation, they just need to use allowHorizontalArrowKeys instead of itemsPerRow (edit: we still need itemsPerRow).

[tgolen]

OK, that really helps. Thank you! I agree with all that you said, and I accept that as a modified proposal if @eh2077 agrees.

Regarding itemsPerRow, I think it's a mistake that it should be used as a proxy for enabling horizontal arrow keys (it sounds like you do too), but I agree itemsPerRow needs to stay there for it's own purpose.

[Krishna2323]

@tgolen, I agree that it would be better to do some modifications but my solution also does work correctly without any issue and diasbleHorizontalKeys prop was introduced for disabling the horizontal keys that's why I purposed to use that.

[tgolen]

Yep, I understand that and I appreciate that you had a nice and simple proposal. I am a pretty firm believer in leaving code in a better place than it was found, so I always appreciate when someone spots something that is confusing and then takes the time to clean it up and make it better.

[allgandalf]

@tgolen , @bernhardoj pretty much explained all your doubts, i have almost similar explanation, an i was the one to point out the confusing names, so do i need to answer your questions still and where do we proceed from here

[Krishna2323]

@tgolen, agree with you on making the code better but that could have in handled in the PR phase since I pointed out the correct root cause and a valid solution. We generally do handle these things in the PR.

[eh2077]

@tgolen Thanks for sharing your thoughts. Yes, I agree with you that we should not only fix it but also clean up the confusing part.

The root cause of this issue is clear. @Krishna2323 first proposed a quick fix solution, while @GandalfGwaihir was the first to point out the confusing prop disableHorizontalKeys. Their proposals are close to success, and I believe there's a lesson to be learned from this experience - Just do a bigger cleanup that is a little larger in scope than the RCA - as suggested by @tgolen

I also agree we should go with @bernhardoj 's proposal, along with their follow-up #38076 (comment), as they provided a consolidated solution to get the code cleaner.

@tgolen All yours.

🎀👀🎀 C+ reviewed

Sorry for the back and forth!

[melvin-bot]

Current assignee @tgolen is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[bernhardoj]

PR is ready

cc: @eh2077

[quinthar]

Hi! What's the next step on this, who is doing it, and when will it be done? /cc @eh2077

[eh2077]

@quinthar C+ has reviewed and completed the checklist of the PR. So, the next step is waiting for internal engineer @tgolen 's review and approval. I think it'll be merged soon when @tgolen is online and gets a chance to look at it.

[quinthar]

@tgolen what's the ETA on this?

[allgandalf]

@tgolen was OOO, the last time i checked

[quinthar]

Hm, why isn't this closed? this was merged a while ago, has it not yet deployed for some reason?

[tgolen]

It was only merged 3 days ago, so it's still waiting to be deployed it looks like. It's in the current deploy checklist, so it should go out soon: #39033

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

• Fix can't use right and left arrow key when a mention suggestion is shown #38383

If no regressions arise, payment will be issued on 2024-04-05. 🎊

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

• @bernhardoj requires payment (Needs manual offer from BZ)
• @eh2077 requires payment (Needs manual offer from BZ)

[melvin-bot]

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:

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

[melvin-bot]

Payment Summary

Upwork Job

• ROLE: @bernhardoj paid $500 via Upwork ✅
• ROLE: @eh2077 paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@bfitzexpensify)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1767261656990289920/hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[eh2077]

Checklist

• [@eh2077] The PR that introduced the bug has been identified. Link to the PR: feat: implement useArrowKeyFocusManager in EmojiPickerMenu #34581
• [@eh2077] 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/34581/files#r1557121091
• [@eh2077] 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
• [@eh2077] Determine if we should create a regression test for this bug. I'm neutral about adding a regression test.
• [@eh2077] 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 steps:

1. Open any chat
2. Type a mention to open up the mention suggestion
3. Press left/right arrow key
4. Verify the text selection moves to left/right

Do you agree with the above test? 👍 or 👎

[bfitzexpensify]

Thanks @eh2077 - agree with those testing steps, and proposed they be added to TestRail in https://github.com/Expensify/Expensify/issues/386521.

Can you please accept the Upwork job and I'll pay it out and then close this issue out as complete?

[eh2077]

@bfitzexpensify Hmm, it's weird that I failed to accept this offer. Can you withdraw it and send me another one?

[bfitzexpensify]

Weird. OK, just re-sent the offer.

[eh2077]

@bfitzexpensify Accepted, thank you!

[bfitzexpensify]

Paid out! Ok, we're all done here, thanks everyone.