[HOLD for payment 2023-09-06] [HOLD for payment 2023-09-04] [$1000] Workspace - CMD+ENTER does not work when inviting new members to WS

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

---

Action Performed:

1. Go to staging.new.expensify.com on Chrome or Safari or open Desktop App
2. Login with Expensifail account
3. Go to existing or create new workspace
4. Go to Settings > Workspace > Members > Invite
5. Select some new members using arrow keys of keyboard and press ENTER
6. Press CMD+ENTER to go to the next page

Expected Result:

User can go to the next page using CMD+ENTER

Actual Result:

Nothing happens

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.57-1

Reproducible in staging?: Yes

Reproducible in production?: No

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

Notes/Photos/Videos: Any additional supporting documentation

Bug6175814_T3611067-CMD-Enter-not-work.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~016a1186aeecdfe466
• Upwork Job ID: 1694821304249151488
• Last Price Increase: 2023-08-24

[OSBotify]

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

[melvin-bot]

Triggered auto assignment to @stitesExpensify (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[jeet-dhandha]

Proposal (Updated)

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

• After selecting workspace members not able to navigate to next screen using cmd+enter

What is the root cause of that problem?

• We don't have any implementation for cmd+enter in WorkspaceInvitePage component.

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

• We can use useKeyboardShortcut like below and it should do the trick.

+ useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER, inviteUser);

return (
       <ScreenWrapper shouldEnableMaxHeight>

What alternative solutions did you explore? (Optional)

• We can implement it directly in SelectionList component to make use in further implementations, just like we have done in OptionsSelector.
  • We can pass an onSumbit prop to SelectionList component with inviteUser function. So than on ctrl+enter it gets called.
  • And inside BaseSelectionComponent below Enter's implemenation we can add this line.

useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER, onSubmit);

[roryabraham]

Shouldn't we use the useKeyboardShortcut hook?

[jeet-dhandha]

Let me check @roryabraham

[jeet-dhandha]

I thought CTRL_ENTER wouldn't have been handled but forgot that BaseOptionSelector is still a class component

[jeet-dhandha]

Yes it works and also i don't have to handle the issue i mentioned above. Thanks @roryabraham. Also i suggest if we can make BaseOptionsSelector into a functional component.

[jeet-dhandha]

Proposal

Updated

[stitesExpensify]

What PR caused this bug?

[jeet-dhandha]

@stitesExpensify No PR has caused this bug. As CMD+ENTER is handled in OptionsSelector by default and in SelectionList its not implemented, which is currently being used in WorkspaceInvitePage page.

[Pujan92]

I am seeing Enter is selecting the item, do we also need to support Ctrl+Enter here?

[stitesExpensify]

In that case, this is not a regression/deploy blocker right? It is new functionality?

[jeet-dhandha]

Yes its a new functionality.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~016a1186aeecdfe466

[OSBotify]

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

[jeet-dhandha]

@roryabraham Please point out the line where CTRL_ENTER implementation was removed in this PR #22622. As then only we can count it as a Regression.

[b4s36t4]

Proposal

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

This is a regression from PR #22622

Previously we have OptionsSelector as the component to render the list of contacts, in which have the listeners for both ENTER and CTRL_ENTER which serves different purpose.

After the refactor we never had any event listener of CTRL_ENTER in SelectionList component which was the issue here.

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 66 to 99 in 53c4743

	componentDidMount() {
	const enterConfig = CONST.KEYBOARD_SHORTCUTS.ENTER;
	this.unsubscribeEnter = KeyboardShortcut.subscribe(
	enterConfig.shortcutKey,
	this.selectFocusedOption,
	enterConfig.descriptionKey,
	enterConfig.modifiers,
	true,
	() => !this.state.allOptions[this.state.focusedIndex],
	);
	const CTRLEnterConfig = CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER;
	this.unsubscribeCTRLEnter = KeyboardShortcut.subscribe(
	CTRLEnterConfig.shortcutKey,
	() => {
	if (this.props.canSelectMultipleOptions) {
	this.props.onConfirmSelection();
	return;
	}
	const focusedOption = this.state.allOptions[this.state.focusedIndex];
	if (!focusedOption) {
	return;
	}
	this.selectRow(focusedOption);
	},
	CTRLEnterConfig.descriptionKey,
	CTRLEnterConfig.modifiers,
	true,
	);
	this.scrollToIndex(this.props.selectedOptions.length ? 0 : this.state.focusedIndex, false);
	}

App/src/components/SelectionList/BaseSelectionList.js

Line 290 in b38181a

useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ENTER, selectFocusedOption, {

What is the root cause of that problem?

Changes we need to do.

1. Introduce a new boolean variable to weather render the footer or not like here

   App/src/components/OptionsSelector/BaseOptionsSelector.js

   Line 304 in 53c4743

   const shouldShowFooter =

   to not conflict with the Next Button we have in the screen and use here to conditionally render

   App/src/components/SelectionList/BaseSelectionList.js

   Lines 383 to 397 in b38181a

   	{shouldShowConfirmButton && (
   	<FixedFooter>
   	<Button
   	success
   	style={[styles.w100]}
   	text={confirmButtonText || translate('common.confirm')}
   	onPress={onConfirm}
   	pressOnEnter
   	enterKeyEventListenerPriority={1}
   	/>
   	</FixedFooter>
   	)}
   	</View>
   	)}
   	</SafeAreaConsumer>

2. Pass inviteUser as onConfirm Prop in the WorkSpaceInvitePage to SelectionList component.

3. Register a listener to listen for CTRL_ENTER commands and do the needful work.

const onCtrlEnter = () => {
    // Do Confirm
    if (canSelectMultiple) {
        if (onConfirm) {
            onConfirm();
        }
        return;
    }

    // Select the focused option
    selectFocusedOption();
};

useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER, onCtrlEnter, 
{captureOnInputs: true, shouldBubble: () => !flattenedSections.allOptions[focusedIndex]});

Mostly replicating what we have in BaseOptionsSelector for CTRL_ENTER command.

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

NA

[burczu]

@jeet-dhandha First of all, if you want to update your proposal you should not add a new comment, instead you should update the previous one and then add a new comment with such content:

## Proposal
[Updated](link to proposal)

Reference here

[Pujan92]

3. Register a listener to listen for CTRL_ENTER commands and do the needful work.

const onCtrlEnter = () => {
    // Do Confirm
    if (canSelectMultiple) {
        if (onConfirm) {
            onConfirm();
        }
        return;
    }

    // Select the focused option
    selectFocusedOption();
};

useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER, onCtrlEnter, 
{captureOnInputs: true, shouldBubble: () => !flattenedSections.allOptions[focusedIndex]});

@b4s36t4 I was thinking the same(only diff for me is to set the default func for onConfirm) but have you faced that the function executes twice for Cmd+Enter?

Screen.Recording.2023-08-25.at.19.53.47.mov

[jeet-dhandha]

@roryabraham - @stitesExpensify
What will happen to those who provided proposal's ?

• As i can see @thiagobrez is gonna help with fixes... https://expensify.slack.com/archives/C01GTK53T8Q/p1692952886670059

[jeet-dhandha]

As in past i know that regression are mostly fixed by the one who caused them and there is no payout for it too.

[mountiny]

Confirmed this is fixed in staging, no report bonus so closing

[melvin-bot]

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

• [CP Staging] fix(selection-list): scroll to top, cmd enter, tab key #25955

If no regressions arise, payment will be issued on 2023-09-04. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @burczu does not require payment (Contractor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

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

• [CP Staging] fix(selection-list): scroll to top, cmd enter, tab key #25955

If no regressions arise, payment will be issued on 2023-09-06. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @burczu does not require payment (Contractor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty