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

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

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.

https://github.com/Expensify/App/blob/92ad1cd2135683b2a0fe4c9b12cb6cd78fbfeaa7/src/hooks/useArrowKeyFocusManager.ts#L171-L174

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 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 https://github.com/Expensify/App/issues/38076#issuecomment-1994680414, as they provided a consolidated solution to get the code cleaner.

@tgolen All yours.

🎀👀🎀 C+ reviewed

Sorry for the back and forth!

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

@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