[C+ Checklist Needs Completion] [$1000] Keyboard navigation and selection is inconsistent in Split Bill UI

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Issue found when executing PR #21310

Action Performed:

1. Open http://staging.new.expensify.com/
2. Open Group DM
3. Click on "+" near message composer
4. Click on "Split bill"
5. Insert amount
6. Click "Next"
7. Click with mouse on any member to deselect it
8. Use the keyboard navigation (up/down arrows + enter) to select/deselect different members

Expected Result:

User can successfully select/deselect members for split bill using keyboard navigation

Actual Result:

Keyboard navigation starts working incorrectly while selecting/deselecting members if user clicked on one of the members before. When user press "enter" for any member - select checkmark immediately returns to previous state. Sometimes 2 members are selected / deselected instantly instead of 1

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.31.2

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Notes/Photos/Videos: Any additional supporting documentation

Bug6103631_Recording__726.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~015b770640f03d0c88
• Upwork Job ID: 1672370582030045184
• Last Price Increase: 2023-11-08
• Automatic offers:
• mollfpr | Reviewer | 27885262
• dukenv0307 | Contributor | 27885264

[melvin-bot]

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

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[greg-schroeder]

I reproduced this. Although the keyboard navigation seems fine to me, the "enter" functionality does seem to be buggy:

2023-06-23_15-24-52.mp4

[melvin-bot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~015b770640f03d0c88

[melvin-bot]

Current assignee @greg-schroeder is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[dukenv0307]

Proposal

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

Keyboard navigation starts working incorrectly while selecting/deselecting members if user clicked on one of the members before. When user press "enter" for any member - select checkmark immediately returns to previous state. Sometimes 2 members are selected / deselected instantly instead of 1

What is the root cause of that problem?

After this PR, the OptionRow is now accessible, when we click on it initially, the PressableWithFeedback will receive the focus. When we press enter, the focused PressableWithFeedback will trigger the onPress, which calls onSelectRow. Meanwhile we also have a listener here which listen to the Enter keypress and call onSelectRow, so we can see that onSelectRow is called twice, causing the issue.

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

We're having 2 focused OptionRows now:

• Our own focus control
• Browser focus (which results from the accessibility improvement above)

We need to align those focus so that there'll be only 1 focused OptionRow, we can do that by:

1. When focusedIndex changes, trigger .focus on that element so the browser will also focus on it.
2. Modify this line, so that if the option row associated with the focusedIndex is currently the browser-focused element, we'll not trigger the selectFocusedOption (because the browser will already handle it)

Steps:

• Add a itemRefs array of same length as the list to store the reference to the OptionRow element
• If the itemRef associated with the focusedIndex, contains the activeElement (get from useActiveElement), that means the focused option is also the browser-focused element
• The focused option is also the browser-focused element, skip triggering selectFocusedOption

What alternative solutions did you explore? (Optional)

NA

[ygshbht]

Proposal

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

The issue arises when the user navigates the Split Bill interface using keyboard commands. The selection and deselection of members using the up/down arrow keys and the enter key does not behave as consistently as one would expect.

What is the root cause of that problem?

This problem could be stemming from how the keyboard events are being handled in the Split Bill UI. It seems the interface isn't fully optimized to respond to keyboard interactions as efficiently as it should.

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

I propose diving into the pertinent parts of the code, specifically files like src\pages\iou\SplitBillDetailsPage.js, and scrutinizing the event handlers tied to keyboard navigation. It's possible that refining these handlers or the state management associated with them can lead to the seamless keyboard-based user experience you're aiming for.

Let me assure you, with my experience and familiarity with similar issues, there's nothing unsolvable here. I am quite confident in delivering a robust solution that rectifies this issue and enhances the overall usability of the Split Bill UI.

[jeet-dhandha]

Proposal

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

There are two problems here:

• When we are moving up/down the user is highlighted but the focus is not moved to that user. (This happens mostly when we first deselect using mouse and then try to select/deselect using keyboard).
• When we are selecting/deselecting the user the list is updated thus changing the user's order.

What is the root cause of that problem?

• Selection focus is not moved to the user when we are moving up/down.
• The list is re-shuffled on each selection/deselection.

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

• First we should fix the selection focus issue.
• Then we should fix the list re-shuffling issue.

(I will update the code for these two when we finalise on how we want to handle this)

[c3024]

Proposal

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

In SplitBill confirmation page where users are selected, if we select/deselect one of the participants and then try to select/deselect with keyboard, the checkboxes toggling is not what we want.

What is the root cause of that problem?

After selecting a row with mouse click, if we click one of the arrow keys, this mouse click selected row gets selected. If we move the selection up or down with arrow keys this mouse click selected row remains focused and another row is also focused depending on the up or down clicks. So, when we click Enter two togglings happen. If the mouse click selected row is the same row selected with keyboard then it selects and deselects. If mouse click selected row is different form the keyboard selected row, both the mouse click selected row and the keyboard selected row toggle. This causes inconsistent behaviour.

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

We should remove the previous selections when in onArrowUpKey() and onArrowDownKey() here.
https://github.com/Expensify/App/blob/820fa93ad1e799c12d8b639b98018b5350ea17af/src/components/ArrowKeyFocusManager.js#L79C5-L82C10
by adding blur in the functions.

onArrowUpKey() {
+if (document.activeElement) {
 +          document.activeElement.blur();
   +     }
....
}

What alternative solutions did you explore? (Optional)

None. Since, the BaseOptionsSelector and OptionRow are common for new group member selection as well, modifying these may create new issues.

Result

Screen.Recording.2023-06-26.at.8.33.10.AM.mov

[mollfpr]

Reviewing now

[mollfpr]

It's better to fix the selection focus to be aligned with the options focus.

@dukenv0307 While preventing the browser selection selecting the option fixes this issue, but it will create another issue focused option is not selected on enter. We should find a way to keep our focus control so the browser can work together.

---

I propose diving into the pertinent parts of the code, specifically files like src\pages\iou\SplitBillDetailsPage.js, and scrutinizing the event handlers tied to keyboard navigation. It's possible that refining these handlers or the state management associated with them can lead to the seamless keyboard-based user experience you're aiming for.

@ygshbht What changes do you suggest? We also have the same issue while selecting the members on the invite new member page.

---

First we should fix the selection focus issue.

@jeet-dhandha How?

---

@c3024 Blurring the browser selection is not preventing both focused options from being selected. It's just dismissing the browser selection.

[c3024]

Thanks for the review.

Only one option focused by keyboard gets selected on clicking Enter after blurring the active element. Video attached shows the same. Am I missing something?

[mollfpr]

Only one option focused by keyboard gets selected on clicking Enter after blurring the active element.

I don't get what you mean. Could you elaborate? It's still a workaround.

For now, the best solution is integrating browser selection and arrow key focus on the option item. We need to make sure that the browser accessibility is still can be used, so our focus control and selection.

[dukenv0307]

@dukenv0307 While preventing the browser selection selecting the option fixes this issue, but it will create another issue focused option is not selected on enter. We should find a way to keep our focus control so the browser can work together.

@mollfpr you mean the one mentioned in my alternative solution should be in the scope of the ticket right?

[mollfpr]

@dukenv0307 I don't understand why it's out of the scope of the ticket.

When users access the option by pressing the tab and pressing enter, they expect the focused option should be selected.

[c3024]

@mollfpr I misunderstood your earlier comment. My suggestion does not fix the integration of browser focus and keyboard focus.

[greg-schroeder]

Work continues on the linked PR - @mollfpr there's an outstanding question for you if you're able to take a look ASAP <3

[melvin-bot]

This issue has not been updated in over 15 days. @joelbettner, @mollfpr, @greg-schroeder, @dukenv0307 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

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

• Prevent enter key event if the focused element is a clickable element #32464

If no regressions arise, payment will be issued on 2024-01-24. 🎊

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

• @mollfpr requires payment automatic offer (Reviewer)
• @dukenv0307 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:

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

[Pujan92]

Not sure whether this should be considered as a regression from this issue PR or not. comment

[mollfpr]

Thanks @Pujan92 for the notice, I'll check it in the morning.

[greg-schroeder]

Okay - can you confirm for me ASAP @mollfpr? I'm going to hold off on the payment for now until I know what the payment amount should be taking regressions into account

[mollfpr]

I tested the changes on the PR #32464, and yes, it did cause the issue here: #34761.

The selected proposal and the root cause explanation indicate that we wrongly passed the component as a function, which makes the issue.

@greg-schroeder I'm not sure how we seeing this as a regression or a side effect of the improvement we did.

[greg-schroeder]

@mollfpr Yeah I see what you're saying. While it was maybe caused by it, it's not really a regression in the traditional sense

[greg-schroeder]

The upwork job for this closed a long time ago due to inactivity so I will create a new one

[greg-schroeder]

Offers sent to @mollfpr and @dukenv0307

Can you complete the checklist @mollfpr? Thanks!

[mollfpr]

 [@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
 @mollfpr] 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:

No offending PR.

[@mollfpr] 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:

The regression step should be good.

[@mollfpr] Determine if we should create a regression test for this bug.
[@mollfpr] 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

1. Create a group chat
2. Click on the plus icon > split bill
3. Enter the amount to go to confirm page
4. Click on the checkbox of one member
5. Use the arrow key to focus on another
6. Click enter key
7. Verify that the member which has the checkbox is focused is selected/de-selected
8. 👍 or 👎

[mollfpr]

@greg-schroeder You can cancel the offer for me; I'll do a manual request in NewDot. Could you give me the payment summary? Thank you!

[greg-schroeder]

Oh, woops, I forgot, I'll cancel the offer.

@mollfpr is due $1k for this job

[greg-schroeder]

Filed regression test, closing this out, I'll pay you right away once you accept offer @dukenv0307

[JmillsExpensify]

$1,000 approved for @mollfpr based on this comment.

[dukenv0307]

Filed regression test, closing this out, I'll pay you right away once you accept offer @dukenv0307

@greg-schroeder Accepted, thank you!

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.