-
Notifications
You must be signed in to change notification settings - Fork 24.3k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Added support for auto-resizing text fields
Summary: public This diff adds support for auto-resizing multiline text fields. This has been a long-requested feature, with several native solutions having been proposed (see #1229 and D2846915). Rather than making this a feature of the native component, this diff simply exposes some extra information in the `onChange` event that makes it easy to implement this in pure JS code. I think this is preferable, since it's simpler, works cross-platform, and avoids any controversy about what the API should look like, or how the props should be named. It also makes it easier to implement custom min/max-height logic. Reviewed By: sahrens Differential Revision: D2849889 fb-gh-sync-id: d9ddf4ba4037d388dac0558aa467d958300aa691
- Loading branch information
1 parent
0893d07
commit 481f560
Showing
3 changed files
with
105 additions
and
5 deletions.
There are no files selected for viewing
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
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
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
481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it work on Android?
481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@satya164 should do.
481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicklockwood Just tested on Android in 0.18 and it works fine. There's a slight flicker though. I now feel stupid coz I was rendering a absolutely positioned phantom view to measure height, and then update the height of the TextInput!
481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what we can do about the flicker. Perhaps @andreicoman11 has some ideas?
481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This flickers on android because of the Java->JS->Java roundtrip that the height prop needs to take. We can't just
accept
the new height that the EditText receives, the source of truth is the shadow node hierarchy that will still have the old value until JS updates it.I'm curious how come iOS isn't affected by this, it might help us find a solution for this.
481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiline text inputs on iOS contain a vertical scrollView. When you type, iOS automatically increases the height of the scrollView content, and it's that height that we send to JS in the change event.
In my autoresizing example, when JS receives the change event, it tells the component to update its height to match the height of the scrollView content. This keeps the height of the scrollview in sync with the height of its content.
The shadow node backing the textinput is completely unaware of the scrollView content height, it only manages the height of the component itself, so there's no conflict. Changing the height of the component doesn't affect the scrollView content height, and changing the height of the content doesn't affect the component height (except after the JS round trip, when my code tells it to update).
If there was any lag, the height of the content might briefly be taller than the scrollview, in which case you would be able to scroll the view vertically until it caught up, but there' no reason why that should make it flicker.
481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will there be a way to measure the initial height? So it doesn't start off with 35?
481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DUBERT as soon as you set the text, a change event should be sent with the content height, which is effectively the initial height. If you don't set any text, the initial content height would be zero.
I specified 35 as the starting (and minimum) height value because that looks about right for a single line of text, and a textfield with zero height isn't very useful.
481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicklockwood makes perfect sense. Can't wait use this! Right now I'm using a second
<Text>
component and measuring that to determine how tall the<TextInput>
should be.481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is extremely useful and works very well.
481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also noticed that when the text changes because of a
setState
/receive props/initial text, it does not emit a change event, so height is not set properly. (Android)481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is the same thing people are seeing on Android, but I'm noticing a flicker on iOS. Looks like when a new line is added the text on the new line is anchored to the bottom of the input and pushes the existing lines up until the height is updated and then the entire block jumps down to where it should be.
I noticed @nicklockwood and @andreicoman11 mentioning a flicker on Android only, wondering if I'm doing something wrong here?
481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working fine for me on iOS. No flicker here.
481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed a flicker on iOS when I set the height of the
TextInput
toevent.nativeEvent.contentSize.height
. If I added a few extra pixels,height: event.nativeEvent.contentSize.height + 10
, the flicker went away.481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also see the flicker on iOS in two situations:
481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicklockwood
That doesn't seem true, the
onChange
event is sent fromtextViewDidChange:
method on iOS, which is triggered only when input is pressed.481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asamiller, @kemcake you're right - this seems to be an oversight. Please open a PR to fix it.
CC @dmmiller - do you know if this is the same on Android?
481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicklockwood 481f560
481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicklockwood It looks like Android sends the height initially as opposed to just on textViewDidChange.
481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicklockwood Seems like this does not work with a placeholder. Is there a way to set the height according to the initial placeholder?
481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicklockwood works fine on multiline=true as designed to be. Is it add support to single line mode also? So that it can work with the onSubmitEditing option.
I tried, but contentSize is undefined in single line mode.
481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicklockwood @satya164 @kemcake - any advance with triggering on change for setting ios initial text ?
481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think to be complete this solution needs a new property on
TextInput
that causes it to be rendered with a height large enough to contain its value.In my case the lack of this is particularly acute as I'm re-rendering the input as the user types -- whenever she does an auto-complete (the autocompleted text is specially formatted and I'm using a child
Text
node to set the content). If the user is near the end of the line when she triggers the complete and the completion pushes text over a new line that new line is hidden.481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the
+10
approach @asamiller proposed works because the magic number 10 is enough to guarantee that an additional newline would be shown. The unintended consequence of this is that the input would be bigger than what you actually need.Maybe a better approach would be to wrap the
TextInput
into aView
and set the height for both the input and the view. The input could have a+ K
height to avoid seeing the flickering effect but the parent view set would the correct height so that we don't see the next free line.481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yonatanmn I use a hidden Text component to get the height of the initial text.
Put a hidden Text element(which style like
position: 'absolute', top: 10000, left: 10000
) aside the TextInput element, and in TextInput's componentDidMount method, usemeasure
method to get the hidden Text element's height.And for the newline flicker problem, currently I add an extra line height(like 18) to the contentSize.height.
Hope these helps.
481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For initial height I added onContentSizeChange on TextInput, it seems to work great:
481f560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
THIS NO LONGER WORKS WITH
onChange
: bac84ce UseonContentSizeChange
instead.