[$250] Web - User has to click twice to select currency if the currency is around the bottom

[lanitochka17]

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.72-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: N/A
Issue reported by: Applause - Internal Team

Action Performed:

1. Navigate to staging.new.expensify.com
2. Click on FAB > Submit expense > Click on the currency to open the currency selector
3. Click on a currency located around the bottom of the list

Expected Result:

The currency gets selected on the first click

Actual Result:

The currency does not get selected on the first click but moves up to be in the middle of the list

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

Bug6475853_1715257148330.bandicam_2024-05-09_11-09-33-903.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0151b3d12408cd70e1
• Upwork Job ID: 1790583776832409600
• Last Price Increase: 2024-05-15
• Automatic offers:
• jayeshmangwani | Reviewer | 0
• tienifr | Contributor | 0

Issue Owner

Current Issue Owner: @jayeshmangwani

[melvin-bot]

Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[lanitochka17]

@JmillsExpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[lanitochka17]

We think that this bug might be related to #vip-vsp

[melvin-bot]

@JmillsExpensify Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0151b3d12408cd70e1

[melvin-bot]

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

[JmillsExpensify]

Added to #wave-collect and opened up for proposals.

[bernhardoj]

Proposal

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

When we hold down the list item, the list scrolls and the click is ignored.

What is the root cause of that problem?

When the item is focused either by keyboard or mouse, it will set the item as the focused index.

App/src/components/SelectionList/BaseSelectionList.tsx

Line 362 in 34bccb6

onFocus={() => setFocusedIndex(normalizedIndex)}

When the focused index changes, we scroll to the index.

App/src/components/SelectionList/BaseSelectionList.tsx

Lines 235 to 237 in 34bccb6

	onFocusedIndexChange: (index: number) => {
	scrollToIndex(index, true);
	},

And because we release the mouse after the list scrolls, the click isn't registered anymore.

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

Don't scroll the list if it's focused by mouse. We can create a new ref called shouldScrollAfterFocus. Set it to false in the list item onMouseDown.

if (shouldPreventDefaultFocusOnSelectRow || isItemFocused) {
    return;
}
shouldScrollAfterFocus.current = false;

We ignore it if shouldPreventDefaultFocusOnSelectRow because the item won't receive focus and if the item is already focused.

Then return early here if shouldScrollAfterFocus is false.

App/src/components/SelectionList/BaseSelectionList.tsx

Lines 235 to 237 in 34bccb6

	onFocusedIndexChange: (index: number) => {
	scrollToIndex(index, true);
	},

if (!shouldScrollAfterFocus.current) {
    shouldScrollAfterFocus.current = true;
    return;
}

What alternative solutions did you explore? (Optional)

Set shouldPreventDefaultFocusOnSelectRow to true by default so the item can't receive focus by mouse.

[tienifr]

Proposal

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

The currency does not get selected on the first click but moves up to be in the middle of the list

What is the root cause of that problem?

When the user long press on the row, it will trigger focus on the row, causing the focusIndex to change and the list will scroll to the row here.

After the scrolling happens and the user release the press, it no longer works because the mouse is no longer hovered above the row.

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

When the user long press on the row, it will trigger focus on the row

This behavior is inconsistent with native platforms, on native when we press in, the row will not be focused automatically. I don't see any reason why we would want the row to gain focus after pressing, because the user will be navigated to another page after pressing the item. So we should not focus on the item when pressing, so it's consistent with native platforms' behavior.

To do this we can simply update this to

onMouseDown={(e) => e.preventDefault()}

What alternative solutions did you explore? (Optional)

If we want to make the row gain focus when it's pressed in web, we need to do the same for native platforms to be consistent. To do this we need to:

1. In PressableWithFeedback here, listen to onPressIn and if shouldPreventDefaultFocusOnSelectRow is false:

• set the focused index of the list to the item that's being pressed
• set a ref enableScrollToFocusedIndex, to false to disable scrolling to focused index, we can also limit this setting to only when the item is not pressed

2. In here, if enableScrollToFocusedIndex is false, set it to true and early return. So we won't be scrolling to the focused index in this case

[jayeshmangwani]

I am trying to reproduce this issue on staging, but currency selection is working fine for me at the bottom of the list, @bernhardoj and @tienifr are you able to reproduce this issue consistently ?

currenncy-double-click.mov

[bernhardoj]

@jayeshmangwani you need to hold down a bit before releasing the click/press

[tienifr]

Yes @jayeshmangwani Please do a long press

[jayeshmangwani]

Thanks for the help; now I can reproduce this

[jayeshmangwani]

I also feel the same as @tienifr ; we should remove the conditional onMouseDown={shouldPreventDefaultFocusOnSelectRow ? (e) => e.preventDefault() : undefined} and instead we should use the onMouseDown={(e) => e.preventDefault()}, Conditional onMouseDown was added in this PR. Still, I am not sure why it was added conditional instead of directly passing the e.preventDefault() for all the onMouseDown; we can go with the @tienifr 's Proposal of using onMouseDown={(e) => e.preventDefault()}.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[tienifr]

@jayeshmangwani PR #42380 is ready to review

[melvin-bot]

This issue has not been updated in over 15 days. @JmillsExpensify, @Julesssss, @jayeshmangwani, @tienifr 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!

[Julesssss]

PR was just merged

[jayeshmangwani]

@JmillsExpensify PR Deployed to production 2 weeks ago. The issue is ready for payout

[JmillsExpensify]

Payment summary:

• Contributor: @tienifr $250
• Reviewer: @jayeshmangwani $250

Both of you are eligible for NewDot payments. Should I cancel the Upwork contracts?

[jayeshmangwani]

Both of you are eligible for NewDot payments. Should I cancel the Upwork contracts?

Yes 👍

[JmillsExpensify]

I'll assume the same for @tienifr. Btw, what about the checklist?

[mallenexpensify]

Contributor: @tienifr due $250 via NewDot
Contributor+: @jayeshmangwani due $250 via NewDot.

@jayeshmangwani it looks like the checklist didn't populate. plz copy/paste the below. Thx

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:

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

[JmillsExpensify]

Thanks @mallenexpensify! I'm closing this issue and will circle back for payments once I review the expenses in New Expensify. Don't forget the checklist please.

[jayeshmangwani]

• [@jayeshmangwani] The PR that introduced the bug has been identified. Link to the PR: Sync keyboard navigation in SelectionList and PopoverMenu #39201
• [@jayeshmangwani] 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: Sync keyboard navigation in SelectionList and PopoverMenu #39201 (comment)
• [@jayeshmangwani] 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: N/A
• [@jayeshmangwani] Determine if we should create a regression test for this bug. Yes
• [@jayeshmangwani] 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

Regression Test Proposal

1. Open the app.
2. Click on the FAB -> Submit expense.
3. Click on the currency to open the currency selector.
4. Long press on a currency located near the bottom of the list.
5. Verify that the currency gets selected after releasing the long press.

Do we agree 👍 or 👎

[jayeshmangwani]

Requested $250 on NewDot

[JmillsExpensify]

$250 approved for @tienifr

[JmillsExpensify]

$250 approved for @jayeshmangwani

[mallenexpensify]

Testrail

• https://github.com/Expensify/Expensify/issues/413282