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

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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:

1. Open the app
2. Open settings
3. Open workspaces
4. Open any workspace
5. Open Members
6. Select any existing user and click on remove
7. Click enter on popup, observe that it does nothing
8. 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?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

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]

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

[melvin-bot]

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

[melvin-bot]

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]

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

[ishpaul777]

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]

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]

Proposal

Updated

Added video demostrating result of code changes.

[FitseTLT]

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]

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.

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

2. 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]

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]

Hey @allroundexperts can you give these a review?

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

@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]

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

[melvin-bot]

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

[melvin-bot]

📣 @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]

📣 @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]

@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]

Sorry @DylanDylann, your solution is still not right.

[DylanDylann]

@shubham1206agra

your solution is still not right.

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

[melvin-bot]

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

[melvin-bot]

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]

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]

@shubham1206agra mind finishing out the checklist above?

[ishpaul777]

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:

[ishpaul777]

@kevinksullivan friendly bump

[melvin-bot]

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

[kevinksullivan]

sorry for the delay, all set!