[WAITING ON SITU TO FINISH CHECKLIST] [LOW] [Splits] [$500] IOU - Tapping split button & selecting 2nd contact are not highlighted like 1st contact

[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: 1.4.35.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: https://expensify.testrail.io/index.php?/tests/view/4263802
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. Go to https://staging.new.expensify.com/
2. Tap fab --- request money --- Scan
3. Tap camera button and upload a picture
4. Note first contact is not highlighted in Resents
5. Now tap split and select the first contact from Resents
6. Note, the contact is shown highlighted with green tick mark after selecting it
7. Tap split button next to second contact and select it

Expected Result:

When user taps split button (no matter first or second selection), the row isn't always highlighted but only green tick is shown

Actual Result:

When user taps split button and select 2nd contact, only green tick shown, it is not highlighted like first contact after selection.

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

Bug6364658_1706869108259.high.1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~010e869b36a20a1985
• Upwork Job ID: 1753418217201557504
• Last Price Increase: 2024-02-09
• Automatic offers:
• situchan | Contributor | 0
• Krishna2323 | Contributor | 0

Issue Owner

Current Issue Owner: @situchan

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~010e869b36a20a1985

[melvin-bot]

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

[melvin-bot]

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

[kbecciv]

We think that this bug might be related to #wave5-free-submitters
CC @dylanexpensify

[allgandalf]

Proposal

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

Tapping split button & selecting 2nd contact are not highlighted like 1st contact

What is the root cause of that problem?

We only highlight/ Focus the top index element.

App/src/components/SelectionList/BaseSelectionList.tsx

Lines 223 to 231 in a0614e9

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

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

Update the function to highlight all the selected participants

What alternative solutions did you explore? (Optional)

N/A

Some Thoughts:
I think this is intended and not a bug, when on laptop, if we use keyboard down button then the same focus moves down the list.

Anyway, if this feels like a bug would like to solve this

[jliexpensify]

@GandalfGwaihir regarding your comment:

I think this is intended and not a bug, when on laptop, if we use keyboard down button then the same focus moves down the list.

I think the bug is that when you select a second person, the first person is still highlighted. I agree that the focus should move down the list, but it should then highlight the 2nd person instead, as it's the most recently selected.

@kbecciv is this essentially what you are saying?

[jliexpensify]

I think this should actually go in #vip-split: https://expensify.slack.com/archives/C05RECHFBEW/p1707097628668729

[Santhosh-Sellavel]

I think this is intended and not a bug, when on laptop, if we use keyboard down button then the same focus moves down the list.

cc: @Expensify/design can you confirm whether this issue is bug or not?

[dannymcclain]

🤔 I do not know. I guess it's not a bug since we keep the first item of a list focused... but why do we keep the first item of a list focused? I think in this situation I would expect none of the list items to have focus. Anyone else?

I think the bug is that when you select a second person, the first person is still highlighted. I agree that the focus should move down the list, but it should then highlight the 2nd person instead, as it's the most recently selected.

I guess if we definitely want to maintain focus inside the list (which I would still love to hear why we want to do that—could be something very obvious that I'm not understanding) then I would agree with Jason that the most recent person selected should be the one that gets focus.

[Santhosh-Sellavel]

The behavior same in the other places too, I think this is designed this way, maybe @shawnborton help here as he was part of the selection list refactor

Screen.Recording.2024-02-07.at.3.00.44.AM.mov

[jliexpensify]

but why do we keep the first item of a list focused?

This is exactly the question I asked myself, and why I assumed it was a bug.

Here's my thought:

The focus should move down the list, but it should then highlight the 2nd person instead, as it's the most recently selected.

[shawnborton]

Yeah I guess this is technically not a bug but I do tend to agree with this one:

but why do we keep the first item of a list focused? I think in this situation I would expect none of the list items to have focus. Anyone else?

It seems like maybe we have competing styles for list selection where selected items typically have a different BG color, but your keyboard focus also gets a darker BG color.

So I wonder if it makes sense to not have the keyboard focus on any of the list items until you explicitly start using the down arrow/tab button to go down from the input? And then we might consider using an outline style for the focus style as opposed to a BG color?

[melvin-bot]

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

[jliexpensify]

@shawnborton I think you (and the Design Team) probably have the final say here! What do you recommend the solution should be?

• Focus on each new user? OR
• Don't focus initially and use an outline style for each new user?

[Krishna2323]

Proposal

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

Tapping split button & selecting 2nd contact are not highlighted like 1st contact

What is the root cause of that problem?

Initial Proposal

This code always sets the focused index to 0 (firstItem) whenever the sections change. https://github.com/Expensify/App/blob/a0614e9f68f1c066958f481b22559d9bc37e7d99/src/components/SelectionList/BaseSelectionList.tsx#L350-L361 ### What changes do you think we should make in order to solve the problem? Instead of focusing always on the first item, I believe we should be focusing in the first non-selected item like we do with `BaseOptionsSelector`, in that component whenever we select any option the focusedIndex is set to the next item.

This is how BaseOptionsSelector works:

base_options_selector.mp4

Steps to update the code:

1. use usePrevious isSelected to store the last entered text.

const lastTextValue = usePrevious(textInputValue);

2. Update updateAndScrollToFocusedIndex to also accept scrollIndex parameter because no matter whichever option is highlighted we always need to scroll to top, we should not scroll to the first non-selected option.

    const updateAndScrollToFocusedIndex = useCallback(
        (newFocusedIndex: number, scrollIndex?: number) => {
            setFocusedIndex(newFocusedIndex);
            scrollToIndex(scrollIndex ?? newFocusedIndex, true);
        },
        [scrollToIndex],
    );

3. Inside the block of code where we always set the focus index to 0, we need to check if lastTextValue !== textInputValue is true or not, if true focus on the firstOption, because when we enter something in the input box we should select the first option, if false move to the next if statement which will set the focus to the first non-selected item.

    useEffect(() => {
        // do not change focus on the first render, as it should focus on the selected item
        if (isInitialSectionListRender) {
            return;
        }

        if (lastTextValue !== textInputValue) {
            updateAndScrollToFocusedIndex(0);
            return;
        }

        // set the focus on the first item when the sections list is changed
        if (sections.length > 0) {
            updateAndScrollToFocusedIndex(flattenedSections.selectedOptions.length, 0);
        }
        // eslint-disable-next-line react-hooks/exhaustive-deps
    }, [sections]);

4. We will use usePrevious because we still want to highlight the first option when adds text to the input box.

Result

fix_demo_option_selection.mp4

Alternative

Prev Alternative

~Instead of setting focused index to first non selected item we set focus to -1, so that no item will be focused.~

Focusing on newly selected option use can be done using updateAndScrollToFocusedIndex(flattenedSections.selectedOptions.length-1, 0)

We need to check if there is any selected option or not before focusing on the 0 index, if there is a option selected then just set the focusedIndex to -1

We should always set the focusedIndex to -1 whenever component updates and we have selections and it is changed. We will use usePrevious for that. Or we can if only flattenedSections.selectedOptions.length is true.

Will discuss more changes with the design team if required.

      if (sections.length > 0 && previousSelectedLength !== flattenedSections.selectedOptions.length) {
            updateAndScrollToFocusedIndex(-1, 0);
            return;
        }

We also need to update BaseOptionsSelector here:

App/src/components/OptionsSelector/BaseOptionsSelector.js

Line 163 in bfd272c

const newFocusedIndex = this.props.selectedOptions.length;

To remove focus and scroll to top we need to follow few steps:

1. Use usePrevious for flattenedSections.selectedOptions.length
2. Modify updateAndScrollToFocusedIndex to also accept second parameter for scrollIndex and if not provided use the newFocusedIndex. We need to do that because we want to scroll even if any options is removed and -1 won't scroll the list.
3. Update useEffect which resets focus.

• Check for prevSelectedOptionsLength === flattenedSections.selectedOptions.length before returning.

• Update condition for determining newSelectedIndex value, if prevSelectedOptionsLength !== flattenedSections.selectedOptions.length return -1.

• Create a new const named newScrollIndex which will be used return 0 if the selected options are changed else it will return newSelectedIndex

    const prevSelectedOptionsLength = usePrevious(flattenedSections.selectedOptions.length);

    useEffect(() => {
        // Avoid changing focus if the textInputValue remains unchanged.
        if ((prevTextInputValue === textInputValue && prevSelectedOptionsLength === flattenedSections.selectedOptions.length) || flattenedSections.allOptions.length === 0) {
            return;
        }
        // Remove the focus if the search input is empty else focus on the first non disabled item
        const newSelectedIndex = textInputValue === '' || prevSelectedOptionsLength !== flattenedSections.selectedOptions.length ? -1 : 0;
        const newScrollIndex = prevSelectedOptionsLength !== flattenedSections.selectedOptions.length ? 0 : newSelectedIndex;

        updateAndScrollToFocusedIndex(newSelectedIndex, newScrollIndex);
    }, [
        canSelectMultiple,
        flattenedSections.allOptions.length,
        prevTextInputValue,
        textInputValue,
        updateAndScrollToFocusedIndex,
        prevSelectedOptionsLength,
        flattenedSections.selectedOptions.length,
    ]);

Result

focus_after_selection.mp4

[dannymcclain]

Hmm that result video actually feels pretty good to me. But I think I'm still debating between that solution and not focusing any items in the list at all.

The main reason I'm not sure we should focus the list at all is because you can't really add someone to a split with your keyboard anyways. If you try to, it seems like what ends up happening is you're just advanced to regular manual request confirmation screen with a single participant (the person who you keyboarded to). So I'm not totally sure what value the focus is providing here. Would love to get @Expensify/design thoughts on each of these approaches.

[shawnborton]

Oh that's a really valid point that keyboard controls basically don't really work in the split flow, so I would be down with getting rid of the keyboard focus as soon as you have selected one split.

[Krishna2323]

Proposal Updated

[jliexpensify]

Nice, ty everyone! @Santhosh-Sellavel - could I get you to review the updated proposals? Thanks!

[jliexpensify]

Back - thanks @trjExpensify for the assist. I'll unassign you!

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

• fix: Focus not removed when selection list changes. #39141

If no regressions arise, payment will be issued on 2024-04-25. 🎊

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

• @Krishna2323 requires payment automatic offer (Contributor)
• @situchan 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:

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

[jliexpensify]

Payment Summary

• C: @Krishna2323 $500
• C+: @situchan $500

I had to create a new Upworks job, please accept!

Also @situchan - please complete the checklist

[Krishna2323]

@jliexpensify, accepted, thanks!

[jliexpensify]

Thanks! Bumping @situchan to accept so I can pay everyone at once + complete checklist.

[jliexpensify]

Bumped @situchan again

[jliexpensify]

@situchan seems to be on leave.

[jliexpensify]

Was hoping to pay both together but seems like @situchan is still on vacation.

@Krishna2323 apologies for the delay, have paid you.

[jliexpensify]

Paid and job closed, waiting on the checklist

[jliexpensify]

Bump @situchan - does the checklist need to be completed?

[situchan]

Not able to find offending PR since this component was changed so many times.
This was caught during regression testing https://expensify.testrail.io/index.php?/tests/view/4263802 so no new regression test is needed.