[HOLD for payment 2024-03-25] [$500] Message action menu - Last option is not selected when pressing up key

[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: v1.4.39-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: https://expensify.testrail.io/index.php?/tests/view/4294106
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. Right click on owned message
2. Press on up key till you rich the 'reply in thread' option
3. Press up key once more

Expected Result:

Focus should be applied on 'Delete comment' option

Actual Result:

Focus is not applied on 'Delete comment' option

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

Bug6373696_1707490365897.Screen_Recording_2024-02-09_at_2.50.43_at_night.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017003c89d832c6ad8
• Upwork Job ID: 1755970808587268096
• Last Price Increase: 2024-02-23
• Automatic offers:
• Ollyws | Reviewer | 0
• FitseTLT | Contributor | 0

[melvin-bot]

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

[melvin-bot]

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

[kbecciv]

We think that this bug might be related to #vip-vsb
CC @quinthar

[melvin-bot]

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

[FitseTLT]

Proposal

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

Last option is not selected when pressing up key

What is the root cause of that problem?

Because index '0' is disabled it breaks early here

App/src/hooks/useArrowKeyFocusManager.ts

Lines 75 to 76 in b9aada8

	if (newFocusedIndex < 0) {
	break;

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

Change this

App/src/hooks/useArrowKeyFocusManager.ts

Lines 86 to 88 in 91ea979

	if (newFocusedIndex < 0) {
	break;
	}

to

if (newFocusedIndex < 0) {
                    if (disableCyclicTraversal) {
                        break;
                    }
                    newFocusedIndex = maxIndex;
                }

And there is a similar problem with arrow key down too as there is no check if it is greater than maxIndex so we can add the below here

if (newFocusedIndex > maxIndex) {
                    if (disableCyclicTraversal) {
                        break;
                    }
                    newFocusedIndex = 0;
                }

What alternative solutions did you explore? (Optional)

[allgandalf]

Proposal

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

What is the root cause of that problem?

We reduce the value of newFocusedIndex everytime in the loop and when it hits beyond 0, we break the loop, thereby we lose the highlight from the list

App/src/hooks/useArrowKeyFocusManager.ts

Lines 74 to 76 in b9aada8

	newFocusedIndex -= allowHorizontalArrowKeys ? itemsPerRow : 1;
	if (newFocusedIndex < 0) {
	break;

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

Instead of checking for the values < 0 (This case is only where the value can be -1 as when we go to the top of the list the last index value is always 0 and nothing below it!)

Update the while loop to check if the newFocusedIndex is at the top of the list, and if so then return the maxIndex, if we enable the property to not traverse cyclic in the list then we stay at the top index itself

while (disabledIndexes.includes(newFocusedIndex)) {
    newFocusedIndex -= allowHorizontalArrowKeys ? itemsPerRow : 1;
    
    if (newFocusedIndex === -1) {
          return disableCyclicTraversal ? actualIndex : maxIndex
                .
                .
             

What alternative solutions did you explore? (Optional)

N/A

[Krishna2323]

Proposal

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

Message action menu - Last option is not selected when pressing up key

What is the root cause of that problem?

In the code below the while loop will be executed when the newFocusedIndex is 0 because disabledIndexes.includes(newFocusedIndex) will become true as we have the emojis container at the index 0 and we don't allow focus on it, and due to that the focusedIndex is set to -1 for once.

App/src/hooks/useArrowKeyFocusManager.ts

Lines 73 to 77 in b9aada8

	while (disabledIndexes.includes(newFocusedIndex)) {
	newFocusedIndex -= allowHorizontalArrowKeys ? itemsPerRow : 1;
	if (newFocusedIndex < 0) {
	break;
	}

We also have similar bug in arrowDownCallback, there also we don't handle the case when the newFocusedIndex becomes greater than the maxIndex. This can be reproduced easily by passing last index number to disabledIndexes array inside BaseReportActionContextMenu or in any component we use useArrowKeyFocusManager. there also if we have the last index as disabled, the newFocusedIndex will be greater than the maxIndex which will cause focus lost.

arrow_down_issue.mp4

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

For Up arrow key:

Instead of exiting the loop, we need to find the next focusable option from last, we can't directly return maxIndex because the maxIndex can also be disabled. For that we need to use the another while loop. Also, instead of using 0 as the minIndex we need to introduce a new parameter minIndex in useArrowKeyFocusManager and the default value will be zero. We need that for special cases like the emoji picker because there we need to get negative index because we want to focus on the input. We will pass headerIndices.length * -1 + 1 as the minIndex from EmojiPickerMenuWithRef.

We need to update the code below:

App/src/hooks/useArrowKeyFocusManager.ts

Lines 75 to 77 in b9aada8

	if (newFocusedIndex < 0) {
	break;
	}

To:

                if (newFocusedIndex < minIndex) {
                    if (disableCyclicTraversal) {
                        return actualIndex;
                    }
                    let newIndexFromLast = maxIndex;
                    while (disabledIndexes.includes(newIndexFromLast)) {
                        newIndexFromLast -= allowHorizontalArrowKeys ? itemsPerRow : 1;
                        if (newIndexFromLast < minIndex) {
                            return actualIndex;
                        }
                    }
                    return newIndexFromLast;
                }

For Down arrow key:

We need to add the code below in the while loop of arrowDownCallback:

    if (newFocusedIndex >= maxIndex) {
        if (disableCyclicTraversal) {
            return actualIndex;
        }
        let newIndexStart = 0;
        while (disabledIndexes.includes(newIndexStart)) {
            newIndexStart += allowHorizontalArrowKeys ? itemsPerRow : 1;
            if (nextIndex >= maxIndex) {
                return actualIndex;
            }
        }
        return newIndexStart;
    }

PS: Minor code changes or refactoring can be done during the PR, the general idea will be the same.

Result

fix_demo_arrow_keys.mp4

[Ollyws]

Will get to this today.

[Ollyws]

Thanks for the proposals everyone.

@FitseTLT unfortunately your proposal will break re-focusing on the emoji search box using the arrow keys.

@Krishna2323 Thanks for the analysis, however we don't use disabled indexes for the down arrow anywhere as far as I can see.

Therefore I think it's fair that we go with the first working proposal which is @GandalfGwaihir's.

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[Krishna2323]

@Ollyws, I believe we disable options in many places, like category in iou creation flow and that is random, so any option can be disabled so its important to handle the nextFocusedIndex correctly instead of passing maxIndex.

Edit: sorry I got it wrong, but still I think we should handle it correctly, the selected proposal just passes maxIndex without checking disabledIndexes.includes(maxIndex).

[Krishna2323]

however we don't use disabled indexes for the down arrow anywhere as far as I can see.

@Ollyws, sorry, I didn't get that, can you pls explain what do you mean by this?

I think we use disabled indexes for the down arrow key.

App/src/hooks/useArrowKeyFocusManager.ts

Line 123 in b635cc5

while (disabledIndexes.includes(newFocusedIndex)) {

[Ollyws]

sorry, I didn't get that, can you pls explain what do you mean by this?

What I meant was, that I can't see anywhere in which this issue affects the app and while it would be nice to fix this along with the issue you mentioned it would mean passing up an earlier working proposal for one that fixes an issue that's arguably out of scope, which doesn't quite seem fair to me.
Generally our approach is to pick the first working proposal and then refine any smaller details in the PR, this way we can get things moving quickly.

[FitseTLT]

@FitseTLT unfortunately your proposal will break re-focusing on the emoji search box using the arrow keys.

@Ollyws Thanks! that was a nice catch 👍

But I have to point that the selected proposal is principally wrong in two ways

1. The basic principle with the condition if (newFocusedIndex < 0) { is that we shouldn't normally have negative indexes and it is correct.
   But the condition
   if (newFocusedIndex === -1) { will narrow down the exception so when itemsPerRow is more than 1 the index could jump to less than -1 and the same issue that we are facing in this current issue will reoccur.
2. return disableCyclicTraversal ? actualIndex : maxIndex returning maxIndex when cyclic traversal is allowed assumes maxIndex is not disabled which is wrong and on cases where the maxIndex is disabled the current issue will re-occur

Now the emoji case is an exception where in EmojiPickerMenu we allow and use negative indexes for focusing on the search input here

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

Lines 95 to 96 in 3a702ce

	if (newIndex < 0 && !searchInputRef.current.isFocused() && shouldFocusInputOnScreenFocus) {
	searchInputRef.current.focus();

So the perfect solution is to add to my proposal an option in useFocusManager where we can allow negative indexes and pass that parameter in emoji's case and other case where we want to allow negative indexes and we will use that additionally in this condition

App/src/hooks/useArrowKeyFocusManager.ts

Lines 75 to 76 in b9aada8

	if (newFocusedIndex < 0) {
	break;

[Krishna2323]

@Ollyws, thanks for the clarification, but the selected proposal doesn't fix the issue correctly and since useArrowKeyFocusManager is a hook we should fix that correctly because it is not a component specific code. Not checking disabledIndexes.includes(maxIndex) will cause regressions if we use useArrowKeyFocusManager with last option as disabled.

There are 2 point which makes my proposal better than the selected one:

1. Checking with disabledIndexes.includes(maxIndex) and looping before passing the nextIndex, like we do currently here:
   

   App/src/hooks/useArrowKeyFocusManager.ts

   Lines 123 to 126 in b635cc5

   	while (disabledIndexes.includes(newFocusedIndex)) {
   	if (actualIndex < 0) {
   	newFocusedIndex += 1;
   	} else {

2. Also fixes the bug with down arrow.

Also, I think they not smaller details and would have been easily missed during the PR phase.

cc: @puneetlath

[allgandalf]

Generally our approach is to pick the first working proposal and then refine any smaller details in the PR, this way we can get things moving quickly.

Hello @FitseTLT @Krishna2323 , as stated, the core solution remains the same, I can improve on your suggestions if they have valid grounds during the PR stage 👍

Thanks and hope you understand

[Krishna2323]

@GandalfGwaihir, proposals should also cover every edge case(if any) and similar bugs instead of the just core idea. Also it should be complete to not cause any regressions in the future.

[FitseTLT]

@GandalfGwaihir, proposals should also cover every edge case(if any) instead of the just core idea.

I think edge cases can be left for PR phase but this case is not at all about being an edge case. The only reason it didn't break the search input focus was a sheer luck that for emoji we have 8 item per row so when it deducts on the while loop it becomes
8 > 0 > -8 Now if you are on the right-most emoji it disables the focusing as in my original proposal. as it becomes 15 > 7 > -1

2024-02-13.20-21-32.mp4

cc @puneetlath @Ollyws

[Krishna2323]

I mean't the similar bugs.

Checking with disabledIndexes.includes(maxIndex) and looping before passing the nextIndex.

This isn't a edge case, this follows the current code implementation approach and is important to not cause similar bugs in future.

[puneetlath]

@Ollyws mind summarizing the current state of this issue for me?

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[Ollyws]

Awaiting PR

[melvin-bot]

@puneetlath, @Ollyws, @FitseTLT Whoops! This issue is 2 days overdue. Let's get this updated quick!

[Ollyws]

same

[melvin-bot]

@puneetlath @Ollyws @FitseTLT this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is today
• If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[melvin-bot]

Current assignee @Ollyws is eligible for the Internal assigner, not assigning anyone new.

[puneetlath]

@FitseTLT @Ollyws How's this going?

[Ollyws]

Just reviewing some details, should be good to go in a day or two.

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

• Fix - Message action menu - Last option is not selected when pressing up key #38007

If no regressions arise, payment will be issued on 2024-03-25. 🎊

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

• @Ollyws requires payment automatic offer (Reviewer)
• @FitseTLT requires payment automatic offer (Contributor)

[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:

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

[puneetlath]

@Ollyws friendly reminder about the checklist so that we can pay on Monday.

[Ollyws]

BugZero Checklist:

• The PR that introduced the bug has been identified. Link to the PR:

#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:

#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

• 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 any chat
2. Right click on any report action
3. Press on up key till you rich the top selectable option
4. Press up key once more
5. Verify that the last menu item is now highlighted

Do we agree 👍 or 👎

[puneetlath]

All paid. Thanks y'all!

Regression test issue is here: https://github.com/Expensify/Expensify/issues/382048