[TextInput] Implements `onKeyPress` #2082

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
@dsibiski
Contributor

dsibiski commented Jul 21, 2015

  • When a key is pressed, it's key value is passed as an argument to the callback handler.
  • Example
 _handleKeyPress: function(e) {
      console.log(e.nativeEvent.key);
  },

  render: function() {
    return (
      <View style={styles.container}>
        <TextInput
            style={{width: 150, height: 25, borderWidth: 0.5}}
            onKeyPress={this._handleKeyPress}
        />
        <TextInput
            style={{width: 150, height: 100, borderWidth: 0.5}}
            onKeyPress={this._handleKeyPress}
            multiline={true}
        />
      </View>
    );
  }
  • Implements shouldChangeCharactersInRange, keyboardInputShouldDelete and deleteBackward for RCTTextField
    • keyboardInputShouldDelete and deleteBackward were implemented because UITextField doesn't trigger shouldChangeCharactersInRange when the field is empty, preventing us from having a true keyPress event.
    • Unfortunately, it looks like keyboardInputShouldDelete is not documented as alluded to in this StackOverflow answer. However, I'm only using it because of a bug in iOS 8.0 to 8.2 where the deleteBackward method doesn't trigger. This might cause this whole feature to be a no-op, though. :(
  • Implements shouldChangeTextInRange for RCTTextView
    • This one is nice and simple since UITextViews (used for multi-line TextInputs) trigger this method even when the textView is empty.
  • Addresses issue: #1882
@sahrens

This comment has been minimized.

Show comment
Hide comment
@sahrens

sahrens Jul 22, 2015

Contributor

Nice work! You're going to have to rebase some conflicts once my diffs sync out though.

Contributor

sahrens commented Jul 22, 2015

Nice work! You're going to have to rebase some conflicts once my diffs sync out though.

@dsibiski

This comment has been minimized.

Show comment
Hide comment
@dsibiski

dsibiski Jul 22, 2015

Contributor

@sahrens Not a problem. :)

Contributor

dsibiski commented Jul 22, 2015

@sahrens Not a problem. :)

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Jul 22, 2015

Collaborator

😍 awesome @dsibiski

Collaborator

brentvatne commented Jul 22, 2015

😍 awesome @dsibiski

@dsibiski

This comment has been minimized.

Show comment
Hide comment
@dsibiski

dsibiski Jul 22, 2015

Contributor

I will need to implement an example for the UIExplorer, but I wanted to get some opinions on what's happening here (and the changes they are doing internally at FB) before knocking that out.

Contributor

dsibiski commented Jul 22, 2015

I will need to implement an example for the UIExplorer, but I wanted to get some opinions on what's happening here (and the changes they are doing internally at FB) before knocking that out.

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Jul 22, 2015

Collaborator

@dsibiski - in your example I see:

 _handleKeyPress: function(e) {
      console.log(e.nativeEvent.text);
  },

imo this should be e.nativeEvent.key. Also, is it deterministic which order onTextChange and onKeyPress fire? If so, we should document (I imagine onKeyPress comes first)

Collaborator

brentvatne commented Jul 22, 2015

@dsibiski - in your example I see:

 _handleKeyPress: function(e) {
      console.log(e.nativeEvent.text);
  },

imo this should be e.nativeEvent.key. Also, is it deterministic which order onTextChange and onKeyPress fire? If so, we should document (I imagine onKeyPress comes first)

@dsibiski

This comment has been minimized.

Show comment
Hide comment
@dsibiski

dsibiski Jul 22, 2015

Contributor

@brentvatne I had piggy-backed off of the sendTextEventWithType:reactTag:text method that was already there in RCTEventDispatcher (it was easier hah!) However, I think you're right, we should have a different return value, instead of "text". I'll perhaps implement sendTextEventWithType:reactTag:key and return the key or eventually a dictionary with other values as well (like ASCII code).

As for the firing order, in my testing, I noticed that onKeyPress fires before onTextChange, but I didn't test it specifically.

Contributor

dsibiski commented Jul 22, 2015

@brentvatne I had piggy-backed off of the sendTextEventWithType:reactTag:text method that was already there in RCTEventDispatcher (it was easier hah!) However, I think you're right, we should have a different return value, instead of "text". I'll perhaps implement sendTextEventWithType:reactTag:key and return the key or eventually a dictionary with other values as well (like ASCII code).

As for the firing order, in my testing, I noticed that onKeyPress fires before onTextChange, but I didn't test it specifically.

@dsibiski

This comment has been minimized.

Show comment
Hide comment
@dsibiski

dsibiski Jul 23, 2015

Contributor

Ok, I rebased @sahrens's recent changes. I made keyPress still fire even after the maxLength threshold has been reached. Which I think is how a normal text input works on the web.

Now, I just need to implement @brentvatne's suggestion for the nativeEvent & a UIExplorer example.

Contributor

dsibiski commented Jul 23, 2015

Ok, I rebased @sahrens's recent changes. I made keyPress still fire even after the maxLength threshold has been reached. Which I think is how a normal text input works on the web.

Now, I just need to implement @brentvatne's suggestion for the nativeEvent & a UIExplorer example.

@dsibiski

This comment has been minimized.

Show comment
Hide comment
@dsibiski

dsibiski Jul 24, 2015

Contributor

nativeEvent.key now used instead of nativeEvent.text. I also added an example to the TextEventsExample in UIExplorer.

/cc @brentvatne @sahrens

Contributor

dsibiski commented Jul 24, 2015

nativeEvent.key now used instead of nativeEvent.text. I also added an example to the TextEventsExample in UIExplorer.

/cc @brentvatne @sahrens

Libraries/Text/RCTTextView.m
+{
+ NSString *keyValue = kBackspaceKeyValue;
+
+ BOOL keyNotBackspace = ![text isEqualToString:@""];

This comment has been minimized.

@sahrens

sahrens Jul 25, 2015

Contributor

can you consolidate this with the enter logic into one if else chain?

@sahrens

sahrens Jul 25, 2015

Contributor

can you consolidate this with the enter logic into one if else chain?

Libraries/Text/RCTTextView.m
@@ -127,6 +128,7 @@ - (NSString *)text
- (BOOL)textView:(UITextView *)textView shouldChangeTextInRange:(NSRange)range replacementText:(NSString *)text
{
+ [self sendKeyValueForText:text];

This comment has been minimized.

@sahrens

sahrens Jul 25, 2015

Contributor

This does weird things with paste and auto-correct type stuff, sending full strings of characters.

@sahrens

sahrens Jul 25, 2015

Contributor

This does weird things with paste and auto-correct type stuff, sending full strings of characters.

Libraries/Text/RCTTextField.m
+ BOOL keyNotBackspace = ![string isEqualToString:@""];
+
+ if (keyNotBackspace) {
+ keyValue = string;

This comment has been minimized.

@sahrens

sahrens Jul 25, 2015

Contributor

Same comment - one if else with this assignment in the final else block.

@sahrens

sahrens Jul 25, 2015

Contributor

Same comment - one if else with this assignment in the final else block.

+ */
+
+extern NSString *const kBackspaceKeyValue;
+extern NSString *const kEnterKeyValue;

This comment has been minimized.

@sahrens

sahrens Jul 25, 2015

Contributor

newline?

@sahrens

sahrens Jul 25, 2015

Contributor

newline?

Libraries/Text/RCTTextField.m
+ return NO;
+ }
+
+ [self sendKeyValueForString:string];

This comment has been minimized.

@sahrens

sahrens Jul 25, 2015

Contributor

This also does weird things with paste.

@sahrens

sahrens Jul 25, 2015

Contributor

This also does weird things with paste.

Libraries/Text/RCTTextField.m
@@ -42,6 +43,14 @@ - (instancetype)initWithEventDispatcher:(RCTEventDispatcher *)eventDispatcher
- (BOOL)textField:(UITextField *)textField shouldChangeCharactersInRange:(NSRange)range replacementString:(NSString *)string
{
+ // If the range length is 1, it signifies a backspace key.
+ // We should ignore them here and let the `deleteBackward` method handle all backspace keys.
+ if (range.length == 1) {

This comment has been minimized.

@sahrens

sahrens Jul 25, 2015

Contributor

Won't this also detect pasting over single character selections? Will that cause weird things to happen?

@sahrens

sahrens Jul 25, 2015

Contributor

Won't this also detect pasting over single character selections? Will that cause weird things to happen?

Examples/UIExplorer/TextInputExample.js
@@ -73,6 +73,9 @@ var TextEventsExample = React.createClass({
onSubmitEditing={(event) => this.updateText(
'onSubmitEditing text: ' + event.nativeEvent.text
)}
+ onKeyPress={(event) => this.updateText(

This comment has been minimized.

@sahrens

sahrens Jul 25, 2015

Contributor

Can you also add a prev3 since more events are firing now?

@sahrens

sahrens Jul 25, 2015

Contributor

Can you also add a prev3 since more events are firing now?

@sahrens sahrens assigned nicklockwood and unassigned sahrens Jul 25, 2015

@sahrens

This comment has been minimized.

Show comment
Hide comment
@sahrens

sahrens Jul 25, 2015

Contributor

I'm going on PTO - nick, can you help out with this one?

Contributor

sahrens commented Jul 25, 2015

I'm going on PTO - nick, can you help out with this one?

@dsibiski

This comment has been minimized.

Show comment
Hide comment
@dsibiski

dsibiski Jul 28, 2015

Contributor

Build failed because of: #2134

Contributor

dsibiski commented Jul 28, 2015

Build failed because of: #2134

@dsibiski

This comment has been minimized.

Show comment
Hide comment
@dsibiski

dsibiski Aug 6, 2015

Contributor

@sahrens @nicklockwood What do you guys think about this? Just rebased the latest master to get rid of the conflicts...

Contributor

dsibiski commented Aug 6, 2015

@sahrens @nicklockwood What do you guys think about this? Just rebased the latest master to get rid of the conflicts...

+ * of patent rights can be found in the PATENTS file in the same directory.
+ */
+
+extern NSString *const kNewlineRawValue;

This comment has been minimized.

@nicklockwood

nicklockwood Aug 6, 2015

Contributor

Please use RCT prefix for constants, and don't prefix with k.

Also, I'm not sure what the point of having these constants is, vs sending the actual ascii values of those keys?

@nicklockwood

nicklockwood Aug 6, 2015

Contributor

Please use RCT prefix for constants, and don't prefix with k.

Also, I'm not sure what the point of having these constants is, vs sending the actual ascii values of those keys?

This comment has been minimized.

@dsibiski

dsibiski Aug 6, 2015

Contributor

@nicklockwood I made them constants mainly because we have to use them in both RCTTextField & RCTTextView, I didn't want that duplication. Also I added them because I thought they were more explicit that their string values. I'm not married to this though and can remove them if preferred.

@dsibiski

dsibiski Aug 6, 2015

Contributor

@nicklockwood I made them constants mainly because we have to use them in both RCTTextField & RCTTextView, I didn't want that duplication. Also I added them because I thought they were more explicit that their string values. I'm not married to this though and can remove them if preferred.

+ * End result: Single character pastes will register and fire a keyPress event with the character
+ * that was pasted.
+ */
+ if (text.length <= 1) {

This comment has been minimized.

@nicklockwood

nicklockwood Aug 6, 2015

Contributor

According to http://stackoverflow.com/questions/19045484/how-to-know-when-paste-event-triggered-in-uitextview it does work, but you need to set the delegate of the TextView or it won't fire.

The delegate is currently set to itself, which may be why it isn't working. @tadeuzagallo has just changed this though because it was causing other issues, so it may work once his fix has landed.

@nicklockwood

nicklockwood Aug 6, 2015

Contributor

According to http://stackoverflow.com/questions/19045484/how-to-know-when-paste-event-triggered-in-uitextview it does work, but you need to set the delegate of the TextView or it won't fire.

The delegate is currently set to itself, which may be why it isn't working. @tadeuzagallo has just changed this though because it was causing other issues, so it may work once his fix has landed.

This comment has been minimized.

@dsibiski

dsibiski Aug 6, 2015

Contributor

Ah nice! This was definitely the portion of this feature that I felt bad about not working correctly. Hopefully it works out with his fixes. Thanks.

@dsibiski

dsibiski Aug 6, 2015

Contributor

Ah nice! This was definitely the portion of this feature that I felt bad about not working correctly. Hopefully it works out with his fixes. Thanks.

@dsibiski

This comment has been minimized.

Show comment
Hide comment
@dsibiski

dsibiski Aug 11, 2015

Contributor

Note to self: Once this commit (vjeux@48548fe) lands, I'll need to move most (or all) of this into the RCTTextFieldManager where shouldChangeCharactersInRange was moved.

Contributor

dsibiski commented Aug 11, 2015

Note to self: Once this commit (vjeux@48548fe) lands, I'll need to move most (or all) of this into the RCTTextFieldManager where shouldChangeCharactersInRange was moved.

@dsibiski

This comment has been minimized.

Show comment
Hide comment
@dsibiski

dsibiski Aug 26, 2015

Contributor

@sahrens @nicklockwood I rebased the current changes and was able to remove some of the 'backspace' press detection hacks. Can you guys check this out when you get a chance?

I think that the "paste" detecting is a little wonky, let me know what you think.

Contributor

dsibiski commented Aug 26, 2015

@sahrens @nicklockwood I rebased the current changes and was able to remove some of the 'backspace' press detection hacks. Can you guys check this out when you get a chance?

I think that the "paste" detecting is a little wonky, let me know what you think.

@GantMan

This comment has been minimized.

Show comment
Hide comment
@GantMan

GantMan Oct 27, 2015

Contributor

Checking in - Is this happening?
I'd love to be able to detect when the return key is pressed for iOS.

Contributor

GantMan commented Oct 27, 2015

Checking in - Is this happening?
I'd love to be able to detect when the return key is pressed for iOS.

@dsibiski

This comment has been minimized.

Show comment
Hide comment
@dsibiski

dsibiski Oct 27, 2015

Contributor

@nicklockwood @sahrens Hey guys, do you guys want me to add Android support to this as well? I imagine that might be the reason for the hold up...

Contributor

dsibiski commented Oct 27, 2015

@nicklockwood @sahrens Hey guys, do you guys want me to add Android support to this as well? I imagine that might be the reason for the hold up...

React/Base/RCTEventDispatcher.m
@"eventCount": @(eventCount),
@"target": reactTag
}];
+
+ if (text) {
+ [body addEntriesFromDictionary:@{@"text": text}];

This comment has been minimized.

@nicklockwood

nicklockwood Oct 27, 2015

Contributor

This can just be:

if (text) {
  body[@"text"] = text;
}
@nicklockwood

nicklockwood Oct 27, 2015

Contributor

This can just be:

if (text) {
  body[@"text"] = text;
}
React/Base/RCTEventDispatcher.m
+ }
+
+ if (key) {
+ [body addEntriesFromDictionary:@{@"key": key}];

This comment has been minimized.

@nicklockwood

nicklockwood Oct 27, 2015

Contributor

As above

@nicklockwood

nicklockwood Oct 27, 2015

Contributor

As above

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Oct 27, 2015

Contributor

Android support would be great, but it's OK to just mark the prop with @platform ios for now.

Contributor

nicklockwood commented Oct 27, 2015

Android support would be great, but it's OK to just mark the prop with @platform ios for now.

+extern NSString *const RCTNewlineRawValue;
+
+extern NSString *const RCTBackspaceKeyValue;
+extern NSString *const RCTEnterKeyValue;

This comment has been minimized.

@nicklockwood

nicklockwood Oct 27, 2015

Contributor

Files should end with a blank line.

@nicklockwood

nicklockwood Oct 27, 2015

Contributor

Files should end with a blank line.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Oct 28, 2015

@dsibiski updated the pull request.

@dsibiski updated the pull request.

@dsibiski

This comment has been minimized.

Show comment
Hide comment
@dsibiski

dsibiski Oct 28, 2015

Contributor

Hmm, somehow the newline at the end of the file didn't come through...it seems to still be there in Xcode... @nicklockwood Do you know if that is something with git?

(Edit) Perhaps it's there and just GitHub doesn't see it...

Contributor

dsibiski commented Oct 28, 2015

Hmm, somehow the newline at the end of the file didn't come through...it seems to still be there in Xcode... @nicklockwood Do you know if that is something with git?

(Edit) Perhaps it's there and just GitHub doesn't see it...

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
Contributor

nicklockwood commented Oct 30, 2015

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@dsibiski

This comment has been minimized.

Show comment
Hide comment
@dsibiski

dsibiski Oct 30, 2015

Contributor

Oops, I better squash my commits...

Contributor

dsibiski commented Oct 30, 2015

Oops, I better squash my commits...

[TextInput] Implements `onKeyPress`
- When a key is pressed, it's `key value` is passed as an argument to the callback handler

@ghost ghost closed this in 6c7c845 Nov 2, 2015

MattFoley added a commit to skillz/react-native that referenced this pull request Nov 9, 2015

Implements `onKeyPress`
Summary: - When a key is pressed, it's `key value` is passed as an argument to the callback handler.
 - For `Enter` and `Backspace` keys, I'm using their `key value` as defined [here](https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key#Key_values). As per JonasJonny & brentvatne's [suggestion](facebook#1882 (comment)).

- Example
```javascript
 _handleKeyPress: function(e) {
      console.log(e.nativeEvent.key);
  },

  render: function() {
    return (
      <View style={styles.container}>
        <TextInput
            style={{width: 150, height: 25, borderWidth: 0.5}}
            onKeyPress={this._handleKeyPress}
        />
        <TextInput
            style={{width: 150, height: 100, borderWidth: 0.5}}
            onKeyPress={this._handleKeyPress}
            multiline={true}
        />
      </View>
    );
  }
```
- Implements [shouldChangeCharactersInRange](https://developer.apple.com/library/prerelease/ios/documentat
Closes facebook#2082

Reviewed By: javache

Differential Revision: D2280460

Pulled By: nicklockwood

fb-gh-sync-id: 1f824f80649043dc2520c089e2531d428d799405

calvinf pushed a commit to calvinf/react-native that referenced this pull request Nov 18, 2015

Implements `onKeyPress`
Summary: - When a key is pressed, it's `key value` is passed as an argument to the callback handler.
 - For `Enter` and `Backspace` keys, I'm using their `key value` as defined [here](https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key#Key_values). As per JonasJonny & brentvatne's [suggestion](facebook#1882 (comment)).

- Example
```javascript
 _handleKeyPress: function(e) {
      console.log(e.nativeEvent.key);
  },

  render: function() {
    return (
      <View style={styles.container}>
        <TextInput
            style={{width: 150, height: 25, borderWidth: 0.5}}
            onKeyPress={this._handleKeyPress}
        />
        <TextInput
            style={{width: 150, height: 100, borderWidth: 0.5}}
            onKeyPress={this._handleKeyPress}
            multiline={true}
        />
      </View>
    );
  }
```
- Implements [shouldChangeCharactersInRange](https://developer.apple.com/library/prerelease/ios/documentat
Closes facebook#2082

Reviewed By: javache

Differential Revision: D2280460

Pulled By: nicklockwood

fb-gh-sync-id: 1f824f80649043dc2520c089e2531d428d799405

Crash-- added a commit to Crash--/react-native that referenced this pull request Dec 24, 2015

Implements `onKeyPress`
Summary: - When a key is pressed, it's `key value` is passed as an argument to the callback handler.
 - For `Enter` and `Backspace` keys, I'm using their `key value` as defined [here](https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key#Key_values). As per JonasJonny & brentvatne's [suggestion](facebook#1882 (comment)).

- Example
```javascript
 _handleKeyPress: function(e) {
      console.log(e.nativeEvent.key);
  },

  render: function() {
    return (
      <View style={styles.container}>
        <TextInput
            style={{width: 150, height: 25, borderWidth: 0.5}}
            onKeyPress={this._handleKeyPress}
        />
        <TextInput
            style={{width: 150, height: 100, borderWidth: 0.5}}
            onKeyPress={this._handleKeyPress}
            multiline={true}
        />
      </View>
    );
  }
```
- Implements [shouldChangeCharactersInRange](https://developer.apple.com/library/prerelease/ios/documentat
Closes facebook#2082

Reviewed By: javache

Differential Revision: D2280460

Pulled By: nicklockwood

fb-gh-sync-id: 1f824f80649043dc2520c089e2531d428d799405
@matc4

This comment has been minimized.

Show comment
Hide comment
@matc4

matc4 Jan 25, 2016

Any plan to have onKeyPress event for Android?

matc4 commented Jan 25, 2016

Any plan to have onKeyPress event for Android?

@ajwhite

This comment has been minimized.

Show comment
Hide comment
@ajwhite

ajwhite Feb 16, 2016

Contributor

Probably a feature someone will have to contribute @matiascba. I'm currently in need, but I haven't scoped out what it looks like under the hood for Android. If it's already set up similarly, it shouldn't be too bad.

Contributor

ajwhite commented Feb 16, 2016

Probably a feature someone will have to contribute @matiascba. I'm currently in need, but I haven't scoped out what it looks like under the hood for Android. If it's already set up similarly, it shouldn't be too bad.

@mpretty-cyro

This comment has been minimized.

Show comment
Hide comment
@mpretty-cyro

mpretty-cyro Feb 22, 2016

Contributor

+1 for onKeyPress in Android

Contributor

mpretty-cyro commented Feb 22, 2016

+1 for onKeyPress in Android

@ajwhite

This comment has been minimized.

Show comment
Hide comment
@ajwhite

ajwhite Feb 23, 2016

Contributor

Working on a fork at the moment, if I finish up I'll refer from here

Contributor

ajwhite commented Feb 23, 2016

Working on a fork at the moment, if I finish up I'll refer from here

@mpretty-cyro

This comment has been minimized.

Show comment
Hide comment
@mpretty-cyro

mpretty-cyro Feb 23, 2016

Contributor

@ajwhite Awesome, thanks for the update

Contributor

mpretty-cyro commented Feb 23, 2016

@ajwhite Awesome, thanks for the update

@jgibbons

This comment has been minimized.

Show comment
Hide comment
@jgibbons

jgibbons Mar 1, 2016

Contributor

@ajwhite Any updates on this or work you've done to date?

Contributor

jgibbons commented Mar 1, 2016

@ajwhite Any updates on this or work you've done to date?

@ajwhite

This comment has been minimized.

Show comment
Hide comment
@ajwhite

ajwhite Mar 1, 2016

Contributor

I haven't any updates at this time, I likely cannot visit this for another couple weeks. Busy bee at work!

Contributor

ajwhite commented Mar 1, 2016

I haven't any updates at this time, I likely cannot visit this for another couple weeks. Busy bee at work!

@PardeepK

This comment has been minimized.

Show comment
Hide comment
@PardeepK

PardeepK Jul 26, 2017

@ajwhite , any update on Android support?
or any reason why it is not there for Android, as it seems very basic requirement for an edit control.

@ajwhite , any update on Android support?
or any reason why it is not there for Android, as it seems very basic requirement for an edit control.

@ajwhite

This comment has been minimized.

Show comment
Hide comment
@ajwhite

ajwhite Jul 26, 2017

Contributor

I don't, sorry folks! It became a bit of a rabbit hole and was deprioritized in our project in lieu of a workaround.

Contributor

ajwhite commented Jul 26, 2017

I don't, sorry folks! It became a bit of a rabbit hole and was deprioritized in our project in lieu of a workaround.

This issue was closed.

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