[HOLD for payment 2024-04-25] [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 contributors@expensify.com 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?

• [x] Android: Native
• [x] Android: mWeb Chrome
• [ ] iOS: Native
• [ ] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/93399543/e848edfc-7fdb-4d41-b40b-43c2a2b2ad3b

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: @jliexpensify

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[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

[GandalfGwaihir]

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.

https://github.com/Expensify/App/blob/a0614e9f68f1c066958f481b22559d9bc37e7d99/src/components/SelectionList/BaseSelectionList.tsx#L223-L231

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

https://github.com/Expensify/App/assets/85645967/ee9ca338-3418-4a1f-bd66-7a7c33eb8344

[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[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:_** https://github.com/Expensify/App/assets/85894871/25e5ab80-e191-4c2f-a840-339d35d75f72 **_Steps to update the code:_** 1. use `usePrevious` `isSelected` to store the last entered text. ```javascript 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. ```javascript 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. ```javascript 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 https://github.com/Expensify/App/assets/85894871/cceb0b43-ea23-4075-8ed4-a8baa7160262

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. ```javascript if (sections.length > 0 && previousSelectedLength !== flattenedSections.selectedOptions.length) { updateAndScrollToFocusedIndex(-1, 0); return; } ``` **_We also need to update `BaseOptionsSelector` here:_** https://github.com/Expensify/App/blob/bfd272c29a968871038f200419151c43b898eb47/src/components/OptionsSelector/BaseOptionsSelector.js#L163

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

https://github.com/Expensify/App/assets/85894871/907bfe9d-7ef3-4738-ba89-35299b471549

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

[Santhosh-Sellavel]

@Krishna2323 If we are no longer focusing on anything I think we might need some cleanup as well. @jliexpensify Unassigning as am planning for OOO, please assign a new C+ here

[situchan]

I can take over.

---

When user taps split button and select 2nd contact, it must be highlighted & green tick must be shown, just how it is shown for first contact after selection.

The expected result should be updated like this:

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

[melvin-bot[bot]]

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

[jliexpensify]

Assigned @situchan and updated expected result

[situchan]

Not able to reproduce anymore on staging. Looks like this was fixed in #34196

[Krishna2323]

so I would be down with getting rid of the keyboard focus as soon as you have selected one split.

@situchan, we also need to remove focus if we have any selected option, so we need to check for flattenedSections.selectedOptions.length also.

[situchan]

@Krishna2323 can you please share video of that "bug" and share with design team if we wanna fix that behavior?

[Krishna2323]

@situchan, here the first option is focused even if we have multiple selected options and since keyboard controls don't work with split flow, so we shouldn't highlight any option.

https://github.com/Expensify/App/assets/85894871/56786781-b39e-4f7a-9b95-445f1b7cea77

[Krishna2323]

@shawnborton, can you pls provide some feedback on the comment above.

[shawnborton]

That makes sense to me, in terms of not highlighting anything given that the keyboard controls don't work for split.

[melvin-bot[bot]]

@jliexpensify @situchan this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[jliexpensify]

@situchan are you able to review? Thanks!

[situchan]

@Krishna2323 mind sharing test branch?

[Krishna2323]

@situchan https://github.com/Krishna2323/App/tree/krishn2323/issue/35665

[Krishna2323]

@situchan, any updates on this?

[situchan]

I will provide feedback by tomorrow

[melvin-bot[bot]]

@jliexpensify @situchan this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[jliexpensify]

Bumping @situchan for feedback

[situchan]

So the expected behavior is to clear highlight when any item is selected (by clicking Split button) / deselected (by clicking ✅ button).

@Krishna2323 what about this case?

When deselected the last remaining selected item, keeps highlight

https://github.com/Expensify/App/assets/108292595/d45fb5e2-e3c0-45c1-8fc8-7b4c9dc094e0

[Krishna2323]

@situchan, I missed that case. We can do const newSelectedIndex = textInputValue === '' || flattenedSections.selectedOptions.length !== prevSelectedOptionsLength ? -1 : 0; instead of textInputValue === '' || flattenedSections.selectedOptions.length ? -1 : 0; to update to -1 whenever selected options length gets changed. We need to also update the if check.

    const prevTextInputValue = usePrevious(textInputValue);
    const prevSelectedOptionsLength = usePrevious(flattenedSections.selectedOptions.length);
    useEffect(() => {
        // Avoid changing focus if the textInputValue remains unchanged and selected options also doesn't change
        if (prevTextInputValue === textInputValue && flattenedSections.selectedOptions.length === prevSelectedOptionsLength) {
            return;
        }
        // Remove the focus if the search input is empty or selected options get s changed else focus on the first non disabled item
        const newSelectedIndex = textInputValue === '' || flattenedSections.selectedOptions.length !== prevSelectedOptionsLength ? -1 : 0;

        updateAndScrollToFocusedIndex(newSelectedIndex);

[situchan]

ok please update proposal accordingly

[jliexpensify]

Bump @Krishna2323 do you have an updated proposal? Thanks!

[Krishna2323]

Sorry, I don't know why I haven't got notified about the proposal update comment. Updating...

[Krishna2323]

@situchan, proposal updated.

[melvin-bot[bot]]

@jliexpensify @situchan this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is today
• If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[melvin-bot[bot]]

Current assignee @situchan is eligible for the Internal assigner, not assigning anyone new.

[jliexpensify]

Bump @situchan for a proposal review! Thanks.

[jliexpensify]

DM-ed Situ in Slack

[melvin-bot[bot]]

@jliexpensify, @situchan Huh... This is 4 days overdue. Who can take care of this?

[situchan]

sorry missed this. @Krishna2323 can you please update testing branch which covers that?

[Krishna2323]

@situchan updated branch here: https://github.com/Krishna2323/App/tree/krishna2323/test2/35665