Implement TextInput onContentSizeChange #8457

janicduplessis · 2016-06-27T23:22:23Z

This adds proper support for tracking a TextInput content size as discussed in #6552 by adding a new callback that is called every time the content size changes including when first rendering the view.

Some points that are up for discussion are what do we want to do with the onChange callback as I don't see any use left for it now that we can track text change in onChangeText and size changes in onContentSizeChange. Also a bit off topic but should we consider renaming onChangeText to onTextChange to keep the naming more consistent (see this naming justification).

This is split in 2 commits for easier review, one for iOS and one for android.

The iOS implementation simply checks if the content size has changed everytime we update it and fire the callback, the only small issue was that the content size had several different values on initial render so I added a check to not fire events before the layoutSubviews where at this point the value is good.

The Android implementation is based on @brentvatne work here that has been used for a while in exponent.

* Test plan*
Tested that the event was properly fired only once per content size change with the right value.
Tested that the event is called initially when there is already some text in the input.
Tested that the event is called when resizing the text view frame.

ghost · 2016-06-27T23:23:26Z

By analyzing the blame information on this pull request, we identified @brunobar79 and @Bhullnatik to be potential reviewers.

janicduplessis · 2016-06-27T23:25:44Z

cc @brentvatne @ide @bestander

brentvatne · 2016-06-27T23:29:05Z

Libraries/Components/TextInput/TextInput.js

@@ -311,6 +311,11 @@ const TextInput = React.createClass({
     */
    onChangeText: PropTypes.func,
    /**
+     * Callback this is called when the text input's content size changes.


Callback this => Callback that

bestander · 2016-06-27T23:36:37Z

Copying @andreicoman11, answer from internal thread:

From both discussions, it looks like the best way to solve this is to add an "autoScale" prop, so that the multiline textinput grows to fit the text inside. Nick seems to have tried this in the past for iOS, I also gave it a shot on Android and it looked doable, but haven't had time to see it through.

brentvatne · 2016-06-27T23:36:40Z

Libraries/Text/RCTTextView.m

+
+  // Start sending content size updates only after the view has been layed out
+  // otherwise we send multiple events with bad dimentions on initial render.
+  _sendContentSizeUpdates = YES;


@javache / @sahrens - can either of you confirm if this is the right thing to do here?

bestander · 2016-06-27T23:37:08Z

Ping @dmmiller, what do you think from product parity POV?

brentvatne · 2016-06-27T23:41:16Z

From both discussions, it looks like the best way to solve this is to add an "autoScale" prop, so that the multiline textinput grows to fit the text inside. Nick seems to have tried this in the past for iOS, I also gave it a shot on Android and it looked doable, but haven't had time to see it through.

I don't think this is actually what we want for all cases -- we can build autoScale on top of this callback if we want, it is a subset of the possible behaviours you can create with this. What if you just want a callback to know the content width in order to overlay something at the end of the text? Or if you want to limit the height (autoScale might grow to multiple props then in this case). Or have a TextInput that grows in width as you type.

That said, I think it could be valuable for autoScale to be implemented natively in addition to this, in the same way that maxLength is native but you could implement the same thing with onChangeText.

edit: not worded the best above, to clarify: we probably want both autoScale and onContentSizeChange just like we have maxLength and controlled value + onChangeText.

brentvatne · 2016-06-27T23:45:10Z

This looks great to me @janicduplessis -- thanks!

janicduplessis · 2016-06-27T23:45:33Z

One advantage of implementing autoScale in native is that it would probably be easier to prevent any text flickering when the input is growing or shrinking, right now with this implementation sometimes on iOS the text moves up for one frame when adding a new line and on android it does move a bit when removing a line. Not sure if there is a way to improve this as everything is async. The main advantage is as @brentvatne mentioned is that it is a lot more flexible and allows implementing multiple behaviours.

janicduplessis · 2016-06-27T23:54:15Z

Also this is only implemented for RCTTextView and not RCTTextField as I didn't see the need for it with a TextField but we might want to implement it for consistency sake.

ide · 2016-06-28T00:24:40Z

Examples/UIExplorer/TextInputExample.android.js

+        onContentSizeChange={(contentSize) => {
+          this.setState({height: contentSize.height});
+        }}
+        onChangeText={(text) => {


Maybe leave this until we've implemented onTextChange?

onChangeText already works, I just mentioned that we should probably rename it for consistency. Unless you mean something else?

andreicoman11 · 2016-06-28T08:58:55Z

ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java

@@ -102,7 +103,23 @@ public ReactEditText(Context context) {
  // TODO: t6408636 verify if we should schedule a layout after a View does a requestLayout()
  @Override
  public boolean isLayoutRequested() {
-    return false;
+    // If we are watching and updating container height based on content size


If you return true here, in the case of single-line text, the cursor will go out of view after you've typed after the end of the visible line. That means that onContentSizeChange should be limited to only work with multiline texts.

andreicoman11 · 2016-06-28T09:12:45Z

Let me expand on what I've posted internally. The problem with implementing behaviors related to size change on top of callbacks is that they will flicker often, because of the asynchronicity of it all. If JS is busy at any point and cannot react to a size change immediately, there will always be a lag between content size change and implemented JS behavior. The worse the device, the more often you will get this lag (looking at you android). The more complicated the JS behavior, the easier it will be for this to happen.
I saw that this change was needed for implementing growing multiline text-input, case for which I think that a native implementation of autoScale is the best solution. This will also work, but not as good. Either way, this change is should go through, especially if there are other cases where it can be used.

dmmiller · 2016-06-28T09:25:37Z

...src/main/java/com/facebook/react/views/recyclerview/RecyclerViewBackedScrollViewManager.java

@@ -85,9 +85,6 @@ public void scrollTo(
  Map getExportedCustomDirectEventTypeConstants() {
    return MapBuilder.builder()
        .put(ScrollEventType.SCROLL.getJSEventName(), MapBuilder.of("registrationName", "onScroll"))
-        .put(


You can't get rid of this. We use this internally in apps.

The event was moved here https://github.com/facebook/react-native/pull/8457/files#diff-0404c7801b7b68d8a58a11f338c5d666R74 since onContentSizeChange is now used by scollview and textinput. Is there a difference between doing it in the manager vs in uimanager that I'm not aware of?

dmmiller · 2016-06-28T09:27:24Z

@nicklockwood or @javache Have either of you had a chance to look at the obj-c implementation?

javache · 2016-06-28T22:31:34Z

Libraries/Components/TextInput/TextInput.js

@@ -673,6 +690,12 @@ const TextInput = React.createClass({
    }
  },

+  _onContentSizeChange(event: Event) {
+    if (this.props.onContentSizeChange) {
+      this.props.onContentSizeChange(event.nativeEvent.contentSize);


This seems unlike any of the other event handlers we provide. If we add a new property to this event, it's really hard to make it backwards compatible.

Yea I wasn't sure about this, I did like onChangeText where we pass only the text. Personally I prefer having only the relevant value passed the the callback instead of the whole event in case where the event is pretty specific.

With that said I don't mind changing this to passing the event either as I can see it has some advantages and we may use this pattern more often.

With destructuring assignment the syntax overhead of unpacking an object vs. getting just the relevant value is only { } so I think we should go with the more future-proof approach and use the object.

javache · 2016-06-28T22:33:57Z

Objective-C looks ok, but I agree with @andreicoman11's concerns about the flicker you'll see when JS is running behind.

artyomtrityak · 2016-06-29T16:26:49Z

Examples/UIExplorer/TextInputExample.android.js

@@ -76,18 +76,21 @@ var TextEventsExample = React.createClass({
 class AutoExpandingTextInput extends React.Component {
  constructor(props) {
    super(props);
-    this.state = {text: '', height: 0};
+    this.state = {


Should FlowType State type be added for this?

brentvatne · 2016-07-06T20:37:47Z

@andreicoman11 - given that @javache gave a 👍 on the ObjC side, is this ok to ship?

andreicoman11 · 2016-07-07T11:14:20Z

Lgtm. @dmmiller do you have any other concerns about this? Otherwise we can shipit

dmmiller · 2016-07-07T12:15:45Z

Let's go for it.

@facebook-github-bot shipit

ghost · 2016-07-07T12:16:30Z

Thanks for importing. If you are an FB employee go to Phabricator to review.

Summary: This adds proper support for tracking a TextInput content size as discussed in #6552 by adding a new callback that is called every time the content size changes including when first rendering the view. Some points that are up for discussion are what do we want to do with the onChange callback as I don't see any use left for it now that we can track text change in onChangeText and size changes in onContentSizeChange. Also a bit off topic but should we consider renaming onChangeText to onTextChange to keep the naming more consistent (see [this naming justification](https://twitter.com/notbrent/status/709445076850597888)). This is split in 2 commits for easier review, one for iOS and one for android. The iOS implementation simply checks if the content size has changed everytime we update it and fire the callback, the only small issue was that the content size had several different values on initial render so I added a check to not fire events before the layoutSubviews where at this point the value is g Closes facebook/react-native#8457 Differential Revision: D3528202 Pulled By: dmmiller fbshipit-source-id: fefe83f10cc5bfde1f5937c48c88b10408e58d9d

andreicoman11 · 2016-08-18T13:36:30Z

@janicduplessis have you tested this on android? Because I happened to play around with it today and it was not working. More precisely, setting 'height' on the component will cause no 'onLayout' events to be dispatched, which was the whole point of this commit.

ghost · 2016-08-18T14:48:10Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

janicduplessis · 2016-08-19T06:31:38Z

@andreicoman11 I tested using this example in UIExplorer on android, let me know if that works for you. https://github.com/facebook/react-native/blob/master/Examples/UIExplorer/js/TextInputExample.android.js#L81

andreicoman11 · 2016-08-19T10:15:24Z

@jeanregisser that is why I reopened this, the example is not working. The way it was failing indicated to me that this might've never worked properly. But if you're certain this worked when this commit landed, then we can look into what has changed and close this.

andreicoman11 · 2016-08-19T12:26:59Z

Turns out this is only broken on some emulators, unclear why. Will investigate independent of this.

janicduplessis · 2016-08-19T16:35:06Z

@andreicoman11 That's weird, thanks for looking into it!

Summary: This adds proper support for tracking a TextInput content size as discussed in facebook#6552 by adding a new callback that is called every time the content size changes including when first rendering the view. Some points that are up for discussion are what do we want to do with the onChange callback as I don't see any use left for it now that we can track text change in onChangeText and size changes in onContentSizeChange. Also a bit off topic but should we consider renaming onChangeText to onTextChange to keep the naming more consistent (see [this naming justification](https://twitter.com/notbrent/status/709445076850597888)). This is split in 2 commits for easier review, one for iOS and one for android. The iOS implementation simply checks if the content size has changed everytime we update it and fire the callback, the only small issue was that the content size had several different values on initial render so I added a check to not fire events before the layoutSubviews where at this point the value is g Closes facebook#8457 Differential Revision: D3528202 Pulled By: dmmiller fbshipit-source-id: fefe83f10cc5bfde1f5937c48c88b10408e58d9d

machard · 2016-08-24T21:58:41Z

Well I also have the problem on a Samsung S4 Mini.
I just receive the first event then nothing

Summary: This adds proper support for tracking a TextInput content size as discussed in facebook#6552 by adding a new callback that is called every time the content size changes including when first rendering the view. Some points that are up for discussion are what do we want to do with the onChange callback as I don't see any use left for it now that we can track text change in onChangeText and size changes in onContentSizeChange. Also a bit off topic but should we consider renaming onChangeText to onTextChange to keep the naming more consistent (see [this naming justification](https://twitter.com/notbrent/status/709445076850597888)). This is split in 2 commits for easier review, one for iOS and one for android. The iOS implementation simply checks if the content size has changed everytime we update it and fire the callback, the only small issue was that the content size had several different values on initial render so I added a check to not fire events before the layoutSubviews where at this point the value is g Closes facebook#8457 Differential Revision: D3528202 Pulled By: dmmiller fbshipit-source-id: fefe83f10cc5bfde1f5937c48c88b10408e58d9d

Summary: This adds proper support for tracking a TextInput content size as discussed in #6552 by adding a new callback that is called every time the content size changes including when first rendering the view. Some points that are up for discussion are what do we want to do with the onChange callback as I don't see any use left for it now that we can track text change in onChangeText and size changes in onContentSizeChange. Also a bit off topic but should we consider renaming onChangeText to onTextChange to keep the naming more consistent (see [this naming justification](https://twitter.com/notbrent/status/709445076850597888)). This is split in 2 commits for easier review, one for iOS and one for android. The iOS implementation simply checks if the content size has changed everytime we update it and fire the callback, the only small issue was that the content size had several different values on initial render so I added a check to not fire events before the layoutSubviews where at this point the value is g Closes facebook/react-native#8457 Differential Revision: D3528202 Pulled By: dmmiller fbshipit-source-id: fefe83f10cc5bfde1f5937c48c88b10408e58d9d

janicduplessis and others added 2 commits June 27, 2016 17:55

Implement TextInput onContentSizeChange on iOS

589f1e1

Implement TextInput onContentSizeChange on Android

4da65ea

ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jun 27, 2016

brentvatne reviewed Jun 27, 2016
View reviewed changes

Fix typo

1e1b928

brentvatne reviewed Jun 27, 2016
View reviewed changes

ide reviewed Jun 28, 2016
View reviewed changes

Fix typos take 2

44004de

andreicoman11 reviewed Jun 28, 2016
View reviewed changes

dmmiller reviewed Jun 28, 2016
View reviewed changes

javache reviewed Jun 28, 2016
View reviewed changes

artyomtrityak reviewed Jun 29, 2016
View reviewed changes

janicduplessis added 2 commits July 4, 2016 15:16

Make sure onContentSizeChange is only called for multiline inputs

a7f28dd

Pass event instead of content size

ee6dcf6

ghost added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Jul 7, 2016

ghost closed this in 2537157 Jul 7, 2016

andreicoman11 reopened this Aug 18, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 18, 2016

ghost added Import Failed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Aug 18, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 19, 2016

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 19, 2016

andreicoman11 closed this Aug 19, 2016

Implement TextInput onContentSizeChange #8457

Implement TextInput onContentSizeChange #8457

Uh oh!

Conversation

janicduplessis commented Jun 27, 2016

Uh oh!

ghost commented Jun 27, 2016

Uh oh!

janicduplessis commented Jun 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bestander commented Jun 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bestander commented Jun 27, 2016

Uh oh!

brentvatne commented Jun 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brentvatne commented Jun 27, 2016

Uh oh!

janicduplessis commented Jun 27, 2016

Uh oh!

janicduplessis commented Jun 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreicoman11 commented Jun 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmmiller commented Jun 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javache commented Jun 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brentvatne commented Jul 6, 2016

Uh oh!

andreicoman11 commented Jul 7, 2016

Uh oh!

dmmiller commented Jul 7, 2016

Uh oh!

ghost commented Jul 7, 2016

Uh oh!

andreicoman11 commented Aug 18, 2016

Uh oh!

ghost commented Aug 18, 2016

Uh oh!

janicduplessis commented Aug 19, 2016

Uh oh!

andreicoman11 commented Aug 19, 2016

Uh oh!

andreicoman11 commented Aug 19, 2016

Uh oh!

janicduplessis commented Aug 19, 2016

Uh oh!

machard commented Aug 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

brentvatne commented Jun 27, 2016 •

edited

Loading

machard commented Aug 24, 2016 •

edited

Loading