fixed modal animations

[parasharrajat]

Please review.

Details

Details can be read #2335 (comment)

Fixed Issues

Fixes #2335

Tests / QA Steps

1. Click the search icon to open the Search Modal.
2. Open all the available models from the FAB button.
3. Open the User Details / Group participants modal.
4. Open the User Settings Modal.
5. For all the modals enter and exit animation should be smooth and consistent.

Tested On

• Web
• Mobile Web
• Desktop
• iOS
• Android

Screenshots

Web

animation.mp4

Mobile Web

animation-mWeb.mp4

Desktop

animation-des.mp4

iOS

Not able to record it as I have MAC VM but the animation on IOS works smoothly.

Android

1620411237903.mp4

[MariaHCD]

Requesting reviews from those who reviewed the original PR as they will have additional context!

[parasharrajat]

I have excluded the changes for the Emoji Picker as my changes caused a regression and i think its better to work on Emoji Picker separately as there couple of things that need to be fixed on it.

@marcaaron Initially I was suggesting to update the react-native-modal to latest minor as they have made few tweaks such as settings Interactions before animations but it won't be helpful on the web as InteractionManager does not work there.

So sticking to navigation event is fine. but the lib may be updated as there are no breaking changes, if wanted.

[marcaaron]

@parasharrajat can we maybe figure out how to apply this solution to the IOU pages? The animations there are still inconsistent.

2021-05-04_10-15-41.mp4

Also, note there is still some lag when opening the search, new chat, new group pages. Which is probably something in addition to waiting for the text input. Could we maybe wait until the transition is done to do some extra work being done on those pages?

I'm also curious if we can incorporate this into the ScreenWrapper component so the logic is not repeated many times. Here's what I'm thinking...

1. Add the this.props.navigation.addListener logic to ScreenWrapper and inside set up the logic if we have passed an onTransitionEnd prop
2. Next figure out which expensive things interfere with the layout transition in each screen and perform that logic after the onTransitionEnd callback is done.

Lmk if that makes sense? I think it will give us a more sustainable way to handle similar issues in the future.

[marcaaron]

I could be wrong, but I thought this could be more useful in the ScreenWrapper and then we just pass a prop like isScreenTransitioning. Maybe it would be more complicated. Perhaps the same could be done with an HOC so we don't need to drill something like an isScreenTransitioning prop down.

[marcaaron]

Of course, it depends on if there are other places where we'll need to do this. I think there already are..

[jasperhuangg]

+1, this looks like something that'll be needed down the road

[parasharrajat]

@marcaaron I noticed the same In our case rendering the optionList is very expensive. I would try to see what can be done to minimize the load.

And good Idea in moving the logic to ScreenWraper.

[jasperhuangg]

I wonder about creating a HOC for isScreenTransitioning, seems like a good idea and I'd like to get your thoughts

[parasharrajat]

As marc mentioned that we can just move this logic up to ScreenWrapper which is already used in all screens thus this hook would be unnecessary at times.

It seems that HOC will be good.

[parasharrajat]

So I profiled the pages and found that the fps is pretty low for the app. We have a lot of long tasks and some microtasks take a long time to finish. I was not really able to pinpoint that to the source in our app as performance tools are not pointing it correctly on chrome but efforts are still continued on it.

Few things I noticed.

1. OptionSelector component's first render is too expensive. if we delay its render until the SearchPage animation has complete, the animation is smooth.
2. React-navigation 's navigation to any screen is very expensive on the web. When we click on the Search Icon it takes an enormous(blocking the thread) 200ms to naviagte. On Native, it performs well due to react-native-screens.
3. I also noticed that React native's Image module uses JS to download images that were seen consuming the thread while the search page was opening.
4. This punch in a question, Are we allowing the browser to reuse the cache for the same images? I think we pass a new hash in the URL for each instance of an image.

Any help in how can I correctly pinpoint the source code while profiling will be great. Browser points to the low lever lib code instead.

[marcaaron]

OptionSelector component's first render is too expensive. if we delay its render until the SearchPage animation has complete, the animation is smooth.

Nice! That might be a good place to start as it is easy to delay the rendering and replace with a loading spinner.

React-navigation 's navigation to any screen is very expensive on the web. When we click on the Search Icon it takes an enormous(blocking the thread) 200ms to naviagte

Not sure how to get around this one. Maybe a custom animation for the drawer content. It seems like an issue that has been reported before.

Are we allowing the browser to reuse the cache for the same images? I think we pass a new hash in the URL for each instance of an image.

We should be. At least for avatars, once they load they are not loaded again.

2021-05-06_07-17-50.mp4

[parasharrajat]

@marcaaron Just found some evidence of Onyx could block the thread. So I think the changes you suggested a while back about reusing the subscription and just emitting data from the memory if there are existing subscribers for the key, will work great here. https://expensify.slack.com/archives/C01GTK53T8Q/p1620065578311900.

onyx.mp4

I could think of few things which are already suggested by a lot of persons.

1. Using Multiget instead of one key at a time then dispatching all changes at once(calling setState once on the onyx instance)
2. The one you suggested.

[marcaaron]

@parasharrajat Got it, I think we can wait for Kid Roca to work on that issue in the Slack thread.

Using Multiget instead of one key at a time then dispatching all changes at once(calling setState once on the onyx instance

This would require a refactor of Onyx, but agree it could be nice. I think it's not in scope for this issue.

The changes here are looking great! So I think we can roll with what we have and then propose additional improvements later?

[parasharrajat]

Yeah, those changes are not under the PR scope. I have just updated the Video for Web where you can see the loader added.
I tried a lot of ways to achieve the best speed but animations are janky as they are running on the same js thread but the rendering of modal screens only is taking 100% CPU allocations thus animation frames are skipped. So far I believe we can only delay the heavy rendering until the animation is finished which run animations near >50fps. I have pushed the changes for the same.

[marcaaron]

Looks good had a few suggestions.

[parasharrajat]

@marcaaron Updated.

[marcaaron]

Looks great and works great.

[jasperhuangg]

tested locally and works, nice! Don't forget to resolve the merge conflicts.

[parasharrajat]

@jasperhuangg Updated.

[OSBotify]

🚀 Deployed to staging in version: 1.0.42-2🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[roryabraham]

This PR may have caused a regression. The weird thing is it's very inconsistent: sometimes the transitionEnd event handler is executed, and sometimes it's not. I have not been able to find a pattern.

[roryabraham]

On the revert branch I am unable to reproduce the regression, so I think we should revert this PR and try again.

[parasharrajat]

Thanks. I was going to point that in the slack. It seems that transitionEnd is not reliable on IOS. We have to find another alternative to it. Damn.

[OSBotify]

🚀 Deployed to production in version: 1.0.44-0🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[MitchExpensify]

Paying now! Apologies for the delay @parasharrajat, nice job!

[parasharrajat]

Wait we have revert the PR. New PR is in WIP. @MitchExpensify

[MitchExpensify]

My mistake. Can you point me to the new PR or does that exist yet?

[parasharrajat]

Here it is #2956. We faced one regression on IOS for which there is no strong reason why it is happening on IOS but let me just ask Marc what does he think about it?

[MitchExpensify]

Awesome, thanks. I'll keep an eye on that and end the contract once its merged and no issues after 7 days.