Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix targetGlobalOriginX/Y in custom layout animations #4052

Merged
merged 3 commits into from
Feb 21, 2023

Conversation

graszka22
Copy link
Member

Summary

Fixes #3991

The bug was caused by snapshot being made before all views have layouts updated. It might happen that the child of the view has snapshot made before the parent view is updated, leading to invalid global coordinates.

On iOS we simply make snapshots after all views are updated.

On Android it's more hacky because we don't know when all views have their layouts updated. Only React knows. I dig up a reset() method in LayoutAnimationController that is called by React after all views are updated. It's not semantically correct, but I think there is no semantically correct way to do that unless we want to patch React code.

Test plan

  • test layout animations examples
  • test shared element transition examples (@piaskowyk they're not merged yet?)
  • test example that reproduces the bug:
Example that reproduces the bug ```js import Animated, { Easing, withRepeat, withTiming, } from 'react-native-reanimated'; import { Button, Dimensions, View } from 'react-native'; import React, { useState } from 'react';

const { width: screenWidth } = Dimensions.get('screen');

const Placeholder = ({ width }) => {
return (
<View
style={{
height: 20,
width,
backgroundColor: 'blue',
marginBottom: 8,
}}>
<Animated.View
entering={SlideThroughScreen}
style={{ height: '100%', width: 2, backgroundColor: 'red' }}
/>

);
};

const SlideThroughScreen = (values) => {
'worklet';
console.log('VALUES', values.targetGlobalOriginX);

const animations = {
originX: withRepeat(
withTiming(-values.targetGlobalOriginX + screenWidth, {
duration: 2000,
easing: Easing.linear,
}),
-1
),
};

const initialValues = {
originX: -values.targetGlobalOriginX,
};

return {
initialValues,
animations,
};
};

export default function AnimatedStyleUpdateExample() {
const [show, setShow] = useState(false);
return (
<View
style={{
flex: 1,
flexDirection: 'column',
marginTop: 150,
}}>
<Button onPress={() => setShow(!show)} title="TOGGLE ANIMATION" />
{show && (
<View
style={{
flexDirection: 'row',
justifyContent: 'space-between',
padding: 24,
}}>
<View style={{ justifyContent: 'space-around' }}>



<View
style={{ justifyContent: 'space-around', alignItems: 'flex-end' }}>




)}

);
}

The red lines should animate from the left to the right edge of the screen, like on this video:

https://user-images.githubusercontent.com/6280457/218586432-a0ef31e2-984b-490e-93c5-af10eca176e4.mp4
</details>

@graszka22 graszka22 requested a review from piaskowyk February 13, 2023 22:16
@graszka22 graszka22 added this pull request to the merge queue Feb 21, 2023
Merged via the queue into main with commit f5e666a Feb 21, 2023
@graszka22 graszka22 deleted the graszka22/fix-targetGlobalOriginX branch February 21, 2023 18:18
fluiddot pushed a commit to wordpress-mobile/react-native-reanimated that referenced this pull request Jun 5, 2023
…sion#4052)

<!-- Thanks for submitting a pull request! We appreciate you spending
the time to work on these changes. Please follow the template so that
the reviewers can easily understand what the code changes affect. -->

## Summary

Fixes software-mansion#3991

The bug was caused by snapshot being made before all views have layouts
updated. It might happen that the child of the view has snapshot made
before the parent view is updated, leading to invalid global
coordinates.

On iOS we simply make snapshots after all views are updated.

On Android it's more hacky because we don't know when all views have
their layouts updated. Only React knows. I dig up a `reset()` method in
`LayoutAnimationController` that is called by React after all views are
updated. It's not semantically correct, but I think there is no
semantically correct way to do that unless we want to patch React code.

## Test plan

- test layout animations examples
- test shared element transition examples (@piaskowyk they're not merged
yet?)
- test example that reproduces the bug:

<details>
<summary>Example that reproduces the bug</summary>
```js
import Animated, {
  Easing,
  withRepeat,
  withTiming,
} from 'react-native-reanimated';
import { Button, Dimensions, View } from 'react-native';
import React, { useState } from 'react';

const { width: screenWidth } = Dimensions.get('screen');

const Placeholder = ({ width }) => {
  return (
    <View
      style={{
        height: 20,
        width,
        backgroundColor: 'blue',
        marginBottom: 8,
      }}>
      <Animated.View
        entering={SlideThroughScreen}
        style={{ height: '100%', width: 2, backgroundColor: 'red' }}
      />
    </View>
  );
};

const SlideThroughScreen = (values) => {
  'worklet';
  console.log('VALUES', values.targetGlobalOriginX);

  const animations = {
    originX: withRepeat(
      withTiming(-values.targetGlobalOriginX + screenWidth, {
        duration: 2000,
        easing: Easing.linear,
      }),
      -1
    ),
  };

  const initialValues = {
    originX: -values.targetGlobalOriginX,
  };

  return {
    initialValues,
    animations,
  };
};

export default function AnimatedStyleUpdateExample() {
  const [show, setShow] = useState(false);
  return (
    <View
      style={{
        flex: 1,
        flexDirection: 'column',
        marginTop: 150,
      }}>
      <Button onPress={() => setShow(!show)} title="TOGGLE ANIMATION" />
      {show && (
        <View
          style={{
            flexDirection: 'row',
            justifyContent: 'space-between',
            padding: 24,
          }}>
          <View style={{ justifyContent: 'space-around' }}>
            <Placeholder width={120} />
            <Placeholder width={80} />
          </View>
          <View
            style={{ justifyContent: 'space-around', alignItems: 'flex-end' }}>
            <Placeholder width={200} />
            <Placeholder width={90} />
          </View>
        </View>
      )}
    </View>
  );
}

```
The red lines should animate from the left to the right edge of the
screen, like on this video:


https://user-images.githubusercontent.com/6280457/218586432-a0ef31e2-984b-490e-93c5-af10eca176e4.mp4
</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong values of targetGlobalOriginX in custom layout animations
2 participants