[HOLD for payment 2023-09-20] [$1000] Web - Select a currency page scroll when open

[mvtglobally]

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. Click FAB button
2. Click Request money
3. Click into current currency, observer Select a currency page scroll when open

Expected Result:

Select a currency page not scroll when open

Actual Result:

Select a currency page scroll when open

Workaround:

unknown

Platforms:

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

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

Version Number: 1.3.49-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: 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/43995119/41d5b39e-929e-4376-a880-ffdc8aace6dd

https://github.com/Expensify/App/assets/43995119/3a6c88cf-a8ed-4637-8839-69ac6cb9d86f

Expensify/Expensify Issue URL: Issue reported by: @namhihi237 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690991772958669

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0175d78ac3d566ccad
• Upwork Job ID: 1691137102130921472
• Last Price Increase: 2023-08-21
• Automatic offers:
• joh42 | Contributor | 26295320
• namhihi237 | Reporter | 26295321

[melvin-bot[bot]]

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

[jeet-dhandha]

Steps to reproduce:

1. Select any one of these from the last currencies, might differ based on screen height.

   

2. Now again go to change currency picker. Verify that list scrolls up.

https://github.com/Expensify/App/assets/78416198/9941548f-0052-4429-b533-c383085c26bd

[sonialiap]

Reproducible. Small jiggle for some currencies and large movement when selecting VND from VUV

https://github.com/Expensify/App/assets/17131195/6d08be37-bd73-4a80-b25b-3766134f6dc3

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~0175d78ac3d566ccad

[melvin-bot[bot]]

Current assignee @sonialiap is eligible for the External assigner, not assigning anyone new.

[melvin-bot[bot]]

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

[joh42]

Please note - this bug is present for currency selection during both the Request money and Split bill flows.

Proposal

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

When we open the currency selector, there is a buggy jerking animation that seems like a scroll. You can also see how the default opening animation is not present.

What is the root cause of that problem?

The root cause is actually that we instantly focus the currency input, causing the default opening animation to bug, which makes it seem like there is a scroll. But what seems like a scroll is a side effect of the broken transition.

In the below video I have only delayed focusing, which removes the scroll bug:

https://github.com/Expensify/App/assets/138504087/3ad2edfc-b3d6-49ac-af83-a533742f1a74

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

I suggest we adopt the generally agreed upon solution for focusing, by using the ScreenWrapper's onEntryTransitionEnd in the IOUCurrencySelection page to focus the OptionsSelector.

An example of this being done:

https://github.com/Expensify/App/blob/21d483c098621b59feb0522475d75ceb13e4e2a8/src/pages/EditRequestDescriptionPage.js#L26-L30

This is actually quite straightforward for the IOUCurrencySelection page:

1. We add a ref to the OptionsSelector in the IOUCurrencySelection page. Since the OptionsSelector is a class component, we do not have to worry about ref forwarding
2. We focus the OptionsSelector by adding something like this to the IOUCurrencySelection page's ScreenWrapper onEntryTransitionEnd={() => optionsSelectorRef.current && optionsSelectorRef.current.textInput && optionsSelectorRef.current.textInput.focus()}. If we want to avoid the tight coupling that comes with directly accessing the textInput, we could add a focus function to the OptionsSelector. Then the IOUCurrencySelection page will have no knowledge of the inner workings of the OptionsSelector
3. We pass autoFocus as false to the OptionsSelector component in the IOUCurrencySelection page

Correcting focusing in the IOUCurrencySelection page fixes this bug for both the Request money and Split bill flows.

What alternative solutions did you explore? (Optional)

Alternative 1: Implement a new prop in the OptionsSelector, called something like didScreenTransitionEnd. When it changes to true, we focus the input. Then we pass the ScreenWrapper's existing didScreenTransitionEnd in the IOUCurrencySelection page to the OptionsSelector. https://github.com/Expensify/App/blob/21d483c098621b59feb0522475d75ceb13e4e2a8/src/components/ScreenWrapper/index.js#L132-L138

This would be the optimal approach if the currency selector was conditionally rendered. Since it is not, I suggest we simply use onEntryTransitionEnd.

Alternative 2: We could pass shouldDelayFocus={true} to the OptionsSelector. This would be a one-line fix, but according to the above-mentioned Slack discussion we should move away from this approach to focusing.

Alternative 3: We could use InteractionManager.runAfterInteractions for focusing, but again, like with shouldDelayFocus, this is something that we should move away from.

[sonialiap]

Not overdue

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

Hi all! @joh42 Your solution and root cause looks great. But before I would recommend your proposal, I wanted to recheck the issue itself - and I'm having trouble with reproducing. Could you check on your end if you are able to reproduce this issue?

https://github.com/Expensify/App/assets/61146594/fcb02e47-37ce-4e89-8a8a-6e5d5e619897

[joh42]

Hi @robertKozik, thanks for reviewing my proposal!

There is a scroll for me, but it is not as smooth as the one shown by others above. It might be worth checking with them to see if they can still reproduce it.

https://github.com/Expensify/App/assets/138504087/32fe0eb8-ba46-430c-8238-5a77ae39f49d

Either way, I do think that the missing transition should be fixed though.

[robertKozik]

Yeah, I agree. I quick tested your solution (with the fixed delay for convenience) and It looks like it's working fine, despite the list behaviour - during testing on latest main/staging/prod I've encountered different jiggles (strange tho). Fixing transition is an additional asset of fixing this task. I think we can go with your proposal @joh42 .

Selected proposal: Proposal Contributor: @joh42

🎀 👀 🎀 C+ reviewed

[melvin-bot[bot]]

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

[melvin-bot[bot]]

@cead22 @sonialiap @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!

[melvin-bot[bot]]

📣 @joh42 🎉 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]]

📣 @namhihi237 🎉 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

[robertKozik]

contributor was just assigned for this issue - not overdue

[joh42]

Thanks! You can expect a PR shortly.

[joh42]

Unfortunately I didn't end up finishing the PR today, I'll continue tomorrow

[joh42]

Okay so I have some good news and some bad news.

The good news is that the small jiggle disappears when we add delayed focusing. However, while the transition obscures the bigger scrolling, making it nearly impossible to detect, it does not remove it.

After digging into it, my conclusion is that the remaining scrolling is actually caused by how many options are being rendered at once.

Analysis

Using breakpoints, we can analyze each step of the rendering process. Here is how it looks when VND is selected:

Next up, a few more options are rendered. However, we can see that when the last option, ZMW, has been rendered, there is still an empty space at the bottom:

Finally, the space at the bottom is removed, which causes VND to move down. We can see that the space below the search input has increased:

The behavior for VUV is similar. Initially, it starts 2 options from the top, but when all options have been rendered, it has moved down to 3 options from the top to make sure there is no empty space at the bottom:

Similar behaviour elsewhere

We can observe similar behavior for the timezone selector. Note how the selected option is ~1.5 options from the top at the start:

But it is then moved down to ~2.5 options from the top:

Conclusion & next steps

My conclusion is that not enough options are being rendered initially to know exactly where the options at the top of the screen should be placed. As such, when all options have been rendered, there is empty space at the bottom. When this empty space is removed, it looks like the options are scrolled.

Increasing the number of options being rendered from 12/5 to 15/15 fixes it for my screen:

https://github.com/Expensify/App/assets/138504087/4851fa6e-9cdd-4993-ab6a-93dbf130b8ce

https://github.com/Expensify/App/assets/138504087/9f794625-10da-4bc3-a019-2b7892c40fc5

Since the currency and timezone selectors use different components (OptionsSelector and SelectionList respectively), I actually think this issue is caused by the underlying React Native component being used by both, SectionList.

Just to be clear, the scroll is barely visible at all when opening the currency selector with a proper transition. It is mostly visible when reloading with the currency selector open. For context, this is how the scroll appears when the transition has been fixed:

https://github.com/Expensify/App/assets/138504087/11aa9cf4-5a43-4b4a-bcdd-6a9c7037e709

If you slow down the playback speed to 0.25, you can see the scroll in question.

There are a few ways to approach the remaining scrolling issue:

• Increase the number of options being rendered regardless of screen size
• Try to calculate how many options should be rendered based on screen size
• Do nothing about the larger scrolling - since the transition fixes the smaller jiggles and mostly hides the larger scrolling (IE only fix the transition)
• Something else?

Tagging you initially @robertKozik to get your opinion on the correct way to move forward.

[joh42]

Also tagging @cead22 / @mountiny for input on how to move forward, since you're both assigned to this issue

[mountiny]

Do nothing about the larger scrolling - since the transition fixes the smaller jiggles and mostly hides the larger scrolling (IE only fix the transition)

Personally I think the fix you have provided for now makes the jiggle unnoticeable so I think its a fix which should be sufficient for now. I think adding more computation to the process might not be that beneficial and it will just be brittle in the end.

As we add more similar selection components to the App, I think it would be good to look at some underlying solution which will fix it everywhere, but what you got that is good enough for this issue imho. @robertKozik what do you think/

[joh42]

Thanks Vit!

[robertKozik]

@joh42 Thank you for providing such a detailed response to the problem; it greatly assists in our decision-making process.

I concur with @mountiny's viewpoint that additional calculations might be an overkill for such a minor issue.

However, I believe we can explore the option of increasing the maximum number of options rendered. I traced down the latest issue that led to the increase in that number. If enlarging the numbers a little won't have a negative impact on performance, it could be a viable solution. As per React Native documentation, initialNumToRender should be sufficient to fill the screen, but in our current scenario, we're encountering a situation where we lack an adequate number for larger screens.

@joh42, in your summary, you mentioned that updating the numbers to 15/15 resolves the remaining jiggles. Do we also need to update the batch number, or would adjusting the initialNumToRender suffice? As increasing batch number from 5 -> 15 is quite a large change.

[joh42]

Thanks for your reply @robertKozik!

Solely increasing the initialNumToRender doesn't seem to change anything on my screen. Here I've set it to 20 while leaving maxToRenderPerBatch at 5, and this is what it looks like initially:

At the same time I agree that increasing the batch number would likely be detrimental to performance.

It is a bit annoying that 5 of the options being rendered are not even visible:

If that was not the case, all options would have been rendered. As far as I can tell we cannot do anything about that though.

[melvin-bot[bot]]

@cead22 @sonialiap @mountiny @robertKozik @joh42 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!

[robertKozik]

Got it @joh42 thanks for checking that out. I checked one more all the comments and I think that the fix which you already have is sufficient, as the rest of the jiggle is almost unnoticeable. Changing the maxToRenderPerBatch can be not worth it in terms of performance.

[joh42]

Thanks @robertKozik, then the PR is ready for review

[melvin-bot[bot]]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @joh42 got assigned: 2023-08-24 01:12:02 Z
• when the PR got merged: 2023-09-11 15:27:47 UTC
• days elapsed: 12

On to the next one 🚀

[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.68-17 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/25835

If no regressions arise, payment will be issued on 2023-09-20. :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:

• @robertKozik does not require payment (Contractor)
• @joh42 requires payment offer (Contributor)
• @namhihi237 requires payment offer (Reporter)

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[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.
• [ ] [@sonialiap] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[mountiny]

@sonialiap ready for payment.

@robertKozik can you please complete the checklist?

[mountiny]

Can you calm down melvin

[robertKozik]

On it 👀

[sonialiap]

@robertKozik does not require payment (Contractor) @joh42 fix + time bonus = $1500 - paid ✔️ @namhihi237 report $250 - paid ✔️

[sonialiap]

@robertKozik just need the checklist checked to close this out 🎉

[robertKozik]

[@robertKozik] The PR that introduced the bug has been identified. Link to the PR: I think we cannot point out exact PR which introduces it, it was tricky hard to observe and minor problem which could be treated lightly beforehand. It could resurfaced after the navigator refactor [@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: N/A [@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: N/A [@robertKozik] Determine if we should create a regression test for this bug. I don't think it's that major problem to introduce the regression test for that. Moreover it's only minor visual one [@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. N/A