[HOLD for payment 2023-10-09] [$1000] The transition animation doesn't work for the request money page

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

HOLD on #26415

Action Performed:

1. Click on the fab button and select request money
2. Enter an amount
3. Click on the currency or on next
4. Watch what happens

Expected Result:

The new page opens with an horizontal animation

Actual Result:

The new page opens without animation

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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.37-2
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

2023-07-05.19-26-27.mp4

Recording.1207.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01571ed2ce05eced7f
• Upwork Job ID: 1679102953265299456
• Last Price Increase: 2023-08-02
• Automatic offers:
• tienifr | Contributor | 26776245
• ShogunFire | Reporter | 26776250

[melvin-bot]

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

[ShogunFire]

Proposal

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

When we click on the currency or next in requestmoney page there is no horizontal sliding animation

What is the root cause of that problem?

The root cause of that problem is that in:

• IOUCurrencySelection
• MoneyRequestParticipantsSelector
• MoneyRequestParticipantsSplitSelector

we use an OptionsSelector for example here

App/src/pages/iou/IOUCurrencySelection.js

Lines 119 to 129 in c0ece67

	<OptionsSelector
	sections={sections}
	onSelectRow={confirmCurrencySelection}
	value={searchValue}
	onChangeText={setSearchValue}
	textInputLabel={translate('common.search')}
	headerMessage={headerMessage}
	safeAreaPaddingBottomStyle={safeAreaPaddingBottomStyle}
	initiallyFocusedOptionKey={initiallyFocusedOptionKey}
	shouldHaveOptionSeparator
	/>

By default OptionSelectors focus the input here:

App/src/components/OptionsSelector/BaseOptionsSelector.js

Line 122 in c0ece67

this.textInput.focus();

And that can cause the transition animation to be stopped.

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

We can use the same method that was used here https://github.com/Expensify/App/pull/22621/files to focus the input, for our case we are not in the same file that the input when we declare the screenwrapper so we will have to use forward ref in BaseOptionSelector.

The problem with that solution is that the input is not focused when we reload the page because onEntryTransitionEnd is never called.

In ScreenWrapper before we declare the listener for the end of the transition we can check the active route index in the stack, if it is 0 it means that we accessed this page without transition and we can call onEntryTransitionEnd directly without passing by the listener

What alternative solutions did you explore? (Optional)

We can set up a boolean isTransitionHappening to true by listening to the event transitionStart, I think we should do that in Navigation. And test if that boolean is true before setting up the listener for the end of the transition

[chiragxarora]

hmmm weird, I think we never had shouldDelayFocus on these, was going through the recent commits on the file and seems like something got wrong after navigation refactor maybe?

[ShogunFire]

@chiragxarora I am not sure but maybe that has to do with that PR where we added withNavigationFocus: https://github.com/Expensify/App/pull/19824/files

[melvin-bot]

@JmillsExpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

[JmillsExpensify]

Hmm, yeah I'm not sure how/why these pages stopped being animated with the page coming in from the right. Regardless, I agree we should consistently animate, so I'm opening this one up to the community.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01571ed2ce05eced7f

[melvin-bot]

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

[melvin-bot]

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

[b4s36t4]

Proposal

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

The transition animation doesn't work for the request money page

What is the root cause of that problem?

Root Cause of the issue is that we have an input inside the modal which is having autoFocus prop default to true which is causing and breaking the animation. Ideally we should only focus the input or render anything at all inside the modal only after transition done.

Existing issues like #22036 does possess same problem like this.

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

The simple solution we can use is adding shouldDelayFocus but according to the other issues I followed up shouldDelayFOcus can't be used because of setTimeOut used inside the logic. Even more to this if the list of user-contacts is high (if it's high traffic account) this issue comes again.

To resolve this we should manually handle focus of input.

Changes

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSplitSelector.js

Line 167 in a047b88

<OptionsSelector

Here we should add autoFocus={false} to disable auto-focus.

From here

App/src/pages/iou/steps/MoneyRequestAmountPage.js

Line 415 in a047b88

{({safeAreaPaddingBottomStyle}) => (

We can take the state didScreenTransitionEnd and pass it to the

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js

Line 98 in a81b63a

<MoneyRequestParticipantsSplitSelector

here.

Like the following

autoFocus={didScreenTransitionEnd}

Using this inside these two components

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js

Line 105 in a81b63a

<MoneyRequestParticipantsSelector

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js

Line 98 in a81b63a

<MoneyRequestParticipantsSplitSelector

We create ref maybe like

const optionRef = useRef(null)

and assign this ref here

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js

Line 156 in a81b63a

<OptionsSelector

using the below code

ref={(element) => optionRef.current = element}

And when we see the props.autoFocus as true within componentDidUpdate or useEffect we do autoFocus using

optionRef.current.textInput.focus()

Also if we have a high traffic account which does have multiple user contacts in that case also the transition could get effected to fix this we should add a check here as well

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js

Line 153 in a81b63a

const isOptionsDataReady = ReportUtils.isReportDataReady() && OptionsListUtils.isPersonalDetailsReady(this.props.personalDetails);

To check weather transition ended or not then only we do show options until then loader will be shown.

What alternative solutions did you explore? (Optional)

I have seen this issue happens in other places as well, we could consider conditional rendering here

App/src/components/ScreenWrapper/index.js

Line 84 in 85739ea

<SafeAreaConsumer>

Adding a conditional rendering based on the state didScreenTransitionEnd could fix this issue at all maybe.

Result

kk.mp4

[chiragxarora]

@b4s36t4 did this work for you?
I tried exactly same thing but I was unable to animate... I see you haven't added the results video

[b4s36t4]

It was working for me.

[chiragxarora]

great!
maybe I missed something

[b4s36t4]

@chiragxarora Updated the proposal with the video.

[ShogunFire]

Hello @b4s36t4 can you also check my proposal here: #22388 (comment)

We use onDelayFocus everywhere so I don't understand @chiragxarora comment about it.

[b4s36t4]

I think he's saying we never did added shouldDelayFocus prop anywhere in the component to make it slide. Yes I see your proposal works, but it's work around #22271 please see this issue :( it did rejected for me because of shouldDelayFocus.

[tienifr]

This has the same RCA as #22271, also my proposal can solve this as well with some more configurations.

@allroundexperts Do you think we should combine all the autofocus-block-transition issues and fix them all at once as you've pointed out in #22271 (comment)?

cc @chiragxarora

[allroundexperts]

This has the same RCA as #22271, also my proposal can solve this as well with some more configurations.

@allroundexperts Do you think we should combine all the autofocus-block-transition issues and fix them all at once as you've pointed out in #22271 (comment)?

cc @chiragxarora

I think we should combine them, yes.

[tienifr]

Update: So far I have found three affected pages:

• RequestMoneyConfirmPage
• SplitBillDetailsPage: select participants
• Pronouns: change pronouns page

Will dig deeper tomorrow.

[ShogunFire]

By the way if I understand correctly someone is trying to fix this problem in every page at the same time here: #19530 (comment)

[roryabraham]

@JmillsExpensify this issue appears to be eligible for the #urgency bonus. The PR was created, tested, and C+ approved within 3 working days of assignment. It was only delayed by me, so I think it's fair that @tienifr and @eVoloshchak get the urgency bonus. Thanks!

[tienifr]

@mountiny Did you comment on the wrong issue? The regression issues were 28417 and 26945. Not this one.

[mountiny]

oops, yes, but speaking of devil this deploy blocker seems highly related to this PR/issue #28523 can you have a look @tienifr @eVoloshchak

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[mountiny]

Coming back from this issue #28523 the PR linked is already in staging, but the request money page transitions are not working. also noticed different bug which I will report in expensify bugs

Screen.Recording.2023-10-02.at.16.06.08.mp4

[tienifr]

@mountiny Hmm I'm not sure about your expectations for transition animation here? I see that it's working fine. Can you elaborate more?

[melvin-bot]

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

• Fix: Transition animation does not work for request money page #27947

If no regressions arise, payment will be issued on 2023-10-09. 🎊

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:

• @eVoloshchak does not require payment (Eligible for Manual Requests)
• @tienifr requires payment offer (Contributor)
• @ShogunFire 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]

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:

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

[mountiny]

@tienifr for example on the 14th second you can see the page blinks completely, in others you can see the transition is not smooth

I understand if that is related to more generic issues such as too heavy rerenders, but wanted to raise that

[JmillsExpensify]

I believe based on the above the correct payment summary is:

• Issue reporter: @ShogunFire $250 (reported before 25 Aug)
• Contributor: @tienifr $1,500 (incl. urgency bonus)
• Contributor+: @eVoloshchak $1,500 (incl. urgency bonus)

@mountiny Did you potentially want to ask for a follow-up PR, or are you good with this summary?

[mountiny]

We are good here

[JmillsExpensify]

Issue reporter and contributor have been paid. @eVoloshchak Please complete the BZ checklist and add a request via NewDot so we can get this issue closed. Thanks!

[eVoloshchak]

• The PR that introduced the bug has been identified. Link to the PR: Refactor/23149 money request page #23979
• 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: https://github.com/Expensify/App/pull/23979/files#r1354565648
• 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: this has already been discussed and there is now a universal way to autofocus a TextInput, which resolves the problem we were having
• Determine if we should create a regression test for this bug.
  Is it easy to test for this bug? Yes
  Is the bug related to an important user flow? Yes
  Is it an impactful bug? No
  The bug has near zero impact and doesn't prevent user from using the app, I don't think a regression test is needed in this case

[JmillsExpensify]

$1,500 payment approved for @eVoloshchak based on BZ summary.