From 089c9a5c9c9a60b6bbff6dda0c9eefa9d501a092 Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Mon, 18 Jul 2022 18:20:22 -0700 Subject: [PATCH] Fix `AttributedString` comparison logic for TextInput state updates Summary: D37801394 (https://github.com/facebook/react-native/commit/51f49ca9982f24de08f5a5654a5210e547bb5b86) attempted to fix an issue of TextInput values being dropped when an uncontrolled component is restyled, and a defaultValue is present. I had missed quite a bit of functionality, where TextInput may have child Text elements, which the native side flattens into a single AttributedString. `lastNativeValue` includes a lossy version of the flattened string produced from the child fragments, so sending it along with the children led to duplicating of the current input on each edit, and things blow up. With some experimentation, I found that the text-loss behavior only happens on Fabric, and is triggered by a state update rather than my original assumption of the view manager command in the `useLayoutEffect` hook. `AndroidTextInputShadowNode` will compare the current and previous flattened strings, to intentionally allow the native value to drift from the React tree if the React tree hasn't changed. This `AttributedString` comparison includes layout metrics as of D20151505 (https://github.com/facebook/react-native/commit/061f54e89086af1c80e5b0460ec715533f99bdb7) meaning a restyle may cause a state update, and clear the text. I do not have full understanding of the flow of state updates to layout, or the underlying issue that led to the equality check including layout information (since TextMeasurementCache seems to explicitly compare LayoutMetrics). D18894538 (https://github.com/facebook/react-native/commit/254ebab1d2b6fac859ab1ae0c9503328fc99a6d0) used a solution of sending a no-op state update to avoid updating text for placeholders, when the Attributed strings are equal (though as of now this code is never reached, since we return earlier on AttributedString equality). I co-opted this mechanism, to avoid sending text updates if the text content and attributes of the AttributedString has not changed, disregarding any layout information. This is how the comparison worked at the time of the diff. I also updated the fragment hashing function to include layout metrics, since it was added to be part of the equality check, and is itself hashable. Changelog: [Android][Fixed] - Fix `AttributedString` comparison logic for TextInput state updates Reviewed By: sammy-SC Differential Revision: D37902643 fbshipit-source-id: c0f8e3112feb19bd0ee62b37bdadeb237a9f725e --- .../attributedstring/AttributedString.cpp | 19 ++++++++++++ .../attributedstring/AttributedString.h | 14 ++++++++- .../AndroidTextInputShadowNode.cpp | 12 ++++---- .../TextInput/TextInputSharedExamples.js | 29 +++++++++++++++++++ 4 files changed, 67 insertions(+), 7 deletions(-) diff --git a/ReactCommon/react/renderer/attributedstring/AttributedString.cpp b/ReactCommon/react/renderer/attributedstring/AttributedString.cpp index d2139c3a52704d..124d44c6be3623 100644 --- a/ReactCommon/react/renderer/attributedstring/AttributedString.cpp +++ b/ReactCommon/react/renderer/attributedstring/AttributedString.cpp @@ -38,6 +38,11 @@ bool Fragment::operator==(const Fragment &rhs) const { rhs.parentShadowView.layoutMetrics); } +bool Fragment::isContentEqual(const Fragment &rhs) const { + return std::tie(string, textAttributes) == + std::tie(rhs.string, rhs.textAttributes); +} + bool Fragment::operator!=(const Fragment &rhs) const { return !(*this == rhs); } @@ -126,6 +131,20 @@ bool AttributedString::operator!=(const AttributedString &rhs) const { return !(*this == rhs); } +bool AttributedString::isContentEqual(const AttributedString &rhs) const { + if (fragments_.size() != rhs.fragments_.size()) { + return false; + } + + for (auto i = 0; i < fragments_.size(); i++) { + if (!fragments_[i].isContentEqual(rhs.fragments_[i])) { + return false; + } + } + + return true; +} + #pragma mark - DebugStringConvertible #if RN_DEBUG_STRING_CONVERTIBLE diff --git a/ReactCommon/react/renderer/attributedstring/AttributedString.h b/ReactCommon/react/renderer/attributedstring/AttributedString.h index 22e205d002e61a..2f1858c6f11c88 100644 --- a/ReactCommon/react/renderer/attributedstring/AttributedString.h +++ b/ReactCommon/react/renderer/attributedstring/AttributedString.h @@ -46,6 +46,12 @@ class AttributedString : public Sealable, public DebugStringConvertible { */ bool isAttachment() const; + /* + * Returns whether the underlying text and attributes are equal, + * disregarding layout or other information. + */ + bool isContentEqual(const Fragment &rhs) const; + bool operator==(const Fragment &rhs) const; bool operator!=(const Fragment &rhs) const; }; @@ -96,6 +102,8 @@ class AttributedString : public Sealable, public DebugStringConvertible { */ bool compareTextAttributesWithoutFrame(const AttributedString &rhs) const; + bool isContentEqual(const AttributedString &rhs) const; + bool operator==(const AttributedString &rhs) const; bool operator!=(const AttributedString &rhs) const; @@ -118,7 +126,11 @@ struct hash { size_t operator()( const facebook::react::AttributedString::Fragment &fragment) const { return folly::hash::hash_combine( - 0, fragment.string, fragment.textAttributes, fragment.parentShadowView); + 0, + fragment.string, + fragment.textAttributes, + fragment.parentShadowView, + fragment.parentShadowView.layoutMetrics); } }; diff --git a/ReactCommon/react/renderer/components/textinput/androidtextinput/react/renderer/components/androidtextinput/AndroidTextInputShadowNode.cpp b/ReactCommon/react/renderer/components/textinput/androidtextinput/react/renderer/components/androidtextinput/AndroidTextInputShadowNode.cpp index fa497f2e13a6f1..5edf5badde2216 100644 --- a/ReactCommon/react/renderer/components/textinput/androidtextinput/react/renderer/components/androidtextinput/AndroidTextInputShadowNode.cpp +++ b/ReactCommon/react/renderer/components/textinput/androidtextinput/react/renderer/components/androidtextinput/AndroidTextInputShadowNode.cpp @@ -141,17 +141,17 @@ void AndroidTextInputShadowNode::updateStateIfNeeded() { auto defaultTextAttributes = TextAttributes::defaultTextAttributes(); defaultTextAttributes.apply(getConcreteProps().textAttributes); - auto newEventCount = - (state.reactTreeAttributedString == reactTreeAttributedString - ? 0 - : getConcreteProps().mostRecentEventCount); - auto newAttributedString = getMostRecentAttributedString(); - // Even if we're here and updating state, it may be only to update the layout // manager If that is the case, make sure we don't update text: pass in the // current attributedString unchanged, and pass in zero for the "event count" // so no changes are applied There's no way to prevent a state update from // flowing to Java, so we just ensure it's a noop in those cases. + auto newEventCount = + state.reactTreeAttributedString.isContentEqual(reactTreeAttributedString) + ? 0 + : getConcreteProps().mostRecentEventCount; + auto newAttributedString = getMostRecentAttributedString(); + setStateData(AndroidTextInputState{ newEventCount, newAttributedString, diff --git a/packages/rn-tester/js/examples/TextInput/TextInputSharedExamples.js b/packages/rn-tester/js/examples/TextInput/TextInputSharedExamples.js index 44304d8d776577..2e9a4d754dcbef 100644 --- a/packages/rn-tester/js/examples/TextInput/TextInputSharedExamples.js +++ b/packages/rn-tester/js/examples/TextInput/TextInputSharedExamples.js @@ -72,6 +72,14 @@ const styles = StyleSheet.create({ margin: 3, fontSize: 12, }, + focusedUncontrolled: { + margin: -2, + borderWidth: 2, + borderColor: '#0a0a0a', + flex: 1, + fontSize: 13, + padding: 4, + }, }); class WithLabel extends React.Component<$FlowFixMeProps> { @@ -477,6 +485,20 @@ class SelectionExample extends React.Component< } } +function UncontrolledExample() { + const [isFocused, setIsFocused] = React.useState(false); + + return ( + setIsFocused(true)} + onBlur={() => setIsFocused(false)} + /> + ); +} + module.exports = ([ { title: 'Auto-focus', @@ -696,4 +718,11 @@ module.exports = ([ ); }, }, + { + title: 'Uncontrolled component with layout changes', + name: 'uncontrolledComponent', + render: function (): React.Node { + return ; + }, + }, ]: Array);