Skip to content

Commit

Permalink
Fix extreme TextInput slowness on Android (#19645)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@gnprice @facebook-github-bot
gnprice authored and facebook-github-bot committed Jun 28, 2018
1 parent b3ef1c3 commit 5017b86
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion ReactAndroid/src/main/java/com/facebook/react/views/text/ReactBaseTextShadowNode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ private static void buildSpannedFromShadowNode(
start, end, new BackgroundColorSpan(textShadowNode.mBackgroundColor)));
}
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {
if (textShadowNode.mLetterSpacing != Float.NaN) {
if (!Float.isNaN(textShadowNode.mLetterSpacing)) {
ops.add(new SetSpanOperation(
start,
end,
Expand Down

2 comments on commit 5017b86

@hramos
Copy link
Contributor

@hramos hramos commented on 5017b86 Jun 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit description here is out of date because it's taken from the original PR description. The actual commit that implemented this change has the following text:

Fix extreme TextInput slowness on Android

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 #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 #17398
aka 5898817 "Implement letterSpacing on Android >= 5.0".

Fixes #19126.

@gnprice
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks, @hramos !

Next time I'll be sure to update the PR description so the commit that's merged gets the right message. :-)

Please sign in to comment.