[$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
• [x] MacOS: Chrome / Safari
• [x] MacOS: Desktop

Screenshots/Videos

https://github.com/Expensify/App/assets/104823336/5418229e-1be4-4fb3-b801-a52bbe7b33d2

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

Issue Owner

Current Issue Owner: @isabelastisser

[melvin-bot[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[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[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[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

[trjExpensify]

Hey @kosmydel! @mountiny @puneetlath @iwiznia and I were talking internally about the inconsistency of keyboard/tab navigation in the app. We think it's time to tackle it holistically throughout the app, and we'll put it under the umbrella of the #vip-vsb project. Some places the keyboard/tab navigation works in the app, others it doesn't. We're forever getting "one-off" bugs related to it etc, some we fix and others we close waiting for a holistic approach. As a next step, @iwiznia had the idea to:

1. Have one person implement tab navigation in one specific place (probably the expense creation flow as that is the most used flow I can think of that uses "forms")
2. Extract that work into something reusable (either a new component or a set of guidelines to follow for keyboard navigation)
3. Update the checklist to include that any new flow has to support keyboard navigation (by using the component/guidelines in 2)
4. Go back and fix all places where keyboard navigation was not implemented following the framework implemented above.

1-3 we'd be keen for you to take on if you want, given you're somewhat making a start by creating this issue. 4 could be as well if you want, but we could also just open that up to the community at large to fix the remaining bugs once 1-3 is in place.

What do you think?

[kosmydel]

@trjExpensify the plan sounds great!

Let me discuss it with my team internally, as I might not have enough capacity to handle it right now.

[trjExpensify]

Awesome, keep us posted!

[melvin-bot[bot]]

@isabelastisser @fedirjh @roryabraham @kosmydel this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[isabelastisser]

@kosmydel any updates here? Thanks!

[melvin-bot[bot]]

@isabelastisser, @fedirjh, @roryabraham, @kosmydel Whoops! This issue is 2 days overdue. Let's get this updated quick!

[isabelastisser]

bump @kosmydel

[dragnoir]

One of the issues with focus-trap is that there's 2 focus states, one is the keyboard navigation and the second is the keyboard focused element as on the screenshot below:

What ENTER key from keyboard will act on? The focusedIndex or the keyboard focused browser element?

If you try a software like NVDA https://www.nvaccess.org/download/ for testing accessibility for bling users. Navigate to new request user selection page as above screenshot, using keyboard arrows and tab will display different behavior and inconsistency.

I think we n eed to consider this for accessibility.

[kosmydel]

bump @kosmydel

Hey @isabelastisser, sorry for the delay. I've talked with my team in Software Mansion, and unfortunately, we don't have enough capacity to handle it this week. We should find someone to work on this next week, as it should be less busy.

[roryabraham]

I also think that before introducing react-focus-trap we would need to go through the new library review process that was introduced semi-recently by creating an issue using this issue template

[dragnoir]

Just for info, react-focus-trap is just a simple function that search for focusable elements htrow the page and loop the focus on those elements. We can copy the function, take what we need and enhance with hooks we already have.

[melvin-bot[bot]]

@isabelastisser @fedirjh @roryabraham @kosmydel this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[isabelastisser]

@roryabraham @trjExpensify I'm unsure of the next steps for this one.

[roryabraham]

we don't currently have an assignee actively working on this. I do not have any plans to push it forward myself.

So I guess we need to find another assignee, which according to @kosmydel should be sometime next week.

I do think that it would be good to clarify the prioritization of this issue, as I'm not sure it's gone through WN.

[roryabraham]

In my opinion weekly priority seems more appropriate here

[trjExpensify]

Yes, and I would deem it a New feature not a Bug really. As for priority, I think it would be a #vip-vsb project.

[melvin-bot[bot]]

Current assignee @isabelastisser is eligible for the NewFeature assigner, not assigning anyone new.

[melvin-bot[bot]]

:warning: It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time :warning:

[isabelastisser]

Added to VIP-VSB.

[adamgrzybowski]

Hi! @filip-solecki and I are currently working on this issue

[isabelastisser]

Hey @adamgrzybowski and @filip-solecki, any updates? Thanks!

[adamgrzybowski]

We had some conversation here. It looks like some components related to syncing tab and arrow navigation are in refactor currently. I decided to focus my attention on focus traps. @kosmydel gave me some insights and it looks like there is quite a lot of that cases that we have to consider.

[isabelastisser]

Hey, @adamgrzybowski, I'm not sure what this means. Can you please clarify it? Thanks! cc @roryabraham

[roryabraham]

It's a bit vague, but for some context we previously had this PR, so it sounds like @adamgrzybowski is taking a similar approach to solve the problem described in the issue description.

[adamgrzybowski]

That's correct. I experimented a little and it looks like placing the focus trap higher in the hierarchy gives the best results. e.g. wrap the whole RHP Navigator. It limits mounting and unmounting the focus trap component.

Currently, I am looking into returning focus after unmounting the focus trap.

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

[WojtekBoman]

Hello! I'll take a look at the issue with syncing tab and keyboard indicators when navigating through the app. More context here

[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

https://github.com/Expensify/App/assets/67908363/03839179-c307-4748-a4a6-fb584116bc6d

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.

https://github.com/Expensify/App/assets/12425932/eefb3d82-7595-4add-834a-b81b1b2ef18a

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:

https://github.com/Expensify/App/assets/12425932/061c7ecd-3390-4144-b251-1b89bbc9acec

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

https://github.com/Expensify/App/assets/56261019/b97e5c5c-7e65-4e94-92b1-fe9527840f19

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[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 https://github.com/Expensify/App/assets/56261019/ed5730bc-fbc0-4e55-a398-7eef68b73964 https://github.com/Expensify/App/assets/56261019/2b4fe241-0559-47c1-85ca-0a71485ca484

[mountiny]

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