compose: Drop TextInput workaround on clearing contents #5074
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
gnprice
added
a-Android
a-compose/send
|
Looks like the CI failure is from eslint: |
gnprice
force-pushed
the
pr-textinput-drop-workaround
branch
from
747537c to
a02ce99
Compare
|
Whoops, thanks -- fixed. |
This library was a workaround for a bug in React Native's TextInput component: it doesn't tell the keyboard to refresh its state after the app changes the text contents programmatically. The workaround only operates in the case where we're clearing the contents entirely. When we update it in any other way, such as for autocomplete, we're still exposed to this RN bug; that's zulip#5072. And then it turns out that since RN v0.60.0, this workaround has been superseded by one in RN which also operates only in the case of completely clearing the contents: zulip#4239 (comment) So we can happily drop it. I tested this manually on a device which I know is affected by the underlying RN bug, because it reproduces zulip#5072: a Samsung J5 (model SM-J510FN) running Android 6.0.1 and Samsung Keyboard 1.5.46. (From other reports I believe these issues appear on recent Samsung devices too; that's just the one I have on hand.) I sent a message, causing us to clear the input. The keyboard state refreshed as it should: typing a new letter inserted only that letter, not also the last word typed in the previous message. Fixes: zulip#4239
chrisbobbe
force-pushed
the
pr-textinput-drop-workaround
branch
from
a02ce99 to
3e276de
Compare
|
Thanks for all your investigative work around this, and for this PR! LGTM, merged. |
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.
This library was a workaround for a bug in React Native's TextInput
component: it doesn't tell the keyboard to refresh its state after
the app changes the text contents programmatically.
The workaround only operates in the case where we're clearing the
contents entirely. When we update it in any other way, such as for
autocomplete, we're still exposed to this RN bug; that's #5072.
And then it turns out that since RN v0.60.0, this workaround has been
superseded by one in RN which also operates only in the case of
completely clearing the contents:
#4239 (comment)
So we can happily drop it.
I tested this manually on a device which I know is affected by the
underlying RN bug, because it reproduces #5072: a Samsung J5 (model
SM-J510FN) running Android 6.0.1 and Samsung Keyboard 1.5.46. (From
other reports I believe these issues appear on recent Samsung devices
too; that's just the one I have on hand.) I sent a message, causing
us to clear the input. The keyboard state refreshed as it should:
typing a new letter inserted only that letter, not also the last word
typed in the previous message.
Fixes: #4239