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: Crashes during CSS transitions of complex properties #6966

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

MatiPl01
Copy link
Member

@MatiPl01 MatiPl01 commented Jan 31, 2025

Summary

There were a few different issues related to boxShadow interpolation in CSS transitions (and to other non-primitive props except transforms, where I had a special case handling logic implemented).

List of changes

  • fixed props diffing function getChangedProps - it handled arrays in the same way as objects, which resulted in creating objects with indexes as keys instead of arrays,
  • cleaned up and simplified getChangedProps implementation,
  • fixed group interpolators (arrays and objects) updateKeyframesFromStyleChange method implementation used by CSS transitions, which didn't support arrays of different lengths / objects with different keys, where keys of the old object not existing in the new one were ignored,
  • fixed boxShadow string parsing function parseBoxShadowString and added tests to this function to make sure it works as expected,

Examples

boxShadow example

Source code
/**
 * This example is meant to be used for temporary purposes only. Code in this
 * file should be replaced with the actual example implementation.
 */

import { useState } from 'react';
import type { ViewStyle } from 'react-native';
import { StyleSheet, View } from 'react-native';
import Animated from 'react-native-reanimated';

import { Button, Screen } from '@/apps/css/components';

const transitionStyles: Array<ViewStyle> = [
  {
    boxShadow: '0 0 10px 0 rgba(0, 0, 0, 0.5)',
  },
  {
    boxShadow:
      '0 0 20px 20px red, 20px 20px 20px 20px blue, -20px -20px 20px 20px green',
  },
];

export default function Playground() {
  const [state, setState] = useState(0);
  const stateToStyle = (num: number) => {
    return transitionStyles[num % transitionStyles.length];
  };

  return (
    <Screen>
      <View style={styles.container}>
        <Button
          title="Change state"
          onPress={() => {
            setState(state + 1);
          }}
        />
        <Animated.View
          style={[
            {
              backgroundColor: 'red',
              height: 65,
              marginTop: 60,
              transitionDuration: '0.5s',
              transitionProperty: 'all',
              transitionTimingFunction: 'ease-in-out',
              width: 65,
            },
            stateToStyle(state),
          ]}
        />
      </View>
    </Screen>
  );
}

const styles = StyleSheet.create({
  container: {
    alignItems: 'center',
    flex: 1,
    justifyContent: 'center',
  },
});
Before After
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-01-31.at.21.18.59.mp4
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-01-31.at.21.01.51.mp4

Separate props shadow example

Source code
/**
 * This example is meant to be used for temporary purposes only. Code in this
 * file should be replaced with the actual example implementation.
 */

import { useState } from 'react';
import type { ViewStyle } from 'react-native';
import { StyleSheet, View } from 'react-native';
import Animated from 'react-native-reanimated';

import { Button, Screen } from '@/apps/css/components';

const transitionStyles: Array<ViewStyle> = [
  {
    shadowColor: 'red',
    shadowOffset: { height: 20, width: 10 },
    shadowOpacity: 1,
    shadowRadius: 10,
  },
  {
    shadowColor: 'blue',
    // this is not a valid value because width and height props must be
    // specified but I added it on purpose to check if transition between
    // objects with different number of properties works
    shadowOffset: { height: 100 },
    shadowOpacity: 1,
    shadowRadius: 50,
  },
];

export default function Playground() {
  const [state, setState] = useState(0);
  const stateToStyle = (num: number) => {
    return transitionStyles[num % transitionStyles.length];
  };

  return (
    <Screen>
      <View style={styles.container}>
        <Button
          title="Change state"
          onPress={() => {
            setState(state + 1);
          }}
        />
        <Animated.View
          style={[
            {
              backgroundColor: 'red',
              height: 65,
              marginTop: 60,
              transitionDuration: '0.5s',
              transitionProperty: 'all',
              transitionTimingFunction: 'ease-in-out',
              width: 65,
            },
            stateToStyle(state),
          ]}
        />
      </View>
    </Screen>
  );
}

const styles = StyleSheet.create({
  container: {
    alignItems: 'center',
    flex: 1,
    justifyContent: 'center',
  },
});

You can see that in the second example shadow is smoothly animated in both axes, whilst, in the first example, animation is smooth only in y axis. This happens because the width property is missing in one of shadowOffset values and previous implementation didn't support this case.

Before After
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-01-31.at.21.19.22.mp4
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-01-31.at.21.02.29.mp4

@MatiPl01 MatiPl01 self-assigned this Jan 31, 2025
@MatiPl01 MatiPl01 force-pushed the @matipl01/css-transition-crashes-fix branch from b247abf to a37bff4 Compare January 31, 2025 15:30
Comment on lines -55 to -57
const size_t valuesCount = newStyleValue.isObject()
? newStyleValue.asObject(rt).asArray(rt).size(rt)
: oldStyleValue.asObject(rt).asArray(rt).size(rt);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was incorrect as it assumed that both arrays have always the same length. The quite new RN prop boxShadow allows passing any number of shadow objects, so we need to make it possible to interpolate between arrays with different number of elements.

Comment on lines +74 to +80
std::unordered_set<std::string> propertyNamesSet;
const jsi::Object *objects[] = {&oldStyleObject, &newStyleObject};
for (const auto *styleObject : objects) {
const auto propertyNames = styleObject->getPropertyNames(rt);
for (size_t i = 0; i < propertyNames.size(rt); ++i) {
propertyNamesSet.insert(
propertyNames.getValueAtIndex(rt, i).asString(rt).utf8(rt));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new implementation ensures that all props are taken into account. It is a similar change to the arrays interpolator, where we can now support arrays of different lengths.

If a prop from old object is missing in the new one, it will be animated to the fallback value, similarly if there is a prop in the new object but there was no such a prop in the old one.

@MatiPl01 MatiPl01 marked this pull request as ready for review January 31, 2025 20:25
@MatiPl01 MatiPl01 requested a review from piaskowyk January 31, 2025 20:26
@MatiPl01 MatiPl01 changed the title [WIP] fix: Crashes during transitions of some CSS properties fix: Crashes during transitions of some CSS properties Jan 31, 2025
@MatiPl01 MatiPl01 changed the title fix: Crashes during transitions of some CSS properties fix: Crashes during CSS transitions of complex properties Jan 31, 2025
@MatiPl01
Copy link
Member Author

I can also see that during the first transition view flickers. I think it is related to a well known react-naive-screen iOS issue with header measurement but we may want to look closer into it in the future.

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.

1 participant