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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TextInput - Multiline] Reimplemented RCTTextView and added auto height scaling. #1229

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
@lukasreichart

lukasreichart commented May 10, 2015

I started with the goal to add auto height scaling functionality to the RCTTextViewclass. As discussed in this thread.
I soon noticed, that I would have to rewrite most of RCTTextView and create at least an object on the shadow thread for it: RCTShadowTextView.

I tried to implement the RCTShadowTextView the same way as RCTShadowText, so there is as much consistency as possible. I did not change the interface of the current RCTTextView implementation, so there shouldn't be any problems with updating an existing application.

Implemented auto height scaling for TextView

The RCTShadowTextView class implements the Measure function of css-layout. If the TextInput componenet does not have a specified height, it will automatically scale to the height that matches the text. It calculates the height using NSLayoutContaniner because UITextView is not thread safe.
Note: It's important to disable scrolling when using the auto scaling.

Data Flow

The displayed text is sent from JS to the object on the Shadow Thread. The shadow object is responsible for generating a NSAttributedString (Same approach as in RCTShadowText).

Updates from javascript go the following way:

JS Object --(NSString)--> RCTShadowTextView --(NSAttributedString)--> RCTShadowText

Updates from UITextViewgo the following way:

RCTShadowText --(Event)--> JS Object --(NSString)--> RCTShadowTextView

Please note that the update chain stops after RCTShadowTextView. This way the UITextView is only updated when we change something from javascript, but not if the user types somehting into the UITextView. In this case we only update the Shadow object. This approach is necessary because we can not constantly reset the UITextView.attributedText property, because this will also remove any autocorrections.

Created the RCTAttributedStringHandler class

The RCTAttributedStringHandler stores text styling attributes. It can be used to generate a styled NSAttributedString from a NSString.
The process of generating the NSAttributedString was pretty much copied from RCTShadowText. The reason why I moved it into a separate object is, that RCTShadowTextView needs to store text attributes for two different Strings. The text string and the placeholder string. (Although they share most of the attributes, the approach to move this functionality into a separate object is much cleaner).

Rewrote the uiBlockToAmendWithShadowRegistry

Rewrote the (RCTViewManagerUIBlock)uiBlockToAmendWithShadowViewRegistry:(RCTSparseArray *)shadowViewRegistry function of RCTTextManager and RCTTextViewManager, so that RCTShadowText and RCTShadowTextView are both able to use the
textLifecycle variable of RCTShadowView.

Small Additions / Notes
  • Added a scrollEnabled property to RCTTextView
  • Removed all iOS default padding from the UITextView. I think this is the best solution, because this way we can fully control the appearance by using the css properties.
  • Updated the UIExplorer example with one additional multiline TextInput that uses the new auto height scaling feature.
  • Currently it's not possible to add a subview to my new implementation of RCTTextView, because of a problem with the css-layout project. I already adressed this problem in the following pull request. If you want to try it out on your own just replace the code on line 318 with the following snippet: if (node.children.length === 0) { return; }. This may also be the first step to be able to embed text nodes.

Any additions and questions are welcome. 馃憤

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne May 11, 2015

Collaborator

@lukasreichart - really interesting stuff! Curious what @nicklockwood has to say about it

Collaborator

brentvatne commented May 11, 2015

@lukasreichart - really interesting stuff! Curious what @nicklockwood has to say about it

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide May 12, 2015

Collaborator

Wow this looks great.

Note: It's important to disable scrolling when using the auto scaling.

Why is this?

Collaborator

ide commented May 12, 2015

Wow this looks great.

Note: It's important to disable scrolling when using the auto scaling.

Why is this?

@sahrens

This comment has been minimized.

Show comment
Hide comment
@sahrens

sahrens May 13, 2015

Contributor

Also cc @a2

Contributor

sahrens commented May 13, 2015

Also cc @a2

@vjeux vjeux assigned a2 May 14, 2015

@brentvatne brentvatne changed the title from Reimplemented RCTTextView and added auto height scaling. to [TextInput - Multiline] Reimplemented RCTTextView and added auto height scaling. May 30, 2015

@sjmueller

This comment has been minimized.

Show comment
Hide comment
@sjmueller

sjmueller Jun 9, 2015

Contributor

Anything preventing this one from being merged? It's much needed functionality for TextInput, but this PR has been pretty quiet the last month.

Contributor

sjmueller commented Jun 9, 2015

Anything preventing this one from being merged? It's much needed functionality for TextInput, but this PR has been pretty quiet the last month.

RCTTextView: numberOfLines is now a property of the shadow view and a鈥
鈥so working correctly with the calculation of the height.
@lukasreichart

This comment has been minimized.

Show comment
Hide comment
@lukasreichart

lukasreichart Jun 9, 2015

@sjmueller I am waiting for the review of @a2 ....

lukasreichart commented Jun 9, 2015

@sjmueller I am waiting for the review of @a2 ....

@qingfeng

This comment has been minimized.

Show comment
Hide comment
@qingfeng

qingfeng Jun 12, 2015

Contributor

cool, guys 馃嵑

Contributor

qingfeng commented Jun 12, 2015

cool, guys 馃嵑

@lukasreichart

This comment has been minimized.

Show comment
Hide comment
@lukasreichart

lukasreichart commented Jun 18, 2015

@brentvatne @ide @a2 @nicklockwood any progress here? 馃憤

@a2 a2 assigned nicklockwood and unassigned a2 Jun 18, 2015

@JonasJonny

This comment has been minimized.

Show comment
Hide comment
@JonasJonny

JonasJonny commented Jul 6, 2015

馃憤

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Jul 7, 2015

Collaborator

Unfortunately not @lukasreichart - I think we just need to hold up until someone has time to properly review :(

Collaborator

brentvatne commented Jul 7, 2015

Unfortunately not @lukasreichart - I think we just need to hold up until someone has time to properly review :(

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Jul 7, 2015

Contributor

Sorry for the delay. It's a complex PR, so it will take a couple of days of somebody's time to review and test, and we're a small team with a long task list :-) We'll get to it eventually though.

Contributor

nicklockwood commented Jul 7, 2015

Sorry for the delay. It's a complex PR, so it will take a couple of days of somebody's time to review and test, and we're a small team with a long task list :-) We'll get to it eventually though.

@Sing-Li

This comment has been minimized.

Show comment
Hide comment
@Sing-Li

Sing-Li Aug 1, 2015

馃憤 Hope this gets merged soon. While not road-block, it is a monumental task to implement similar behaviors otherwise 馃槮

Sing-Li commented Aug 1, 2015

馃憤 Hope this gets merged soon. While not road-block, it is a monumental task to implement similar behaviors otherwise 馃槮

@yodaiken

This comment has been minimized.

Show comment
Hide comment
@yodaiken

yodaiken Aug 4, 2015

Hi just want to echo that this is a really key feature!

yodaiken commented Aug 4, 2015

Hi just want to echo that this is a really key feature!

@lukasreichart

This comment has been minimized.

Show comment
Hide comment
@lukasreichart

lukasreichart Aug 25, 2015

@nicklockwood I just noticed, that have you have rewritten some of the RCTText and RCTTextView code since I've created this PR. I could rewrite my PR so it would be easier to merge for you. Would this help? I really want to close this, because this feature makes working with text so much easier.

lukasreichart commented Aug 25, 2015

@nicklockwood I just noticed, that have you have rewritten some of the RCTText and RCTTextView code since I've created this PR. I could rewrite my PR so it would be easier to merge for you. Would this help? I really want to close this, because this feature makes working with text so much easier.

@adbl

This comment has been minimized.

Show comment
Hide comment
@adbl

adbl Aug 27, 2015

Contributor

+1

Contributor

adbl commented Aug 27, 2015

+1

@sjmueller

This comment has been minimized.

Show comment
Hide comment
@sjmueller

sjmueller Sep 2, 2015

Contributor

@lukasreichart it would be awesome if you could rewrite the PR. Many of us are using your changes manually, and it would be great to have a version that works with the latest build of RN.

Contributor

sjmueller commented Sep 2, 2015

@lukasreichart it would be awesome if you could rewrite the PR. Many of us are using your changes manually, and it would be great to have a version that works with the latest build of RN.

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Oct 25, 2015

Collaborator

@javache @nicklockwood - do you guys have any thoughts on this?

Collaborator

brentvatne commented Oct 25, 2015

@javache @nicklockwood - do you guys have any thoughts on this?

@kinhunt

This comment has been minimized.

Show comment
Hide comment
@kinhunt

kinhunt Nov 14, 2015

@lukasreichart eager to try 馃憤

kinhunt commented Nov 14, 2015

@lukasreichart eager to try 馃憤

@deanmcpherson

This comment has been minimized.

Show comment
Hide comment
@deanmcpherson

deanmcpherson Nov 26, 2015

Contributor

Any move on this?

Contributor

deanmcpherson commented Nov 26, 2015

Any move on this?

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Nov 26, 2015

Contributor

Hey, really sorry for the radio silence on this. It is an important feature, but as you say we now have two competing solutions, both of which are very complex to review, and it affects a critical component used in most of our apps.

I haven't forgotten about this, and I'll try to get one or other solution merged in the near future.

Contributor

nicklockwood commented Nov 26, 2015

Hey, really sorry for the radio silence on this. It is an important feature, but as you say we now have two competing solutions, both of which are very complex to review, and it affects a critical component used in most of our apps.

I haven't forgotten about this, and I'll try to get one or other solution merged in the near future.

@christopherdro

This comment has been minimized.

Show comment
Hide comment
@christopherdro

christopherdro Nov 26, 2015

Contributor

馃憤 Thanks @nicklockwood!

Contributor

christopherdro commented Nov 26, 2015

馃憤 Thanks @nicklockwood!

@kinhunt

This comment has been minimized.

Show comment
Hide comment
@kinhunt

kinhunt Nov 26, 2015

greate

On Thu, Nov 26, 2015 at 7:59 PM, Christopher Dro notifications@github.com
wrote:

[image: 馃憤] Thanks @nicklockwood https://github.com/nicklockwood!


Reply to this email directly or view it on GitHub
#1229 (comment)
.

kinhunt commented Nov 26, 2015

greate

On Thu, Nov 26, 2015 at 7:59 PM, Christopher Dro notifications@github.com
wrote:

[image: 馃憤] Thanks @nicklockwood https://github.com/nicklockwood!


Reply to this email directly or view it on GitHub
#1229 (comment)
.

@cgilboy

This comment has been minimized.

Show comment
Hide comment
@cgilboy

cgilboy Dec 10, 2015

Are there any updates to this or PR #3097? We would really like to see this functionality merged in :)

cgilboy commented Dec 10, 2015

Are there any updates to this or PR #3097? We would really like to see this functionality merged in :)

@jeremiecarlson

This comment has been minimized.

Show comment
Hide comment
@jeremiecarlson

jeremiecarlson commented Dec 10, 2015

+1

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Dec 13, 2015

Collaborator

@cgilboy - as mentioned above, Nick is working on it! There's truly no better person to have on the job, so it's in good hands :)

Collaborator

brentvatne commented Dec 13, 2015

@cgilboy - as mentioned above, Nick is working on it! There's truly no better person to have on the job, so it's in good hands :)

@martincik

This comment has been minimized.

Show comment
Hide comment
@martincik

martincik Dec 28, 2015

+1 for merge, please.

martincik commented Dec 28, 2015

+1 for merge, please.

@sebglazebrook

This comment has been minimized.

Show comment
Hide comment
@sebglazebrook

sebglazebrook commented Dec 31, 2015

+1 for merge

@manask88

This comment has been minimized.

Show comment
Hide comment
@manask88

manask88 Jan 20, 2016

Contributor

hows this going? any plan to merge/ work on this PR soon?

Contributor

manask88 commented Jan 20, 2016

hows this going? any plan to merge/ work on this PR soon?

ghost pushed a commit that referenced this pull request Jan 25, 2016

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
@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Jan 25, 2016

Contributor

Sorry for the long silence on this. At the time this PR was created, calculating the text layout on the shadow queue made a lot of sense, but now that RCTTextView is making heavy use of the scrollview.contentSize anyway, there's not really any performance gain to doing this work in the background, and it massively increases the complexity.

I did try rebasing this against the latest codebase, but it seems to be broken in a way that I can't fix, and in light of the much simpler solution I've implemented in 481f560, I'm not sure it's worth trying to revive this.

Thanks for all your efforts, and sorry we didn't manage to make this work.

Contributor

nicklockwood commented Jan 25, 2016

Sorry for the long silence on this. At the time this PR was created, calculating the text layout on the shadow queue made a lot of sense, but now that RCTTextView is making heavy use of the scrollview.contentSize anyway, there's not really any performance gain to doing this work in the background, and it massively increases the complexity.

I did try rebasing this against the latest codebase, but it seems to be broken in a way that I can't fix, and in light of the much simpler solution I've implemented in 481f560, I'm not sure it's worth trying to revive this.

Thanks for all your efforts, and sorry we didn't manage to make this work.

doostin added a commit to doostin/react-native that referenced this pull request Feb 1, 2016

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 facebook#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

jaysoo added a commit to jaysoo/react-native that referenced this pull request Feb 2, 2016

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 facebook#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
@deanmcpherson

This comment has been minimized.

Show comment
Hide comment
@deanmcpherson

deanmcpherson Feb 17, 2016

Contributor

Hey @nicklockwood, quick question - the onChange event that we depend upon here only fires when a user enters text via a keyboard. This means that if there is more than one line of text loaded in (e.g. resuming a form), the height will be restricted to whatever the default is. Is there any way to trigger (or measure) the contentSize so that we can set an accurate initial height?

Contributor

deanmcpherson commented Feb 17, 2016

Hey @nicklockwood, quick question - the onChange event that we depend upon here only fires when a user enters text via a keyboard. This means that if there is more than one line of text loaded in (e.g. resuming a form), the height will be restricted to whatever the default is. Is there any way to trigger (or measure) the contentSize so that we can set an accurate initial height?

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Feb 18, 2016

Collaborator

Great point @deanmcpherson, I'm also curious about this

Collaborator

brentvatne commented Feb 18, 2016

Great point @deanmcpherson, I'm also curious about this

@deanmcpherson

This comment has been minimized.

Show comment
Hide comment
@deanmcpherson

deanmcpherson Mar 2, 2016

Contributor

So I've have had a go at firing the same onChange event in the updateContentSize method of RCTTextView.m, which works correctly in adjusting the textinput height to the value automagically. It doesn't appear to be firing too often, but obviously isn't the cleanest way of handling this.

I did have a go at creating a separate event name to listen on for inputs, (e.g. onContentSize) but couldn't quite get it to come through correctly. Though I imagine throwing in custom events isn't really a great way forward either.

Thoughts @nicklockwood @brentvatne?

Contributor

deanmcpherson commented Mar 2, 2016

So I've have had a go at firing the same onChange event in the updateContentSize method of RCTTextView.m, which works correctly in adjusting the textinput height to the value automagically. It doesn't appear to be firing too often, but obviously isn't the cleanest way of handling this.

I did have a go at creating a separate event name to listen on for inputs, (e.g. onContentSize) but couldn't quite get it to come through correctly. Though I imagine throwing in custom events isn't really a great way forward either.

Thoughts @nicklockwood @brentvatne?

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Mar 2, 2016

Contributor

What we ideally want here is to:

  1. Only call onChange once per set of changes (e.g. if text and contentSize both change at once - which is usually the case - we should only fire one onChange event)
  2. Only call onChange if something has actually changed (i.e. don't fire it unconditionally in updateContentSize if updateContentSize sometimes gets called when the contentSize hasn't changed).

The former can be ensured by only firing onChange in one place. The latter can be ensured by keeping track of the previous contentSize, text, etc. and comparing them before firing the event.

If you have a solution that seems to work, put up a PR and we can review it and suggest changes if needed.

Contributor

nicklockwood commented Mar 2, 2016

What we ideally want here is to:

  1. Only call onChange once per set of changes (e.g. if text and contentSize both change at once - which is usually the case - we should only fire one onChange event)
  2. Only call onChange if something has actually changed (i.e. don't fire it unconditionally in updateContentSize if updateContentSize sometimes gets called when the contentSize hasn't changed).

The former can be ensured by only firing onChange in one place. The latter can be ensured by keeping track of the previous contentSize, text, etc. and comparing them before firing the event.

If you have a solution that seems to work, put up a PR and we can review it and suggest changes if needed.

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Mar 3, 2016

Collaborator

Thanks @nicklockwood! @deanmcpherson - do you want to try to PR this suggested approach? @mkonicek we need something similar for Android too

Collaborator

brentvatne commented Mar 3, 2016

Thanks @nicklockwood! @deanmcpherson - do you want to try to PR this suggested approach? @mkonicek we need something similar for Android too

@deanmcpherson

This comment has been minimized.

Show comment
Hide comment
@deanmcpherson

deanmcpherson Mar 3, 2016

Contributor

I've noticed a few instances where my implementation isn't working, in particular using <Text> children instead of value for dynamic styling of content seems to sometimes set an incorrect height sometimes. I'm guessing that there is some kind of race condition happening. I imagine that will be resolved by following 1) and 2) above. I'm happy to try a PR for this.

@nicklockwood do you think it makes sense for onChange to fire when no text value has changed? For most uses of TextInput where the contentSize isn't important, it might be unexpected behaviour for the user.

Contributor

deanmcpherson commented Mar 3, 2016

I've noticed a few instances where my implementation isn't working, in particular using <Text> children instead of value for dynamic styling of content seems to sometimes set an incorrect height sometimes. I'm guessing that there is some kind of race condition happening. I imagine that will be resolved by following 1) and 2) above. I'm happy to try a PR for this.

@nicklockwood do you think it makes sense for onChange to fire when no text value has changed? For most uses of TextInput where the contentSize isn't important, it might be unexpected behaviour for the user.

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Mar 3, 2016

Contributor

@deanmcpherson

do you think it makes sense for onChange to fire when no text value has changed?

I agree it's not ideal, but it's the least bad option I can think of, and according to @dmmiller that's how it works on Android currently (unless I misunderstood?)

Contributor

nicklockwood commented Mar 3, 2016

@deanmcpherson

do you think it makes sense for onChange to fire when no text value has changed?

I agree it's not ideal, but it's the least bad option I can think of, and according to @dmmiller that's how it works on Android currently (unless I misunderstood?)

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Mar 3, 2016

Contributor

What we should probably do is add a separate onChangeContentSize event which can be observerved independently of onTextChange.

Let's get the APIs consistent between platforms before we start changing them though.

Contributor

nicklockwood commented Mar 3, 2016

What we should probably do is add a separate onChangeContentSize event which can be observerved independently of onTextChange.

Let's get the APIs consistent between platforms before we start changing them though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment