-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
base: main
Are you sure you want to change the base?
Changes from all commits
3c87d37
24e223f
2f68811
a37bff4
a8823ec
0417d4e
7c40940
f5db22e
3bc03d9
322b8ce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
} | ||
} | ||
|
||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)); | ||
} | ||
} | ||
|
||
|
@@ -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 |
There was a problem hiding this comment.
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.