Page MenuHomePhabricator

[native] Workarounds for bugs after reanimated migration
ClosedPublic

Authored by angelika on Wed, Dec 11, 11:53 AM.
Tags
None
Referenced Files
F3507376: D14129.id46348.diff
Fri, Dec 20, 8:34 PM
F3505535: D14129.id46423.diff
Fri, Dec 20, 1:42 PM
F3504611: D14129.id46522.diff
Fri, Dec 20, 9:40 AM
Unknown Object (File)
Fri, Dec 20, 5:39 AM
Unknown Object (File)
Thu, Dec 19, 12:31 PM
Unknown Object (File)
Thu, Dec 19, 10:21 AM
Unknown Object (File)
Thu, Dec 19, 4:11 AM
Unknown Object (File)
Thu, Dec 19, 4:09 AM
Subscribers
None

Details

Summary

TL;DR: There is a race condition in reanimated v2 and there is a bunch of bugs because of that but after patching reanimated to disable some weird optimisation it works and the patch will likely not be needed when we migrate to reanimated v3.

Currently there are some issues with tooltip transitions described in this comment: https://linear.app/comm/issue/ENG-8149/convert-tooltip-and-overlaynavigator-to-reanimated-2-syntax#comment-20595acc
When navigating from a text message to a thread via tooltip:

  1. The tooltip first hides and then the navigation animation runs (screen slides in from the right side). On master branch the tooltip animation runs concurrently with navigation animation.
  2. The date has incorrect color
  3. The message that is being animated is "duplicated" when the next screen slides in.

And adding requestAnimationFrame fixes them all. However even after adding it there is a bug (4.) on android (physical device) that after navigating to the thread, the message that was animated disappears. In fact it’s related to the issues 2 and 3 described above.

I investigated the bugs but I didn’t have enough time to make sure that’s exactly what happens and there are some details missing so some of the explanation is just a guess.

All of those issues are caused by a race condition within reanimated. When an animated component with animated style is mounted, in component’s componentDidMount the style is “attached” to the view. That means the view tag is added to style’s view descriptors array.
Then (simplified explantion here) when animation is run on every frame worklet passed to useAnimated is evaluated and the new styles are set to a component using updateProps function.

Now let’s look at the issue 2, it will be the easiest to explain. See headerStyle animation in text-message-tooltip-button.react.js. It looks like this (I omitted irrelevant parts):

const headerStyle = useAnimatedStyle(() => {
    const opacity = interpolate(
      progressV2.value,
      [0, 0.05],
      [0, 1],
      Extrapolate.CLAMP,
    );
    return {
      opacity,
    };

What I think happens is that the animation is run too quickly after mounting the animated components. The mapper in useAnimatedStyle is started with empty sharableViewDescriptors so at the beginning of the animation the view is not updated. Then the diff between old styles and new styles is empty because opacity no longer changes (if the input range is [0, 0.05] the opacity goes to 1 very quickly) so even if there is a tag added to view descriptors, it’s too late.
That’s why requestAnimationFrame fixed it - it delayed the animation so that the descriptor is added.

What’s interesting and I discovered it by accident, adding console.log that prints anything inside a worklet in useAnimatedStyle also fixes the issue. Why?
Reanimated has an optimalization: if a worklet has a function call other than interpolate (so in our case, a call to console.log) or an if statement the animation is implemented a bit differently. This is complicated but to sum up:

Here: https://github.com/software-mansion/react-native-reanimated/blob/2.17.0/src/reanimated2/hook/useAnimatedStyle.ts#L268 the whole newValues object is applied (so the whole animated style), instead of just a diff between old and new style like here https://github.com/software-mansion/react-native-reanimated/blob/2.17.0/src/reanimated2/hook/useAnimatedStyle.ts#L498 (the diff returned here is passed to updateProps but it happens on the native side here: https://github.com/software-mansion/react-native-reanimated/blob/2.17.0/Common/cpp/Tools/Mapper.cpp#L37
After disabling the optimisation the issues 2, 3, 4 are solved even without requestAnimationFrame. I haven’t verified that issues 3 and 4 are caused by the same behaviour but it seems to be likely.

Now as the solution I added a patch that disables the above-mentioned optimisation. It probably comes with some performance hit (haven’t measured that) but disappearing messages are bad. And I’m afraid bugs like those will slow us down in the future migration, so I hope this fix will fix them all. Also soon we’ll migrate to reanimated v3 anyway. In v3 there is no such optimisation and afair the related code was rewritten. So there is a high chance the patch will not be needed and the upgrade will fix the issues.

Depends on D14128

Test Plan

Open up a text message tooltip and navigate to thread. Verify the transition looks good and the message doesn't disappear.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

angelika held this revision as a draft.

Thank you for a detailed and thorough explanation!! You went really deep here, love it :)

This revision is now accepted and ready to land.Wed, Dec 11, 8:01 PM

Rebase and address feedback