[iOS] Fabric: Fixes crash of dynamic color when light/dark mode changed #48496
Conversation
facebook-github-bot
added
CLA Signed
|
Also fixed #48493 |
There was a problem hiding this comment.
Thanks for fixing it! The code looks good, but I think we missed a bit.
I think we have to invalidate the cache uiColorHashValue_ when the uiColor_ changes.
Otherwise we risk to set a color, compute the hash, change the color, but the hash remains the same.
WDYT?
@cipolleschi Thanks for the review, seems we don't have any interface which can change the |
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| @@ -142,6 +148,40 @@ int32_t ColorFromUIColor(const std::shared_ptr<void> &uiColor) | |||
| return static_cast<float>(rgba[channelId]); | |||
| } | |||
|
|
|||
| int32_t Color::getUIColorHash() const | |||
| { | |||
| if (!uiColorHashValue_) { | |||
There was a problem hiding this comment.
Would this work?
UIColor *color = (UIColor *)unwrapManagedObject(uiColor_);
return [color hashValue];
There was a problem hiding this comment.
The point here is: the UIColor does not change when the UITraitCollection changes.
It changes only when we resolve the uicolor against the trait collection.
The uiColor is the actual invariant here, we don't need to compute the hash resolving the uicolor against all the trait collections.
There was a problem hiding this comment.
@javache @cipolleschi I don't know details about how UIColor wether implements hash correctly. But seems the hash value is different even two dynamic colors have the same color in all trait collections.
For example: Two methods like below, [self dynamicColor].hash is not equal to [self dynamicColor1].hash. Actually they are the same color. So it may have potential issues if we use the hash value to check equality?
+ (UIColor *)dynamicColor {
return [UIColor colorWithDynamicProvider:^UIColor * _Nonnull(UITraitCollection * _Nonnull traitCollection) {
if (traitCollection.userInterfaceStyle == UIUserInterfaceStyleDark) {
return [UIColor whiteColor];
} else {
return [UIColor blackColor];
}
}];
}
+ (UIColor *)dynamicColor1 {
return [UIColor colorWithDynamicProvider:^UIColor * _Nonnull(UITraitCollection * _Nonnull traitCollection) {
if (traitCollection.userInterfaceStyle == UIUserInterfaceStyleDark) {
return [UIColor whiteColor];
} else {
return [UIColor blackColor];
}
}];
}
PS. The crash disappeared if we return color.hash.
There was a problem hiding this comment.
So UIColor hashValue is probably just falling back to NSObject, which means the value won't change over the objects lifetime, but also can't be reliably used to compare objects that are equal :(
There was a problem hiding this comment.
My concern is that hashing is supposed to be cheap, and we use it implicitly in a number of different places. This makes the common case of a non-platform color significantly more expensive.
There was a problem hiding this comment.
The hash is computed only once, when needed the first time, and then cached in the uiColorHashValue_. Isn't that enough? The tradeoff is that we recompute the same hash if we have two instances of the same color, but that should be ok, no?
|
Alternative suggestion: let's pre-calculate the hash and store it as an associated object on the UIColor One more issue potentially here (which would lead to cache misses!): |
zhongwuzw
force-pushed
the
features/fix_dynamic_crash
branch
from
42b5cd4 to
8949170
Compare
@javache @cipolleschi I updated the code to pre-calculate the hash in the constructor. Please review that 👀 ! |
...eactCommon/react/renderer/graphics/platform/ios/react/renderer/graphics/HostPlatformColor.mm
Outdated
Show resolved
Hide resolved
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| auto seed = size_t{0}; | ||
| facebook::react::hash_combine(seed, color.getColor()); | ||
| facebook::react::hash_combine(seed, color.getUIColorHash()); | ||
| return seed; |
There was a problem hiding this comment.
@javache No value in calling hash_combine again with seed 0.
| auto seed = size_t{0}; | |
| facebook::react::hash_combine(seed, color.getColor()); | |
| facebook::react::hash_combine(seed, color.getUIColorHash()); | |
| return seed; | |
| return color.getUIColorHash(); |
| auto colorHash = facebook::react::hash_combine(intColor, 0); | ||
| color.reactHash = colorHash; |
There was a problem hiding this comment.
| auto colorHash = facebook::react::hash_combine(intColor, 0); | |
| color.reactHash = colorHash; | |
| color.reactHash = [colorHash](facebook::react::hash_combine(intColor, 0)); |
| auto colorHash = facebook::react::hash_combine(dark, light, highContrastDark, highContrastLight, 0); | ||
| color.reactHash = colorHash; |
There was a problem hiding this comment.
| auto colorHash = facebook::react::hash_combine(dark, light, highContrastDark, highContrastLight, 0); | |
| color.reactHash = colorHash; | |
| color.reactHash = [colorHash;](acebook::react::hash_combine(dark, light, highContrastDark, highContrastLight, 0);) |
| auto colorHash = facebook::react::hash_combine(color, components.colorSpace == ColorSpace::DisplayP3); | ||
| uiColor.reactHash = colorHash; |
There was a problem hiding this comment.
| auto colorHash = facebook::react::hash_combine(color, components.colorSpace == ColorSpace::DisplayP3); | |
| uiColor.reactHash = colorHash; | |
| uiColor.reactHash = facebook::react::hash_combine(color, components.colorSpace == ColorSpace::DisplayP3); |
| @@ -114,14 +195,20 @@ int32_t ColorFromUIColor(const std::shared_ptr<void> &uiColor) | |||
|
|
|||
| Color::Color(std::shared_ptr<void> uiColor) | |||
There was a problem hiding this comment.
When would this still get called? Can we make this constructor private so we can guarantee all UIColor objects are created with the hash in place?
There was a problem hiding this comment.
Seems UndefinedColor and semantic color used it, I mark it private and add a static method createSemanticColor to create semantic color.
|
@javache Hi, please help to review again :) |
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@cipolleschi merged this pull request in cbaff1c. |
|
This pull request was successfully merged by @zhongwuzw in cbaff1c When will my fix make it into a release? | How to file a pick request? |
|
This pull request has been reverted by 04afbd9. |
|
Is there anything I can do because of this commit was reverted? :) |
|
Yeah, you can. Thanks for the offer. A solution would be to remove the two files we added and use two plain C functions instead, |
|
@cipolleschi Thanks for your information. Maybe it's because not added -Objc C flag? I moved the category to |
|
@javache was suggesting to drop the category altogether, in favor of the simpler c functions... HostPlatformColor is already an Objective-C++ file, so even if we keep the category it should be good. Let's wait what he thinks about the category approach. |
|
Let's use a static function here. Categories have overhead in Objective-C and we have various compiler transforms that may cause issues here. |
|
@cipolleschi @javache I store the hash as the member of color and calculate it in the constructor. |
|
Our system is not very happy with me trying to import a PR that result merged internally. I'm talking with our OSS team that manages the GitHub integration to see if we can find a solution. |
|
New PR please see #49265. |
Summary:
The reason is when light/dark mode changed, the
hashvalue also changed because we usedcolor.getColor(). leads to size balanced break.Changelog:
[IOS] [FIXED] - Fabric: Fixes crash of dynamic color when light/dark mode changed
Test Plan:
RNTester -> PlatformColor example -> changed the dark/light mode in the system settings -> go back to App and pop and push the PlatformColor example, it would crash: