[$1000] Pressing space bar while details page is opened, scrolls up the reports

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Click on an avatar in a money request
2. After the details page has opened, press space bar

Expected Result

Nothing should happen

Actual Result

Chats are scrolled up

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.2.78-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:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Screen.Recording.2023-02-28.at.3.29.35.PM.mov

Recording.118.mp4

Expensify/Expensify Issue URL:
Issue reported by: @esh-g
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1677578613083529

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017ddd7486addffc52
• Upwork Job ID: 1632967018098249728
• Last Price Increase: 2023-08-08
• Automatic offers:
• fedirjh | Reviewer | 26060321
• esh-g | Reporter | 26060323

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

@stephanieelliott Whoops! This issue is 2 days overdue. Let's get this updated quick!

[MelvinBot]

@stephanieelliott Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[stephanieelliott]

Yeah this is weird behavior. Made a small tweak to the issue description (changed report to request money), looks all set to go to a contributor for a fix!

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~017ddd7486addffc52

[MelvinBot]

Current assignee @stephanieelliott is eligible for the External assigner, not assigning anyone new.

[MelvinBot]

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

[MelvinBot]

Triggered auto assignment to @joelbettner (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[getusha]

Is it expected that I am able to see this happening without having to open the details page, @rushatgabhane?

[tienifr]

Proposal

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

After the details page has opened, press space bar will scroll the chats up.

What is the root cause of that problem?

This happens because the current StackView implementation that we have does not have focus traps. When we press space or up/down arrows, the keyboard event will be triggered on the body element and cause the chat to scroll up. (Chat scrolling up is normal behavior, just that it should not happen when a modal is opened on top)

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

We need to add focus trap to the pages in the right sidebar, we can use this package https://github.com/focus-trap/focus-trap-react, and add the <FocusTrap> element in here

App/src/pages/DetailsPage.js

Line 101 in f1769bc

<ScreenWrapper>

(other pages with the same issue will also need to be fixed)

What alternative solutions did you explore? (Optional)

We can add the focus trap in the react-navigation itself, or wrap all our StackView with the focus trap instead of adding it in individual pages.

[MelvinBot]

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.

[you1996]

Proposal

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

When the details page is open, pressing on space bar will cause scroll down/up (based on the initial position).

What is the root cause of that problem?

As we can see, the chats section has a stack of components, with the details modal being the most recent one. Any scroll event triggered by the user (in this case, by pressing the space key) will be applied to the body element since the details page does not have a body element of its own. This means that the scroll event will automatically be applied to the parent component, which is a normal and expected behavior.

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

As the problem is caused by normal behavior, I suggest simply disabling scrolling using the space key when the details page is open. Adding more complexity, packages, or changing the stack view behavior is not the best solution. Therefore, we need to add an event listener on the details page that listens for a "keydown" event with a code of 32 (space) in the componentDidMount(), and prevent the default action on the body, we should also add a little function to handle the event.

componentDidMount() {
       ...
       document.addEventListener("keydown", this._handleKeyDownEvent);
}

What alternative solutions did you explore? (Optional)

We also considered an alternative solution of disabling scrolling using the space key on all pages in the app by adding the listener on the app component. This ensures that the issue will not occur again.

[MelvinBot]

📣 @you1996! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[you1996]

Contributor details
Your Expensify account email: youssribentaghalline@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01b676ad45ba72ff4f

[MelvinBot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[MelvinBot]

📣 @pradhankukiran! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[MelvinBot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[roryabraham]

Looks like we're currently HOLDing for #32815

[roryabraham]

Also, I'm going to go ahead and make this lower-priority issue into a weekly

[kosmydel]

Looks like we're currently HOLDing for #32815

@roryabraham linked PR fixes regression which wasn't coming from my changes (but it was linked to my PR anyway), so I think we don't need to hold.

[stephanieelliott]

#32815 was merged, so I think we can proceed here either way. Conflicts have been resolved in #32800, currently waiting on @fedirjh's review!

[stephanieelliott]

PR is being actively reviewed!

[stephanieelliott]

PR is reviewed, but we're going to do the QA before merging.

[stephanieelliott]

Kicked off QA here

[stephanieelliott]

Regression testing was completed (notes), no issues 🎉

[stephanieelliott]

PR is under review

[roryabraham]

@kosmydel reports this was unknowingly fixed by #ideal-nav. I wasn't able to reproduce this anymore. I think we should close it out?

[JmillsExpensify]

Ha, well would you look at that!

[stephanieelliott]

Love it when that happens! Ok, given that we had 3 PRs reviewed (2 of which were merged), I think payment is due here however the regression penalty would apply. Some quick math:

Original job price $1000

• 50% regression penalty PR 1 (Add focus trap to the RHP #24316) = $500
• 50% regression penalty PR 2 (Add focus trap to the RHP (v2) #27670) = $250

---

That would make the payout summary:

• Contributor: @kosmydel N/A, expert contributor
• Contributor+: @fedirjh $250 via Upwork (offer extended, please accept)

Upwork job is here: https://www.upwork.com/jobs/~0112187286e2fd23a2

[stephanieelliott]

All paid!

[kosmydel]

@kosmydel reports this was unknowingly fixed by #ideal-nav. I wasn't able to reproduce this anymore. I think we should close it out?

Hey, good to know that the issue doesn't occur anymore.

However, I want to confirm, if we don't want to introduce the FocusTrap anyway. It did not only meant to solve this issue, but also improved the accessibility and tab navigation on the web/desktop.

cc @luacmartins, @roryabraham

[roryabraham]

It did not only meant to solve this issue, but also improved the accessibility and tab navigation on the web/desktop.

@kosmydel can you be more specific about what accessibility problem it solves?

[kosmydel]

@kosmydel can you be more specific about what accessibility problem it solves?

Tab navigation is used together with e.g. VoiceOver on macOS to help people with visual disabilities.

Currently, if the e.g. RHP modals don't support focus trap, tab navigation is almost impossible (or at least cumbersome). We are "tabbing" outside the modal, which is probably not what a user expects after pressing the button that opens it. The navigation order is not correct.

Here is more info about navigation order.

[roryabraham]

Ok, I think that makes sense. @kosmydel can you create a separate issue using the standard issue template Action Performed / Expected Result / Actual Result / Workaround, and in the first comment post a proposal to fix it? Then tag me there.

Generally we aren't prioritizing accessibility too highly but given that this is already pretty much ready and we've already discovered then fixed the regressions it caused, we can talk about it.

[tienifr]

@roryabraham @stephanieelliott Sorry for coming late, I think I'm also eligible for partial compensation (suggested by @fedirjh here)

Additionally, I believe @tienifr is eligible for partial payment since they were the ones who initially recommended utilizing the focus-trap-react library.