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

Merged
merged 10 commits into from
Feb 4, 2025

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
@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.

@MatiPl01 MatiPl01 enabled auto-merge February 4, 2025 15:03
@MatiPl01 MatiPl01 added this pull request to the merge queue Feb 4, 2025
Merged via the queue into main with commit a8ceb7d Feb 4, 2025
20 checks passed
@MatiPl01 MatiPl01 deleted the @matipl01/css-transition-crashes-fix branch February 4, 2025 15:07
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.

2 participants