[HOLD for payment 2024-05-13] [$250] Web - Pressing on keyboard enter key on notification option of a room takes back two steps

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

---

Version Number: 1.4.63-0
Reproducible in staging?: y
Reproducible in production?: no
If this was caught during regression testing, add the test name, ID and link from TestRail: EXP around https://expensify.testrail.io/index.php?/tests/view/4494233
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause internal team
Slack conversation:

Action Performed:

1. Navigate to staging.new.expensify.com
2. Create a room
3. Open the room page
4. Click on header
5. Click on settings
6. Click on Notify me about new messages
7. Select a notification preference option by using the keyboard arrow keys
8. Press enter on one of the options

Expected Result:

App should navigate to Settings page

Actual Result:

App navigates to Room details page

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

Bug6454735_1713521652242.bandicam_2024-04-19_13-08-56-915.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01889a2544214bb02d
• Upwork Job ID: 1781342777391833088
• Last Price Increase: 2024-04-19
• Automatic offers:
• suneox | Contributor | 0

Issue Owner

Current Issue Owner: @kevinksullivan

[melvin-bot]

Triggered auto assignment to @kevinksullivan (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.

[melvin-bot]

Triggered auto assignment to @mountiny (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

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

[kavimuru]

@mountiny 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.

[kavimuru]

We think this bug might be related to #vip-vsb

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01889a2544214bb02d

[melvin-bot]

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

[suneox]

Proposal

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

Web - Pressing on keyboard enter key on notification option of a room takes back two steps

What is the root cause of that problem?

We have a double event raised when pressing the enter key at BaseListItem, and keyboard shortcuts at this lines
So updateNotificationPreference will we call 2 times and navigate back also call 2 times

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

Due to at BaseSelectionList we have handle keyboard shortcut to select item, so we should prevent press event at this line when submit by keyboard by adding props shouldPreventEnterKeySubmit into BaseListItem then check condition event from keyboard or not

      ref={pressableRef}
      onPress={(e) => {
          if (shouldPreventEnterKeySubmit && e instanceof KeyboardEvent && e.key === 'Enter') {
              return;
          }
          onSelectRow(item);
      }}

Finally set shouldPreventEnterKeySubmit at renderItem function in BaseSelectionList

What alternative solutions did you explore? (Optional)

Instead of add props to BaseListItem, we can check condition and pass props disableEnterShortcut into BaseSelectionList

[gijoe0295]

Dupe of #40507.

[allroundexperts]

Since we have a better proposal here, we can close the other.

[allroundexperts]

@suneox Were you able to figure out the PR which caused this issue?

[suneox]

@suneox Were you able to figure out the PR which caused this issue?

Sure, Let me find

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

[suneox]

@suneox Were you able to figure out the PR which caused this issue?

@allroundexperts I think it looks like a regression from this PR when the fix uses a tab key to focus an item, but the owner isn't aware the code base also has a shortcut key to select an item with the condition isFocused to active the shortcut

App/src/components/SelectionList/BaseSelectionList.tsx

Lines 486 to 491 in 9a115b6

	useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ENTER, selectFocusedOption, {
	captureOnInputs: true,
	shouldBubble: !flattenedSections.allOptions[focusedIndex],
	shouldStopPropagation,
	isActive: !disableKeyboardShortcuts && !disableEnterShortcut && isFocused,
	});

So We have 2 places trigger onSelectRow event at this line (current focused) and event by shortcut key

Tested branch

[allroundexperts]

PR: #40642

[suneox]

PR: #40642

Hi @allroundexperts could you please check my commit your PR was missing set shouldPreventEnterKeySubmit=true for the BaseSelectionList so the result still not fix

[allroundexperts]

Checking 👀

[suneox]

Hi @allroundexperts could you please check my commit your PR was missing set shouldPreventEnterKeySubmit=true for the BaseSelectionList so the result still not fix

App/src/pages/settings/Report/NotificationPreferencePage.tsx

Lines 41 to 45 in 4dc0d91

	<SelectionList
	sections={[{data: notificationPreferenceOptions}]}
	ListItem={RadioListItem}
	onSelectRow={(option) =>
	report && ReportActions.updateNotificationPreference(report.reportID, report.notificationPreference, option.value, true, undefined, undefined, report)

I'd like to explain for my commit change

At page NotificationPreferencePage SelectionList using ListItem={RadioListItem} so we have to forward shouldPreventEnterKeySubmit props inside RadioListItem

And we can use CONST.KEYBOARD_SHORTCUTS.ENTER.shortcutKey instead of string 'Enter'

[allroundexperts]

Hi @allroundexperts could you please check my commit your PR was missing set shouldPreventEnterKeySubmit=true for the BaseSelectionList so the result still not fix

App/src/pages/settings/Report/NotificationPreferencePage.tsx

Lines 41 to 45 in 4dc0d91

	<SelectionList
	sections={[{data: notificationPreferenceOptions}]}
	ListItem={RadioListItem}
	onSelectRow={(option) =>
	report && ReportActions.updateNotificationPreference(report.reportID, report.notificationPreference, option.value, true, undefined, undefined, report)

I'd like to explain for my commit change

At page NotificationPreferencePage SelectionList using ListItem={RadioListItem} so we have to forward shouldPreventEnterKeySubmit props inside RadioListItem

And we can use CONST.KEYBOARD_SHORTCUTS.ENTER.shortcutKey instead of string 'Enter'

Is this limited to RadioListItem only? I feel like we should do this for all list items.

[suneox]

Is this limited to RadioListItem only? I feel like we should do this for all list items.

Yes, maybe we should apply for all lists but we don't have any context for other list items. So I think we can go ahead with this list first. But we have to set shouldPreventEnterKeySubmit=true at BaseSelectionList instead of passing from props

[allroundexperts]

@suneox Makes sense. Let's fix the radio item list first. Updated the PR.

[allroundexperts]

Will update the screen recordings in the next few mins as well.

[suneox]

Could you please check your merge change? It's mixing with another commit and the code change also update MoneyRequestView file

[allroundexperts]

@suneox Updated and added screen recordings. Let's take this convo to the PR.

[suneox]

@suneox you can test the PR as a C+ role

@mountiny Could you assign me a review on this PR

PR look good to me but i can not approve these change

[mountiny]

@suneox I believe you cannot review a you are not C+, but you can leave comments with the checklist and approve by comment

[suneox]

The PR has been deployed to production 3 days ago, I think we should update hold for payment

[suneox]

Hi @mountiny This issue has been deployed to production over 1 week, so I think we should process payment. Thank you!

[suneox]

Friendly bump @mountiny

[suneox]

@mountiny could you please process payment for this issue?

[kevinksullivan]

Payment summary:

• @suneox $250
• @allroundexperts $250 (NewDot)

[JmillsExpensify]

$250 approved for @allroundexperts