Skip to content

Commit 7136be2

Browse files
NickGerlemanfacebook-github-bot
authored andcommitted
Fixes for AttributedString Comparison (facebook#50147)
Summary: Pull Request resolved: facebook#50147 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 fbshipit-source-id: 462749d5ca10d10bf0dab88089253a2bb8e603fb
1 parent 7488f66 commit 7136be2

File tree

4 files changed

+11
-20
lines changed

4 files changed

+11
-20
lines changed

packages/react-native/ReactCommon/react/renderer/attributedstring/AttributedString.cpp

+2-9
Original file line numberDiff line numberDiff line change
@@ -47,10 +47,6 @@ bool Fragment::isContentEqual(const Fragment& rhs) const {
4747
std::tie(rhs.string, rhs.textAttributes);
4848
}
4949

50-
bool Fragment::operator!=(const Fragment& rhs) const {
51-
return !(*this == rhs);
52-
}
53-
5450
#pragma mark - AttributedString
5551

5652
void AttributedString::appendFragment(Fragment&& fragment) {
@@ -113,11 +109,8 @@ bool AttributedString::compareTextAttributesWithoutFrame(
113109
}
114110

115111
bool AttributedString::operator==(const AttributedString& rhs) const {
116-
return fragments_ == rhs.fragments_;
117-
}
118-
119-
bool AttributedString::operator!=(const AttributedString& rhs) const {
120-
return !(*this == rhs);
112+
return std::tie(fragments_, baseAttributes_) ==
113+
std::tie(rhs.fragments_, rhs.baseAttributes_);
121114
}
122115

123116
bool AttributedString::isContentEqual(const AttributedString& rhs) const {

packages/react-native/ReactCommon/react/renderer/attributedstring/AttributedString.h

+3-3
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,6 @@ class AttributedString : public Sealable, public DebugStringConvertible {
5151
bool isContentEqual(const Fragment& rhs) const;
5252

5353
bool operator==(const Fragment& rhs) const;
54-
bool operator!=(const Fragment& rhs) const;
5554
};
5655

5756
class Range {
@@ -104,7 +103,6 @@ class AttributedString : public Sealable, public DebugStringConvertible {
104103
bool isContentEqual(const AttributedString& rhs) const;
105104

106105
bool operator==(const AttributedString& rhs) const;
107-
bool operator!=(const AttributedString& rhs) const;
108106

109107
#pragma mark - DebugStringConvertible
110108

@@ -127,7 +125,7 @@ struct hash<facebook::react::AttributedString::Fragment> {
127125
return facebook::react::hash_combine(
128126
fragment.string,
129127
fragment.textAttributes,
130-
fragment.parentShadowView,
128+
fragment.parentShadowView.tag,
131129
fragment.parentShadowView.layoutMetrics);
132130
}
133131
};
@@ -138,6 +136,8 @@ struct hash<facebook::react::AttributedString> {
138136
const facebook::react::AttributedString& attributedString) const {
139137
auto seed = size_t{0};
140138

139+
facebook::react::hash_combine(
140+
seed, attributedString.getBaseTextAttributes());
141141
for (const auto& fragment : attributedString.getFragments()) {
142142
facebook::react::hash_combine(seed, fragment);
143143
}

packages/react-native/ReactCommon/react/renderer/attributedstring/TextAttributes.cpp

+4-6
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,8 @@ bool TextAttributes::operator==(const TextAttributes& rhs) const {
146146
layoutDirection,
147147
accessibilityRole,
148148
role,
149-
textTransform) ==
149+
textTransform,
150+
textAlignVertical) ==
150151
std::tie(
151152
rhs.foregroundColor,
152153
rhs.backgroundColor,
@@ -170,7 +171,8 @@ bool TextAttributes::operator==(const TextAttributes& rhs) const {
170171
rhs.layoutDirection,
171172
rhs.accessibilityRole,
172173
rhs.role,
173-
rhs.textTransform) &&
174+
rhs.textTransform,
175+
rhs.textAlignVertical) &&
174176
floatEquality(maxFontSizeMultiplier, rhs.maxFontSizeMultiplier) &&
175177
floatEquality(opacity, rhs.opacity) &&
176178
floatEquality(fontSize, rhs.fontSize) &&
@@ -180,10 +182,6 @@ bool TextAttributes::operator==(const TextAttributes& rhs) const {
180182
floatEquality(textShadowRadius, rhs.textShadowRadius);
181183
}
182184

183-
bool TextAttributes::operator!=(const TextAttributes& rhs) const {
184-
return !(*this == rhs);
185-
}
186-
187185
TextAttributes TextAttributes::defaultTextAttributes() {
188186
static auto textAttributes = [] {
189187
auto textAttributes = TextAttributes{};

packages/react-native/ReactCommon/react/renderer/attributedstring/TextAttributes.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,6 @@ class TextAttributes : public DebugStringConvertible {
9595
#pragma mark - Operators
9696

9797
bool operator==(const TextAttributes& rhs) const;
98-
bool operator!=(const TextAttributes& rhs) const;
9998

10099
#pragma mark - DebugStringConvertible
101100

@@ -142,7 +141,8 @@ struct hash<facebook::react::TextAttributes> {
142141
textAttributes.isPressable,
143142
textAttributes.layoutDirection,
144143
textAttributes.accessibilityRole,
145-
textAttributes.role);
144+
textAttributes.role,
145+
textAttributes.textAlignVertical);
146146
}
147147
};
148148
} // namespace std

0 commit comments

Comments
 (0)