From c44b0be6b8644936382835ad99ce43f090ae0324 Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Wed, 19 Mar 2025 15:38:31 -0700 Subject: [PATCH] Fixes for AttributedString Comparison (#50147) Summary: Noticed a couple bugs here, around a crash from assertion internal to the caching map which hashes on the AttributedString, that may or may not be related. 1. We are missing `baseTextAttributes` for both hashing and equality (which is mostly innocuous, but still wrong) 2. We were not hashing or comparing `textAlignVertical` 3. For equality, we were comparing parent shadow view tag and metrics, but for hashing, we were hashing the whole ShadowView. I think #3 could cause issues, since we could see different hash despite equality, which could break invariants. Changelog: [Internal] Reviewed By: lunaleaps Differential Revision: D71500246 --- .../renderer/attributedstring/AttributedString.cpp | 11 ++--------- .../renderer/attributedstring/AttributedString.h | 6 +++--- .../renderer/attributedstring/TextAttributes.cpp | 10 ++++------ .../react/renderer/attributedstring/TextAttributes.h | 4 ++-- 4 files changed, 11 insertions(+), 20 deletions(-) diff --git a/packages/react-native/ReactCommon/react/renderer/attributedstring/AttributedString.cpp b/packages/react-native/ReactCommon/react/renderer/attributedstring/AttributedString.cpp index f5c0a1c020db9e..9b74d2a76b56c4 100644 --- a/packages/react-native/ReactCommon/react/renderer/attributedstring/AttributedString.cpp +++ b/packages/react-native/ReactCommon/react/renderer/attributedstring/AttributedString.cpp @@ -47,10 +47,6 @@ bool Fragment::isContentEqual(const Fragment& rhs) const { std::tie(rhs.string, rhs.textAttributes); } -bool Fragment::operator!=(const Fragment& rhs) const { - return !(*this == rhs); -} - #pragma mark - AttributedString void AttributedString::appendFragment(Fragment&& fragment) { @@ -113,11 +109,8 @@ bool AttributedString::compareTextAttributesWithoutFrame( } bool AttributedString::operator==(const AttributedString& rhs) const { - return fragments_ == rhs.fragments_; -} - -bool AttributedString::operator!=(const AttributedString& rhs) const { - return !(*this == rhs); + return std::tie(fragments_, baseAttributes_) == + std::tie(rhs.fragments_, rhs.baseAttributes_); } bool AttributedString::isContentEqual(const AttributedString& rhs) const { diff --git a/packages/react-native/ReactCommon/react/renderer/attributedstring/AttributedString.h b/packages/react-native/ReactCommon/react/renderer/attributedstring/AttributedString.h index 7bd57b37c13a8a..7bc0fca2d96af5 100644 --- a/packages/react-native/ReactCommon/react/renderer/attributedstring/AttributedString.h +++ b/packages/react-native/ReactCommon/react/renderer/attributedstring/AttributedString.h @@ -51,7 +51,6 @@ class AttributedString : public Sealable, public DebugStringConvertible { bool isContentEqual(const Fragment& rhs) const; bool operator==(const Fragment& rhs) const; - bool operator!=(const Fragment& rhs) const; }; class Range { @@ -104,7 +103,6 @@ class AttributedString : public Sealable, public DebugStringConvertible { bool isContentEqual(const AttributedString& rhs) const; bool operator==(const AttributedString& rhs) const; - bool operator!=(const AttributedString& rhs) const; #pragma mark - DebugStringConvertible @@ -127,7 +125,7 @@ struct hash { return facebook::react::hash_combine( fragment.string, fragment.textAttributes, - fragment.parentShadowView, + fragment.parentShadowView.tag, fragment.parentShadowView.layoutMetrics); } }; @@ -138,6 +136,8 @@ struct hash { const facebook::react::AttributedString& attributedString) const { auto seed = size_t{0}; + facebook::react::hash_combine( + seed, attributedString.getBaseTextAttributes()); for (const auto& fragment : attributedString.getFragments()) { facebook::react::hash_combine(seed, fragment); } diff --git a/packages/react-native/ReactCommon/react/renderer/attributedstring/TextAttributes.cpp b/packages/react-native/ReactCommon/react/renderer/attributedstring/TextAttributes.cpp index 193b75b71cc9fe..29388136227f53 100644 --- a/packages/react-native/ReactCommon/react/renderer/attributedstring/TextAttributes.cpp +++ b/packages/react-native/ReactCommon/react/renderer/attributedstring/TextAttributes.cpp @@ -146,7 +146,8 @@ bool TextAttributes::operator==(const TextAttributes& rhs) const { layoutDirection, accessibilityRole, role, - textTransform) == + textTransform, + textAlignVertical) == std::tie( rhs.foregroundColor, rhs.backgroundColor, @@ -170,7 +171,8 @@ bool TextAttributes::operator==(const TextAttributes& rhs) const { rhs.layoutDirection, rhs.accessibilityRole, rhs.role, - rhs.textTransform) && + rhs.textTransform, + rhs.textAlignVertical) && floatEquality(maxFontSizeMultiplier, rhs.maxFontSizeMultiplier) && floatEquality(opacity, rhs.opacity) && floatEquality(fontSize, rhs.fontSize) && @@ -180,10 +182,6 @@ bool TextAttributes::operator==(const TextAttributes& rhs) const { floatEquality(textShadowRadius, rhs.textShadowRadius); } -bool TextAttributes::operator!=(const TextAttributes& rhs) const { - return !(*this == rhs); -} - TextAttributes TextAttributes::defaultTextAttributes() { static auto textAttributes = [] { auto textAttributes = TextAttributes{}; diff --git a/packages/react-native/ReactCommon/react/renderer/attributedstring/TextAttributes.h b/packages/react-native/ReactCommon/react/renderer/attributedstring/TextAttributes.h index 55b4de33223fc2..623a747194f15b 100644 --- a/packages/react-native/ReactCommon/react/renderer/attributedstring/TextAttributes.h +++ b/packages/react-native/ReactCommon/react/renderer/attributedstring/TextAttributes.h @@ -95,7 +95,6 @@ class TextAttributes : public DebugStringConvertible { #pragma mark - Operators bool operator==(const TextAttributes& rhs) const; - bool operator!=(const TextAttributes& rhs) const; #pragma mark - DebugStringConvertible @@ -142,7 +141,8 @@ struct hash { textAttributes.isPressable, textAttributes.layoutDirection, textAttributes.accessibilityRole, - textAttributes.role); + textAttributes.role, + textAttributes.textAlignVertical); } }; } // namespace std