-
Notifications
You must be signed in to change notification settings - Fork 24.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TextInput becomes slow after writing and clearing #19126
Comments
This is a really terrible bug. As we didn't know how to fix it (besides from downgrading RN) we switched our inputs to be uncontrolled and that stopped the slowdown. However, on iOS we needed to apply this fix to get the text input to clear in cases such as chat: #18278. |
I concur, I'm experiencing it exactly like this too. I tested on Android emulator as well as on device (Oneplus 5T). The problem remains even after switching DEV mode off. Moreover, the system seems to stay laggy until you unmount the component. I'm using a controlled component. What could be the problem here? |
Based on the GIF I would say it's an issue with the TextInput JS code. I currently had to downgrade to RN 54. But this is temporary. Right now this is blocking issue. |
I tried spinning up a fresh This seems to only be an issue with controlled Seems like a very fundamental issue as |
Same issue here on MacOS X and RN 0.55.0 - android. I tested textinput in RN 0.54.1 and it works as expected. |
I'm having a kind of similar issue regarding the TextInput but using placeholders. After navigating to page with multiple TextInputs, it takes a fraction of a second for the TextInput to show the placeholder, and sometimes it won't show the placeholder at all until I would click on the TextInput to start typing. Using |
On some Androids we are also having issues with the "control" - #19085 |
This reverts 5898817 "Implement letterSpacing on Android >= 5.0". Testing shows that that commit is the cause of facebook#19126, where in a controlled TextInput after some text is first added, then deleted, further interaction with the TextInput becomes extremely slow. Fixes facebook#19126.
I bisected the changes between v0.54 and v0.55, and found the commit that causes this issue: it's 5898817 "Implement letterSpacing on Android >= 5.0". And indeed reverting that change fixes the issue. I just sent #19645 with a revert of that change. The test app I used to repro the issue follows @simone201 's report, with @miguelespinoza 's addition. I
import React, { Component } from 'react';
import { TextInput, View } from 'react-native';
type Props = {};
export default class App extends Component<Props> {
state = {
value: '',
};
handleTextChange = (value) => {
this.setState({ value });
};
render() {
return (
<View>
<TextInput
value={this.state.value}
onChangeText={this.handleTextChange}
/>
</View>
);
}
} Pending #19645 , others who are hitting this issue may be able to fix it by applying that patch to the RN version they use. That's what I expect to do. One other inference from identifying the commit that caused it: that commit was intended to have an effect mainly on Android 5.0 Lollipop and up. It's likely that this issue only happens there. (My testing was on Android 8.1 Oreo.) Thanks to @simone201 and @miguelespinoza for their detailed descriptions of the issue! These were very helpful in pinning down in the first place the mysterious issue we were seeing in our app, and in the bisection I just did to locate the cause within RN. |
This reverts 5898817 "Implement letterSpacing on Android >= 5.0". Testing shows that that commit is the cause of facebook#19126, where in a controlled TextInput after some text is first added, then deleted, further interaction with the TextInput becomes extremely slow. Fixes facebook#19126.
Oh, I should add -- here's a version of that patch on top of the latest release (v0.55.4): (The version in my PR is rebased on top of master, which involved resolving a few merge conflicts from unrelated changes.) |
@gnprice you rock, thank you very much! |
This issue is affecting the newest Mattermost Android release as well. Makes it unusable after having the app open for a few seconds. Any ETA on a possible fix date? |
This reverts 5898817 "Implement letterSpacing on Android >= 5.0". Testing shows that that commit is the cause of facebook#19126, where in a controlled TextInput after some text is first added, then deleted, further interaction with the TextInput becomes extremely slow. Fixes facebook#19126.
Because NaN is special, the `!=` version of this condition will always be true -- even if `mLetterSpacing` is also `Float.NaN`, which is its default value. It should instead be `!Float.isNaN(...)`. The effect of the broken condition is that every text shadow node will create a `CustomLetterSpacingSpan` around its contents, even when the `letterSpacing` prop was never set. Empirically, that in turn causes facebook#19126: in a controlled TextInput after some text is first added, then deleted, further interaction with the TextInput becomes extremely slow. Perhaps we're somehow ending up with large numbers of these shadow nodes (that sounds like a bug in itself, but I guess it's normally low-impact); then this code would have caused them each to generate more work to do. In any case, fixing this condition causes that issue to disappear. The affected logic was introduced between v0.54 and v0.55, in facebook#17398 aka 5898817 "Implement letterSpacing on Android >= 5.0". Fixes facebook#19126.
Because NaN is special, the `!=` version of this condition will always be true -- even if `mLetterSpacing` is also `Float.NaN`, which is its default value. It should instead be `!Float.isNaN(...)`. The effect of the broken condition is that every text shadow node will create a `CustomLetterSpacingSpan` around its contents, even when the `letterSpacing` prop was never set. Empirically, that in turn causes facebook#19126: in a controlled TextInput after some text is first added, then deleted, further interaction with the TextInput becomes extremely slow. Perhaps we're somehow ending up with large numbers of these shadow nodes (that sounds like a bug in itself, but I guess it's normally low-impact); then this code would have caused them each to generate more work to do. In any case, fixing this condition causes that issue to disappear. The affected logic was introduced between v0.54 and v0.55, in facebook#17398 aka 5898817 "Implement letterSpacing on Android >= 5.0". Fixes facebook#19126.
After some further debugging today (details on #19645), the fix is reduced to a very simple one! Just this line:
Hopefully that'll make it into master soon and get cherry-picked into 0.56. |
@gnprice can this be cherrypicked/backported to 0.55.x? 🙏
|
Summary: This reverts 5898817 "Implement letterSpacing on Android >= 5.0". Testing shows that that commit is the cause of facebook#19126, where in a controlled TextInput after some text is first added, then deleted, further interaction with the TextInput becomes extremely slow. Fixes facebook#19126. Tried the repro case from facebook#19126 without this change, then with it. The issue reproduces, then doesn't. Closes facebook#19645 Differential Revision: D8675230 Pulled By: hramos fbshipit-source-id: e2c2d352ee781898721b2dff4738572d1a6b7471
@gianpaj the final |
@gianpaj Looks like it's slated for a cherry-pick into 0.56: For 0.55.x, there's a discussion over here on whether it makes sense for the RN maintainers to publish further releases in that series: |
Patch react-native to pick up the fix for facebook/react-native#19126 from facebook/react-native@5017b86 Will revert this patch once the fix lands in the Expo react-native fork.
Summary: This reverts 5898817 "Implement letterSpacing on Android >= 5.0". Testing shows that that commit is the cause of #19126, where in a controlled TextInput after some text is first added, then deleted, further interaction with the TextInput becomes extremely slow. Fixes #19126. Tried the repro case from #19126 without this change, then with it. The issue reproduces, then doesn't. Closes #19645 Differential Revision: D8675230 Pulled By: hramos fbshipit-source-id: e2c2d352ee781898721b2dff4738572d1a6b7471
Because NaN is special, the `!=` version of this condition will always be true -- even if `mLetterSpacing` is also `Float.NaN`, which is its default value. It should instead be `!Float.isNaN(...)`. The effect of the broken condition is that every text shadow node will create a `CustomLetterSpacingSpan` around its contents, even when the `letterSpacing` prop was never set. Empirically, that in turn causes facebook#19126: in a controlled TextInput after some text is first added, then deleted, further interaction with the TextInput becomes extremely slow. Perhaps we're somehow ending up with large numbers of these shadow nodes (that sounds like a bug in itself, but I guess it's normally low-impact); then this code would have caused them each to generate more work to do. In any case, fixing this condition causes that issue to disappear. The affected logic was introduced between v0.54 and v0.55, in facebook#17398 aka 5898817 "Implement letterSpacing on Android >= 5.0". Fixes facebook#19126.
Because NaN is special, the `!=` version of this condition will always be true -- even if `mLetterSpacing` is also `Float.NaN`, which is its default value. It should instead be `!Float.isNaN(...)`. The effect of the broken condition is that every text shadow node will create a `CustomLetterSpacingSpan` around its contents, even when the `letterSpacing` prop was never set. Empirically, that in turn causes facebook#19126: in a controlled TextInput after some text is first added, then deleted, further interaction with the TextInput becomes extremely slow. Perhaps we're somehow ending up with large numbers of these shadow nodes (that sounds like a bug in itself, but I guess it's normally low-impact); then this code would have caused them each to generate more work to do. In any case, fixing this condition causes that issue to disappear. The affected logic was introduced between v0.54 and v0.55, in facebook#17398 aka 5898817 "Implement letterSpacing on Android >= 5.0". Fixes facebook#19126.
@gnprice I'm still able to reproduce the issue on 0.56.0 as well as when I take 0.55.4 and apply your patch. https://github.com/s-nel/rn19126 |
@s-nel yeah same here |
@s-nel Huh, that is strange. I can't quite get your repro app to build -- I get this error:
That's surely something irrelevant, but meanwhile -- do you reproduce with my test app? It's at https://github.com/gnprice/rn-textinput-test . It uses v0.55.4 with my patch (the one-line version, just fixing the NaN-usage bug) applied. |
Hm that's strange. The repro app is using the default config from Yes I can reproduce from your repository after typing for a couple minutes straight. I don't have to clear anything as the original bug description indicates. Here you can see my JS framerate drop to single digits from adding a few characters |
I upgraded my app to RN 0.56 (after many issues) but this seems to have been fixed. I tested on the emulator and 2 real devices (nexus 5X and moto g first gen) |
Made total upgrade app to 0.56. Having trouble with many 3rd party packages, but it doesn't matter because the controlled Text component still has the huge slow down after some text deletions. |
Ive checked (my local file - node_modules/react-native....ReactBaseTextShadowNode.java) that changes from many PR (seen above) are already IN RN 0.56. But in fact, the problem for me hasn't been solved. Tested on 2 different xiaomi and 1 samsung devices. All inputs are going slowdown after delete pressed many times. By the way, app running on device emulator works prefect as well! |
If this problem still persists then perhaps this issue should be reopened? I can't test it myself on 0.56 as it breaks on windows |
@s-nel Thanks, this is super helpful data! In particular: that is a lot more typing than I had previously been doing to reproduce this issue. I just tried again with my test app, on RN 0.55.4 + patch. If I type continuously for about 60 seconds -- maybe 400-500 characters, at a rough estimate (just gibberish, both thumbs constantly typing letters) -- then the perf overlay gets to about "10 dropped so far", and zero stutters. If I continue and get up to about 90 seconds, the "dropped" figure climbs faster, and the framerate (in "JS: __ fps") noticeably drops. At about 150 seconds (~1000 characters?), the "dropped" figure rapidly climbs past 200, and the framerate hangs out around 30. Even at that point, it's still "0 stutters". And if I ignore the perf overlay and try typing normal text and watching it like I were using an app for real: it's a bit laggy, but doesn't make things feel unusable or outright broken, and I think most of the time I wouldn't even notice. In the 0.55 release without the fix, I'd type much less (maybe 200 characters) and reach a state where the "stutters" counter rapidly climbed and the displayed framerate was often 0.0 fps. The user experience in this state is that you type and type, and your keyboard responds fine, but the app looks frozen for multiple seconds before updating with seconds' worth of typing all at once. So: the patch makes things a lot better, but there's still an underlying bug here. I think the outstanding bug is probably best tracked as a new issue, because the symptoms are pretty different and because there's been a lot of discussion on this thread that would be confusing when talking about the issue that's still there. I have a few speculative thoughts on the remaining issue, which I'll make as another comment after this one. |
I can confirm this as well @gnprice we have seen the same thing. The fix definitely makes a massive difference. Takes the app from unusable to usable for sure. But there is certainly a more serious underlying issue here. |
Here's what I wrote at #19645 (comment) when I figured out the bug that PR fixed (which was introduced between v0.54 and v0.55 and caused the extreme version of this problem, as described in the original reports in this thread -- for short, "bug #19126"):
So this #19126 (i.e., the bug that's fixed in v0.56 by #19645) was basically a multiplier on the effect of some other, underlying bug. I'll file an issue for the underlying bug in a moment, so it'll have a number too. As far as I know, nobody had previously reported the underlying bug before #19126 made its effects so severe. But now with @s-nel 's detailed observations I think we're looking at the same underlying bug. I don't know anything more about what causes it; even the hypothesis that we're ending up with large numbers of text shadow nodes is just an inference from how it interacts with the line fixed in #19645. More debugging required. Which I probably won't do because the remaining symptoms are no longer one of the top issues in our own app, but I hope somebody else will! |
Summary: This reverts 5898817 "Implement letterSpacing on Android >= 5.0". Testing shows that that commit is the cause of facebook#19126, where in a controlled TextInput after some text is first added, then deleted, further interaction with the TextInput becomes extremely slow. Fixes facebook#19126. Tried the repro case from facebook#19126 without this change, then with it. The issue reproduces, then doesn't. Closes facebook#19645 Differential Revision: D8675230 Pulled By: hramos fbshipit-source-id: e2c2d352ee781898721b2dff4738572d1a6b7471
I'll lock this in order to favour #20119 for followup discussion. |
Cherry pick from: facebook@5017b86 Because NaN is special, the `!=` version of this condition will always be true -- even if `mLetterSpacing` is also `Float.NaN`, which is its default value. It should instead be `!Float.isNaN(...)`. The effect of the broken condition is that every text shadow node will create a `CustomLetterSpacingSpan` around its contents, even when the `letterSpacing` prop was never set. Empirically, that in turn causes facebook#19126: in a controlled TextInput after some text is first added, then deleted, further interaction with the TextInput becomes extremely slow. Perhaps we're somehow ending up with large numbers of these shadow nodes (that sounds like a bug in itself, but I guess it's normally low-impact); then this code would have caused them each to generate more work to do. In any case, fixing this condition causes that issue to disappear. The affected logic was introduced between v0.54 and v0.55, in facebook#17398 aka 5898817 "Implement letterSpacing on Android >= 5.0". Fixes facebook#19126.
Summary: This reverts 5898817 "Implement letterSpacing on Android >= 5.0". Testing shows that that commit is the cause of facebook#19126, where in a controlled TextInput after some text is first added, then deleted, further interaction with the TextInput becomes extremely slow. Fixes facebook#19126. Tried the repro case from facebook#19126 without this change, then with it. The issue reproduces, then doesn't. Closes facebook#19645 Differential Revision: D8675230 Pulled By: hramos fbshipit-source-id: e2c2d352ee781898721b2dff4738572d1a6b7471
Summary: This reverts 5898817 "Implement letterSpacing on Android >= 5.0". Testing shows that that commit is the cause of facebook#19126, where in a controlled TextInput after some text is first added, then deleted, further interaction with the TextInput becomes extremely slow. Fixes facebook#19126. Tried the repro case from facebook#19126 without this change, then with it. The issue reproduces, then doesn't. Closes facebook#19645 Differential Revision: D8675230 Pulled By: hramos fbshipit-source-id: e2c2d352ee781898721b2dff4738572d1a6b7471
Environment
OS: Linux 4.16
Node: 8.11.1
Yarn: 1.6.0
npm: 6.0.0
Watchman: Not Found
Xcode: N/A
Android Studio: 3.1.2 AI-173.4720617
Packages: (wanted => installed)
react: 16.3.2 => 16.3.2
react-native: 0.55.3 => 0.55.3
Steps to Reproduce
Expected Behavior
TextInput should not start lagging after something has been written inside and deleted.
Actual Behavior
TextInput lags after 40-50 times you delete the input, programmatically or via keyboard, doesn't really matter also the amount of text inside of it because happens also with a single character.
The text was updated successfully, but these errors were encountered: