[HOLD for payment 2023-07-26] [$1000] Web - Admin of a workspace is selected and the remove button is activated by Pressing the space key

[kbecciv]

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 setting
2. Workspace
3. Navigate to members
4. Click on the admin check box and then press the space key.
   Notice that Admin of a workspace is selected and the remove Button is activated

Expected Result:

Can't select Admin of a workspace

Actual Result:

Admin of a workspace is selected and the remove Button is activated

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.37-5
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
Notes/Photos/Videos: Any additional supporting documentation

Test63_Adminselect-1.mp4

Reproduce.mov

Recording.3482.mp4

Expensify/Expensify Issue URL:
Issue reported by: @daveSeife
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1688674750643479

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~014fb70565a6eaf9ee
• Upwork Job ID: 1678356671465074688
• Last Price Increase: 2023-07-10

[melvin-bot]

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

[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

[PrashantMangukiya]

Proposal

Posting proposal early as per new guidelines

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

Workspace Admin can be selected and Remove Button enabled by Pressing the space key

What is the root cause of that problem?

When we click checkbox for admin row and press spacebar, it will toggle onPress event for the checkbox from line 321 as shown in code below.

App/src/pages/workspace/WorkspaceMembersPage.js

Lines 318 to 323 in 776ea5d

	<Checkbox
	disabled={disabled}
	isChecked={isChecked}
	onPress={() => toggleUser(item.accountID, item.pendingAction)}
	accessibilityLabel={item.displayName}
	/>

Here we disabled checkbox at line 319 so user will not able to check via mouse, but it will trigger onPress via spacebar so it will toggle admin user also. This is the root cause of the problem.

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

We have to update onPress at line 321 as shown below.
i.e. If checkbox not disabled then call toggleUser() otherwise not.

 <Checkbox
      disabled={disabled}
      isChecked={isChecked}
      //onPress={() => toggleUser(item.accountID, item.pendingAction)}   // *** OLD Code
      onPress={() => !disabled && toggleUser(item.accountID, item.pendingAction)}   // *** Updated Code
      accessibilityLabel={item.displayName}
  />

It will solve the issue as shown in result video.

Note: We can adjust/optimize onPress logic during pr if need.

Additional suggestion:
We can also update onPress for PressableWithFeedback at line 309 here and onSelectRow at line 341 here But 309 and 341 not affected by spacebar so this is not required. We can just update onPress for checkbox.

What alternative solutions did you explore? (Optional)

We can create toggleUserHandler as shown below
const toggleUserHandler = () => !disabled && toggleUser(item.accountID, item.pendingAction);

Replace line 309 with onPress={toggleUserHandler}

and replace line 321 with onPress={toggleUserHandler}

Replace line 341 with onSelectRow={toggleUserHandler}

Result

admin-select.mov

[Pujan92]

Proposal

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

In the Workspace members page admin can be selectable with a space key for the removal

What is the root cause of that problem?

Within Checkbox component which uses GenericPressable component we are not returning early for onKeyPressHandler for disabled state which we are doing for onPressHandler. This seems to be the cause for the issue.

App/src/components/Pressable/GenericPressable/BaseGenericPressable.js

Lines 88 to 114 in ed21fda

	const onPressHandler = useCallback(
	(event) => {
	if (isDisabled) {
	return;
	}
	if (!onPress) {
	return;
	}
	if (shouldUseHapticsOnPress) {
	HapticFeedback.press();
	}
	if (ref && ref.current) {
	ref.current.blur();
	}
	onPress(event);
	Accessibility.moveAccessibilityFocus(nextFocusRef);
	},
	[shouldUseHapticsOnPress, onPress, nextFocusRef, ref, isDisabled],
	);
	const onKeyPressHandler = useCallback(
	(event) => {
	if (event.key !== 'Enter') {
	return;
	}
	onPressHandler(event);

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

We can retrun early when the component is disabled and won't allow executing onPressHandler for onKeyPressHandler space key.

     const onKeyPressHandler = useCallback(
        (event) => {
            if (isDisabled) {
                return;
            }
            if (event.key !== 'Enter') {
                return;
            }
            onPressHandler(event);
        },
        [onPressHandler, isDisabled],
    );

What alternative solutions did you explore? (Optional)

We can only execute handleSpaceKey when it is not disabled within Checkbox component.
onKeyDown={!props.disabled ? handleSpaceKey : undefined}

App/src/components/Checkbox.js

Line 81 in ed21fda

onKeyDown={handleSpaceKey}

[bernhardoj]

Proposal

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

We can toggle disabled checkbox with space.

What is the root cause of that problem?

Checkbox component has a onKeyDown listener that will toggle the value by pressing Space key.

App/src/components/Checkbox.js

Lines 55 to 62 in ed21fda

	function Checkbox(props) {
	const handleSpaceKey = (event) => {
	if (event.code !== 'Space') {
	return;
	}
	props.onPress();
	};

If the member is an admin, we will disable the Checkbox. Both onKeyDown and disabled props are passed down to GenericPressable. In GenericPressable, we do quite a lot of manually disabling events when the pressable is disabled.

App/src/components/Pressable/GenericPressable/BaseGenericPressable.js

Lines 140 to 144 in ed21fda

	onPress={!isDisabled ? onPressHandler : undefined}
	onLongPress={!isDisabled && onLongPress ? onLongPressHandler : undefined}
	onKeyPress={!isDisabled ? onKeyPressHandler : undefined}
	onPressIn={!isDisabled ? onPressIn : undefined}
	onPressOut={!isDisabled ? onPressOut : undefined}

But in GenericPressable, we don't specifically handle onKeyDown and just pass it as the rest.

App/src/components/Pressable/GenericPressable/BaseGenericPressable.js

Lines 165 to 166 in ed21fda

	{...rest}
	>

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

The other solution above will do the job, but I just want to share my suggestion to completely fix the underlying issue. There will be quite a lot of changes 😄

In my opinion, it's not very reliable to pass undefined to every possible callback available in Pressable. We can easily miss it, like in this issue. So, we should completely disable the Pressable when isDisabled is true.

disable={isDisabled}

This will set pointerEvents to none, thus disabling any interaction with the component. The downside of this is, we can't have a not-allowed cursor. This is normal, but as we want to show not-allowed cursor when it's disabled, we can wrap the Pressable with a View and move the cursor style from Pressable to the View. With these changes, we don't need isDisabled && onPress and such anymore.

Last, we have OpacityView in PressableWithFeedback. To not have multiple unnecessary View, we can move this to GenericPressable (and it's logic). We will conditionally render between OpacityView and View based on whether we want a feedback or not.

const Wrapper = props.withFeedback ? OpacityView : View;

What alternative solutions did you explore? (Optional)

Disable the keydown listener in BaseGenericPressable when the button is disabled.

onKeyDown={!isDisabled ? onKeyDown : undefined}

[b4s36t4]

Proposal

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

Web - Admin of a workspace is selected and the remove button is activated by Pressing the space key

What is the root cause of that problem?

Root cause of this issue is that we have a onKeyDown handler in CheckBox which will listen to events and call onPress if it's Space event. If we observe the underlying component GenericPressable is having a handler for onKeyPress. But in the CheckBox Component we're giving a onKeyDown handler which not relevant. And there's no propType Definition for onKeyPress as well which means it's an unintended behaviour.

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

Update the propKey here

App/src/components/Checkbox.js

Line 81 in 9d73692

onKeyDown={handleSpaceKey}

to onKeyPress.

What alternative solutions did you explore? (Optional)

Maybe handle the onKeyDown event in underlying GenericPressable component. But I think onKeyPress should work in most cases unless we're doing different on onKeyDown and onKeyUp. So sticking to the onKeyPress prop should be good enough.

[anmurali]

Reproduced on staging in Chrome/ Mac

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~014fb70565a6eaf9ee

[melvin-bot]

Current assignee @anmurali is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[melvin-bot]

@eVoloshchak, @anmurali Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[anmurali]

@eVoloshchak can you review the proposals here and pick one?

[eVoloshchak]

@PrashantMangukiya's proposal will work, but only for this specific page, I think we should strive for a more universal approach

@Pujan92 , I don't think your approach will resolve the issue. There is no need to add isDisabled check to onKeyPressHandler, since the same check is already performed in

App/src/components/Pressable/GenericPressable/BaseGenericPressable.js

Line 142 in ed21fda

onKeyPress={!isDisabled ? onKeyPressHandler : undefined}

@bernhardoj's proposal looks good to me!

In my opinion, it's not very reliable to pass undefined to every possible callback available in Pressable. We can easily miss it, like in this issue

I think it's ok to have a separate method/logic for each callback. There is a finite number of callbacks for Pressable, there won't be a bunch of new ones, and that will allow us to have a more flexible logic if we need to

🎀👀🎀 C+ reviewed!

[melvin-bot]

Triggered auto assignment to @bondydaa, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[eVoloshchak]

@b4s36t4, thanks for the proposal!

The onkeypress event is deprecated.
It is not fired for all keys (like ALT, CTRL, SHIFT, ESC) in all browsers.
To detect if the user presses a key, always use the onkeydown event. It works for all keys.

[bernhardoj]

I think it's ok to have a separate method/logic for each callback

@eVoloshchak so, are we going to do this in BaseGenericPressable (bcs I didn't specifically mention this solution, if yes, I will add it to the alternative):
onKeyDown={!isDisabled && props.onKeyDown : null}

or the disabled solution?

[melvin-bot]

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

• Disable onkeydown when Pressable is disabled #22926

If no regressions arise, payment will be issued on 2023-07-26. 🎊

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

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]

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:

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

[eVoloshchak]

• The PR that introduced the bug has been identified. Link to the PR: 17051 - migrated Checkbox to PressableWithFeedback #20257
• 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: https://github.com/Expensify/App/pull/20257/files#r1275368556
• 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: I don't think an additional discussion is needed here, this should have been caught during the review

[eVoloshchak]

Regression Test Proposal

1. Go to setting -> Workspace
2. Navigate to members
3. Click on the admin check box and then press the space key.
4. Verify the admin checkbox isn't selected

Do we agree 👍 or 👎

[eVoloshchak]

Requested the payment via NewDot

[JmillsExpensify]

@anmurali Can you please summarize the appropriate individual payments for all parties involved in this issue? This is holding up @eVoloshchak's NewDot payments. More information on this compliance process in Slack.

[melvin-bot]

@bondydaa, @eVoloshchak, @anmurali, @bernhardoj Huh... This is 4 days overdue. Who can take care of this?

[JmillsExpensify]

Reached our via DM

[anmurali]

• Reported @daveSeife $250
• Contributor @bernhardoj $1000 + $500 (speed bonus)
• Contributor + @eVoloshchak $1000 + $500 (speed bonus)

[JmillsExpensify]

Reviewed details for @eVoloshchak. These details are accurate based on summary from Business Reviewer and are now approved for payment in NewDot.

[melvin-bot]

@bondydaa, @eVoloshchak, @anmurali, @bernhardoj Whoops! This issue is 2 days overdue. Let's get this updated quick!

[bondydaa]

i think waiting for payment still @JmillsExpensify @anmurali ?

[JmillsExpensify]

No for NewDot, we're good on that front. 🙌🏼

[anmurali]

@bernhardoj - upwork.com/nx/wm/offer/26037915
You need to accept the offer and then I can pay and close this.

[bernhardoj]

@anmurali accepted!

[anmurali]

Paid.