[$500] Improve Tab Navigation & Accessibility

[kosmydel]

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: Latest
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): N/A
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL: N/A
Issue reported by: me
Slack conversation: N/A

Action Performed:

1. Open report.
2. Click the user avatar (next to the message).
3. The RHP opens.
4. Try to tab navigate.

Expected Result:

You should navigate only inside the RHP modal. You shouldn't be able to escape it (unless you press escape/back button). This would improve the accessibility for users with visual disabilities.

Actual Result:

You can navigate outside the RHP modal.

Workaround:

N / A

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

Screen.Recording.2024-02-14.at.13.09.16.mov

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01681c9914c65b84bc
• Upwork Job ID: 1758337605760794624
• Last Price Increase: 2024-02-16
• Automatic offers:
• c3024 | Reviewer | 0
• fedirjh | Contributor | 0

[melvin-bot]

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

[kosmydel]

Coming from here.
cc @roryabraham

---

Proposal

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

Users can navigate outside the RHP modal using the Tab/Shift+Tab.

What is the root cause of that problem?

We are not using FocusTrap on the RHP modals.

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

Just add the focus trap like in this PR.

What alternative solutions did you explore? (Optional)

N/A

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01681c9914c65b84bc

[melvin-bot]

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

[melvin-bot]

📣 @c3024 🎉 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

[roryabraham]

Let's swap in @fedirjh as C+ since he's got context on the focus trap

[melvin-bot]

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

[roryabraham]

Not overdue, go away melvin

[kosmydel]

Thanks @roryabraham!
I'm going to work on this next week, as this week I'm busy with higher priority issues and I'm OOO from Wednesday.

[roryabraham]

This is less urgent so I'm going to move it to weekly

[adamgrzybowski]

Hi! I was working on few issues related to the improve tab navigation & accessibility and here is my quick summary

Syncing multiple focus traps

Initially, I tested using focus trap only for navigators to avoid problems with syncing but that wasn't ideal. When navigating back inside e.g. RHP the focus wouldn't be returned to the element that opened the screen.

Instead, I went back to the idea of using focus-trap for the screen wrapper. There is a trapStack prop that helps syncing multiple focus traps. It seems to work well in my draft.

Autofocus

I was looking for a general solution to cover this requirement:

I assume that if we are going back to the screen with autofocus, then this element should be focused. If not then we should focus element focused before activating the focus trap. Is that right? @roryabraham

I experimented with a solution where the focus trap waits for the screen to finish animating the transition to check if a text input is focused. In that case, the focus trap wouldn't set focus by itself.

This solution looked promising and adding new screens with autofocus wouldn't require any work. But I couldn't make it work. There were always some broken cases.

After some consideration, it looks like the best solution is just to have a list with the screens having autofocus and avoid overriding focus there.

I created a draft with this solution here. It would be great if somebody could test it a little. I think I haven't listed all screens with autofocus and there may be more unique cases that we need to handle.

Video

focus.mov

Wrong focus when going back to already opened screens.

This one is fixed in my draft. It turned out that some pressables were defocusing after an interaction. This was the reason why some screens had focused document.body on going back to them.

Syncing tab and arrows

I passed my context to @WojtekBoman and it looks like he already posted a PR with a solution for this one 🚀

Remaining accessibility issues

The topic of accessibility turned out to be much broader than I initially expected. There are still some things that should be addressed. I am wondering if we could create separate issues for them so we could divide the work more easily. Especially given that I have a list of pending issues related to navigation that I should give my attention to.

Confirming interaction with enter

It seems like confirming interaction with enter can trigger multiple calls of onPressHandler in the BaseGenericPressable. This can break the app sometimes e.g. confirming goBack with enter can pop too many screens from the stack.

Screens with broken tab navigation

I found two screens where not all elements are accessible with tab navigation. Send money screen and request money screen. They need more investigation.

Screens

cc: @isabelastisser @roryabraham

[roryabraham]

I am wondering if we could create separate issues for [accessibility issues] so we could divide the work more easily

this sounds like a great idea, but we might want to discuss prioritization this because accessibility isn't really the most critical concern for our roadmap today. Let's frame these accessibility issues as problem statements, then post them in slack to discuss which one(s) we want to pursue fixing right now. We probably want to prioritize the highest-ROI things first

[roryabraham]

overall thanks for your investigation, it looks great

[dragnoir]

@adamgrzybowski, I've reviewed your pull request and am impressed with the progress thus far.

I'd like to discuss a recurring issue we're facing—multiple triggers of onPressHandler. Do you have any insights into what might be causing this behavior?

Furthermore, I'd like to delve into two scenarios that have sparked considerable debate and would benefit from your input and testing in conjunction with your current work:

1- Right click on a reportItem, where focus should start and where focus should be when the BaseReportActionContextMenu will be removed.

20240403_211105.mp4

In the context of using OptionsSelector, when one element is selected and another is focused using the tab key, what action should the enter key trigger? Your insights on this would be invaluable. Here's a video of the scenario for further examination:

20240403_211230.mp4

[adamgrzybowski]

Just to let you know guys it looks like I need to switch my attention to the new search tab in the bottom tab for a moment.

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

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

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

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

[jnowakow]

Hello, I'll take over this issue from Adam as he's out of office now.
@dragnoir I don't have so much insight as Adam but I'll try to start the conversation about the issues you mention above.

Right click on a reportItem, where focus should start and where focus should be when the BaseReportActionContextMenu will be removed.

I think that when the BaseReportActionContextMenu is open it should be the place when focus is trapped. Right now you can focus all elements on page which I don't think it good.

Video

focus-context-menu.mov

When the BaseReportActionContextMenu is dismissed then focus return to it previous state - nothing is focused.

In the context of using OptionsSelector, when one element is selected and another is focused using the tab key, what action should the enter key trigger? Your insights on this would be invaluable. Here's a video of the scenario for further examination:

I believe it was fixed by Wojtek here. Arrow and tab navigation is in sync and there situation that other entries are selected and focused at the same time.

[melvin-bot]

This issue has not been updated in over 15 days. @isabelastisser, @fedirjh, @roryabraham, @WojtekBoman, @kosmydel 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!

[trjExpensify]

Assigning @jnowakow for tracking and moved back to Weekly. 👍

[jnowakow]

I think focus and tab navigation are fixed here. Maybe it needs to be polished but in general it's ready. @roryabraham, I've created new library request and it's here.

I see two other topics that are connected to this topic but not necessary in scope. They might be need to be discussed and prioritized.

First topic is keyboard navigation on mobile devices. More context here. At first glance it looks like not obvious problem but affecting small number of users.

Second issue is clicking focused elements with space and enter. According to documentation both keys should trigger click event but it's not true in our app. I think it should be investigated and fixed as it may be confusing for the users.

Examples of bugs with clicking elements with space and enter

space-not-triggering-action.mov

space-and-enter-working-different.mov

[mountiny]

This should be unblocked and you can resume work on the PR @jnowakow

[melvin-bot]

This issue has not been updated in over 15 days. @isabelastisser, @fedirjh, @roryabraham, @WojtekBoman, @jnowakow, @kosmydel 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!