[$500] Error shows up whenever tried to go back using keyboard

[m-natarajan]

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: 1.4.24-7
Reproducible in staging?: yes
Reproducible in production?: no
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
Expensify/Expensify Issue URL:
Issue reported by: @shubham1206agra
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1705158369037609

Action Performed:

1. Open App
2. Open FAB > Assign task
3. Type something in title, and go to next screen.
4. With keyboard, try to go back or press on any menuitem.

Expected Result:

No validation error should show up, and page should go back on using back button.

Actual Result:

Validation error should shows up, and page doesn't go back.

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

Recording.2660.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~016f34652481906d29
• Upwork Job ID: 1746196695910436864
• Last Price Increase: 2024-01-20
• Automatic offers:
• situchan | Contributor | 28115099
• shubham1206agra | Contributor | 28117382

Issue Owner

Current Issue Owner: @sakluger

Issue Owner

Current Issue Owner: @sakluger

Issue Owner

Current Issue Owner: @sakluger

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~016f34652481906d29

[melvin-bot]

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

[melvin-bot]

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

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

Triggered auto assignment to @Julesssss (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[bernhardoj]

Proposal

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

The task validation error shows up when we try to select other pressable using keyboard.

What is the root cause of that problem?

This is another regression from #33055 where it updates the useActiveElementRole to use ref instead of state, so it won't re-render the components that use it when the role changes.

useActiveElementRole is used to disable e ENTER shortcut for a button, in our case the task confirm button.

App/src/components/Button/index.tsx

Line 129 in 753ce92

const shouldDisableEnterShortcut = useMemo(() => accessibilityRoles.includes(activeElementRole ?? '') && activeElementRole !== CONST.ACCESSIBILITY_ROLE.TEXT, [activeElementRole]);

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

Use state back in useActiveElementRole

[dukenv0307]

Proposal

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

Validation error should shows up, and page doesn't go back.

What is the root cause of that problem?

After this PR, in useActiveElementRole we return a ref instead of a state to improve performance and reduce component rerendering, so when the activeElementRole changes, it won't trigger component rerendering, leading to this issue with the Button where we use useActiveElementRole (here)

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

I think it was a good call to use ref in useActiveElementRole instead of state, and we should keep that, otherwise we'll reintroduce the performance issue that we were trying to fix.

What we need to do here is to make sure isActive here and inside useKeyboardShortcut works properly.

Currently the isActive is a calculated state, which changes whenever the dependencies (like activeElementRole) changes. This is not necessary because isActive is really not used much, it's only valuable in case when the user presses on the keyboard shortcut, based on it we know that some listeners should not action on that shortcut.

We should make isActive a function which will return the calculation that it currently have, and make it use the .current of the activeElementRoleRef to get the spot value of activeElementRole at the time isActive is called.

And then we should pass isActive inside KeyboardShortcut.subscribe here and early return here if callback.isActive() is false (same thing we did with other conditions like excludeNodes.

What alternative solutions did you explore? (Optional)

Instead of making isActive a function, making it a ref should also work, the main point here is to make isActive have a value that will be evaluated when the event handler is actually triggered, not changing it every time a dependency like activeElementRole changes.

Result

Working well after the fix:

Screen.Recording.2024-01-13.at.11.26.16.PM.mov

[mountiny]

This is kind of an accessibility issue so removing the DB label @hurali97 could you or someone from callstack fix this as a regression from the PR please? thanks!

[Julesssss]

@shubham1206agra has already suggested an improvement to the linked issue here that resolves this issue.

[shubham1206agra]

Proposal

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

Error shows up whenever tried to go back using keyboard

What is the root cause of that problem?

This is regression from #33055 where it updates the useActiveElementRole to use ref instead of state, so it won't re-render the components that use it when the role changes.

We cannot use state for storing activeRole directly instead of ref as this is causing some performance lag.

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

I have tried to further optimize @hurali97's proposal here #34480.

I tried to do 2 main things

1. Instead of creating 2 new listener for focusin, and focusout for every use of useActiveElementRole, we are going to create context of activeRole, and will listen only once for events inside the Provider, which will be supplied to App root component directly. The aim was to remove the redundancy of listeners, as we know every button will have these listeners, and there are almost no places on App where we don't have a button.
2. Instead of using the above context for every button rendered, we have used pressOnEnter prop to conditionally render KeyboardShortcutComponent, which will help in less re-render on changing of activeRole.

Now we use state for storing activeRole instead of ref.

What alternative solutions did you explore? (Optional)

[bernhardoj]

Using state back won't introduce the performance lag in KeyboardShortcutComponent (as it renders nothing), but possibly in BaseSelectionList. BaseSelectionList should follow the same pattern as Button to avoid any unnecessary re-render of the whole component.

[shubham1206agra]

Using state back won't introduce the performance lag in KeyboardShortcutComponent (as it renders nothing), but possibly in BaseSelectionList. BaseSelectionList should follow the same pattern as Button to avoid any unnecessary re-render of the whole component.

But it tries to run state update logic for every button on changing of activeRole, and will takes some resources to perform these unnecessary calculations. I have checked this, and RAM usage does spike a little.

BaseSelectionList should follow the same pattern as Button to avoid any unnecessary re-render of the whole component.

I can also do the same changes once these changes are finalized.

[dukenv0307]

@shubham1206agra has alreadysuggested an improvement to the linked issue #28916 (comment) that resolves this issue.

Hi @Julesssss, just checking if this issue should go through regular proposal evaluation process? I think we have some proposals posted here before @shubham1206agra 's posted suggestion.

[sakluger]

Sorry for the delay! Thanks @mallenexpensify for adding the daily label.

Summarizing payment on this issue:

Contributor: @shubham1206agra $500, payable via manual request
Contributor+: @situchan $500, paid via Upwork

[sakluger]

Hey @shubham1206agra, please let me know once you've accepted the offer. Thanks!

[shubham1206agra]

@sakluger Can you hold my payment temporarily as per https://expensify.slack.com/archives/C02NK2DQWUX/p1710150138788529?

[melvin-bot]

@sakluger, @Julesssss, @shubham1206agra, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

[sakluger]

@shubham1206agra what's the latest?

[shubham1206agra]

@sakluger I have discussed this internally. You may close this issue as I am keeping track of payment internally and will ask to pay once the issue is resolved. Just write in the payment summary that I have not been paid yet.

[sakluger]

Sounds good @shubham1206agra. I've withdrawn the Upwork offer.

The payment summary is here.

[shubham1206agra]

@sakluger You can process payment here now.

[shubham1206agra]

@sakluger Bump on the above.

[shubham1206agra]

@sakluger Bump here.

[sakluger]

@shubham1206agra fyi I don't always see comments on already closed GH issues. Feel free to bump me in Slack sooner if payment is due but the issue is closed.

[sakluger]

@shubham1206agra here is the Upwork offer (https://www.upwork.com/nx/wm/offer/102288115) since it sounds like we are not going to pay via manual request.

[shubham1206agra]

@sakluger Accepted

[sakluger]

Thanks! All paid 🙇