Skip to content
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

Add support for multiline TextInput via UITextView #991

Closed
wants to merge 9 commits into from
Closed

Add support for multiline TextInput via UITextView #991

wants to merge 9 commits into from

Conversation

brentvatne
Copy link
Collaborator

@nicklockwood - Could I get a review of this?

Just took RCTTextField and ported it from UITextField to UITextView as you mentioned in another discussion, and removed any UITextField specific attributes.

  • How do you think this should behave when there are subviews?
  • Do you know how we can respond to the UIControlEventEditingDidEndOnExit event to respond to submit? Because UITextView isn't a UIControl we can't just use addTarget with UIControlEventEditingDidEndOnExit.
  • Any other feedback?

Still going to look over the UITextView docs in more detail and make sure we expose all important options, and add it to the UIExplorer example, just putting this out here for feedback.

multiline

@facebook-github-bot 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 Apr 24, 2015
@@ -308,6 +314,10 @@
13B080151A69489C00A75B9A /* RCTTextField.m */,
13B080161A69489C00A75B9A /* RCTTextFieldManager.h */,
13B080171A69489C00A75B9A /* RCTTextFieldManager.m */,
BBE9C5BE1AE9D24200D3069B /* RCTTextView.h */,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add this to the RCTText lib instead of the main React lib? Otherwise it will clash with our private FBTextView implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

(I know that's rather inconsistent with the RCTTextField - we should move that too, but I'll do that in a separate diff.)

@nicklockwood
Copy link
Contributor

This looks fantastic, thank you!

The only thing I can see that's missing is placeholder text support. There's no built-in support for this in UITextView, so it will need to be implemented as a private UILabel inside RCTTextView that shows and hides automatically when the text is edited.

@brentvatne
Copy link
Collaborator Author

@nicklockwood - having a few issues with getting it to respect padding, both on the placeholder (UILabel as you suggested) and the UITextView.

On the UITextView, padding works as expected only when there is text, otherwise it goes a bit nuts:
placeholder

On UILabel, I think all we need to do is have it factor in the contentInsets of the the UITextView, of course UILabel doesn't support and I was wondering what your approach would be.

Thanks!

@nicklockwood
Copy link
Contributor

OK, so there's a couple of things happening here: The contentInset property you declare in the interface is currently redundant because that property is declared already on UITextView and inherited.

You won't find it in the docs because it's not a property of UITextView itself, but is inherited from UIScrollView, which is UITextView's superclass.

Here's a good writeup of the problem, and the only viable solution, which seems to be to wrap the control in another view and apply the padding to that outer view instead: http://stackoverflow.com/questions/18560412/uitextview-content-inset - in React we could do that wrapping either in the native or the JS code, but I think native makes more sense in this case.

Probably easiest to make RCTTextView a subclass of RCTView, and add a regular UITextView as a private subview that it controls. The contentInset property would then be used to adjust the frame of the subview, not the inset of the scrollView.

To fix the placeholder label, just adjust it's frame to match tht inner textView's (it's frame may need to be inset by a few pixels to make the label text perfectly align with the textView text though).

@brentvatne
Copy link
Collaborator Author

@nicklockwood - thanks, have that working now, ran into another issue with setting fonts that seems to be an existing bug with RCTTextField:

RCTConvert.m
RCTTextFieldManager.m

Each time you set one of the font styles it obliterates the others, (UIFont *)UIFont:(UIFont *)font withFamily:(id)family size:(id)size weight:(id)weight style:(id)style doesn't seem to behave as you would expect (ignore nil values, create new UIFont with the non-nil options applied, return it). I'll look at resolving this too.

@brentvatne
Copy link
Collaborator Author

@nicklockwood - quickly hacked this together, fixes the issue, will need to circle back later to make sure it's robust:

+ (UIFont *)UIFont:(UIFont *)font withFamily:(id)family
              size:(id)size weight:(id)weight style:(id)style
{
  // Defaults
  NSString *const RCTDefaultFontFamily = @"Helvetica Neue";
  const RCTFontWeight RCTDefaultFontWeight = UIFontWeightRegular;
  const CGFloat RCTDefaultFontSize = 14;
  CGFloat fontSize;

  // Get existing properties
  BOOL isItalic = NO;
  BOOL isCondensed = NO;
  RCTFontWeight fontWeight = RCTDefaultFontWeight;
  if (font) {
    if (!family) {
      family = font.familyName;
    }

    isItalic = RCTFontIsItalic(font);
    isCondensed = RCTFontIsCondensed(font);
  }

  // Get font style
  if (style) {
    isItalic = [self RCTFontStyle:style];
  }

  // Get font size
  if (!size && font) {
    fontSize = font.pointSize;
  } else {
    fontSize = [self CGFloat:size] ?: RCTDefaultFontSize;
  }

  // Get font family
  NSString *familyName = [self NSString:family] ?: RCTDefaultFontFamily;


  // Get font weight
  if (!weight && font) {
    fontWeight = RCTWeightOfFont(font);
  } else {
    fontWeight = [self RCTFontWeight:weight];
  }

  if ([UIFont fontNamesForFamilyName:familyName].count == 0) {
    font = [UIFont fontWithName:familyName size:fontSize];
    if (font) {
      // It's actually a font name, not a font family name,
      // but we'll do what was meant, not what was said.
      familyName = font.familyName;
      NSDictionary *traits = [font.fontDescriptor objectForKey:UIFontDescriptorTraitsAttribute];
      fontWeight = [traits[UIFontWeightTrait] doubleValue];
    } else {
      // Not a valid font or family
      RCTLogError(@"Unrecognized font family '%@'", familyName);
      familyName = RCTDefaultFontFamily;
    }
  }

  // Get closest match
  UIFont *bestMatch = [UIFont fontWithName:font.fontName size: fontSize];
  CGFloat closestWeight;
  if (font && [font.familyName isEqualToString: familyName]) {
    closestWeight = RCTWeightOfFont(font);
  } else {
    closestWeight = INFINITY;
  }
  for (NSString *name in [UIFont fontNamesForFamilyName:familyName]) {
    UIFont *match = [UIFont fontWithName:name size:fontSize];
    if (isItalic == RCTFontIsItalic(match) &&
        isCondensed == RCTFontIsCondensed(match)) {
      CGFloat testWeight = RCTWeightOfFont(match);
      if (ABS(testWeight - fontWeight) < ABS(closestWeight - fontWeight)) {
        bestMatch = match;
        closestWeight = testWeight;
      }
    }
  }

  // Safety net
  if (!bestMatch) {
    RCTLogError(@"Could not find font with family: '%@', size: %@, \
                weight: %@, style: %@", family, size, weight, style);
    bestMatch = [UIFont fontWithName:[[UIFont fontNamesForFamilyName:familyName] firstObject]
                                size:fontSize];
  }

  return bestMatch;
}

This fixes a bug with the regular RCTTextField where multiple font attributes can't be set as well.

@nicklockwood
Copy link
Contributor

Yes, that is indeed how the UIFont stuff is supposed to work. If you can provide some examples of it not working, I'll dig in and see if I can figure out the problem.

@brentvatne
Copy link
Collaborator Author

@nicklockwood - try setting fontWeight: bold and fontFamily: 'Superclarendon' on a TextInput styles, only one will work at a time

@nicklockwood
Copy link
Contributor

Hah, you were too quick for me ;-)

@brentvatne
Copy link
Collaborator Author

😄

@brentvatne
Copy link
Collaborator Author

Heading out for a run, will finish this off and push code up later tonight! Edit: Actually, tomorrow 😄 Edit Edit: Actually, Monday 😄

@brentvatne
Copy link
Collaborator Author

Possible API for more placeholder customization - probably for a separate diff but just documenting here: #1011 (comment)

@josebalius
Copy link

Looking for to this getting merged guys! Great job.

Brent Vatne added 2 commits April 27, 2015 19:48
- Change to put UITextView inside of RCTView to be able to set padding
- Fix issues with setting fonts - RCTConvert would lose properties along
  the way.
@brentvatne
Copy link
Collaborator Author

@nicklockwood - Changes up now, seems to be working well!

I'm curious - I didn't notice tests for RCTConvert, do you think it's worth setting that up here to test the changes I made to it? If so, any chance you could pull the branch and add some scaffolding for the tests so I can be sure I'm following your preferred style?

Brent Vatne added 3 commits April 27, 2015 21:42
- Use UIScrollView instead of UILabel for placeholder to ensure that
  placement of the placeholder text is identical to the underlying text
@nicklockwood
Copy link
Contributor

It looks like the <Text> example is totally broken - I think the changes to +[RCTConvert UIFontXX] may need some more work :-)

@brentvatne
Copy link
Collaborator Author

@nicklockwood - woops, I'll check that out, thanks

@brentvatne
Copy link
Collaborator Author

@nicklockwood - I've reverted those changes, I think they are better suited for their own diff as they have implications beyond multiline text.

@brentvatne
Copy link
Collaborator Author

@nicklockwood - Scratch that, I fixed the issue and added an example that includes a styled multiline text input. Everything in the example app is working fine now with that fix! Still some work to go to get all props supported though.

@brentvatne
Copy link
Collaborator Author

@nicklockwood - finished adding support for remaining attributes, ready for review again 😄

@nicklockwood
Copy link
Contributor

This is great, thanks! I've merged it internally.

yanxi123-com added a commit to yanxi123-com/tcomb-form-native that referenced this pull request May 1, 2015
multiline issue is fixed by facebook/react-native#991 . Fix the bit issue after using the latest react-native.
@lukasredev
Copy link

I just compared the implementation of RCTTextView with RCTText. Is there a reson why the TextView functionality is completely implemented in RCTTextView and not separated into a RCTTextView and RCTShadowTextView class?

I'am asking because I think I'd be nice to have the RCTTextView automatically measure it's height like implemented in the RCTShadowText object. I forked the repository and moved most of the code from RCTTextView into a RCTShadowTextView class and implemented the RCTMeasure function.

What do you think about this approach?

@nicklockwood
Copy link
Contributor

@lukasreichart UITextView isn't thread safe, so creating and setting one up on the shadow thread might be problematic.

It would be good if it was possible to calculate the height though. Maybe it can be done by emulating the TextView layout using an NSTextContainer in the shadow view?

@ide
Copy link
Contributor

ide commented May 6, 2015

@lukasreichart that sounds really good in that the default height (0px) is useless at the moment.

Following the line of thinking in @nicklockwood's suggestion, you might want to take a look at AsyncDisplayKit's ASTextNode/ASEditableTextNode to see how they support background-thread layout and also other features like text selection. It may make sense to jettison UITextView and use a reimplemented version altogether, which might sound crazy but ASEditableTextNode did it on top of TextKit.

@ide
Copy link
Contributor

ide commented May 6, 2015

setting one up on the shadow thread might be problematic.

Yeah - if my memory serves, UITextView is indeed a UIKit class that needs to do its layout on the main thread.

@nicklockwood
Copy link
Contributor

@ide "which might sound crazy" - yep, sounds pretty crazy :-)

At least as a first step, I think given the padding, font and text, it should be possible to calculate the correct size using the NSString size functions, or an NSTextContainer. It might not be pixel perfect, but it would be a hell of a lot simpler than re-implementing all the editing functions.

It would certainly be a cool project to try to write UITextView from scratch, but apart from anything else, we're trying to keep the total code size of the library down by re-using standard UIKit controls as much as possible. If we're going to build completely bespoke stuff, it's preferable to do it in JS so it's cross-platform.

@lukasredev
Copy link

@nicklockwood Thanks for clearing this up.
Your approach sounds reasonable to me. I'll try it and let you know as soon as I got something to work.

@ide 👍 Thanks for the input, I'll check it out. ( lots of work )

@lukasredev
Copy link

@nicklockwood Quick question: What's the best practise to send an event from a RCTTextView instance to it's counter part on the shadow thread?
I need to update the shadow object, when the text of the UITextView changes ...

@nicklockwood
Copy link
Contributor

@lucasreichart Interesting... I don't think we use that pattern anywhere. There's two problems - the first is getting a reference to the shadow queue, and the second is getting a reference to the shadowView. The former can be done with:

dispatch_async(self.bridge.uiManager.methodQueue, ^{ ... });

The latter is a bit tricker. Do you need that information to be updated live as the user types, or is it sufficient to get it whenever layout is performed?

@lukasredev
Copy link

Situation: If the user is typing to the TextView the RCTTextView calculates if it's necessary to update the layout. When the layout is updated the fillCSSNode of the RCTShadowTextView is called, which sets the measure function for the css node.
In the measure function I need to be able to access the current text displayed by the UITextView to calculate the height.

You're probably right I don't need live updates, but I need to access the text attribute of the UITextView whenever layout is performed.

@nicklockwood
Copy link
Contributor

TextView should probably be a managed component, so that a re-render clears the text unless the text state has been updated. If that was the case, you could ensure that the shadowView always had the correct text value simply by adding RCT_EXPORT_SHADOW_PROPERTY(text, NSString) to the RCTViewManager.

@vjeux am I understanding the concept of managed components correctly here?

@vjeux
Copy link
Contributor

vjeux commented May 6, 2015

We call them controlled component and yes they do what you described.

The tricky part with react native is because of the asynchronous nature having them controlled is harder to implement correctly. Cc @sahrens who did most of the work there

@HananeAlSamrout
Copy link

hello, please fix also the issue of multiline with textAlign, try textAlign right with multiline true, it is not working for the 'placeHolder' , but for the text there is no problem.

acoates-ms added a commit to acoates-ms/react-native that referenced this pull request Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants