[HOLD for payment 2023-07-14] [$1000] Animation glitches on CTRL+shift+K shortcut after CTRL+K

[kavimuru]

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. Press CTRL+K to open search
3. Press CTRL+shift+K to open new group menu and observe that app pushes whole content to left and search menu is partially visible for few seconds creating glitched animation

Expected Result:

Animation between keyboard shortcuts should be clean as it is when we press CTRL+K after CTRL+shift+K

Actual Result:

Animation glitches when we press CTRL+shift+K after CTRL+K

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.26-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

bad.animation.between.shortcuts.mp4

Recording.953.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01d07caa1456efda56
• Upwork Job ID: 1668365752332894208
• Last Price Increase: 2023-06-19

[melvin-bot]

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

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[Thanos30]

Proposal

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

Using consecutive alternations between CTRL+SHIFT+K and CTRL+K breaks the animation of the Right Modal. Also noticed that the order doesn't matter, you can start with either CTRL+K or CTRL+SHIFT+K , as long as you use both and then try to repeat it will break the animation. TLDR it breaks on A - B - A pattern.

What is the root cause of that problem?

The root cause of the problem is the Navigation type. I logged the Navigation types while trying to reproduce the issue and these were the logs:

• CTRL+K -> action.type went from NAVIGATE to PUSH
• CTRL+SHIFT+K -> action type remained NAVIGATE
• CTRL+K -> action type remained NAVIGATE

While the action type was supposed to change to REPLACE, it remained as NAVIGATE, and the NAVIGATE type leads to that strange animation on the transition of the 2 shortcuts. The reason this happens is because this scenario doesn't meet the requirements for the below part of the navigation code to be triggered, located in linkTo.js:

  // If this action is navigating to the RightModalNavigator and the last route on the root navigator is also RightModalNavigator
            // then we want to replace the current RHP state with new one
        } else if (type === 'UP') {
            action.type = 'REPLACE';

As you can see, we only expect to do that when we call the navigation with the type being 'UP', which is used on native navigation stuff.

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

After discussing the issue with @WoLewicki ( thank you for your support on this one 🙏 ), the ideal way to approach this issue would be to keep the animation of RHP while moving into deeper levels of the same screen, but discard the animation when moving onto a new top-level RHP.

The solution will take place on linkTo.js file with the following additions:

    const currentRoute = _.last(root.getState().routes);
    const navigatedRoute = _.last(state.routes);

    const bothAreRHP = currentRoute.name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR && navigatedRoute.name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR;

    const currentScreen = currentRoute.params ? currentRoute.params.screen : null;
    const navigatedScreen = navigatedRoute.state.routes[0].name

    const sameScreen = currentScreen === navigatedScreen;

In the above code, we get the last route of our current state and the destination route after the navigation command, making sure first that we are facing a RHP to RHP scenario.

After that, we get the immediate path of the RHP both in our current state and the one we are navigating to.

If both are on the same name (e.x Search) , then we won't discard the animation, since the navigation is between levels of the same screen ( Search ). If these 2 are different (e.x Search -> NewGroup), then we will discard the animation.

Below I post a video demonstrating the logs of these 2 variables above, to show the functionality:

Screen.Recording.2023-06-19.at.8.28.56.PM.mov

All we have to do is check wether we are on a RHP to RHP navigation, and the check wether we are on the same screen as previously, otherwise remove the animation and make the action type REPLACE

Video of solution:

Screen.Recording.2023-06-13.at.1.57.11.AM.mov

[melvin-bot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[mountiny]

@WoLewicki @adamgrzybowski could you review the proposals on this issue please cc @abdulrahuman5196

[HezekielT]

Proposal

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

Animation glitches on CTRL+SHIFT+K shortcut after CTRL+K

What is root cause of that problem?

When we open modals that are in RightModalNavigator.js, we add a defaultModalScreenOptions option to the stack screen as shown below.

App/src/libs/Navigation/AppNavigator/Navigators/RightModalNavigator.js

Lines 22 to 26 in 71da78e

	<Stack.Screen
	name="NewGroup"
	options={defaultModalScreenOptions}
	component={ModalStackNavigators.NewGroupModalStackNavigator}
	/>

So every time modals in RightModalNavigator.js are opened, defaultModalScreenOptions will be added to the screens.

Now in defaultModalScreenOptions.js , we call cardStyleInterpolators.forHorizontalIOS as shown below.

App/src/libs/Navigation/AppNavigator/defaultModalScreenOptions.js

Lines 4 to 10 in 71da78e

	const defaultModalScreenOptions = {
	headerShown: false,
	animationEnabled: true,
	gestureDirection: 'horizontal',
	cardStyle: styles.navigationScreenCardStyle,
	cardStyleInterpolator: CardStyleInterpolators.forHorizontalIOS,
	};

This cardStyleInterpolators.forHorizontalIOS is responsible for the overlayOpacity, shadowOpacity and animation(transition) of modals when they are opened.

Now this is where the problem arises. Every time we open a modal that is in RightModalNavigator.js ,
cardStyleInterpolators.forHorizontalIOS will create a new animation which results in the animation glitch.

Note – This issue is not limited only to shortcut modals but also for every modals that are in RightModalNavigator.js since they also include defaultModalScreenOptions as their option.

So, for example, the animation will also glitch when we open setting => CTRL+K

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

In defaultModalScreenOptions.js here, implement the following changes and the issue will be solved.

const defaultModalScreenOptions = {
   ....
-  cardStyleInterpolator: CardStyleInterpolators.forHorizontalIOS,
+ cardStyleInterpolator: (props) => {
+        return props.index === 0 && CardStyleInterpolators.forHorizontalIOS(props)
+  },
}

Detailed explanation on how the above solution works -

When we call CardStyleInterpolators.forHorizontalIOS in defaultModalScreenOptions.js, we also implicitly pass a prop which will be used by the @react-navigation/stack to do the overlayOpacity, animation etc.. as shown below.

https://github.com/react-navigation/react-navigation/blob/7d8b5156dff9a0c6a5ddca6c15dc7e5c65e94bec/packages/stack/src/TransitionConfigs/CardStyleInterpolators.tsx#L14-L19

Now there is another prop called index in additon to current, next, inverted etc.., which increments every time(starting from zero) when we open a new modal that is in defaultModalScreenOptions.js. So, for example, in this issue when we open CTRL + SHIFT + K => CTRL + K, the CTRL + SHIFT + K will have an index value of 0 and CTRL + K will have an index value of 1. Therefore, by passing the props explicitly and by using this index value as suggested above, we call CardStyleInterpolators.forHorizontalIOS only on the lowest modal and prevent it from being called by other highest modals hence solving the animation glitch.

This solution is simple and general.

What alternative solutions did you explore? (Optional)

None

[WoLewicki]

In RightModalNavigator.js you can see that all flows from different RHP are different navigators, but it is a one big navigator at the top. So when you change something at the top level, e.g. from Settings to New Chat, there should be no animation (and I believe that REPLACE should be called there since we want to swap those and not want to be able go back to the previous one). I would go with some kind of a fusion of those two proposals, where we want to detect if we are switching screens at the top level of RHP, and then do a replace without animation. And when you are on the first index, when you push or pop it, there should be an animation. Is it clear enough?

[Thanos30]

@WoLewicki This is more or less in my proposal, we would check if RHP is already open to use REPLACE and discard the animation, unless I understood what you said incorrectly.

[WoLewicki]

Yeah, it just depends how would you like to check if there is RHP already. Ideally it would be checked in e.g. linkTo.js or Navigation.js and based on the states produced.

[Thanos30]

On linkTo.js we can use the navigation parameter to get the state, navigation.getState(), which returns the routes and we can see if RightHandPane is there.

[WoLewicki]

Yeah, but we can navigate inside one of the flows in RHP, so we have to check if we would navigate at the top level of it with the newest action.

[Thanos30]

Alright that's a fair point, had to go out for a moment, will check that once I am back

[Thanos30]

@WoLewicki I noticed now that via the state ( const state = getStateFromPath(path); ) we can check the depth of the route, where we can see if we are going on an inner level or top level.

[mountiny]

@Thanos30 have you been able to accommodate the feedback to your proposal?

[Thanos30]

@Thanos30 have you been able to accommodate the feedback to your proposal?

I believe so, yeah, I am just waiting for a confirmation on my latest comment concerning the depth of the route, to determine if we are heading to top level or inner level

[melvin-bot]

@WoLewicki, @mountiny, @abdulrahuman5196, @adelekennedy Whoops! This issue is 2 days overdue. Let's get this updated quick!

[mountiny]

PR merged will need one quick follow up

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

• [navigation-refactor] Use minimal action to navigate inside RHP #21067

If no regressions arise, payment will be issued on 2023-07-14. 🎊

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

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:

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

[abdulrahuman5196]

The PR that introduced the bug has been identified. Link to the PR:
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:
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 issue was caused after the navigation refractor and fixed as part navigation refractor followups. So we can't point a single PR for this issue.

Determine if we should create a regression test for this bug.

Yes.

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.

1. Press CTRL+K to open search
2. Press CTRL+shift+K
3. New group panel should open smoothly without any shakes/glitches.

[abdulrahuman5196]

@adelekennedy
BZ checklist is complete here #20621 (comment)

Payment processing for this issue should be unblocked now

[adelekennedy]

@abdulrahuman5196 thank you! Processing payment now

[mountiny]

$250 for @Thanos30 for help with the proposal
$1000 to @abdulrahuman5196
$250 to @dhanashree-sawant

Thanks for paying this out @adelekennedy

[abdulrahuman5196]

Actually payment hasn't been processed yet.

cc: @adelekennedy

[adelekennedy]

@Thanos30 @abdulrahuman5196 @dhanashree-sawant please apply here: https://www.upwork.com/jobs/~01848cfeb779ada330

[abdulrahuman5196]

Applied @adelekennedy

[Thanos30]

Same here @adelekennedy , Thanks 🙏

[dhanashree-sawant]

Thanks @adelekennedy, I have also applied to the job.

[adelekennedy]

Just pending @dhanashree-sawant payment

[dhanashree-sawant]

Thanks @adelekennedy, offer accepted.

[adelekennedy]

Done!