[WAITING ON CHECKLIST] [$1000] Web - Secondary contact method transition bug - clicking on a contact method that has yet to be validated opens the magic code input (validation) page with a sped up animation

[kbecciv]

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

---

Action Performed:

1. Go to settings > profile > contact method
2. Click on new contact method
3. Enter an email address
4. Click add
5. Click on the email address you just entered
6. Observe how the transition animation to the magic code input (validation) page is sped up/doesn’t work properly. If you look closely you can see that the text (sometimes) wobbles

Expected Result:

The page transition should be animated like other page transitions, without it being sped up.

Actual Result:

The page transition appears to not be animated/is sped up.

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome
• MacOS / Desktop

Version Number: 1.3.44.0
Reproducible in staging?: n/a
Reproducible in production?: n/a
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

Screen.Recording.2023-07-24.at.20.29.13.mov

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01b86e6c6c9fa20453
• Upwork Job ID: 1684009715954917376
2023-07-26
• Automatic offers:
• joh42 | Contributor | 25814019
• joh42 | Reporter | 25814020

[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

[kbecciv]

Proposal: @joh42
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690229176544109

Proposal

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

We are trying to solve the sped up animation when the secondary contact method validation page is opened.

What is the root cause of that problem?

The root cause of the problem is that focus is given to the magic code text input instantly, before the transition animation has finished.

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

The solution requiring the least amount of changes to the codebase would be to always pass shouldDelayFocus={true} from the BaseValidateCodeForm component to the MagicCodeInput component. Currently that is only passed as true for Android. Alternatively we could pass it as true for only Android and Chrome, but that would be more complex.

App/src/pages/settings/Profile/Contacts/ValidateCodeForm/BaseValidateCodeForm.js

Line 142 in beebb67

shouldDelayFocus={shouldDelayFocus}

That would be enough to solve this issue. However, while we're at it, we might want to improve the code a bit:

We can simplify the MagicCodeInput component by removing the following useEffect

App/src/components/MagicCodeInput.js

Lines 156 to 176 in 2c15e97

	useEffect(() => {
	if (!props.autoFocus) {
	return;
	}
	let focusTimeout = null;
	if (props.shouldDelayFocus) {
	focusTimeout = setTimeout(() => inputRefs.current[0].focus(), CONST.ANIMATED_TRANSITION);
	} else {
	inputRefs.current[0].focus();
	}
	return () => {
	if (!focusTimeout) {
	return;
	}
	clearTimeout(focusTimeout);
	};
	// We only want this to run on mount
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, []);

Instead we could pass shouldDelayFocus along to the first TextInput component, as opposed to not passing autoFocus if shouldDelayFocus is true:

App/src/components/MagicCodeInput.js

Lines 327 to 329 in 2c15e97

	<TextInput
	ref={(ref) => (inputRefs.current[index] = ref)}
	autoFocus={index === 0 && props.autoFocus && !props.shouldDelayFocus}

The TextInput component already supports the shouldDelayFocus prop, so removing that redundant functionality from MagicCodeInput would make the code a bit DRYer.

App/src/components/TextInput/BaseTextInput.js

Lines 67 to 70 in a656c82

	if (props.shouldDelayFocus) {
	focusTimeout = setTimeout(() => input.current.focus(), CONST.ANIMATED_TRANSITION);
	return;
	}

What alternative solutions did you explore? (Optional)

There is currently an ongoing Slack discussion regarding a general approach to solving this issue. The two alternate solutions to knowing when we should set focus is to use either InteractionManager.runAfterInteractions or onTransitionEnd. Both of those would require more changes than using the already implemented shouldDelayFocus. I nonetheless think we should wait for that discussion to conclude, and then consider if we should go with the solution that is deemed optimal even if it is more complex to implement.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01b86e6c6c9fa20453

[melvin-bot]

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

[melvin-bot]

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

[jliexpensify]

Tested on newest version and yes, it is more sped up compared to the default contact method transition.

[joh42]

Thanks for reposting my proposal from Slack! I'm just going to append the following to the alternative solutions section:
At present the discussion in Slack is leaning towards standardizing focusing using the ScreenWrapper component's onEntryTransitionEnd prop. If we do decide that it should be implemented for this issue, I think it should be done in the TextInput component to make sure the code is as DRY as possible. The tradeoff to keep in mind is that if we keep using shouldDelayFocus, we would not have to change that component at all.

Edit:
Actually, we could likely implement focusing solely using didScreenTransitionEnd in the MagicCodeInput component, possibly by passing autoFocus={index === 0 && props.autoFocus && didScreenTransitionEnd} to the TextInput component:

App/src/components/MagicCodeInput.js

Lines 327 to 329 in 2c15e97

	<TextInput
	ref={(ref) => (inputRefs.current[index] = ref)}
	autoFocus={index === 0 && props.autoFocus && !props.shouldDelayFocus}

Getting didScreenTransitionEnd like here:

App/src/pages/tasks/TaskAssigneeSelectorModal.js

Lines 181 to 182 in 8b23658

	<ScreenWrapper includeSafeAreaPaddingBottom={false}>
	{({didScreenTransitionEnd, safeAreaPaddingBottomStyle}) => (

That way we won't have to worry about passing shouldDelayFocus to the MagicCodeInput component, but rather we'll always focus after transitions.

[tienifr]

Proposal

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

The page transition appears to not be animated/is sped up.

What is the root cause of that problem?

In here, we're autofocusing the input, the input is focused before the screen transition completes, which speeds up the animation.

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

We need to use onEntryTransitionEnd and a ref to do this as we have done in various places in the app, and as aligned universally here.

1. Expose the ref of MagicCodeInput through the 2 ValidateCodeForms and TwoFactorAuthForm (the places that are using MagicCodeInput
2. In VerifyPage, ContactMethodDetailsPage and SignInPage, in onEntryTransitionEnd of ScreenWrapper, use the above ref to focus the input

There's an issue with this approach which is the input will not autofocus if we reload the page sometimes, but it's being addressed separately here so we don't need to handle it in this issue.

What alternative solutions did you explore? (Optional)

Use shouldDelayFocus, but we've agreed universally to use onEntryTransitionEnd and the ref as the standard

[jliexpensify]

Bumo @parasharrajat got some proposals!

[parasharrajat]

This is being discussed internally https://expensify.slack.com/archives/C02NK2DQWUX/p1690384611541439

[parasharrajat]

Looks like there are a few similar issues. #22850 (comment)

[joh42]

Looks like there are a few similar issues. #22850 (comment)

@parasharrajat While that issue might be tangential, focusing on reloading works well here:

Screen.Recording.2023-07-27.at.14.22.33.mov

The issue is solely related to the animation not working:

Screen.Recording.2023-07-27.at.14.23.15.mov

If there are bugs with using TransitionEnd, I suggest we keep using shouldDelayFocus here to make sure we don't break focusing, but still fix the animation

[joh42]

So that issue is more related to the alternative solution I mentioned rather than the animation issue

[parasharrajat]

I agree but similar solutions are being adopted on that issue which will indirectly solve this issue. There are more such issues.

[parasharrajat]

OK, I kind of agree that this is a different issue.

@tienifr's proposal looks good to me.

@joh42 shouldDelayFocus we will be getting away from this gradually based on the Slack thread. And I noticed that you posted an updated proposal that uses onEntryTransitionEnd but it does not look correct. autoFocus={index === 0 && props.autoFocus && didScreenTransitionEnd}#23593 (comment) will not work as changing autofocus value dynamically does not have any effect. It is mounted only.

Note: @joh42 did mention the ongoing slack thread and using onEntryTransitionEnd but they didn't use the same technique as @tienifr's proposal. It might be possible that they were also thinking the same solution but tried to optimize it with the above solution which ended up not working.

@thienlnam There is a note above for the possible duplicates.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[joh42]

@parasharrajat I'm a bit confused, what part of that proposal was not covered by my original proposal already? According to the contributing guidelines:
Note: Before submitting a proposal on an issue, be sure to read any other existing proposals. ALL NEW PROPOSALS MUST BE DIFFERENT FROM EXISTING PROPOSALS. The difference should be important, meaningful or considerable.

I clearly mentioned using transitionEnd, and while I agree that the example I wrote in the appended comment is incorrect, per https://expensify.slack.com/archives/C01GTK53T8Q/p1690295235023339?thread_ts=1690291230.607589&cid=C01GTK53T8Q proposals should not get into the nitty gritty, but rather identify the root cause and propose a solution, which I thought I did by mentioning onTransitionEnd in my original proposal. As far as I'm concerned, that proposal just goes into the nitty gritty of my alternative solution

[joh42]

Also, I'm new here, so I'm genuinely curious: should proposals contain high-level solutions, or should they actually delve into what I would consider the nitty gritty, implementation details?

[parasharrajat]

Your proposal was correctly laid out but your technique had a problem as mentioned before #23593 (comment). A working solution should be posted to a minimum.

But I agree there you did mention it earlier so I am going to talk about this internally to take a collective decision. https://expensify.slack.com/archives/C02NK2DQWUX/p1690384611541439

[joh42]

Regarding a compensation split, I would like to mention the following:

Unless I’m mistaken, the so-called ‘exact solution’ is incorrect. The SignInPage does not and should not use shouldDelayFocus as there is no transition animation. Rather, it should be focused as soon as it mounts. Default focusing using autoFocus is not impacted by this issue.

Besides, relying on onEntryTransitionEnd to delay focusing on the SigninPage would not work, as the BaseValidateCodeForm component, which contains the MagicCodeInput, is conditionally rendered, which means that onEntryTransitionEnd is not fired after that component is shown. If that page required delayed focusing, we would have to check didScreenTransitionEnd when the MagicCodeInput component mounts.

App/src/pages/signin/SignInPage.js

Line 153 in 8cdb506

{shouldShowValidateCodeForm && <ValidateCodeForm />}

To be clear, this issue is only related to shouldDelayFocus, and the only change required to fix the issue at hand for the affected pages is removing shouldDelayFocus in favor of focusing using onEntryTransitionEnd.

As such, the ‘exact solution’ is incorrect as it involves a page that does not have this issue, is not affected by the solution to this issue, and said solution would not even work for that page.

It’s also worth noting that this is not the first time you submit a proposal that is similar to an existing one and then ask for a compensation split. In issue https://github.com/Expensify/App/issues/16582 your request for a compensation split was rebuffed by an Expensify employee with the following motivation:

Quote from comment 1:

The extra details in your proposal were good, but IMO both proposals were suggesting the same solution. So a 50/50 split doesn't seem that fair to me, given that your proposal followed an existing one and you didn't have to complete the checklist.

Quote from comment 2:

Typically I might go with a more detailed proposal, but I don't want to encourage contributors to 'swipe' existing proposals just by adding more details, or to add the full diff to the prosal [sic] when it isn't necessary.

That being said, if @thienlnam disagrees with me here and thinks a compensation split is appropriate I’ll go along with it, but I think there is a case to be made against it.

[thienlnam]

There are plenty of opportunities for more work here, and I've already made my decision. Let's work on other bugs rather than continuing to go back and fourth on this issue which wastes everyone's time.

We'll continue on in this issue as planned - thanks!

[joh42]

I just noticed that the Storybook stories for the MagicCodeInput are broken on main. Should I write that in Slack? Or should I report that as a bug? @parasharrajat / @thienlnam

And should I check off the Storybook checkbox in the PR checklist as it's unrelated to this PR? All other steps for the PR are completed.

[joh42]

Checking off the Storybook checkbox even though I cannot actually test it, thanks for responding in Slack

[melvin-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-07-28 17:53:57 Z
• when the PR got merged: 2023-08-03 18:08:38 UTC
• days elapsed: 4

On to the next one 🚀

[jliexpensify]

@thienlnam @parasharrajat - would you agree that a bonus is valid since there are 2 weekend days?

[thienlnam]

Even excluding the weekend days this unfortunately does not qualify

[jliexpensify]

Ok, here's a payment summary:

• Bug Reporter / Contributor: @joh42 $250 + $1000
• C+: @parasharrajat $1000 - request on New.Dot

Upwork job

[jliexpensify]

@joh42 can you add your full name (Johannes Idarsson) to your GH profile as well? Cheers!

[joh42]

Added, thanks @jliexpensify

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

• 23593 secondary contact method transition #24002

If no regressions arise, payment will be issued on 2023-08-16. 🎊

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:

• @parasharrajat does not require payment (Eligable for Manual Requests)
• @joh42 requires payment offer (Contributor)

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:

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

[jliexpensify]

Hmm it was actually deployed to production last week...odd. @parasharrajat let me know if there's a regression test you want GH'd.

[jliexpensify]

Paid Johannes, job closed. Just awaiting checklist.

[parasharrajat]

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:

• [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR: Its a new change
• [@parasharrajat] 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: NA
• [@parasharrajat] 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: NA, it has been done quite a few times
• [@parasharrajat] Determine if we should create a regression test for this bug. Yes
• [@parasharrajat] 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.

Regression Test Steps

Secondary contact method

1. Go to settings > profile > contact method
2. Click on the new contact method
3. Enter an email address
4. Click add
5. Click on the email address you just entered
6. Observe that the transition animation is not sped up, and the magic code input is focused

Two-factor authentication

1. Make sure you are on an account where two-factor authentication has not been enabled
2. Go to settings > security > two-factor authentication
3. Click copy so you can proceed
4. Click next
5. Observe that the transition animation is not sped up, and the magic code input is focused

Do you agree 👍 or 👎 ?

[jliexpensify]

Cheers, done!

[parasharrajat]

Payment requested.

[JmillsExpensify]

Reviewed the details for @parasharrajat. This $1,000 payment is approved for NewDot based on the BZ summary above.