[HOLD for payment 2023-10-09] Member - Green highlight on member jumps to other member when selecting member

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

---

Issue found when executing PR #22622

Action Performed:

1. Go to staging.new.expensify,com
2. Go to workspace settings with some members
3. Go to Members
4. Select any member

Expected Result:

There will be no issue when member is selected

Actual Result:

The green highlight on the member row jumps to other member row when the member is selected, which can be confusing

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-0

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

Bug6175900_20230825_023116.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

[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 @deetergp (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[ishpaul777]

Proposal

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

Green highlight on member jumps to other member and selected/unselected member is still focused.

What is the root cause of that problem?

When a member is checked/unchecked it is intention the focussed index change to next index. The root cause here is that the checked/unchecked member still has focus which keep the dim background on that member

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

we want to blur the focussed member by passing ref to PressableWithFeedback in CheckboxListItem.

function CheckboxListItem({item, isFocused = false, onSelectRow, onDismissError = () => {}}) {
    const hasError = !_.isEmpty(item.errors);
+    const ref = React.useRef(null);
    return (
        <OfflineWithFeedback
            onClose={() => onDismissError(item)}
            pendingAction={item.pendingAction}
            errors={item.errors}
            errorRowStyles={styles.ph5}
        >
            <PressableWithFeedback
+                ref={ref}

// rest of component

[getusha]

Proposal

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

What is the root cause of that problem?

This is a regression from #22622
Highlighting the next item after selecting a member is useful on some scenarios like selecting the member using the enter key from the keyboard especially on mobile devices, it will help fasten the flow. but when selecting a member using a mouse/click it should not highlight the next index, because it will become confusing instead of helping unlike using keyboard.

This is the code causing this issue, highlighting the next item on selection.

App/src/components/SelectionList/BaseSelectionList.js

Lines 182 to 183 in b38181a

	const nextAvailableIndex = _.findIndex(flattenedSections.allOptions, (option, i) => i > index && !option.isDisabled);
	setFocusedIndex(nextAvailableIndex);

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

We should check if the selection is from keyboard/shortcut and only set the next index if it's coming from it

We should check whether the selection was made from the keyboard / a shortcut (up, down and enter key), and only set the next index if it was made from the keyboard/shortcut

We can add a param to selectRow for example called usingArrowKey which will be used as a value to check if it is called from selectFocusedOption which is utilized to be used for ArrowKeyFocusManager and by passing true, we can use the value from here whether to focus the next item if it is from arrow focus.

App/src/components/SelectionList/BaseSelectionList.js

Lines 177 to 178 in b38181a

	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

selectFocusedOption is being used for handling the shortcut and keyboard calls.

App/src/components/SelectionList/BaseSelectionList.js

Lines 290 to 293 in b38181a

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

inside selectFocusedOption we will pass the additional argument like the following:

 selectRow(focusedOption, focusedIndex, true);

App/src/components/SelectionList/BaseSelectionList.js

Lines 194 to 202 in b38181a

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

Result:

Screen.Recording.2023-08-24.at.11.20.35.PM.mov

What alternative solutions did you explore? (Optional)

We can remove this code, if the arrow navigation is not required.

App/src/components/SelectionList/BaseSelectionList.js

Lines 182 to 183 in b38181a

	const nextAvailableIndex = _.findIndex(flattenedSections.allOptions, (option, i) => i > index && !option.isDisabled);
	setFocusedIndex(nextAvailableIndex);

and set the focused indext to -1, we probably should remove other codes related to this.

[Santhosh-Sellavel]

@deetergp
This is not a deploy blocker, I guess this is expected cc: @cristipaval @thiagobrez.

If this needs changed can be handled along with phase 3 of SelectionList Improvement!
cc: @shawnborton

[ishpaul777]

I agree with @getusha this behaviour feels confusing sometimes for eg. user is checking off backward with arrowkeys

Screen.Recording.2023-08-25.at.2.02.50.AM.mov

[shawnborton]

Hmm I agree, that is weird and feels unexpected.

[thiagobrez]

This is not a deploy blocker, I guess this is expected
If this needs changed can be handled along with phase 3 of SelectionList Improvement!

Agree with @Santhosh-Sellavel here. We can handle it on #20354

@shawnborton So I guess we should keep the highlight in the same item when selecting it?

Here's a relevant comment regarding the highlight in Workspace Members vs. Workspace Invite members: #22622 (comment)

[melvin-bot]

Triggered auto assignment to @dannymcclain (Design), see these Stack Overflow questions for more details.

[mountiny]

Coming from here, removing the deploy blocker as this was planned behaviour with the new list but its a new concept which feels unexpected. Would be great to discuss with the design team what we should do about this https://expensify.slack.com/archives/C01GTK53T8Q/p1692963954482309?thread_ts=1692952886.670059&cid=C01GTK53T8Q

[shawnborton]

So I guess we should keep the highlight in the same item when selecting it?

That would be my vote, yup!

[cristipaval]

Holding it on #20354 as per this comment

[thiagobrez]

PR is ready to review here: #27246

[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.3.75-12 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix(selection-list): highlight on lists with 1 section #27246

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

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:

• @thiagobrez does not require payment (Contractor)
• @allroundexperts does not require payment (Eligible for Manual Requests)

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]

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

[cristipaval]

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

• fix(selection-list): highlight on lists with 1 section #27246

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

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:

• @thiagobrez does not require payment (Contractor)
• @allroundexperts does not require payment (Eligible for Manual Requests)

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

@muttmuure, I just added the Bug label to have someone issuing the payment for the C+ 🙏

[muttmuure]

Sibtain gets paid on NewDot so adding a summary here

C+ - $500 - @allroundexperts

[JmillsExpensify]

$500 payment approved for @allroundexperts based on BZ summary.

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.