Android TextInput: Support allowFontScaling on placeholder
#22992
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
Currently, the `TextInput's` placeholder is always sized as though `allowFontScaling` is `true`. Note that `allowFontScaling` works fine for the content of the `TextInput`. The reason is that we set the font size in two places: in the shadow node and in the Android view. The shadow node logic determines the size of the content and the Android view logic determines the size of the placeholder. The handler for the `allowFontScaling` prop is only present in the shadow node logic. Consequently, the Android view logic always uses the default value of `true` for the `allowFontScaling` prop. The fix is to add logic for handling the `allowFontScaling` prop to the Android view. It would be great if we could handle all text props in one spot instead of duplicating code between the shadow node and the Android view. That would eliminate this whole class of bugs. However, I don't have enough familiarity with the history of this code to know how hard that would be or if it's even possible. Fixes facebook#18827. Test Plan: ---------- Verified that `Text`, the content of a `TextInput`, and the placeholder of a `TextInput` are all rendered the same way for all values of `allowFontScaling` (`undefined`, `true`, `false`). Changelog: ---------- [Android] [Fixed] - TextInput: Support `allowFontScaling` on placeholder Adam Comella Microsoft Corp.
facebook-github-bot
added
the
CLA Signed
react-native-bot
added
🔶Components
Component: TextInput
janicduplessis
approved these changes
Jan 15, 2019
|
@facebook-github-bot shipit |
facebook-github-bot
added
the
Import Started
facebook-github-bot
approved these changes
Jan 15, 2019
There was a problem hiding this comment.
@janicduplessis is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Will re-import, something went wrong yesterday. |
There was a problem hiding this comment.
@cpojer has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
hramos
removed
Import Started
matt-oakes
pushed a commit
to matt-oakes/react-native
that referenced
this pull request
Feb 7, 2019
…k#22992) Summary: Currently, the `TextInput's` placeholder is always sized as though `allowFontScaling` is `true`. Note that `allowFontScaling` works fine for the content of the `TextInput`. The reason is that we set the font size in two places: in the shadow node and in the Android view. The shadow node logic determines the size of the content and the Android view logic determines the size of the placeholder. The handler for the `allowFontScaling` prop is only present in the shadow node logic. Consequently, the Android view logic always uses the default value of `true` for the `allowFontScaling` prop. The fix is to add logic for handling the `allowFontScaling` prop to the Android view. It would be great if we could handle all text props in one spot instead of duplicating code between the shadow node and the Android view. That would eliminate this whole class of bugs. However, I don't have enough familiarity with the history of this code to know how hard that would be or if it's even possible. Fixes facebook#18827. Pull Request resolved: facebook#22992 Differential Revision: D13671400 Pulled By: cpojer fbshipit-source-id: 40bae3cfb0ca6298e91a81c05211538935f5a0e8
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Currently, the
TextInput'splaceholder is always sized as thoughallowFontScalingistrue.Note that
allowFontScalingworks fine for the content of theTextInput. The reason is that we set the font size in two places: in the shadow node and in the Android view. The shadow node logic determines the size of the content and the Android view logic determines the size of the placeholder. The handler for theallowFontScalingprop is only present in the shadow node logic. Consequently, the Android view logic always uses the default value oftruefor theallowFontScalingprop.The fix is to add logic for handling the
allowFontScalingprop to the Android view.It would be great if we could handle all text props in one spot instead of duplicating code between the shadow node and the Android view. That would eliminate this whole class of bugs. However, I don't have enough familiarity with the history of this code to know how hard that would be or if it's even possible.
Fixes #18827.
Test Plan:
Verified that
Text, the content of aTextInput, and the placeholder of aTextInputare all rendered the same way for all values ofallowFontScaling(undefined,true,false).Changelog:
[Android] [Fixed] - TextInput: Support
allowFontScalingon placeholderAdam Comella
Microsoft Corp.