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
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ namespace reanimated {
using namespace facebook;

using PropertyNames = std::vector<std::string>;
using PropertyValues = std::unique_ptr<jsi::Value>;
using PropertyPath = std::vector<std::string>;
/**
* If nullopt - all style properties can trigger transition
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ jsi::Value CSSTransition::run(
progressProvider_.runProgressProviders(
timestamp,
changedProps.changedPropertyNames,
styleInterpolator_.getReversedPropertyNames(rt, *changedProps.newProps));
styleInterpolator_.getReversedPropertyNames(rt, changedProps.newProps));
styleInterpolator_.updateInterpolatedProperties(
rt, changedProps, progressProvider_.getPropertyProgressProviders());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,21 +52,33 @@ void ArrayPropertiesInterpolator::updateKeyframesFromStyleChange(
jsi::Runtime &rt,
const jsi::Value &oldStyleValue,
const jsi::Value &newStyleValue) {
const size_t valuesCount = newStyleValue.isObject()
? newStyleValue.asObject(rt).asArray(rt).size(rt)
: oldStyleValue.asObject(rt).asArray(rt).size(rt);
Comment on lines -55 to -57
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.

auto getArrayFromStyle = [&rt](const jsi::Value &style) {
if (!style.isObject()) {
return jsi::Array(rt, 0);
}
auto obj = style.asObject(rt);
return obj.isArray(rt) ? obj.asArray(rt) : jsi::Array(rt, 0);
};

const auto oldStyleArray = getArrayFromStyle(oldStyleValue);
const auto newStyleArray = getArrayFromStyle(newStyleValue);

const size_t valuesCount =
std::max(oldStyleArray.size(rt), newStyleArray.size(rt));

resizeInterpolators(valuesCount);

for (size_t i = 0; i < valuesCount; ++i) {
interpolators_[i]->updateKeyframesFromStyleChange(
rt,
oldStyleValue.isObject()
? oldStyleValue.asObject(rt).asArray(rt).getValueAtIndex(rt, i)
: jsi::Value::undefined(),
newStyleValue.isObject()
? newStyleValue.asObject(rt).asArray(rt).getValueAtIndex(rt, i)
: jsi::Value::undefined());
// These index checks ensure that interpolation works between 2 arrays
// with different lengths
const auto oldValue = oldStyleArray.size(rt) > i
? oldStyleArray.getValueAtIndex(rt, i)
: jsi::Value::undefined();
const auto newValue = newStyleArray.size(rt) > i
? newStyleArray.getValueAtIndex(rt, i)
: jsi::Value::undefined();

interpolators_[i]->updateKeyframesFromStyleChange(rt, oldValue, newValue);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include <reanimated/CSS/interpolation/groups/GroupPropertiesInterpolator.h>
#include <reanimated/CSS/util/interpolators.h>

#include <algorithm>
#include <memory>

namespace reanimated {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,20 +53,9 @@ void RecordPropertiesInterpolator::updateKeyframes(
propertyNames.getValueAtIndex(rt, i).asString(rt).utf8(rt);
const jsi::Value &propertyKeyframes = keyframesObject.getProperty(
rt, jsi::PropNameID::forUtf8(rt, propertyName));
auto interpolatorIt = interpolators_.find(propertyName);

if (interpolatorIt == interpolators_.end()) {
const auto newInterpolator = createPropertyInterpolator(
propertyName,
propertyPath_,
factories_,
progressProvider_,
viewStylesRepository_);
interpolatorIt =
interpolators_.emplace(propertyName, newInterpolator).first;
}

interpolatorIt->second->updateKeyframes(rt, propertyKeyframes);
maybeCreateInterpolator(propertyName);
interpolators_.at(propertyName)->updateKeyframes(rt, propertyKeyframes);
}
}

Expand All @@ -76,35 +65,34 @@ void RecordPropertiesInterpolator::updateKeyframesFromStyleChange(
const jsi::Value &newStyleValue) {
// TODO - maybe add a possibility to remove interpolators that are no longer
// used (for now, for simplicity, we only add new ones)
const jsi::Array propertyNames = newStyleValue.isObject()
? newStyleValue.asObject(rt).getPropertyNames(rt)
: oldStyleValue.asObject(rt).getPropertyNames(rt);
const size_t propertiesCount = propertyNames.size(rt);

for (size_t i = 0; i < propertiesCount; ++i) {
const std::string propertyName =
propertyNames.getValueAtIndex(rt, i).asString(rt).utf8(rt);
auto interpolatorIt = interpolators_.find(propertyName);

if (interpolatorIt == interpolators_.end()) {
const auto newInterpolator = createPropertyInterpolator(
propertyName,
propertyPath_,
factories_,
progressProvider_,
viewStylesRepository_);
interpolatorIt =
interpolators_.emplace(propertyName, newInterpolator).first;
const auto oldStyleObject =
oldStyleValue.isObject() ? oldStyleValue.asObject(rt) : jsi::Object(rt);
const auto newStyleObject =
newStyleValue.isObject() ? newStyleValue.asObject(rt) : jsi::Object(rt);

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));
Comment on lines +74 to +80
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.

}
}

interpolatorIt->second->updateKeyframesFromStyleChange(
rt,
oldStyleValue.isObject()
? oldStyleValue.asObject(rt).getProperty(rt, propertyName.c_str())
: jsi::Value::undefined(),
newStyleValue.isObject()
? newStyleValue.asObject(rt).getProperty(rt, propertyName.c_str())
: jsi::Value::undefined());
for (const auto &propertyName : propertyNamesSet) {
maybeCreateInterpolator(propertyName);

auto getValue = [&](const jsi::Object &obj) {
return obj.hasProperty(rt, propertyName.c_str())
? obj.getProperty(rt, jsi::PropNameID::forUtf8(rt, propertyName))
: jsi::Value::undefined();
};

interpolators_.at(propertyName)
->updateKeyframesFromStyleChange(
rt, getValue(oldStyleObject), getValue(newStyleObject));
}
}

Expand All @@ -121,6 +109,19 @@ jsi::Value RecordPropertiesInterpolator::mapInterpolators(
return result;
}

void RecordPropertiesInterpolator::maybeCreateInterpolator(
const std::string &propertyName) {
if (interpolators_.find(propertyName) == interpolators_.end()) {
const auto newInterpolator = createPropertyInterpolator(
propertyName,
propertyPath_,
factories_,
progressProvider_,
viewStylesRepository_);
interpolators_.emplace(propertyName, newInterpolator);
}
}

} // namespace reanimated

#endif // RCT_NEW_ARCH_ENABLED
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include <memory>
#include <string>
#include <unordered_set>

namespace reanimated {

Expand Down Expand Up @@ -34,6 +35,8 @@ class RecordPropertiesInterpolator : public GroupPropertiesInterpolator {
const std::function<jsi::Value(PropertyInterpolator &)> &callback)
const override;

void maybeCreateInterpolator(const std::string &propertyName);

private:
const InterpolatorFactoriesRecord &factories_;
PropertyInterpolatorsRecord interpolators_;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,8 @@ void TransitionStyleInterpolator::updateInterpolatedProperties(
jsi::Runtime &rt,
const ChangedProps &changedProps,
const TransitionPropertyProgressProviders &progressProviders) {
const auto oldPropsObj = changedProps.oldProps->asObject(rt);
const auto newPropsObj = changedProps.newProps->asObject(rt);
const auto oldPropsObj = changedProps.oldProps.asObject(rt);
const auto newPropsObj = changedProps.newProps.asObject(rt);

for (const auto &propertyName : changedProps.changedPropertyNames) {
auto interpolatorIt = interpolators_.find(propertyName);
Expand Down
Loading
Loading