fix(textinput): backporting of fixes for next 0.69 patch release (V2) #36929
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
Summary: Prototype of View Recycling for View + generic APIs. Changelog: [Added][Android] Adding experimental View Recycling for Fabric on Android. Reviewed By: mdvacca Differential Revision: D36608419 fbshipit-source-id: c469ce2fe12ef9332d3def591118befc4a619870 # Conflicts: # ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java
Summary: See previous diff for details on general approach and benchmarks. Changelog: [Internal] Reviewed By: mdvacca Differential Revision: D36676887 fbshipit-source-id: b177dcd19f1ea687bf7d2d4f2f637d2924723340
Summary: Followups to View Recycling diffs to improve things / clean up things a bit. This also fixes memory warnings which were not hooked up before. Changelog: [Internal] Reviewed By: mdvacca Differential Revision: D36707792 fbshipit-source-id: 410e70bf0eeec5569566138af547e1601394d0e6
Summary: In the constructor we should get the default gravity params (as we did previously) and then never change these; thus, we can also make these fields final. These are used in `initView` during construction and upon recycling to reset vertical and horizontal alignment to their defaults. Changelog: [Internal] Reviewed By: genkikondo Differential Revision: D36885646 fbshipit-source-id: 2f4d0b125b8645a380a08965e08db3ba1f12cae3
Summary: Fix crashes that can occur on older versions of Android due to not-yet-implemented APIs. Changelog: [Internal] Reviewed By: cortinico Differential Revision: D37321713 fbshipit-source-id: a27aaf4b28e19a86f4cb10808162102177b9f306
Summary: setFocusable(int) was added in Android O, and setHyphenationFreqauency was added in M. Changelog: [Internal] Reviewed By: kacieb Differential Revision: D37462117 fbshipit-source-id: e59d2de49dbdcddfdba25def6bb39695c65efe89
Summary: Changelog: [Android][Fixed] Resolved bug with Text components in new arch losing text alignment state. Reviewed By: mdvacca Differential Revision: D34108943 fbshipit-source-id: 3992e9406345be919b5e3595fc1f9e61cf67a699 # Conflicts: # ReactAndroid/src/main/java/com/facebook/react/views/text/TextAttributeProps.java # ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java # Conflicts: # ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java
…alizationEnabled feature flag"
facebook-github-bot
added
CLA Signed
kelset
requested review from
NickGerleman,
lunaleaps,
fortmarek and
cipolleschi
|
We technically do not need 92b8981 if it causes merge conflicts. That commit mostly cleans up some existing code which the new logic duplicates. |
|
Closing in favour of #37000 |
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:
In the first PR (#36876).
Things seemed around ok, but then when CI went red on 0.71 on this commit 9be2959 I realized one more commit was needed, this one: 843d57e and when trying to pick that in this 0.69-stable branch, all hell breaks loose. You can see it in the commit history of this PR: to cleaning pick that one I had to pick a bunch of other commits, but even with that, there's still some issues going on Android side.
So I'm putting this in draft mode while we try to figure out how to backport the samsung fixes to 0.69 more properly.
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.