Skip to content

Commit

Permalink
BREAKING: Better TextInput: contentSize property was removed from `…
Browse files Browse the repository at this point in the history
…<TextInput>.onChange` event (Second attempt)

Summary:
`contentSize` was removed from both iOS and Android, tests was updated.
USE `onContentSizeChange` INSTEAD.

Why?
 * It always was a hack;
 * We already have dedicated event for it: `onContentSizeChange`;
 * `onChange` has nothing to do with layout actually;
 * We have to maintain `onChange` handler as fast and simple as possible, this feature complicates it a lot;
 * It was undocumented feature;
 * We already have native auto-expandable <TextInput>, so it illuminates 99% current use cases of this feature.

Reviewed By: mmmulani

Differential Revision: D4989881

fbshipit-source-id: 674bb98c89ada1fca7b3b20b304736b2a3b8304e
  • Loading branch information
shergin authored and facebook-github-bot committed May 29, 2017
1 parent 9fae268 commit bac84ce
Show file tree
Hide file tree
Showing 3 changed files with 0 additions and 47 deletions.
21 changes: 0 additions & 21 deletions Libraries/Text/RCTTextView.m
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@ @implementation RCTTextView
NSAttributedString *_pendingAttributedText;

UITextRange *_previousSelectionRange;
NSUInteger _previousTextLength;
CGFloat _previousContentHeight;
NSString *_predictedText;

BOOL _blockTextShouldChange;
Expand Down Expand Up @@ -490,31 +488,12 @@ - (void)textViewDidChange:(UITextView *)textView
_nativeUpdatesInFlight = NO;
_nativeEventCount++;

// TODO: t16435709 This part will be removed soon.
if (!self.reactTag || !_onChange) {
return;
}

// When the context size increases, iOS updates the contentSize twice; once
// with a lower height, then again with the correct height. To prevent a
// spurious event from being sent, we track the previous, and only send the
// update event if it matches our expectation that greater text length
// should result in increased height. This assumption is, of course, not
// necessarily true because shorter text might include more linebreaks, but
// in practice this works well enough.
NSUInteger textLength = textView.text.length;
CGFloat contentHeight = textView.contentSize.height;
if (textLength >= _previousTextLength) {
contentHeight = MAX(contentHeight, _previousContentHeight);
}
_previousTextLength = textLength;
_previousContentHeight = contentHeight;
_onChange(@{
@"text": self.text,
@"contentSize": @{
@"height": @(contentHeight),
@"width": @(textView.contentSize.width)
},
@"target": self.reactTag,
@"eventCount": @(_nativeEventCount),
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,20 +23,14 @@ public class ReactTextChangedEvent extends Event<ReactTextChangedEvent> {
public static final String EVENT_NAME = "topChange";

private String mText;
private float mContentWidth;
private float mContentHeight;
private int mEventCount;

public ReactTextChangedEvent(
int viewId,
String text,
float contentSizeWidth,
float contentSizeHeight,
int eventCount) {
super(viewId);
mText = text;
mContentWidth = contentSizeWidth;
mContentHeight = contentSizeHeight;
mEventCount = eventCount;
}

Expand All @@ -53,13 +47,7 @@ public void dispatch(RCTEventEmitter rctEventEmitter) {
private WritableMap serializeEventData() {
WritableMap eventData = Arguments.createMap();
eventData.putString("text", mText);

WritableMap contentSize = Arguments.createMap();
contentSize.putDouble("width", mContentWidth);
contentSize.putDouble("height", mContentHeight);
eventData.putMap("contentSize", contentSize);
eventData.putInt("eventCount", mEventCount);

eventData.putInt("target", getViewTag());
return eventData;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -672,26 +672,12 @@ public void onTextChanged(CharSequence s, int start, int before, int count) {
return;
}

// TODO: remove contentSize from onTextChanged entirely now that onChangeContentSize exists?
int contentWidth = mEditText.getWidth();
int contentHeight = mEditText.getHeight();

// Use instead size of text content within EditText when available
if (mEditText.getLayout() != null) {
contentWidth = mEditText.getCompoundPaddingLeft() + mEditText.getLayout().getWidth() +
mEditText.getCompoundPaddingRight();
contentHeight = mEditText.getCompoundPaddingTop() + mEditText.getLayout().getHeight() +
mEditText.getCompoundPaddingBottom();
}

// The event that contains the event counter and updates it must be sent first.
// TODO: t7936714 merge these events
mEventDispatcher.dispatchEvent(
new ReactTextChangedEvent(
mEditText.getId(),
s.toString(),
PixelUtil.toDIPFromPixel(contentWidth),
PixelUtil.toDIPFromPixel(contentHeight),
mEditText.incrementAndGetEventCounter()));

mEventDispatcher.dispatchEvent(
Expand Down

11 comments on commit bac84ce

@Guardiannw
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be missing something, but as far as I can find, there isn't a native auto-expanding feature out for android yet. Am I wrong on this? I would like to know if there is or isn't because this was one of the premises for this PR

@Palisand
Copy link

Choose a reason for hiding this comment

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

@shergin and @mmmulani I too would like to know where this "native auto-expandable <TextInput>" resides. Usage of onChange in order to expand an input was detailed here. There were some problems with this approach (mentioned in the comments), including a very annoying flicker whenever the size of the input was expanded. Has this commit resolved those issues? Please don't leave us in the dark here.

@shergin
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Guardiannw @Palisand Unfortunately, auto-expanding feature works only on iOS for now. We plan to implement this for Android as well, but we don't have time estimate though. (PR is always welcome. 😄)
You still can use onContentSize instead of onChange to implement auto-expanding feature on JS size.

@Palisand
Copy link

@Palisand Palisand commented on bac84ce Jul 22, 2017

Choose a reason for hiding this comment

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

@shergin Thanks for getting back. Now out at the risk of sounding stupid I must ask, how can I access the iOS auto-expanding feature? I have already restored my "custom" auto-expand with the onChange -> onContentSizeChange change, but I still get a flicker (more pronounced in iOS).

@shergin
Copy link
Contributor Author

@shergin shergin commented on bac84ce Jul 22, 2017

Choose a reason for hiding this comment

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

@Palisand Thank you to cc'ed my by @-mentioned my username. 😄
Auto-expanding? It just works.

<TextInput multiline={true} style={{width: 300, minHeight: 30, maxHeight: 200}} />

(I recently fixed one badly issue in this feature, so please test on master.)

@Palisand
Copy link

@Palisand Palisand commented on bac84ce Jul 24, 2017

Choose a reason for hiding this comment

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

@shergin I'm at my wits' end and seeing as how you have made several contributions toward a better TextInput, I'm hoping you can shed some light on how to get the screen position of the cursor within a TextInput. Having the y position of the cursor exposed would allow one to scroll to the proper line when it is completely or partially out of view and to do so regardless of where it is located within the input's text.

@shergin
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Palisand I plan to implement exact same feature natively (at least for iOS, for now). (I already fixed a issue with auto scrolling to just focused <TextInput>.) AFAIK, RN does not expose such kind of API.

@dantman
Copy link
Contributor

@dantman dantman commented on bac84ce Sep 5, 2017

Choose a reason for hiding this comment

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

What version was auto-expanding input added for each platform?

@shergin
Copy link
Contributor Author

@shergin shergin commented on bac84ce Sep 5, 2017

Choose a reason for hiding this comment

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

@dantman It is available only on iOS now. I hope we will have Android implementation is master this month.

@dantman
Copy link
Contributor

@dantman dantman commented on bac84ce Sep 5, 2017

Choose a reason for hiding this comment

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

@shergin Ok, can you tell me which RN release it was added for iOS. I should be able to branch code by version. I've got a library where I implement custom auto-growing input into the text field. If this change ends up changing the behaviour of that / or it's more optimum to just use min/max height were available I might want to switch to that when run in an environment it's available.

@shergin
Copy link
Contributor Author

@shergin shergin commented on bac84ce Sep 6, 2017

Choose a reason for hiding this comment

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

@dantman You can figure out exact version looking at the git history of RCTText library. I don't have any idea about the OSS versioning, I am sorry.

Please sign in to comment.