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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• 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

android.native.timezone.position.error.mov

Recording.4895.mp4

mac.chrome.desktop.timezone.maintains.search.position.1.mov

ios.safari.native.timezone.maintains.search.position.1.mov

windows.chrome.timezone.focus.not.shift.to.first.1.mp4

android.chrome.timezone.maintains.search.position.1.mp4

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]

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

[melvin-bot]

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

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:

Screen.Recording.2023-10-08.at.2.31.41.AM.mov

Screen.Recording.2023-10-20.at.2.10.21.AM.mov

[Santhosh-Sellavel]

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

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

📣 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: #29080 (comment)
Contributor: @abzokhattab

🎀 👀 🎀 C+ reviewed

[abzokhattab]

Yes the new PR will fix the issue you are referring to @jjcoffee as this condition in the useEffect will prevent the refocus from occurring.

However, the focus appears only when you start typing otherwise no element is focused.

if we want to set the selection on the first unselected element when selecting a row as well then we should
change this to:

App/src/components/SelectionList/BaseSelectionList.tsx

Line 305 in d53c12f

onSelectRow={() => selectRow(item, true)}

                onSelectRow={() => selectRow(item)}

FYI the shouldUnfocusRow was attribute was introduced to solve this issue however i dont think this issue will occur after adding the sorting function

what do you think @fedirjh should we remove this argument?

[fedirjh]

The bug seems to be fixed in the current version of the PR, so let's avoid any unnecessary changes.

[melvin-bot]

This issue has not been updated in over 15 days. @jliexpensify, @MonilBhavsar, @fedirjh, @abzokhattab eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

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

• Fix selection list focus #34196

If no regressions arise, payment will be issued on 2024-02-26. 🎊

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

• @fedirjh requires payment (Needs manual offer from BZ)
• @abzokhattab requires payment automatic offer (Contributor)
• @dhanashree-sawant requires payment automatic offer (Reporter)

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

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

[fedirjh]

BugZero Checklist:

• Link to the PR: Focus on the first item of the selection list when user changes the search input #30438
• Link to comment: Author is aware and fixed the issue.
• Link to discussion: N/A
• Determine if we should create a regression test for this bug: Y

Regression Test Proposal

1. Open the app
2. Open settings -> profile -> timezone
3. Select any timezone from the visible timezones when scrolled to the top eg: type as and select Africa/Asmara
4. Search any term in search (eg: tr) 
5. Verify that focus is changed to the first value as it normally does for all other similar UI
6. Remove the search text
7. Verify the focus is removed completely 

• Do we agree 👍 or 👎

[jliexpensify]

Wow this is an old issue, and also a regression? @MonilBhavsar can you check this summary?

• Bug Reporter: @dhanashree-sawant $50
• Contributor: @abzokhattab $500
• C+: @fedirjh $500

NEW UPWORKS JOB

[abzokhattab]

I have two points here

1. First, As per the contribution guides, the contributor should be paid 50% less if there is a regression
2. Secondly, I was selected for this issue twice which means 50% on the first PR + 500$ for the second PR if no regressions appear which means 250+500= 750$

Please let me know

[jliexpensify]

Hi @abzokhattab - thanks for surfacing this! Just clarifying: were you paid for the other job? If so, the 50% regression penalty should have been applied there.

My understanding is that if a regression occurs, then the original Contributor is assigned to the new bug to also resolve that. I had always thought that this counted as part of the Contributor "resolving" the regression, but perhaps my understanding is incorrect? I'll review the .MD.

[jliexpensify]

Hi @abzokhattab - just circling back: the .MD isn't super clear (it just has one line around regression payments), but I confirmed with a colleague:

• When a regression occurs, the new bug/job is assigned to the original Contributor and C+
• It's expected that both parties will resolve the regression
• Once the regression is fixed and deployed, after 7 days, 50% is deducted from the original job, that cause the regression
• There is no payment for the new job (i.e. regression), as this forms part of the Contributor and C+'s responsibility to fix the regression

For this one, you'd see a 50% deduction on your original job. @fedirjh is paid the full amount as he's a new C+ assigned to the PR.

[abzokhattab]

Thanks @jliexpensify but there was a second proposal assignment here, the issue was not auto-assigned to me again after the regression, however, fedirih and Monil had a long conversation discussing the proposals and agreed on my proposal which I think requires another payment.

From my understanding, if my proposal wasn't chosen the second time, the compensation should be limited to $250. but since we had a second proposals assignment I think this is considered as a new issue.

my proposals were selected here and here

Thanks again for your understanding. please let me know what you think

[jliexpensify]

Ok, so I reviewed the entire set of comments - as I am not an Engineer, I may not be understanding fully, but here's what I think:

• It looks like the regression was initially pointed out here
• So this would put this bug (dated Oct 9th, 2023) as the original job.
• If this is correct, then this would mean your payment is reduced to $250 - correct?

A couple of clarifying questions from me:

1. The initial payment was dated November 2nd. My assumption is that this wasn't for the regression, as the regression was reported on the 2nd, and there's no way we could have merged and deployed to production on the same day. Is this correct?

2. Assuming the above is correct, was the 2nd proposal that you submitted on November 4th directly addressing the regression?

3. And then all subsequent proposals updates of the November 4th proposal?

[dhanashree-sawant]

Not to disturb the communication but just to inform, thanks @jliexpensify, I have accepted the offer on upwork.

[MonilBhavsar]

#29080 (comment)

@jliexpensify @abzokhattab also needs to be paid $500, as this issue was fixed by two PR's. One PR that fixed original bug and another PR that fixed the regression.

[jliexpensify]

Got it, thanks @MonilBhavsar and apologies @abzokhattab - having that context really helps. My assumption was that this was a bug that was a regression, and every proposal after was focused on the regression.

I've adjusted it to pay @abzokhattab $500

[jliexpensify]

@MonilBhavsar just checking - you don't mean $500 + $250 (regression penalty) right?

[MonilBhavsar]

No, since they made a follow up to fix the issue completely. We are fine to pay $500

[jliexpensify]

Paid and job closed!