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

kbecciv · 2023-12-16T15:21:34Z

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

melvin-bot · 2023-12-16T15:21:39Z

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

melvin-bot · 2023-12-16T15:21:39Z

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

melvin-bot · 2023-12-16T15:21:41Z

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

melvin-bot · 2023-12-16T15:21:45Z

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

ishpaul777 · 2023-12-16T15:47:02Z

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

neonbhai · 2023-12-16T15:51:35Z

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

neonbhai · 2023-12-16T15:59:49Z

Proposal

Updated

Added video demostrating result of code changes.

FitseTLT · 2023-12-16T16:17:59Z

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.

DylanDylann · 2023-12-16T18:18:30Z

Proposal

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

Workspace - Enter click does not trigger workspace remove members button

What is the root cause of that problem?

Currently, in the WorkspaceMembersPage, we have two component that are listening to the ENTER event:
The first one is the ConfirmModal (That based on the ConfirmContent (Called listener 1)
And the other one is SelectionList (That based on the BaseSelectionList (Called listener 2)
The listener 1 is setup first, and both listeners have the same priority (0), that leads to once the ENTER is pressed, it will call the listener 2 's callback rather then listener 1 's callback. (Because the listener 2 ' callback is at the top of the callback stack)
That leads to the current behavior: Pressing on ENTER will deselect the member option rather than trigger the workspace remove members button.

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

Generally, we need to make sure listener 1 is always at the top of the callback stack and set the shouldBubble: false for this, so when the ENTER is pressed, listener 1 's callback is called and then skip the rest listeners.

Method 1: Always re-subscribe the listener 1 once the modal is opened, so it will make sure the listener 1 is always pushed to the top of the callback stack. (This also works for other similar cases, not just this case)

Do it by updating:

App/src/components/ConfirmContent.js

Line 169 in 3ed43ff

export default ConfirmContent;

to:

export default compose(
    withOnyx({
        modal: {
            key: ONYXKEYS.MODAL,
        },
    }),
)(ConfirmContent);

In this solution, once the modal is opened, the ConfirmContent will re-render, so the listener 1 is re-subscribed, so it will be push to the top of the callback stack.

Method 2: Set the listener 1 's priority to 0, and listener 2 's priority to 1 in this case, so will can make sure the listener 1 's callback is always called.

What alternative solutions did you explore? (Optional)

We can disable the listener 2 once the ```willAlertModalBecomeVisible: true````. Do it by updating:

App/src/components/SelectionList/BaseSelectionList.js

Line 387 in 8e2fb67

isActive: !disableKeyboardShortcuts && !disableEnterShortcut && isFocused,

to:

  isActive: !disableKeyboardShortcuts && !disableEnterShortcut && isFocused && !modal.willAlertModalBecomeVisible,

Krishna2323 · 2023-12-16T18:31:04Z

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 issue arises from the fact that, when the modal is open, pressing the Enter key triggers the event listener in the BaseSelectionList component. Consequently, instead of executing the intended action of pressing the Remove button within the modal, the Enter key leads to the unintended deselection of the currently selected item. Despite having a functioning logic to disable the Enter key, it appears that this logic is not effectively preventing interference with the BaseSelectionList component when the modal is active.

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

To resolve the issue, we can enhance the modal's accessibility by introducing a new attribute, role, within the CONST file. Specifically, we'll add a DIALOG: 'dialog' attribute to represent the role of a dialog. Inside the BaseModal component, a custom backdrop will be implemented, assigned the role of a dialog, and incorporated into the modal structure. Additionally, the disableEnterShortcut logic will be updated to recognize the focused element's role. If the focused element has the role of a dialog, the logic will prevent the Enter key from triggering.

// Custom Backdrop
<PressableWithoutFeedback
    tabIndex={0}
    onPress={onClose}
    role={CONST.ROLE.DIALOG}
    accessibilityLabel="modal"
    style={{flex: 1, backgroundColor: theme.overlay, opacity: hideBackdrop ? 0 : variables.overlayOpacity}}
/>

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

Result

modal_press_on_enter.mp4

kevinksullivan · 2023-12-19T23:58:14Z

Hey @allroundexperts can you give these a review?

melvin-bot · 2023-12-23T16:00:03Z

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot · 2023-12-25T11:44:13Z

@kevinksullivan, @allroundexperts Huh... This is 4 days overdue. Who can take care of this?

melvin-bot · 2023-12-27T11:37:20Z

@kevinksullivan, @allroundexperts 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

melvin-bot · 2023-12-29T11:23:22Z

@kevinksullivan, @allroundexperts 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

melvin-bot · 2023-12-30T16:40:19Z

@kevinksullivan @allroundexperts this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot · 2023-12-30T16:56:22Z

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot · 2024-01-02T17:35:17Z

@kevinksullivan, @allroundexperts 12 days overdue now... This issue's end is nigh!

melvin-bot · 2024-01-15T23:11:36Z

📣 @shubham1206agra 🎉 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 · 2024-01-15T23:11:40Z

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

DylanDylann · 2024-01-16T05:28:04Z

@shubham1206agra @amyevans Sorry for my late. I`ve updated the proposal (What alternative solutions did you explore? (Optional) ) so in the future, we do not need to make any changes once the modal is opened

shubham1206agra · 2024-01-16T05:45:57Z

Sorry @DylanDylann, your solution is still not right.

DylanDylann · 2024-01-16T05:51:00Z

@shubham1206agra

your solution is still not right.

Can you give me more details, thanks. I just tested and it works well

melvin-bot · 2024-01-18T03:04:44Z

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

melvin-bot · 2024-01-18T03:04:47Z

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)

melvin-bot · 2024-01-18T03:04:49Z

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:

kevinksullivan · 2024-01-24T23:27:08Z

@shubham1206agra mind finishing out the checklist above?

ishpaul777 · 2024-01-25T00:11:57Z

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

shubham1206agra · 2024-02-01T08:59:36Z

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:

ishpaul777 · 2024-02-02T22:10:42Z

@kevinksullivan friendly bump

melvin-bot · 2024-02-06T15:10:11Z

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

kevinksullivan · 2024-02-06T15:14:36Z

sorry for the delay, all set!

kbecciv added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 16, 2023

melvin-bot bot assigned kevinksullivan Dec 16, 2023

melvin-bot bot changed the title ~~Workspace - Enter click does not trigger workspace remove members button~~ [$500] Workspace - Enter click does not trigger workspace remove members button Dec 16, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 16, 2023

melvin-bot bot assigned allroundexperts Dec 16, 2023

melvin-bot bot added the Overdue label Dec 19, 2023

melvin-bot bot added Overdue and removed Overdue labels Dec 19, 2023

melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Jan 5, 2024

ishpaul777 mentioned this issue Jan 16, 2024

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

Merged

50 tasks

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 Weekly KSv2 labels Jan 16, 2024

melvin-bot bot changed the title ~~[$500] Workspace - Enter click does not trigger workspace remove members button~~ [HOLD for payment 2024-01-25] [$500] Workspace - Enter click does not trigger workspace remove members button Jan 18, 2024

melvin-bot bot removed the Reviewing Has a PR in review label Jan 18, 2024

shubham1206agra mentioned this issue Feb 1, 2024

refactor: SelectionList multiple selection #22622

Merged

62 tasks

amyevans added Daily KSv2 and removed Weekly KSv2 labels Feb 1, 2024

melvin-bot bot added the Overdue label Feb 5, 2024

kevinksullivan closed this as completed Feb 6, 2024

melvin-bot bot removed the Overdue label Feb 6, 2024

shubham1206agra mentioned this issue Apr 16, 2024

[HOLD for #42645] [$250] Chat - Focus Sticks on Cancel Button After Clearing Message & Saving with Enter Key #39820

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kbecciv commented Dec 16, 2023 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Dec 16, 2023

melvin-bot bot commented Dec 16, 2023

melvin-bot bot commented Dec 16, 2023

melvin-bot bot commented Dec 16, 2023

ishpaul777 commented Dec 16, 2023 •

edited

Loading

neonbhai commented Dec 16, 2023 •

edited

Loading

neonbhai commented Dec 16, 2023 •

edited

Loading

FitseTLT commented Dec 16, 2023 •

edited

Loading

DylanDylann commented Dec 16, 2023 •

edited

Loading

Krishna2323 commented Dec 16, 2023 •

edited

Loading

kevinksullivan commented Dec 19, 2023

melvin-bot bot commented Dec 23, 2023

melvin-bot bot commented Dec 25, 2023

melvin-bot bot commented Dec 27, 2023

melvin-bot bot commented Dec 29, 2023

melvin-bot bot commented Dec 30, 2023

melvin-bot bot commented Dec 30, 2023

melvin-bot bot commented Jan 2, 2024

melvin-bot bot commented Jan 15, 2024

melvin-bot bot commented Jan 15, 2024

DylanDylann commented Jan 16, 2024 •

edited

Loading

shubham1206agra commented Jan 16, 2024

DylanDylann commented Jan 16, 2024 •

edited

Loading

melvin-bot bot commented Jan 18, 2024

melvin-bot bot commented Jan 18, 2024

melvin-bot bot commented Jan 18, 2024

kevinksullivan commented Jan 24, 2024

ishpaul777 commented Jan 25, 2024

shubham1206agra commented Feb 1, 2024

ishpaul777 commented Feb 2, 2024

melvin-bot bot commented Feb 6, 2024

kevinksullivan commented Feb 6, 2024

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

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

Comments

kbecciv commented Dec 16, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Dec 16, 2023

melvin-bot bot commented Dec 16, 2023

melvin-bot bot commented Dec 16, 2023

Bug0 Triage Checklist (Main S/O)

melvin-bot bot commented Dec 16, 2023

ishpaul777 commented Dec 16, 2023 • edited Loading

Problem

Proposal

Root Cause

Changes

neonbhai commented Dec 16, 2023 • edited Loading

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 Dec 16, 2023 • edited Loading

Proposal

Updated

FitseTLT commented Dec 16, 2023 • edited Loading

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)

DylanDylann commented Dec 16, 2023 • edited Loading

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)

Krishna2323 commented Dec 16, 2023 • edited Loading

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

kevinksullivan commented Dec 19, 2023

melvin-bot bot commented Dec 23, 2023

melvin-bot bot commented Dec 25, 2023

melvin-bot bot commented Dec 27, 2023

melvin-bot bot commented Dec 29, 2023

melvin-bot bot commented Dec 30, 2023

melvin-bot bot commented Dec 30, 2023

melvin-bot bot commented Jan 2, 2024

melvin-bot bot commented Jan 15, 2024

melvin-bot bot commented Jan 15, 2024

DylanDylann commented Jan 16, 2024 • edited Loading

shubham1206agra commented Jan 16, 2024

DylanDylann commented Jan 16, 2024 • edited Loading

melvin-bot bot commented Jan 18, 2024

melvin-bot bot commented Jan 18, 2024

melvin-bot bot commented Jan 18, 2024

kevinksullivan commented Jan 24, 2024

ishpaul777 commented Jan 25, 2024

shubham1206agra commented Feb 1, 2024

ishpaul777 commented Feb 2, 2024

melvin-bot bot commented Feb 6, 2024

kevinksullivan commented Feb 6, 2024

kbecciv commented Dec 16, 2023 •

edited by melvin-bot bot

Loading

ishpaul777 commented Dec 16, 2023 •

edited

Loading

neonbhai commented Dec 16, 2023 •

edited

Loading

neonbhai commented Dec 16, 2023 •

edited

Loading

FitseTLT commented Dec 16, 2023 •

edited

Loading

DylanDylann commented Dec 16, 2023 •

edited

Loading

Krishna2323 commented Dec 16, 2023 •

edited

Loading

DylanDylann commented Jan 16, 2024 •

edited

Loading

DylanDylann commented Jan 16, 2024 •

edited

Loading