[$1000] Add animation when the emoji picker is closed

[mountiny]

Problem

On mobile, the emoji picker popover is not being closed with animation compared to other popovers.

This is border line bug and new improvement, it feel like inconcsistency bug

Why is it important

This is not consistent UI

Solution

Lets add an animation to this popover when its being closed

Comign from https://expensify.slack.com/archives/C01GTK53T8Q/p1682013158901689

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~010b1720ceac20d4b1
• Upwork Job ID: 1651348863138013184
• Last Price Increase: 2023-04-26

[MelvinBot]

Triggered auto assignment to @alexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• [x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [x] 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
• [x] 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.
• [x] 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.
• [x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[mountiny]

@esh-g you have raised it

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~010b1720ceac20d4b1

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[alexpensify]

Since this was discussed in Slack, I've assigned External

[kmorales13]

Proposal

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

On mobile, the emoji picker popover is not being closed with animation compared to other popovers.

What is the root cause of that problem?

Currently the animationOutTiming prop is set to 1ms.

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

We would need to remove/update this line here https://github.com/Expensify/App/blob/9ec77046be868425e7ff129aaf1cf20cebe64e37/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js#L309 We would also need to verify there's no performance impact, since I'm guessing that's the reason this was set to 1 in the first place.

[mountiny]

@aimane-chnaif @kmorales13 what kind of animation library is providing the animations, I think we would like to move everything to Reanimated 3 is possible. That coul dbe part of this issue since otherwise its a simple change

[kmorales13]

@aimane-chnaif @kmorales13 what kind of animation library is providing the animations, I think we would like to move everything to Reanimated 3 is possible. That coul dbe part of this issue since otherwise its a simple change

The library used for Popovers/Modals, react-native-modal, uses react-native-animatable internally for animations

[aimane-chnaif]

@mountiny should we give priority to @esh-g since they suggested P/S and researched enough time? Here's slack convo

[mountiny]

@aimane-chnaif I think its same as a bug, in this case so not necessarily if we can find as good solution from other contributors.

[esh-g]

Please note that the solution that @kmorales13 is suggesting was already suggested by me in the slack conversation (https://expensify.slack.com/archives/C01GTK53T8Q/p1681233318285209).

Since this was a one-line change, I didn't post a Proposal yet as I was testing for better performance and accommodation of the animation for EmojiPicker as well. Because if you see in the video below, having animation on the contextMenu but not the EmojiPicker can look a bit odd. (As also suggested by the name of the issue)

https://user-images.githubusercontent.com/77237602/234795378-a8bf09a2-dbdc-4a50-ad87-a87fc420282f.mov

But even when we enable the animation for EmojiPicker, due to laggy performance, the animations are barely noticeable.

https://user-images.githubusercontent.com/77237602/234797794-5a3101b3-3573-4456-bcb3-2bedb39dd199.mov

The solution is to use the nativeDriver which can help massively with the performance.

https://user-images.githubusercontent.com/77237602/234798278-1c6b638e-b979-4e42-a20c-c813ec487477.mov

But it also causes the contextMenu to flash before opening the EmojiPicker. So, after I am able to solve that, I'll post a proposal here for both contextMenu and EmojiPicker

cc @mountiny @aimane-chnaif

[aimane-chnaif]

@esh-g ok good, we won't assign anyone until we 1000% confirm that it won't cause any performance issue or any other regression.

[mountiny]

Interesting, I would like to hear from SWM if they can think of anything which might have better performance/ reliability for our needs here instead of the react-native-modal

[mountiny]

Thanks for raising that @esh-g I have also asked SWM if they have any suggestions how to improve this performance or maybe even use a different library for this popover which would use Reanimated for hopefully better performance

[esh-g]

@mountiny Should we just create a fork of react-native-modal with reanimated? We already are using a patch in the library and maybe this can help us prevent unwanted updates if we maintain it ourselves?

[mountiny]

We are really trying to avoid any forks, lets wait for what SWM will say about this.

[mountiny]

@esh-g ok let's continue with your direction, have you had any luck resolving the flickering issue?

[alexpensify]

@mountiny should we mark this as external and assign it to @esh-g or wait for more proposals here?

[mountiny]

I would wait for the proposal to follow the process we have in place for external contirbutors.

[alexpensify]

Sounds like a plan and thank you for clarifying!

[esh-g]

Proposal

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

Adding animation to context menu and Emoji Picker.

What is the root cause of that problem?

We have disabled the animation out for context menu with animationOutTiming={1} and the same for the In and Out timings for EmojiPicker.

What changes do you think should be made in order to solve the problem?

We first need to remove the animationOutTiming={1} and animationInTiming={1} from the following files: https://github.com/Expensify/App/blob/e96ce6b1a141f777b924efb5a959f9bde4eae8ea/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js#L309 https://github.com/Expensify/App/blob/911134b4016fee1318fd690d3ca48edd5b1b3bca/src/components/EmojiPicker/EmojiPicker.js#L167-L168 We need to add useNativeDriver and hideModalContentWhileAnimating in index.ios.js in BaseModal.

Result

https://user-images.githubusercontent.com/77237602/235757054-a771c8a7-b14f-4b97-9671-9361483d4165.mov

Using hideModalContentWhileAnimating has been suggested by the maintainer of react-native-modal to eliminate the flickering issue. So that is essential here.

Alternate Solutions

• We can also, try to make a fork of the react-native-modal library to use reanimated as suggested here: https://github.com/Expensify/App/issues/17982#issuecomment-1526849441

[esh-g]

@mountiny Sorry for the delay with the proposal, I have posted a simple solution that fixes the issue without using reanimated, I hope we can perform testing on that. But if we find any problems, then we can look at creating a fork with reanimated as well. Please be sure to let me know of any further concerns 😇

[alexpensify]

@aimane-chnaif - can you please review the proposal? Thank you!

[aimane-chnaif]

I think earlier the flashing was being caused due to both the modals having useNativeDriver. So, this implementation eliminates that concern as well.

@esh-g I think it's recommended to use useNativeDriver to get benefit of native smooth animation. Is there a way to fix this in upstream?

[esh-g]

@aimane-chnaif Actually useNativeDriver was disabled on iOS for a known issue in react-native-modal library.

https://github.com/Expensify/App/blob/200ca8591b8c342718f1a7ab3ff4296270783f42/src/components/Modal/index.android.js#L6-L7

• The fix suggested by the maintainer of react-native-modal is to use hideModalContentWhileAnimating prop for the modal. https://github.com/react-native-modal/react-native-modal/issues/92#issuecomment-368097944

But strangely, I have not been able to reproduce the flickering anymore even when using useNativeDriver anymore on iOS 16.1. Here is the branch if you want to test it once: https://github.com/esh-g/Expensify/tree/contextMenu-emojiPicker-animations Let me know if you experience flickering issues on iOS

If this doesn't work and flickering still occurs, then we can devise the further plan to either add hideModalContentWhileAnimating or maybe move to reanimated altogether. I tried hideModalContentWhileAnimating and it appears cause no change in how the animation looks as well.

[esh-g]

@aimane-chnaif Okay, I just re-started the dev server, and the flickering is there again. I apologise for the comment above, but I tested adding the hideModalContentWhileAnimating and it seems to fix the issue. I've added the hideModalContentWhileAnimating to the iOS modal on the branch for testing.

https://github.com/esh-g/Expensify/blob/8bad89b573a37a5d7b3b9b7afeab0336078bc766/src/components/Modal/index.ios.js#L11

So, I hope that solves the flickering issues.

[MelvinBot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸