Android TextInput: Improve application of styles for value prop#22461
Closed
rigdern wants to merge 1 commit intofacebook:masterfrom
Closed
Android TextInput: Improve application of styles for value prop#22461rigdern wants to merge 1 commit intofacebook:masterfrom
value prop#22461rigdern wants to merge 1 commit intofacebook:masterfrom
Conversation
facebook-github-bot
added
the
CLA Signed
Generated by 🚫 dangerJS |
react-native-bot
added
🔶Components
Component: Text
Component: TextInput
…lue` or `defaultValue` props, React Native didn't apply any of the styles in `buildSpannedFromShadowNode` to the text. This is because `spannedFromShadowNode` appends `value` after calling `buildSpannedFromShadowNode`. Many styles worked because their logic is included in both `buildSpannedFromShadowNode` and `ReactTextInputManager`. However, some only appear in `buildSpannedFromShadowNode` such as `textDecorationLine` (it would be good to understand why we need to duplicate styling logic in `buildSpannedFromShadowNode` & `ReactTextInputManager` and to know whether `ReactTextInputManager` should be handling `textDecorationLine`). Also, this commit improves consistency between iOS and Android if you specify both `value` and children on a `TextInput`. Prior to this, iOS concatenated the strings such that the `value` prop came before the children whereas Android put the children before the `value` prop. Now Android matches iOS's behavior and puts the `value` prop before the children. These appear to be regressions. The `value` prop used to be appended before calling `buildSpannedFromShadowNode` (this behavior appears to have been changed by accident in facebook@80027ce#diff-4f5947f2fe0381c4a6373a30e596b8c3). The fix is to append the `value` prop before calling `buildSpannedFromShadowNode`. Additionally, we have to expose a new `start` parameter on `buildSpannedFromShadowNode` so that we can tell it to include the text from the `value` prop in the range that it styles. Without this, the start of the styled text would be immediately after `value` because `value` is appended before calling `buildSpannedFromShadowNode`. Test Plan: ---------- Used a variety of props to style some `TextInputs` (e.g. `textDecorationLine`, `fontSize`, `letterSpacing`). Verified that the `TextInputs` looked the same regardless of whether the text was provided via the `value` prop, children, or both. Changelog: ---------- [Android] [Fixed] - TextInput: Improve application of styles for `value` prop Adam Comella Microsoft Corp.
rigdern
force-pushed
the
rigdern/text-attributes0
branch
from
141bba9 to
df28436
Compare
Contributor
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Contributor
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Contributor
|
Heads up this will probably conflict with #22670, whichever lands first. |
| buildSpannedFromShadowNode(textShadowNode, sb, ops); | ||
|
|
||
| if (text != null) { | ||
| // Handle text that is provided via a prop (e.g. the `value` and `defaultValue` props on |
Contributor
There was a problem hiding this comment.
Thanks for adding this, took me a while to figure out what this was for when working on fixing textTransform.
Collaborator
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
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.
Prior to this change, when you passed text to
TextInputvia thevalueordefaultValueprops, React Native didn't apply any of the styles inbuildSpannedFromShadowNodeto the text. This is becausespannedFromShadowNodeappendsvalueafter callingbuildSpannedFromShadowNode. Many styles worked because their logic is included in bothbuildSpannedFromShadowNodeandReactTextInputManager. However, some only appear inbuildSpannedFromShadowNodesuch astextDecorationLine(it would be good to understand why we need to duplicate styling logic inbuildSpannedFromShadowNode&ReactTextInputManagerand to know whetherReactTextInputManagershould be handlingtextDecorationLine).Also, this commit improves consistency between iOS and Android if you specify both
valueand children on aTextInput. Prior to this, iOS concatenated the strings such that thevalueprop came before the children whereas Android put the children before thevalueprop. Now Android matches iOS's behavior and puts thevalueprop before the children.These appear to be regressions. The
valueprop used to be appended before callingbuildSpannedFromShadowNode(this behavior appears to have been changed by accident in 80027ce#diff-4f5947f2fe0381c4a6373a30e596b8c3).The fix is to append the
valueprop before callingbuildSpannedFromShadowNode. Additionally, we have to expose a newstartparameter onbuildSpannedFromShadowNodeso that we can tell it to include the text from thevalueprop in the range that it styles. Without this, the start of the styled text would be immediately aftervaluebecausevalueis appended before callingbuildSpannedFromShadowNodeTest Plan:
Used a variety of props to style some
TextInputs(e.g.textDecorationLine,fontSize,letterSpacing). Verified that theTextInputslooked the same regardless of whether the text was provided via thevalueprop, children, or both.Changelog:
[Android] [Fixed] - TextInput: Improve application of styles for
valuepropAdam Comella
Microsoft Corp.