[HOLD for payment 2024-01-25] [$500] Workspace - Enter click does not trigger workspace remove members button

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: v1.4.11-6
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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

Open the app
Open settings
Open workspaces
Open any workspace
Open Members
Select any existing user and click on remove
Click enter on popup, observe that it does nothing
Click enter again and observe that it cancels the remove

Expected Result:

App should trigger remove button in popup on enter click as we do throughout the app for similar buttons

Actual Result:

App does not trigger remove button in popup for remove members on click of enter

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

Bug6310160_1702370222507.windows_chrome_-_enter_click_remove_button_issue.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01fd33a43f9c145685
Upwork Job ID: 1736043912087367680
Last Price Increase: 2024-01-13
Automatic offers:

shubham1206agra | Reviewer | 28103052
ishpaul777 | Contributor | 28103053

@kevinksullivan

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

Job added to Upwork: https://www.upwork.com/jobs/~01fd33a43f9c145685

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

@allroundexperts

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

Problem

Proposal

Workspace - Enter click does not trigger workspace remove members

Root Cause

When enter is triggered first time the sectionlist item is unselected because there is enter key listner still active on the page.

https://github.com/Expensify/App/blob/main/src/components/SelectionList/BaseSelectionList.js#L384

Changes

We have prop disableKeyboardShortcuts in SectionList, we just need to pass is true when modal is visible to enter key click is consumed by the button not the list item

Proposal

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

Enter click does not trigger workspace remove members button

What is the root cause of that problem?

This is because we don't disable the Keyboard Shortcut on BaseSelectionList when a modal is opened:

App/src/components/SelectionList/BaseSelectionList.js

Line 159 in 3ed43ff

    
           const disableEnterShortcut = activeElement && [CONST.ROLE.BUTTON, CONST.ROLE.CHECKBOX].includes(activeElement.role);

When a modal is opened the active element changes to modal view, and activeElement.role is null
but since we are missing a null check, the shortcut is not disabled/

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

We should update the line here:

App/src/components/SelectionList/BaseSelectionList.js

Line 159 in 3ed43ff

    
           const disableEnterShortcut = activeElement && [CONST.ROLE.BUTTON, CONST.ROLE.CHECKBOX].includes(activeElement.role);

to:

const disableEnterShortcut = activeElement && ( _.isNull(activeElement.role) || [CONST.ROLE.BUTTON, CONST.ROLE.CHECKBOX].includes(activeElement.role));

Result:

Screen.Recording.2023-12-16.at.9.27.42.PM.mov

Proposal

Updated

Added video demostrating result of code changes.

Proposal

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

Enter click does not trigger workspace remove members button

What is the root cause of that problem?

The root cause of the problem is that pressing enter after the modal is open will be listened by the BaseSelectionList so it deselects the selected item instead of pressing the Remove button in the modal. Basically we have got a proper disabling enter key logic in here

App/src/components/SelectionList/BaseSelectionList.js

Lines 159 to 160 in 8bef4bb

    
           const disableEnterShortcut = activeElement && [CONST.ROLE.BUTTON, CONST.ROLE.CHECKBOX].includes(activeElement.role);

which disables when the active element is checkbox or button and which is correct but in this case Remove button is not focused.

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

We should exploit the correct disabling enter shourtcut here

App/src/components/SelectionList/BaseSelectionList.js

Lines 159 to 160 in 8bef4bb

    
           const disableEnterShortcut = activeElement && [CONST.ROLE.BUTTON, CONST.ROLE.CHECKBOX].includes(activeElement.role);

And we should focus on the remove button whenever the modal is rendered or focused (useEffect or useFocusEffect). These will solve similar problems where we use the confirm modal.
In ConfirmContent

const confirmButton = useRef(null);

    useEffect(() => {
        confirmButton.current?.focus();
    },[]);

What alternative solutions did you explore? (Optional)

In SelectionList we can create a new prop disableKeyboard and disable both arrow key and enter key shortcuts when the modal is open. So in WorkspaceMembersPage we can pass the prop depending on the state removeMembersConfirmModalVisible to disable keyboard on the SelectionList while the modal is open. I don't think it is proper that arrow keys are working for the list while the modal is open as it does now. We can also use this prop on other selection list instances where we want to disable the keyboard when modal is open.

Reviewing label has been removed, please complete the "BugZero Checklist".

@shubham1206agra

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.26-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

fixes the issue "Enter click does not trigger workspace remove members button" #34563

If no regressions arise, payment will be issued on 2024-01-25. 🎊

For reference, here are some details about the assignees on this issue:

@shubham1206agra requires payment automatic offer (Reviewer)
@ishpaul777 requires payment automatic offer (Contributor)

@amyevans

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:

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

@shubham1206agra

@shubham1206agra mind finishing out the checklist above?

@kevinksullivan

Hey @kevinksullivan, Shumbam is OOO until 29th Jan. https://expensify.slack.com/archives/C02NK2DQWUX/p1706026202997839

@shubham1206agra

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:

[@shubham1206agra] The PR that introduced the bug has been identified. Link to the PR: refactor: SelectionList multiple selection #22622
[@shubham1206agra] 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: refactor: SelectionList multiple selection #22622 (comment)
[@shubham1206agra] 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: Not required
[@shubham1206agra] Determine if we should create a regression test for this bug. No
[@shubham1206agra] 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.
[@kevinksullivan] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@kevinksullivan

@kevinksullivan friendly bump

@amyevans

@amyevans, @kevinksullivan, @shubham1206agra, @ishpaul777 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

sorry for the delay, all set!

Navigation Menu

Repository navigation

[HOLD for payment 2024-01-25] [$500] Workspace - Enter click does not trigger workspace remove members button #33201

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Bug0 Triage Checklist (Main S/O)

Problem

Proposal

Root Cause

Changes

Proposal

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

What is the root cause of that problem?

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

Result:

Proposal

Updated

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

73 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	const disableEnterShortcut = activeElement && [CONST.ROLE.BUTTON, CONST.ROLE.CHECKBOX].includes(activeElement.role);

[HOLD for payment 2024-01-25] [$500] Workspace - Enter click does not trigger workspace remove members button #33201

Description

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Activity

melvin-bot commented on Dec 16, 2023

melvin-bot commented on Dec 16, 2023

melvin-bot commented on Dec 16, 2023

Bug0 Triage Checklist (Main S/O)

melvin-bot commented on Dec 16, 2023

ishpaul777 commented on Dec 16, 2023

Problem

Proposal

Root Cause

Changes

neonbhai commented on Dec 16, 2023

Proposal

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

What is the root cause of that problem?

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

Result:

neonbhai commented on Dec 16, 2023

Proposal

Updated

FitseTLT commented on Dec 16, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

73 remaining items

melvin-bot commented on Jan 18, 2024

melvin-bot commented on Jan 18, 2024

melvin-bot commented on Jan 18, 2024

kevinksullivan commented on Jan 24, 2024

ishpaul777 commented on Jan 25, 2024

shubham1206agra commented on Feb 1, 2024

ishpaul777 commented on Feb 2, 2024

melvin-bot commented on Feb 6, 2024

kevinksullivan commented on Feb 6, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions