Skip to content

Conversation

@rozele
Copy link
Contributor

@rozele rozele commented Mar 9, 2021

This change adds two new props and a new behavior for multiline TextInput.

New props:

  1. clearTextOnSubmit: similar to clearTextOnFocus, except that it clears out the TextInput whenever the onSubmitEditing event would be dispatched.
  2. submitKeyEvents: registers a set of key down events that will trigger onSubmitEditing for multiline TextInput.

New behavior:
Previously, onSubmitEditing would never fire when multiline={true} for a TextInput. Now, onSubmitEditing may fire for a multiline TextInput if a key down event matching the configuration in submitKeyEvents is observed.

Fixes #7330

Microsoft Reviewers: Open in CodeFlow

@rozele rozele requested a review from a team as a code owner March 9, 2021 18:14
@rozele
Copy link
Contributor Author

rozele commented Mar 9, 2021

I haven't added anything to TextInput.windows.js (though it works w/o declaring the prop types) and this should probably include an e2e test / update to RNTester. Will work on those once we decide if this is worth landing.

@rozele rozele force-pushed the issue7330 branch 2 times, most recently from 42f8f6e to 413b3f4 Compare March 9, 2021 18:41
@asklar
Copy link
Member

asklar commented Mar 9, 2021

@rozele thanks for sending this out! I'd love for these types of changes to be discussed at one of the design reviews. For example, is it necessary to have a property for "clear on submit" instead of just having the app do the clear on an onSubmit event handler?

@rozele
Copy link
Contributor Author

rozele commented Mar 9, 2021

@asklar The referenced issue (#7330) discusses the motivation for the prop. Happy to discuss this in the coming weeks (though I believe our next opportunity isn't until ~3/24).

Super::updateProperties(props);

// We need to re-register the PreviewKeyDown handler so it is invoked after the ShadowNodeBase handler
if (hasKeyDownEvents) {
Copy link
Contributor

@rectified95 rectified95 Apr 6, 2021

Choose a reason for hiding this comment

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

Is this necessary? I thought Subclass::UpdateProperties gets called after the ShadowNodeBase keyboard handler gets bound in the PreviewKeyboardEventHandlerOnRoot constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - this is necessary. Although the root-level keyboard event handler for emitting key events gets subscribed before any of these props get set, the view-level keyboard event handler that sets Handled(true) on the routed events gets subscribed in Super::updateProperties.

If we subscribe to PreviewKeyDown for onSubmitEditing before we subscribe to PreviewKeyDown for keyDownEvents, then we'll always call onSubmitEditing, even if the key was intended to be marked Handled by the keyDownEvents prop.

Note, this wasn't a problem before because you could just use keyDownEvents={[{code: 'Enter', handledEventPhase: Capturing}]} to mark the event as Handled in the preview phase, and the KeyDown event that was previously used for onSubmitEditing would not be called.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Then, I have a few questions:

  1. Since UpdateProperties will be called at least once (and potentially many times during the lifespan of the component), do we need still need the initial registration up in registerEvents()? Not sure how expensive these are, though..
  2. This seems to result in the re-registration of the Preview handler every time that UpdateProperties is called for a TextInput with keyDownEvents specified - can we avoid that?
  3. Just to confirm - do you need to switch from listening to KeyDown to PreviewKeyDown, so that you can stop the submission on 'Enter'?

Copy link
Contributor Author

@rozele rozele Apr 7, 2021

Choose a reason for hiding this comment

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

  1. We still need the initial registration in case keyDownEvents is never set.
  2. I don't believe it's that expensive, but it will only be re-registered any time someone changes keyDownEvents, which is likely to be infrequent. It's probably not worth the added complexity to add a state field that signals when we've already re-registered.
  3. Not exactly. We now need to handle onSubmitEditing in PreviewKeyDown as opposed to KeyDown because we want submitKeyEvents to apply even to TextBox with AcceptsReturn == true. If we used KeyDown, there would be no way to intercept 'Enter' before it created a newline character.

Copy link
Contributor

@rectified95 rectified95 Apr 16, 2021

Choose a reason for hiding this comment

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

If we subscribe to PreviewKeyDown for onSubmitEditing before we subscribe to PreviewKeyDown for keyDownEvents, then we'll always call onSubmitEditing, even if the key was intended to be marked Handled by the keyDownEvents prop.

Just for my information - this relies on the event handlers being called in the order of their registration for a given event type - is that guaranteed not to change (and documented somewhere)?

@rectified95
Copy link
Contributor

rectified95 commented Apr 7, 2021

I haven't added anything to TextInput.windows.js (though it works w/o declaring the prop types) and this should probably include an e2e test / update to RNTester. Will work on those once we decide if this is worth landing.

Can you add some examples to RNTester? Otheriwse, I think it looks great.

rozele added 4 commits April 9, 2021 11:49
This change adds two new props and a new behavior for multiline
TextInput.

New props:
1. `clearTextOnSubmit`: similar to `clearTextOnFocus`, except that it
clears out the TextInput whenever the `onSubmitEditing` event would be
dispatched.
2. `submitKeyEvents`: registers a set of key down events that will
trigger `onSubmitEditing` for multiline TextInput.

New behavior:
Previously, `onSubmitEditing` would never fire when `multiline={true}`
for a TextInput. Now, `onSubmitEditing` may fire for a multiline
TextInput if a key down event matching the configuration in
`submitKeyEvents` is observed.

Fixes microsoft#7330
@rozele
Copy link
Contributor Author

rozele commented Apr 9, 2021

@rectified95 Added an RNTester example:

playground.2021-04-09.12-30-18.mp4

@stmoy stmoy requested a review from asklar April 14, 2021 17:26
@rectified95 rectified95 merged commit 914219e into microsoft:master Apr 23, 2021
rozele added a commit to rozele/react-native-windows that referenced this pull request Oct 6, 2021
…e-windows#7333)

Summary:
This change adds two new props and a new behavior for multiline
TextInput.

New props:
1. `clearTextOnSubmit`: similar to `clearTextOnFocus`, except that it
clears out the TextInput whenever the `onSubmitEditing` event would be
dispatched.
2. `submitKeyEvents`: registers a set of key down events that will
trigger `onSubmitEditing` for multiline TextInput.

New behavior:
Previously, `onSubmitEditing` would never fire when `multiline={true}`
for a TextInput. Now, `onSubmitEditing` may fire for a multiline
TextInput if a key down event matching the configuration in
`submitKeyEvents` is observed.

Fixes microsoft#7330

See also microsoft#7333

Test Plan:
1. Clone into FBS
2. Modify ComposerEditor to leverage `onSubmitEditing` and `submitKeyEvents` instead of `keyDownEvents` when `Platform.OS === 'windows'
3. Ensure tasks below are resolved


{F481095305}


Reviewers: skyle, mylando

Reviewed By: skyle

Subscribers: eliwhite

Differential Revision: https://phabricator.intern.facebook.com/D26918559

Tasks: T84376196, T86162323, T85964399, T85628355

Signature: 26918559:1615423313:ad9d9ebe90e27919cc10b6cd4fa48041f1069006
christophpurrer pushed a commit to christophpurrer/react-native-macos that referenced this pull request Aug 11, 2022
This brings macOS parity with react-native-windows microsoft/react-native-windows#7333 for MultilineTextInput fields
See react-native-windows PR for desired feature set
We can't do it for SinglelineTextInput fields (at least not w/o hacks) as it does not support overriding the keyDown event :-(
https://stackoverflow.com/a/6076492
christophpurrer pushed a commit to christophpurrer/react-native-macos that referenced this pull request Aug 28, 2022
This brings macOS parity with react-native-windows microsoft/react-native-windows#7333 for MultilineTextInput fields
See react-native-windows PR for desired feature set.

We can't do it for SinglelineTextInput fields (at least not w/o hacks) as it does not support overriding the keyDown event :-(
https://stackoverflow.com/a/6076492
christophpurrer pushed a commit to christophpurrer/react-native-macos that referenced this pull request Aug 28, 2022
This brings macOS parity with react-native-windows microsoft/react-native-windows#7333 for MultilineTextInput fields
See react-native-windows PR for desired feature set.

We can't do it for SinglelineTextInput fields (at least not w/o hacks) as it does not support overriding the keyDown event :-(
https://stackoverflow.com/a/6076492
christophpurrer pushed a commit to christophpurrer/react-native-macos that referenced this pull request Aug 28, 2022
This brings macOS parity with react-native-windows microsoft/react-native-windows#7333 for MultilineTextInput fields
See react-native-windows PR for desired feature set.

We can't do it for SinglelineTextInput fields (at least not w/o hacks) as it does not support overriding the keyDown event :-(
https://stackoverflow.com/a/6076492
christophpurrer pushed a commit to christophpurrer/react-native-macos that referenced this pull request Sep 8, 2022
This brings macOS parity with react-native-windows microsoft/react-native-windows#7333 for MultilineTextInput fields
See react-native-windows PR for desired feature set.

We can't do it for SinglelineTextInput fields (at least not w/o hacks) as it does not support overriding the keyDown event :-(
https://stackoverflow.com/a/6076492
christophpurrer pushed a commit to christophpurrer/react-native-macos that referenced this pull request Sep 8, 2022
This brings macOS parity with react-native-windows microsoft/react-native-windows#7333 for MultilineTextInput fields
See react-native-windows PR for desired feature set.

We can't do it for SinglelineTextInput fields (at least not w/o hacks) as it does not support overriding the keyDown event :-(
https://stackoverflow.com/a/6076492
christophpurrer pushed a commit to christophpurrer/react-native-macos that referenced this pull request Sep 9, 2022
This brings macOS parity with react-native-windows microsoft/react-native-windows#7333 for MultilineTextInput fields
See react-native-windows PR for desired feature set.

We can't do it for SinglelineTextInput fields (at least not w/o hacks) as it does not support overriding the keyDown event :-(
https://stackoverflow.com/a/6076492
christophpurrer pushed a commit to christophpurrer/react-native-macos that referenced this pull request Sep 9, 2022
This brings macOS parity with react-native-windows microsoft/react-native-windows#7333 for MultilineTextInput fields
See react-native-windows PR for desired feature set.

We can't do it for SinglelineTextInput fields (at least not w/o hacks) as it does not support overriding the keyDown event :-(
https://stackoverflow.com/a/6076492
christophpurrer pushed a commit to christophpurrer/react-native-macos that referenced this pull request Sep 9, 2022
This brings macOS parity with react-native-windows microsoft/react-native-windows#7333 for MultilineTextInput fields
See react-native-windows PR for desired feature set.

We can't do it for SinglelineTextInput fields (at least not w/o hacks) as it does not support overriding the keyDown event :-(
https://stackoverflow.com/a/6076492
christophpurrer pushed a commit to christophpurrer/react-native-macos that referenced this pull request Sep 12, 2022
This brings macOS parity with react-native-windows microsoft/react-native-windows#7333 for MultilineTextInput fields
See react-native-windows PR for desired feature set.

We can't do it for SinglelineTextInput fields (at least not w/o hacks) as it does not support overriding the keyDown event :-(
https://stackoverflow.com/a/6076492
christophpurrer pushed a commit to christophpurrer/react-native-macos that referenced this pull request Sep 21, 2022
This brings macOS parity with react-native-windows microsoft/react-native-windows#7333 for MultilineTextInput fields
See react-native-windows PR for desired feature set.

We can't do it for SinglelineTextInput fields (at least not w/o hacks) as it does not support overriding the keyDown event :-(
https://stackoverflow.com/a/6076492
christophpurrer pushed a commit to christophpurrer/react-native-macos that referenced this pull request Sep 21, 2022
This brings macOS parity with react-native-windows microsoft/react-native-windows#7333 for MultilineTextInput fields
See react-native-windows PR for desired feature set.

We can't do it for SinglelineTextInput fields (at least not w/o hacks) as it does not support overriding the keyDown event :-(
https://stackoverflow.com/a/6076492
Saadnajmi pushed a commit to microsoft/react-native-macos that referenced this pull request Sep 21, 2022
This brings macOS parity with react-native-windows microsoft/react-native-windows#7333 for MultilineTextInput fields
See react-native-windows PR for desired feature set.

We can't do it for SinglelineTextInput fields (at least not w/o hacks) as it does not support overriding the keyDown event :-(
https://stackoverflow.com/a/6076492

Co-authored-by: Alex Chiu <ackchiu@fb.com>
shwanton pushed a commit to shwanton/react-native-macos that referenced this pull request Feb 13, 2023
…1423)

This brings macOS parity with react-native-windows microsoft/react-native-windows#7333 for MultilineTextInput fields
See react-native-windows PR for desired feature set.

We can't do it for SinglelineTextInput fields (at least not w/o hacks) as it does not support overriding the keyDown event :-(
https://stackoverflow.com/a/6076492

Co-authored-by: Alex Chiu <ackchiu@fb.com>
# Conflicts:
#	Libraries/Components/TextInput/TextInput.js
#	Libraries/Text/TextInput/RCTBackedTextInputDelegate.h
#	Libraries/Text/TextInput/RCTBaseTextInputView.h
#	Libraries/Text/TextInput/RCTBaseTextInputView.m
#	Libraries/Text/TextInput/RCTBaseTextInputViewManager.m
shwanton pushed a commit to shwanton/react-native-macos that referenced this pull request Mar 10, 2023
…1423)

This brings macOS parity with react-native-windows microsoft/react-native-windows#7333 for MultilineTextInput fields
See react-native-windows PR for desired feature set.

We can't do it for SinglelineTextInput fields (at least not w/o hacks) as it does not support overriding the keyDown event :-(
https://stackoverflow.com/a/6076492

Co-authored-by: Alex Chiu <ackchiu@fb.com>
# Conflicts:
#	Libraries/Components/TextInput/TextInput.js
#	Libraries/Text/TextInput/RCTBackedTextInputDelegate.h
#	Libraries/Text/TextInput/RCTBaseTextInputView.h
#	Libraries/Text/TextInput/RCTBaseTextInputView.m
#	Libraries/Text/TextInput/RCTBaseTextInputViewManager.m
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Proposal: Declarative submit and clear props for TextInput

3 participants