[$500] Workspace - Invite member - Member highlights on deselect all members

[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 settings> workspace
2. Create new workspace if not created
3. Go to members
4. Invite any 2 members
5. Click on select all check box
6. Deselect last selected member
7. Deselect second selected member

Expected Result:

Member should not be highlights

Actual Result:

Member highlights

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

RPReplay_Final1694422450.MP4

Screen_Recording_20230913_125101_New.Expensify.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01f0d617af5beeb293
• Upwork Job ID: 1702003634763894784
• Last Price Increase: 2023-09-13
• Automatic offers:
• s77rt | Reviewer | 26724768
• akamefi202 | Contributor | 26724771
• gadhiyamanan | Reporter | 26724774

[melvin-bot]

Triggered auto assignment to @Christinadobrzyn (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

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @joekaufmanexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

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

[joekaufmanexpensify]

@Christinadobrzyn I was duplicate assigned here, so un-assigning as this doesn't need two BZ members. We are fixing this here.

[akamefi202]

Proposal

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

Member highlights on deselect all members in workspace members page.

What is the root cause of that problem?

App/src/components/SelectionList/BaseSelectionList.js

Lines 173 to 177 in 7deca7a

	if (sections.length === 1) {
	// If the list has only 1 section (e.g. Workspace Members list), we always focus the next available item
	const nextAvailableIndex = _.findIndex(flattenedSections.allOptions, (option, i) => i > index && !option.isDisabled);
	setFocusedIndex(nextAvailableIndex);
	} else {

If the user select/deselect a member, selectRow function is called and the function focuses the next member in the list.

App/src/components/SelectionList/BaseSelectionList.js

Lines 192 to 200 in 7deca7a

	const selectFocusedOption = () => {
	const focusedOption = flattenedSections.allOptions[focusedIndex];
	if (!focusedOption || focusedOption.isDisabled) {
	return;
	}
	selectRow(focusedOption, focusedIndex);
	};

This is implemented for continuous selection & deselection by Enter keyboard shortcut.
The code is written inside selectRow function and it will be called from selectFocusedOption.

But selectRow function is also called if the user select/deselect by click/press.
So the next member will be focused too and we see current issue.

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

App/src/components/SelectionList/BaseSelectionList.js

Lines 253 to 259 in 4ff85db

	<UserListItem
	item={item}
	isFocused={isFocused}
	onSelectRow={() => selectRow(item, index)}
	onDismissError={onDismissError}
	showTooltip={showTooltip}
	/>

I think we need to call onSelectRow function directly instead of calling selectRow function if the user select/deselect by click/press.
So the app will only toggle the item without moving focus to the next item.

What alternative solutions did you explore? (Optional)

N/A

[DylanDylann]

Proposal

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

• Member highlights on deselect all members in workspace members page.

What is the root cause of that problem?

• Currently, we have a logic to auto-focus on the next available item when the users click/press ENTER on the item. The logic is like below:
  

  App/src/components/SelectionList/BaseSelectionList.js

  Lines 170 to 190 in 4ff85db

  	const selectRow = (item, index) => {
  	// In single-selection lists we don't care about updating the focused index, because the list is closed after selecting an item
  	if (canSelectMultiple) {
  	if (sections.length === 1) {
  	// If the list has only 1 section (e.g. Workspace Members list), we always focus the next available item
  	const nextAvailableIndex = _.findIndex(flattenedSections.allOptions, (option, i) => i > index && !option.isDisabled);
  	setFocusedIndex(nextAvailableIndex);
  	} else {
  	// If the list has multiple sections (e.g. Workspace Invite list), we focus the first one after all the selected (selected items are always at the top)
  	const selectedOptionsCount = item.isSelected ? flattenedSections.selectedOptions.length - 1 : flattenedSections.selectedOptions.length + 1;
  	setFocusedIndex(selectedOptionsCount);
  	if (!item.isSelected) {
  	// If we're selecting an item, scroll to it's position at the top, so we can see it
  	scrollToIndex(Math.max(selectedOptionsCount - 1, 0), true);
  	}
  	}
  	}
  	onSelectRow(item);
  	};

  
  

  App/src/components/SelectionList/BaseSelectionList.js

  Lines 192 to 200 in 4ff85db

  	const selectFocusedOption = () => {
  	const focusedOption = flattenedSections.allOptions[focusedIndex];
  	if (!focusedOption || focusedOption.isDisabled) {
  	return;
  	}
  	selectRow(focusedOption, focusedIndex);
  	};

  
  

  App/src/components/SelectionList/BaseSelectionList.js

  Lines 288 to 293 in 4ff85db

  	/** Selects row when pressing Enter */
  	useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ENTER, selectFocusedOption, {
  	captureOnInputs: true,
  	shouldBubble: () => !flattenedSections.allOptions[focusedIndex],
  	isActive: !activeElement,
  	});

• And this logic is called for both case: user select item and user deselect item, which lead to the bug: When user deselects the member, the next one is highlights

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

• In my opinion, I do not think it is a bug, just needs an improvement
• The logic "Auto focus on the next available item" is only useful when the user uses the ENTER key to select/deselect the items. So we can add the condition to appy this auto-focus logic: The device are desktop/desktop-web. Like below:

 const selectRow = (item, index) => {
        // In single-selection lists we don't care about updating the focused index, because the list is closed after selecting an item
        if (canSelectMultiple && !DeviceCapabilities.canUseTouchScreen()) {
                 // Auto-focus logic
         }

        onSelectRow(item);
    };

• Based on DeviceCapabilities.canUseTouchScreen(), we maybe can check if the device is desktop or not

What alternative solutions did you explore? (Optional)

• Also we can just apply the auto-focus next item in case user press the ENTER key to select the items like:

 const selectRow = (item, index, isPressingEnter) => {
          if(isPressingEnterKey) { // Logic auto-focus here}
          onSelectRow(item)
}

• The isPressingEnter `s default value is false. So the clicking item does not auto-focus on next available item. In the callback function to handle ENTER press - selectFocusedOption, we will call selectRow with isPressingEnter is true

Result

• First proposal: Mobile web

Screencast.from.14-09-2023.12.25.47.webm

[s77rt]

@akamefi202 Thanks for the proposal. Your RCA is correct and the solution looks good to me.

🎀 👀 🎀 C+ reviewed
Link to proposal

[melvin-bot]

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

[s77rt]

@DylanDylann Thanks for the proposal. Your RCA is correct. The first solution i.e. the use of DeviceCapabilities.canUseTouchScreen does not seem correct for this case (the feature is related to keyboard not touchscreen) and the second solution is a pretty much the same as the one proposed by @akamefi202.

[DylanDylann]

@s77rt I think based on DeviceCapabilities.canUseTouchScreen and maybe any additional conditions in the detail PR, we can know that whether the device is desktop or not. So only desktop device can use the keyboard in this case, right? The main idea from my proposal is that: We will disable the auto-focus logic with the devices that do not have ENTER button

[Christinadobrzyn]

Hey @techievivek can you take a peek at this #27377 (comment)

[s77rt]

@DylanDylann The !DeviceCapabilities.canUseTouchScreen() condition would be true on Web/Desktop, this won't change anything and the issue would be still reproducible on those platforms.

[DylanDylann]

@s77rt why "this won't change anything and the issue would be still reproducible on those platforms."? In mobile, !DeviceCapabilities.canUseTouchScreen() will be false so the auto-focus logic is not applied based on my proposal. Please help give me more detail

[DylanDylann]

Also, should we need to confirm whether the auto-focus on the item when deselecting item is intention or not @s77rt

[s77rt]

@DylanDylann

why "this won't change anything and the issue would be still reproducible on those platforms."? In mobile, !DeviceCapabilities.canUseTouchScreen() will be false so the auto-focus logic is not applied

Right, but on Web/Desktop the issue would be still there. We need to fix the bug for all the platforms.

should we need to confirm whether the auto-focus on the item when deselecting item is intention or not

It's intentional based on the code comments

[melvin-bot]

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

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

[melvin-bot]

📣 @gadhiyamanan 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[akamefi202]

Thank you. The PR will be ready by tomorrow.

[akamefi202]

@s77rt I closed the PR.

[Christinadobrzyn]

Reached out to BZ about payment - https://expensify.slack.com/archives/C01SKUP7QR0/p1695743819720659

[Christinadobrzyn]

Follow up from the team is to do a case-by-case evaluation of the work done on the GH and PR.

@s77rt @akamefi202 @techievivek what do you think is fair payment for the work on this job/PR?

[s77rt]

I think 50% would be fair for this case.

[Christinadobrzyn]

Sounds good! Sorry, I'm a little lost here... The job was fixed elsewhere and we can close this, right?

How's this for payouts due:

Issue Reporter: $50 @gadhiyamanan
Contributor: $250 @akamefi202
Contributor+: $250 @s77rt

Eligible for 50% #urgency bonus? N

Upwork job is here.

[akamefi202]

@s77rt @Christinadobrzyn @techievivek
Can we please close this issue soon if everything is clear here?

[s77rt]

@Christinadobrzyn Sorry I missed the question

The job was fixed elsewhere and we can close this, right?

Yes. it was fixed here #27246

[Christinadobrzyn]

Thanks! Paid this out in Upwork based on #27377 (comment)

Closing!