[HOLD for payment 2024-02-26] [$500] App does not focus to first option on search if selected from first page option in timezone

[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. Open the app
2. Open settings->profile->timezone
3. Open timezone and scroll to top
4. Select any one timezone from the visible timezones when scrolled to top eg: Africa/Asmara
5. Search any term in search (eg: IN) and observe that focus is not changed to first value as it normally does for all other similar UI

Expected Result:

App should shift focus to first option on search in timezone

Actual Result:

After scroll to top and selecting any one timezone from visible options, app does not shift focus to first option on search in timezone

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• [x] Android / native
• [x] Android / Chrome
• [x] iOS / native
• [x] iOS / Safari
• [x] MacOS / Chrome / Safari
• [x] MacOS / Desktop

Version Number: 1.3.79.3 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

https://github.com/Expensify/App/assets/93399543/a9ee2e41-187c-4095-980e-97af702999dd

https://github.com/Expensify/App/assets/93399543/fd932312-3d29-4096-9c28-ce662ea5169a

https://github.com/Expensify/App/assets/93399543/6b6311c0-389d-4b98-b696-c1cc716f35a0

https://github.com/Expensify/App/assets/93399543/fbd86a63-4943-48fc-ab51-9f17acd20268

https://github.com/Expensify/App/assets/93399543/0bf66c86-a9c0-437a-94f1-b3dbc146db5c

https://github.com/Expensify/App/assets/93399543/ceff9bce-a5d3-4666-a29d-d3c42eb412b4

Expensify/Expensify Issue URL: Issue reported by: @dhanashree-sawant Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1696672477964269

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0178208b9c15b89907
• Upwork Job ID: 1711342124473376768
• Last Price Increase: 2023-10-16
• Automatic offers:
• abzokhattab | Contributor | 27368232
• dhanashree-sawant | Reporter | 27368237

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~0178208b9c15b89907

[melvin-bot[bot]]

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

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

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

[abzokhattab]

Proposal

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

SelectionList does not change the focus to first option when the options list changes

What is the root cause of that problem?

This issue occurs because we dont change the focus to the first item when the sections list changes in the SelectionList component in this file

https://github.com/Expensify/App/blob/df377f22e5820e6e524b065360e186f77a8e9625/src/components/SelectionList/BaseSelectionList.js

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

To change the focus on the first item whenever the sections list changes , we need to add the following useEffect that updates the focusIndex to zero whenever the section changes

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

        // set the focus on the first item when the sections list is changed
        if (sections.length > 0) {
            updateAndScrollToFocusedIndex(0);
        }
    }, [sections]);

Alternative Solution:

Change SelectionList to OptionsSelector like we are doing in the iou/currency selection compoenent

so we should add this

<OptionsSelector
  headerMessage={
    timezoneInputText.trim() && !timezoneOptions.length
      ? translate("common.noResultsFound")
      : ""
  }
  textInputLabel={translate("timezonePage.timezone")}
  value={timezoneInputText}
  onChangeText={filterShownTimezones}
  onSelectRow={saveSelectedTimezone}
  sections={[
    { data: timezoneOptions, indexOffset: 0, isDisabled: timezone.automatic },
  ]}
  initiallyFocusedOptionKey={_.get(
    _.filter(timezoneOptions, (tz) => tz.text === timezone.selected)[0],
    "keyForList"
  )}
  showScrollIndicator
/>

and also import the greencheck:

const greenCheckmark = {
  src: Expensicons.Checkmark,
  color: themeColors.success,
};

and use it in the allTimeZones var:

const allTimezones = useRef(
  _.chain(TIMEZONES)
    .filter((tz) => !tz.startsWith("Etc/GMT"))
    .map((text) => ({
      text,
      customIcon: text === timezone.selected ? greenCheckmark : undefined,
      keyForList: getKey(text),
      isSelected: text === timezone.selected,
    }))
    .value()
);

POC:

https://github.com/Expensify/App/assets/59809993/8afa22b0-03bd-4456-acdc-cf708fad711b

https://github.com/Expensify/App/assets/59809993/06fbb3a1-af83-4b64-9b4f-ba557e883ef8

[Santhosh-Sellavel]

@jliexpensify Can you reassign a new C+ by applying external label again? Unassigning this one due to low bandwidth

[melvin-bot[bot]]

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

[maxconnectAbhi]

Not able to replicate

[jliexpensify]

Thanks @maxconnectAbhi - looks like the app is v80-1, so I will also confirm

[jliexpensify]

Hmm it still seems to be an issue on the Android app - when Is search In, my assumption is that it would default me to the country first (i.e. India). Instead, it shows me America/Indiana - so I don't think this is fixed.

[robertKozik]

"@jliexpensify, is the focus applied on the first one option though? Correct me if I'm wrong, but from your comment it looks like a different issue connected with the search function."

[jliexpensify]

Hmm maybe? I think this is what I'm unclear on:

observe that focus is not changed to first value as it normally does for all other similar UI

Does this mean that when typing IN, then whatever is focused on should be bolded?

[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? 💸

[robertKozik]

I came back to it, and I can confirm that this bug is still reproductible. I'll review the proposal

[robertKozik]

@abzokhattab, I appreciate your proposal. Since you already have the proof of concept implemented, could you verify if it behaves as intended for the case shown in the video? Specifically, select Africa/Asmara and then type in. I've found it to be quite dependent on user input, so I'd appreciate a double-check on a scenario that is currently not working.

[abzokhattab]

Hi @robertKozik yeah i already used Africa/Asmara in the attached POC above please check it again.

let me know if you want to test it with any other values

[robertKozik]

Thanks @abzokhattab for checking. Let's see your proposal in action 🪨 Selected Proposal: https://github.com/Expensify/App/issues/29080#issuecomment-1752827570 Contributor: @abzokhattab

🎀 👀 🎀 C+ reviewed

[melvin-bot[bot]]

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

[MonilBhavsar]

@abzokhattab sorry could you please explain root cause in other way - why we're not focussing the first option like we do in OptionSelector. I see we're using SelectionList component at many places. So is focussing not working as expected in that component?

[abzokhattab]

@MonilBhavsar

I see we're using SelectionList component at many places. So is focussing not working as expected in that component?

Yes focusing on the first item when the options list changes is not implemented so far in the SelectionList component so this issue exists in all components that uses SelectionList, here is another example: the workspace/settings/currency page which uses the SelectionList component:

https://github.com/Expensify/App/assets/59809993/ec1bfbb9-3af8-4398-ae51-5b79a78362e5

If we want the focus to be on the first item whenever the sections list changes like in the OptionsList,

we can acheive that by adding this useEffect to the BaseSelectionList:

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

        // set the focus on the first item when the sections list is changed

        if (sections.length > 0) {
            updateAndScrollToFocusedIndex(0);
        }
    }, [sections]);

Result:

https://github.com/Expensify/App/assets/59809993/06fbb3a1-af83-4b64-9b4f-ba557e883ef8

Proposal is updated as well

[MonilBhavsar]

Thanks! I would vote to fix focussing on SelectionList component rather than using OptionListSelector instead of SelectionList. @robertKozik could you please take a look at the updated proposal

[MonilBhavsar]

Awaiting review of updated proposal from @robertKozik

[melvin-bot[bot]]

@jliexpensify @MonilBhavsar @robertKozik 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!

[MonilBhavsar]

Already updated, Melvin

[robertKozik]

Sorry for the delay 🙇🏼 The updated proposal looks good as well, If we don't want to replace the component. Let's see it in action to test all the cases.

CC. @MonilBhavsar

[MonilBhavsar]

Agree. We should test all pages that uses SelectionList component

[melvin-bot[bot]]

📣 @abzokhattab 🎉 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[bot]]

📣 @dhanashree-sawant 🎉 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

[abzokhattab]

PR is ready

[abzokhattab]

bump @robertKozik @MonilBhavsar

[situchan]

@abzokhattab can you please handle this regression? It's blocking another issue.

[abzokhattab]

Looking into it today

[melvin-bot[bot]]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot[bot]]

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

• https://github.com/Expensify/App/pull/30438

If no regressions arise, payment will be issued on 2023-11-09. :confetti_ball:

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:

• @abzokhattab requires payment offer (Contributor)
• @robertKozik does not require payment (Contractor)
• @dhanashree-sawant requires payment offer (Reporter)

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

• [ ] [@robertKozik] The PR that introduced the bug has been identified. Link to the PR:
• [ ] [@robertKozik] 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:
• [ ] [@robertKozik] 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:
• [ ] [@robertKozik] Determine if we should create a regression test for this bug.
• [ ] [@robertKozik] 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:

[abzokhattab]

Proposal

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

App focuses on first item when refreshing the workspace/currency page

What is the root cause of that problem?

the sections gets rerendered multiple times caussing the useEffect in this proposal to focus on the first item on refresh the selection page https://github.com/Expensify/App/issues/29080#issuecomment-1752827570

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

To address the issue with the multiselection item, two key challenges need resolution:

Ordering of List: Ensure the list order is consistent, with disabled items first, followed by selected items, and then unselected items. This maintains alignment with the "options selector component" behavior.

Input Focus on Change: Utilize useEffect to manage input focus. This involves updating the focus to the first non-selectable item upon changing the input value It is crucial to note that relying solely on the textInputValue for determining the first non-selectable item is insufficient. This is due to the asynchronous nature of the onchange call, which updates the selectOptions/sections and the first non-selected item would be outdated or have an unaccurate index.

For the first challenge, a sorting function, getSectionsSortedBySelected, is introduced. This function organizes the sections list based on the desired order of disabled, selected, and unselected items.

To integrate these changes, use the following useEffect and sorting function in the SelectionList component:

    const prevInputValue = useRef(textInputValue);
    useEffect(() => {
        // do not change focus on the first render, as it should focus on the selected item
        if (isInitialRender || prevInputValue.current === textInputValue) {
            return;
        }
        // set the focus on the first item when the sections list is changed
        if (sections.length > 0) {
            let newSelectedIndex;

            if (textInputValue === '') {
                // if the textInputValue is empty then focus is removed
                newSelectedIndex = -1;
            } else {
                // if single selection then focus on the first item
                // else focus on the first non-selected item
                newSelectedIndex = canSelectMultiple ? flattenedSections.selectedOptions.length + flattenedSections.disabledOptionsIndexes.length : 0;
            }

            updateAndScrollToFocusedIndex(newSelectedIndex);
        }
        prevInputValue.current = textInputValue;
        // eslint-disable-next-line react-hooks/exhaustive-deps
    }, [sections, textInputValue]);

    const getSectionsSortedBySelected = (sectionsList) => {
        // don't change the order if canSelectMultiple is false
        if (!canSelectMultiple) {
            return sectionsList;
        }
        const newSections = [];
        _.each(sectionsList, (section) => {
            const selected = [];
            // eslint-disable-next-line rulesdir/no-negated-variables
            const notSelected = [];

            const disabled = [];
            _.each(section.data, (item) => {
                if (item.isDisabled) {
                    disabled.push(item);
                } else if (item.isSelected) {
                    selected.push(item);
                } else {
                    notSelected.push(item);
                }
            });
            const newData = [...disabled, ...selected, ...notSelected];
            newSections.push({
                ...section,
                data: newData,
            });
        });
        return newSections;
    };

then used this function inside the SelectionList component call inside the base component instead of the sections object:

sections={getSectionsSortedBySelected(sections)}

Proof of concept:

https://github.com/Expensify/App/assets/59809993/1a8551a5-396a-4fb2-b139-fc37a1be02a1

Alternatively:

if we dicide to keep the focus on the first item in the multiselection the same as we normally do in the single selection lists, we wont need the sections in this case and then we can rely on the input value change:

in this case we can use the onChangeHelper function:

    const onChangeTextHelper = useCallback(
        (value) => {
            let newSelectedIndex;
            if (!canSelectMultiple) {
                // if the textInputValue is empty then focus is removed
                // else focus on the first item
                newSelectedIndex = textInputValue === '' ? -1 : 0;
            } else {
                // if no item is selected then focus is removed, else focus on the first item
                const selectedItemsLength = flattenedSections.selectedOptions.length;
                const disabledItemsLength = flattenedSections.disabledOptionsIndexes.length;
                newSelectedIndex = selectedItemsLength === 0 ? -1 : disabledItemsLength;
            }

            onChangeText(value);
            updateAndScrollToFocusedIndex(newSelectedIndex);
        },
        // eslint-disable-next-line react-hooks/exhaustive-deps
        [canSelectMultiple, onChangeText, updateAndScrollToFocusedIndex],
    );

[MonilBhavsar]

@robertKozik would like to have your thoughts here

[MonilBhavsar]

Removed Awaiting payment labels and from title since we need to fix this before issuing payment

[melvin-bot[bot]]

@jliexpensify @MonilBhavsar @abzokhattab @robertKozik 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 @robertKozik is eligible for the Internal assigner, not assigning anyone new.

[tienifr]

Coming from https://github.com/Expensify/App/issues/30951, @robertKozik Please do consider my proposal here as well, I think the original approach accepted here is not correct

cc @MonilBhavsar

Proposal

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

The currency list scrolls up.

What is the root cause of that problem?

In here, we'll scroll to top whenever the sections change, but the sections can change when the selected item change too, so even when the original list is not changed, it will still scroll to the top.

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

Listening to sections changes is not ideal due to the above reason.

Based on the updated requirement in here, we should do the following:

1. Remove this faulty block
2. Add an useEffect that will listen to the textInputValue changes and do the focusing accordingly

const prevTextInputValue = usePrevious(textInputValue);

useEffect(() => {
    // only do the focusing when the text input actually changes
    if (prevTextInputValue === textInputValue) {
        return;
    }

    if (textInputValue) {
        // For multiple selections list, focus on the first non-selected item. 
        // For single selection list, focus on first item
        updateAndScrollToFocusedIndex(canSelectMultiple ? flattenedSections.selectedOptions.length : 0);
    } else {
        // When the text input is cleared, remove focus completely
        // I think this should apply to both the multiple and single selection list, but we can modify accordingly if the behavior for multiple selection list is different.
        setFocusedIndex(-1);
    }

}, [textInputValue, prevTextInputValue, flattenedSections.selectedOptions.length, canSelectMultiple, updateAndScrollToFocusedIndex]);

On the initial render, focus the initial item only if there is a pre-selected item; otherwise, no item should be focused.

If there are no selected items, do not focus on any item.

These requirements already work like that currently, no change needed.

The above will cover all cases of the requirement, if there's any adjustment required in the behavior, we can work that out in the PR.

What alternative solutions did you explore? (Optional)

Consider doing the same for OptionsSelector

If there are no selected items, do not focus on any item.

For this, I'm assuming we only do this for initial render, if the text input changes and there're no selected items, I think we still should focus on the first non-selected item because the user is likely to select it. But if that's not the case and we shouldn't focus on any item in any case, only a minor change in the above code is required.

[mkhutornyi]

It's same regression as https://github.com/Expensify/App/pull/30438#issuecomment-1789081358. QA team finally reported what we've found 🙂

[jliexpensify]

Bump @robertKozik

[dukenv0307]

Proposal

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

The first option in Notifications preference is always highlighted

What is the root cause of that problem?

We change focusedIndex and scroll to the first option whenever the sections are changed. It doesn't makes sense because it can be changed when the search value isn't changed.

https://github.com/Expensify/App/blob/5df95a115bbc943a7149eee6eeb5ea7e5a64a32d/src/components/SelectionList/BaseSelectionList.js#L349

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

We should only scroll to the first option when both textInputValue and sections are changed.

What alternative solutions did you explore? (Optional)

[wlegolas]

Hello everyone.

I sent this proposal https://github.com/Expensify/App/issues/31087#issuecomment-1803039218 in another issue that is related to the same problem.

[robertKozik]

Sorry for the delay here, I believe changing the implementation and unifying it with the battle-tested approach from OptionsSelector (described by @tienifr here) is the way to go here. More tweaking of the current approach can lead to even more regressions/problems, while the OptionSelector way to achieve this is currently working as expected

[abzokhattab]

I think it makes more sense to be tied to the input changes though @robertKozik .. since the requirement is "the first item is selected whenever the user changes the search value in the text input field"

[tienifr]

Sorry for the delay here, I believe changing the implementation and unifying it with the battle-tested approach from OptionsSelector (described by @tienifr https://github.com/Expensify/App/issues/29080#issuecomment-1797811942) is the way to go here.

Yeah this will make it consistent with the current OptionsSelector behavior as well. @MonilBhavsar @jliexpensify If we're good here I can quickly raise a PR to make this right.

[jliexpensify]

Going to bump @MonilBhavsar so he doesn't miss this - he'll have an answer for you!

[s-alves10]

@robertKozik @MonilBhavsar

This is a general issue for SelectionList. I think we can simply solve both https://github.com/Expensify/App/issues/31110 and this issue by replacing the dependency of useEffect with textInputValue. This makes sense because the purpose is to focus the first item when search input is changed and textInputValue is synchronized with sections. Please check this proposal as well.