fix(textinput): backporting of fixes for next 0.69 patch release #36876
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: D42721684 (be69c8b) left a pretty bad bug when using Fabric for Android. I missed that in Fabric specifically, on edit we will cache the Spannable backing the EditText for use in future measurement. Because we've stripped the sizing spans, Spannable measurement has incorrect font size, and the TextInput size will change (collapsing) after the first edit. This effectively breaks any uncontrolled TextInput which does not have explicit dimensions set. Changelog: [Android][Fixed] - Fix measurement of uncontrolled TextInput after edit Reviewed By: sammy-SC Differential Revision: D43158407 fbshipit-source-id: 51602eab06c9a50e2b60ef0ed87bdb4df025e51e
Summary: Pull Request resolved: #36543 This is part of a series of changes to minimize the number of spans committed to EditText, as a mitigation for platform issues on Samsung devices. See this [GitHub thread]( #35936 (comment)) for greater context on the platform behavior. We cache the backing EditText span on text change to later measure. To measure outside of a TextInput we need to restore any spans we removed. Spans may overlap, so base attributes should be behind everything else. The logic here for dealing with precedence is incorrect, and we should instead accomplish this by twiddling with the `SPAN_PRIORITY` bits. Changelog: [Android][Fixed] - Minimize Spans 1/N: Fix precedence Reviewed By: javache Differential Revision: D44240779 fbshipit-source-id: f731b353587888faad946b8cf1e868095cdeced3
…ic (#36546) Summary: Pull Request resolved: #36546 This is part of a series of changes to minimize the number of spans committed to EditText, as a mitigation for platform issues on Samsung devices. See this [GitHub thread]( #35936 (comment)) for greater context on the platform behavior. This change generalizes `stripAttributeEquivalentSpans()` to allow plugging in different spans. Changelog: [Internal] Reviewed By: rshest Differential Revision: D44240781 fbshipit-source-id: 89005266020f216368e9ad9ce382699bd8db85a8 # Conflicts: # ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java
Summary: Pull Request resolved: #36547 This is part of a series of changes to minimize the number of spans committed to EditText, as a mitigation for platform issues on Samsung devices. See this [GitHub thread]( #35936 (comment)) for greater context on the platform behavior. This adds `ReactBackgroundColorSpan` to the list of spans eligible to be stripped. Changelog: [Android][Fixed] - Minimize Spans 3/N: ReactBackgroundColorSpan Reviewed By: javache Differential Revision: D44240782 fbshipit-source-id: 2ded1a1687a41cf6d5f83e89ffadd2d932089969
Summary: Pull Request resolved: #36545 This is part of a series of changes to minimize the number of spans committed to EditText, as a mitigation for platform issues on Samsung devices. See this [GitHub thread]( #35936 (comment)) for greater context on the platform behavior. This adds ReactForegroundColorSpan to the list of spans eligible to be stripped. Changelog: [Android][Fixed] - Minimize Spans 4/N: ReactForegroundColorSpan Reviewed By: javache Differential Revision: D44240780 fbshipit-source-id: d86939cc2d7ed9116a4167026c7d48928fc51757
Summary: Pull Request resolved: #36544 This is part of a series of changes to minimize the number of spans committed to EditText, as a mitigation for platform issues on Samsung devices. See this [GitHub thread]( #35936 (comment)) for greater context on the platform behavior. This change makes us apply strikethrough and underline as paint flags to the underlying EditText, instead of just the spans. We then opt ReactUnderlineSpan and ReactStrikethroughSpan into being strippable. This does actually create visual behavior changes, where child text will inherit any underline or strikethrough of the root EditText (including if the child specifies `textDecorationLine: "none"`. The new behavior is consistent with both iOS and web though, so it seems like more of a bugfix than a regression. Changelog: [Android][Fixed] - Minimize Spans 5/N: Strikethrough and Underline Reviewed By: rshest Differential Revision: D44240778 fbshipit-source-id: d564dfc0121057a5e3b09bb71b8f5662e28be17e
Summary: Pull Request resolved: #36548 This is part of a series of changes to minimize the number of spans committed to EditText, as a mitigation for platform issues on Samsung devices. See this [GitHub thread]( #35936 (comment)) for greater context on the platform behavior. This change lets us set `letterSpacing` on the EditText instead of using our custom span. Changelog: [Android][Fixed] - Minimize EditText Spans 6/N: letterSpacing Reviewed By: rshest Differential Revision: D44240777 fbshipit-source-id: 9bd10c3261257037d8cacf37971011aaa94d1a77
Summary: Pull Request resolved: #36576 This is part of a series of changes to minimize the number of spans committed to EditText, as a mitigation for platform issues on Samsung devices. See this [GitHub thread]( #35936 (comment)) for greater context on the platform behavior. This change addresses some minor CR feedback and removes the temporary list of spans in favor of applying them directly. Changelog: [Internal] Reviewed By: javache Differential Revision: D44295190 fbshipit-source-id: bd784e2c514301d45d0bacd8ee6de5c512fc565c
Summary: Pull Request resolved: #36577 This is part of a series of changes to minimize the number of spans committed to EditText, as a mitigation for platform issues on Samsung devices. See this [GitHub thread]( #35936 (comment)) for greater context on the platform behavior. This change allows us to strip CustomStyleSpan. We already set all but `fontVariant` on the underlying EditText, so we just need to route that through as well. Note that because this span is non-parcelable, it is seemingly not subject to the buggy behavior on Samsung devices of infinitely cloning the spans, but non-parcelable spans have different issues on the devices (they disappear), so moving `fontVariant` to the top-level makes sense here. Changelog: [Android][Fixed] - Minimize EditText Spans 8/N: CustomStyleSpan Reviewed By: javache Differential Revision: D44297384 fbshipit-source-id: ed4c000e961dd456a2a8f4397e27c23a87defb6e
Summary: In D36345402 (56e9aa3) I changed the behaviour for mount items to be skipped if they were just setting zero values. AndroidTextInput is the only component that I'm aware of that has non-zero padding by default, and we account for this when creating the native shadow node. This optimization broken TextInput use-cases that explicitly request zero-padding, since we end up ignoring it. To keep this optimization, explicitly init ReactTextInput's padding to 0, but only in Fabric. `updateState` was the closest thing I could find to a Fabric-only callback, once it's fully rolled out, we can also move this to the constructor. Changelog: [Internal] Reviewed By: JoshuaGross Differential Revision: D36545775 fbshipit-source-id: 07bb96032c69d7e350980b0b975e637b66c307ed
Reviewed By: IanChilds Differential Revision: D37594044 fbshipit-source-id: 0bbcaaed951a212651d3cc0fc3371751ced13852
Summary: ViewManager does a null-check already, and so inside the `updateState` method, we can safely assume this parameter is no longer nullable. Changelog: [Internal] Reviewed By: JoshuaGross Differential Revision: D36545806 fbshipit-source-id: 92eefa518f11c17b91aa3da2e14066d36fa4f4c2
Summary: Support MapBuffer in TextInput State. Changelog: [Internal] Reviewed By: NickGerleman Differential Revision: D38546271 fbshipit-source-id: 93e7fd79d9d8473dd646410f3047fcfafa8516f1
Summary: Pull Request resolved: #36575 This is part of a series of changes to minimize the number of spans committed to EditText, as a mitigation for platform issues on Samsung devices. See this [GitHub thread]( #35936 (comment)) for greater context on the platform behavior. D23670779 addedd a previous mechanism to add spans for measurement caching, like we needed to do as part of this change. It is called in more specific cases (e.g. when there is a text hint but no text), but it edits the live EditText spannable instead of the cache copy, and does not handle nested text at all. We are already adding spans back to the input after this, behind everything else, and can replace it with the code we have been adding. Changelog: [Android][Fixed] - Mimimize EditText Spans 9/9: Remove `addSpansForMeasurement()` Reviewed By: javache Differential Revision: D44298159 fbshipit-source-id: 1af44a39de7550b7e66e45db9ebc3523ae9ff002 # Conflicts: # ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java
facebook-github-bot
added
CLA Signed
kelset
requested review from
NickGerleman,
lunaleaps,
fortmarek and
cipolleschi
|
new one here -> #36929 |
This comment was marked as spam.
This comment was marked as spam.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
This PR takes care of backporting the needed commits for the textinput samsung issue to 0.69 stable.
I had a couple of merge conflicts to address:
At a glance, none of them should impact negatively the 0.69 release branch but it's a few commits... so it'd be better to review deeply.
Let's make sure to NOT squash and merge this when merging, so that we can keep all the separate commit references.
Changelog:
[ANDROID] [FIXED] - backporting of fixes for next 0.69 patch release
Test Plan:
Test in RNTester that the textinput component page all works ok.