Remove `react-native-text-input-reset` dependency, if possible #4239

chrisbobbe · 2020-08-26T07:43:47Z

I see this deprecation warning when running ./gradlew help --warning-mode=all from android (recommended by this doc):

> Configure project :react-native-text-input-reset
WARNING: Configuration 'compile' is obsolete and has been replaced with 'implementation' and 'api'.
It will be removed soon. For more information see: http://d.android.com/r/tools/update-dependency-configurations.html

Its GitHub page says it was only meant to be temporary, so I'm not confident that this deprecation warning will ever be addressed. But from a quick skim of the RN issue it was aimed to be a workaround for, facebook/react-native#12462, it's not immediately clear that that issue has been fixed in any version of React Native.

A complete fix must include:

this dependency is removed
we're confident that Android bugfixes: TextInput issues facebook/react-native#12462 doesn't affect us

The text was updated successfully, but these errors were encountered:

aryanshridhar · 2020-08-26T13:56:33Z

Taking this issue :)

aryanshridhar · 2020-08-26T18:36:58Z

Hey , I see no difference while removing TextInputReset from here as the behaviour of ComposeBox still remains the same (contents of textinput being properly cleared)

Should I go on with it , and open up an PR removing this package ?

chrisbobbe · 2020-08-26T19:20:11Z

Thanks for looking into this, and it sounds promising that it's working on your device! It would be great if you could read through facebook/react-native#12462 and confirm that we can be reasonably sure that it won't affect other people's Android devices, too. I'm not very familiar with the details, but there seemed to be some nuances that suggest some people might see the bug, while others don't.

edit: In fact, looking back at facebook/react-native#12462, it looks like it describes a change in React Native that wasn't made, with some discussion about why. The GitHub page for react-native-text-input-reset also links to a Google bug tracker, so maybe there's something wrong with an Android release that may (or may not) have been fixed?

aryanshridhar · 2020-08-27T05:31:31Z

As rightly said by you , looks like the same problem still persists in many android device and hasn't been merged due to lack of verification of the same .
However this issue can still be solved using native module method by integrating this code to zulip-mobile .
Currently looking into it and trying it out .

Edit : Hey , I have linked a pr to it now by integrating the above mentioned method

Part of the RN v0.61 -> v0.62 changes to the template app [1], corresponding to several commits, listed below. Probably best to do this before the main upgrade commit. The "Fixed issues" and the "Known issues" sections of the release notes (linked for each upgrade below) are particularly informative for the patch-version upgrades, where, otherwise, the release notes seem identical to those of the corresponding minor version. ----- v5.6 (facebook/react-native@be2a2529a) ----- Release notes from Gradle upstream: https://docs.gradle.org/5.6/release-notes.html Upgrade guide: https://docs.gradle.org/5.6/userguide/upgrading_version_5.html#changes_5.6 Before doing this upgrade, I ran `gradle help --scan` (actually, `./gradlew help --scan`) to check for deprecations that might turn into failures after the upgrade, as recommended by the upgrade guide. The report (https://scans.gradle.com/s/2pgdvrpgnpyms [2]) didn't have a "Deprecations" item in the left nav, as shown in the guide's screenshot, but it did show some deprecation warnings in the console. (A similar output was seen from the alternative command they suggest, `gradle help --warning-mode all`; I ran it with `./gradlew`.) We see this in the console for three of our dependencies: ``` WARNING: Configuration 'compile' is obsolete and has been replaced with 'implementation' and 'api'. It will be removed soon. For more information see: http://d.android.com/r/tools/update-dependency-configurations.html ``` The three dependencies are (with our issues to upgrade/replace, two of them filed just now): - `react-native-photo-view` zulip#4217 - `react-native-text-input-reset` zulip#4239 - `react-native-device-info` zulip#4240 The upgrade guide lists a few new deprecations; we may get warnings after the upgrade if they apply to us, but I suspect they may be given in the "Deprecations" section mentioned above, which we don't have, for whatever reason. It also lists several potential breaking changes, and I haven't understood them well enough to find if they apply to us. We hope to catch any that do in `tools/test android --full`. Unfortunately, we did run into a known issue with 5.6 (gradle/gradle#10347) that halted progress verifying the upgrade. React Native saw it too; facebook/react-native#26227 was filed to upgrade to 5.6.1 in response. That was closed and superseded by facebook/react-native#26349 (landed in facebook/react-native@b1c954b1f), done here and discussed below. ----- v5.6.2 (facebook/react-native@b1c954b1f) ----- Release notes from Gradle upstream: https://docs.gradle.org/5.6.1/release-notes.html https://docs.gradle.org/5.6.2/release-notes.html Upgrade guides: https://docs.gradle.org/5.6.1/userguide/upgrading_version_5.html#changes_5.6.1 https://docs.gradle.org/5.6.2/userguide/upgrading_version_5.html#changes_5.6.2 The release notes and upgrade guides appear close to identical to those for v5.6. One thing we could and did do here, with gradle/gradle#10347 fixed, is proceed with verifying the v5.6 upgrade, together with the changes from v5.6 to v5.6.2. The build scan report at v5.6.2 (https://scans.gradle.com/s/peks3ratsf7ee) was very similar to the one we ran before the v5.6 upgrade; those three deprecation warnings didn't turn into errors. `tools/test --full android` succeeded right away. ----- v5.6.3 (facebook/react-native@ff6b2ff32) ----- Release notes from Gradle upstream: https://docs.gradle.org/5.6.3/release-notes.html Upgrade guides: https://docs.gradle.org/5.6.3/userguide/upgrading_version_5.html#changes_5.6.3 All near-identical to the previous docs. The build scan report at v5.6.3 (https://gradle.com/s/3holiaylf6jku) also looks very similar to those at past versions. At first, I got a build failure that I didn't get with the previous upgrades: ``` java.nio.file.NoSuchFileException: /Users/chrisbobbe/dev/zulip-mobile/android/app/build/intermediates/external_file_lib_dex_archives/debug/out at sun.nio.fs.UnixException.translateToIOException(UnixException.java:86) at sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:102) [...] ``` It went away after deleting `android/.gradle` and running `./gradlew clean` (from SO: https://stackoverflow.com/a/62025502). ----- v5.6.4 (facebook/react-native@928f4434b) ----- Release notes from Gradle upstream: https://docs.gradle.org/5.6.4/release-notes.html Upgrade guides: https://docs.gradle.org/5.6.4/userguide/upgrading_version_5.html#changes_5.6.4 The build scan report at v5.6.4 (https://gradle.com/s/2szkm2hvdrejy) also looks very similar to those at past versions. ----- v6.0.1 (facebook/react-native@701e66bde) ----- Release notes from Gradle upstream: https://docs.gradle.org/6.0.1/release-notes.html Upgrade guides: https://docs.gradle.org/6.0.1/userguide/upgrading_version_5.html#changes_6.0.1 The upgrade guide lists many new deprecations, and we are newly seeing some deprecation warnings in the build scan (see below). It also lists several potential breaking changes, and I haven't understood them well enough to find if they apply to us. We hope to catch any that do in `tools/test android --full`. This time, the build scan report for v6.0.1 (https://scans.gradle.com/s/l7q26kntclub6) does have a "Deprecations" section. Its contents: """ BuildListener#buildStarted(Gradle) has been deprecated. This is scheduled to be removed in Gradle 7.0. 32 usages The maven plugin has been deprecated. This is scheduled to be removed in Gradle 7.0. Please use the maven-publish plugin instead. 19 usages """ Expanding the 32 usages in the first of these, all but one are in the `com.android.library` plugin; one is in the `android` plugin. Expanding the 19 usages in the second of these, they're all in the `org.gradle.maven` plugin. I encountered a build failure ("No file known for: classes.dex") but was able to fix it by clearing `android.gradle` and running `./gradlew clean`. [1] https://react-native-community.github.io/upgrade-helper/?from=0.61.5&to=0.62.2 [2] The build scan at this link will be "available indefinitely", according to https://scans.gradle.com/.

Part of the RN v0.61 -> v0.62 changes to the template app [2], corresponding to the following commits: - facebook/react-native@be2a2529a - facebook/react-native@b1c954b1f - facebook/react-native@ff6b2ff32 - facebook/react-native@928f4434b - facebook/react-native@701e66bde Probably best to do this before the main upgrade commit. We treat this as an upgrade from 5.5.x to the latest 5.6.x (which is the latest 5.x), and then to 6.0.1 (the latest 6.0.x). [1] - facebook/react-native@be2a2529a also added some comments in gradle.properties, which we reproduce. - We ran into a couple of different build failures, so, add those to the troubleshooting doc with the cache-clearing commands that worked to resolve them (from SO [3]). Release notes from Gradle upstream: https://docs.gradle.org/5.6.4/release-notes.html https://docs.gradle.org/6.0.1/release-notes.html Upgrade guides: https://docs.gradle.org/5.6.4/userguide/upgrading_version_5.html#changes_5.6.4 https://docs.gradle.org/6.0.1/userguide/upgrading_version_5.html#changes_6.0.1 We didn't spot any deprecations or announced breaking changes as applying to us, though it's a bit hard to tell (in particular, we use plenty of Gradle code that isn't ours, so we wouldn't necessarily spot something). We hope we'd catch any by just trying the build, e.g., with `tools/test android --full`. The upgrade guides recommend a "build scan", which we did by running `./gradlew help --scan`. In particular, the upgrade guides ask that you find a "Deprecations" section in the left nav of a web page showing the result of the scan. The build scan report at v5.6.4 [4] doesn't have a "Deprecations" section. This might mean that the feature was missing at that version, or it might mean that it looked for deprecations expressed in a certain way [5] and didn't find any. The report at v6.0.1 [6] did have a "Deprecations" section, with the following two warnings: """ BuildListener#buildStarted(Gradle) has been deprecated. This is scheduled to be removed in Gradle 7.0. 32 usages The maven plugin has been deprecated. This is scheduled to be removed in Gradle 7.0. Please use the maven-publish plugin instead. 19 usages """ Expanding the 32 usages in the first of these, all but one are in the `com.android.library` plugin; one is in the `android` plugin. Expanding the 19 usages in the second of these, they're all in the `org.gradle.maven` plugin. In the console output from the build-scan command at both versions (shown in the build scan reports) we see some deprecation warnings from the Android Gradle Plugin, rather than Gradle itself. They apply to three of our dependencies, and we have issues to replace those dependencies (two of the issues filed just now): - `react-native-photo-view`; see zulip#4217 - `react-native-text-input-reset`; see zulip#4239 - `react-native-device-info`; see zulip#4240 [1] See discussion around zulip#4244 (comment) for some nuances about this. [2] https://react-native-community.github.io/upgrade-helper/?from=0.61.5&to=0.62.2 [3] https://stackoverflow.com/a/62025502 [4] https://gradle.com/s/2szkm2hvdrejy. The build scan at this link will be "available indefinitely", according to https://scans.gradle.com/. [5] zulip#4244 (comment) [6] https://scans.gradle.com/s/l7q26kntclub6

yash-rajpal · 2020-12-01T20:52:37Z

Is this issue still available? If it is I would like to work upon it.
I understand the issue and also looked at the native required to clear the text input and restart the keyboard. Should I work on this?

gnprice · 2021-10-23T00:36:03Z

This turned out not to be necessary: the library remains maintained after all, and the maintainer promptly merged last year a PR @chrisbobbe sent to fix this warning:
nikolaiwarner/react-native-text-input-reset#14

We could still cut the library out if we learn that the issue it's meant to work around no longer applies, but that'd take some work to investigate, and there's no longer an obvious motivation to do so.

gnprice · 2021-10-23T01:28:43Z

Well, perhaps I spoke too soon -- @chrisbobbe points out to me that while the library's maintainer did merge that PR, they haven't actually done a release since then: the latest is still (per https://www.npmjs.com/package/react-native-text-input-reset) 1.0.4, from 2019.

gnprice · 2021-10-23T02:00:35Z

We added our use of this library in #1889, in 2018-02. It was meant to fix #1835, reported a few weeks earlier:

Sometimes the compose box is not fully cleared

This seems to be Android-specific issue.
When sending a message, focusing on the compose box sometimes preserves the last part of the message.

This also seems to be Samsung-specific.

No further details on what sort of device this was happening on.

The RN PR thread that the library originates from is from 2017-02, and says:

Keyboard app on some devices does not detect programmatical change of text value in EditText view. As a result, the text value set by program and user input could be mixed in unpredictable way. The issue was seen on LG G3/G4 phones with Android v5/v6.

And in 2018-07, someone reported that this workaround was effective on a Samsung J5 device.

On the other side, the main risk of this workaround is that what it does is restart the input method. Which… well, hmm, actually seems to be documented in Android upstream as exactly what you should do:

You should call this when the text within your view changes outside of the normal input method or key input flow, such as when an application calls TextView.setText().

And there's no call to that method in the RN tree; the only mention of restartInput in our whole node_modules/ is the one in this library.

So maybe this isn't really even a workaround (to any Android or keyboard issue) after all; or rather, the library is a workaround for RN's TextInput not already doing the exact same thing as the library does. And the only reason RN gets away with not doing this is, perhaps, that some Android keyboards have their own workarounds for apps neglecting to do this.

Oho. But RN does have this other workaround that appears to be for the same issue:

    // On some devices, when the text is cleared, buggy keyboards will not clear the composing
    // text so, we have to set text to null, which will clear the currently composing text.
    if (reactTextUpdate.getText().length() == 0) {
      setText(null);
    } else {

That was added in facebook/react-native#18859 , merged 2019-02, released in v0.60.

In particular, just like this react-native-text-input-reset library, that workaround operates only when completely emptying the input contents. That doesn't cover all the cases that restartInput call says to cover… but it's the case for which each of these issues were reported.

I think that means that we can just drop this workaround, and our use of this library.

When doing so, it would be nice to test on a device as close to the ones where the issue was originally reported as possible. An older Samsung J-series device or LG G-series device would be good.

gnprice · 2021-10-23T02:15:40Z

Oho. But RN does have this other workaround that appears to be for the same issue: […] that workaround operates only when completely emptying the input contents. That doesn't cover all the cases that restartInput doc says to cover… but it's the case for which each of these issues were reported.

Well, and following links from the RN thread, here's a report just last year (2020-09) of what looks like the same issue appearing in an RN app in a case that wasn't about clearing the whole contents:
status-im/status-mobile#11216

So it really seems like RN should do the thing the Android docs say to do, and call restartInput whenever it's updating the text.

But this doesn't affect us, because our TextInput is uncontrolled -- RN isn't updating the text in the React way. The only time it gets updated from the app's side (as opposed to by the input method) is when we clear it. (Which is also the only case this library would cover anyway.) And with that workaround that RN did add, in v0.60, that's covered.

So we still don't need this library, since RN v0.60, and indeed nor does anybody else.

gnprice · 2021-10-23T04:39:04Z

RN isn't updating the text in the React way. The only time it gets updated from the app's side (as opposed to by the input method) is when we clear it.

On the way home, I realized this is too glib. There are several other occasions where we update the contents of the compose-message text input from the app side:

On autocomplete/typeahead.
On attaching a file, or set of files; actually several times, first to insert the placeholders and then once when each file upload completes (or fails).
On inserting a video-call link.

These can be found as the call sites of setMessageInputValue in ComposeBox.js.

So, those should all be exposed to this RN bug (the fact that it's not calling restartInput, which the Android docs say you should do).

I can't find any reports we've had of the issue, though. The closest is #4939, which is about the TextInput's behavior when we modify the text, but isn't this issue.

… Aha! And indeed the issue reproduces for me when I go try it on a Samsung device. I'll file an issue. [→ #5072]

Regardless of that: we should remove this library, because the one case where it does anything is a case that RN already works around since v0.60, namely when you're clearing the text entirely.

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.

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 added the dependencies Pull requests that update a dependency file label Aug 26, 2020

chrisbobbe assigned aryanshridhar Aug 26, 2020

chrisbobbe added the a-Android label Aug 26, 2020

aryanshridhar mentioned this issue Aug 27, 2020

Removed react-native-text-input-reset dependency #4243

Closed

gnprice mentioned this issue Dec 8, 2020

narrow: Add getNarrowsForMessage. #4317

Merged

gnprice closed this as completed Oct 23, 2021

gnprice reopened this Oct 23, 2021

gnprice mentioned this issue Oct 23, 2021

Keyboard state is wrong after picking @-mention or stream from autocomplete #5072

Open

gnprice mentioned this issue Oct 25, 2021

compose: Drop TextInput workaround on clearing contents #5074

Merged

gnprice unassigned aryanshridhar Oct 25, 2021

gnprice self-assigned this Oct 25, 2021

chrisbobbe closed this as completed in 3e276de Oct 26, 2021

chrisbobbe closed this as completed in #5074 Oct 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `react-native-text-input-reset` dependency, if possible #4239

Remove `react-native-text-input-reset` dependency, if possible #4239

chrisbobbe commented Aug 26, 2020 •

edited

aryanshridhar commented Aug 26, 2020

aryanshridhar commented Aug 26, 2020

chrisbobbe commented Aug 26, 2020 •

edited

aryanshridhar commented Aug 27, 2020 •

edited

yash-rajpal commented Dec 1, 2020

gnprice commented Oct 23, 2021

gnprice commented Oct 23, 2021

gnprice commented Oct 23, 2021

gnprice commented Oct 23, 2021

gnprice commented Oct 23, 2021 •

edited

Remove react-native-text-input-reset dependency, if possible #4239

Remove react-native-text-input-reset dependency, if possible #4239

Comments

chrisbobbe commented Aug 26, 2020 • edited

aryanshridhar commented Aug 26, 2020

aryanshridhar commented Aug 26, 2020

chrisbobbe commented Aug 26, 2020 • edited

aryanshridhar commented Aug 27, 2020 • edited

yash-rajpal commented Dec 1, 2020

gnprice commented Oct 23, 2021

gnprice commented Oct 23, 2021

gnprice commented Oct 23, 2021

gnprice commented Oct 23, 2021

gnprice commented Oct 23, 2021 • edited

Remove `react-native-text-input-reset` dependency, if possible #4239

Remove `react-native-text-input-reset` dependency, if possible #4239

chrisbobbe commented Aug 26, 2020 •

edited

chrisbobbe commented Aug 26, 2020 •

edited

aryanshridhar commented Aug 27, 2020 •

edited

gnprice commented Oct 23, 2021 •

edited