Unset phone and visible-password keyboardType flags on Android #17249
Conversation
facebook-github-bot
added
the
CLA Signed
|
@facebook-github-bot label Android Generated by 🚫 dangerJS |
| @@ -527,6 +527,13 @@ public void setAutoCapitalize(ReactEditText view, int autoCapitalize) { | |||
|
|
|||
| @ReactProp(name = "keyboardType") | |||
| public void setKeyboardType(ReactEditText view, @Nullable String keyboardType) { | |||
| int passwordVisibilityFlag = InputType.TYPE_TEXT_VARIATION_VISIBLE_PASSWORD & | |||
There was a problem hiding this comment.
So this flag is only the visibility bit if I understand correctly? It won't unset the fact that it is a password or not.
There was a problem hiding this comment.
That is correct. We wouldn't want to set/unset general password status in keyboardType, only in secureTextEntry.
| int keyboardTypeFlags = INPUT_TYPE_KEYBOARD_NUMBERED | | ||
| InputType.TYPE_TEXT_VARIATION_EMAIL_ADDRESS | | ||
| InputType.TYPE_CLASS_TEXT | InputType.TYPE_CLASS_PHONE | | ||
| passwordVisibilityFlag; | ||
| int flagsToSet = InputType.TYPE_CLASS_TEXT; | ||
| if (KEYBOARD_TYPE_NUMERIC.equalsIgnoreCase(keyboardType)) { | ||
| flagsToSet = INPUT_TYPE_KEYBOARD_NUMBERED; |
There was a problem hiding this comment.
A bit later in this code we set the TYPE_TEXT_VARIATION_VISIBLE_PASSWORD flag, should we just set passwordVisibilityFlag instead?
There was a problem hiding this comment.
That's a good question. I think it comes down to how the field should behave when keyboardType="visible-password" but secureTextEntry="false". Due to the way Android handles the flags, if we set the flag I called passwordVisibilityFlag without setting the InputType.TYPE_TEXT_VARIATION_PASSWORD flag, it will actually treat it like InputType.TYPE_TEXT_VARIATION_URI.
So three options I see:
- Somehow forbid
keyboardType="visible-password"withsecureTextEntry="false" - Let
keyboardType="visible-password"override thesecureTextEntryprop (equivalent to the current PR code) - Allow the consumer to set the props inconsistently (probably with a warning) and risk an unexpected
InputType.TYPE_TEXT_VARIATION_URI
I should probably leave that decision up to the core maintainers.
There was a problem hiding this comment.
Makes sense, let's keep the current behavior (current PR code).
| int passwordVisibilityFlag = InputType.TYPE_TEXT_VARIATION_VISIBLE_PASSWORD & | ||
| ~InputType.TYPE_TEXT_VARIATION_PASSWORD; | ||
|
|
||
| int keyboardTypeFlags = INPUT_TYPE_KEYBOARD_NUMBERED | |
There was a problem hiding this comment.
Let's move this and passwordVisibilityFlag to private static final members in the class.
There was a problem hiding this comment.
Will do, commit incoming.
|
Thanks for figuring this out, this flag code is pretty hard to understand -_- |
|
Glad I could help! |
|
Pinging @janicduplessis just to make sure my update didn't get lost in the shuffle of the holidays. |
|
Thanks @BrandonWilliamsCS, will merge after holidays! |
|
@janicduplessis I don't mean to be a pest, but is this still slated for merge? |
|
Thanks for reminding me, yes! @facebook-github-bot shipit |
facebook-github-bot
added
the
Import Started
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.
|
This PR seems to not have been merged. I'm facing the same issue on RN 64, which should have been fixed by this PR, and I don't see this code in master. Or did I miss something ? |
|
this issue still happen react-native v.0.63, keyboardType visible-password make my password visible |
Motivation
Attempted fix for issue #17248
Test Plan
Added unit tests that cover the affected function and manually repeated the reproduction steps found in the issue.
Note: I had to apply the attached patch file to actually run the tests because they were not enabled. I didn't include this in the PR because it seemed like a secondary problem with possible ramifications (see comment near patched line) beyond this issue. For example, other, unrelated tests break when that line is uncommented.
textInputTestEnable.patch.txt
If I should apply the patch to this PR or re-enable the tests in some other fashion, please let me know and I can do so.
Related PRs
(none)
Release Notes
[ANDROID] [BUGFIX] [TextInput] - Fix an issue when swapping to and from the
'visible-password'or'phone-pad'keyboard types.