Reworking keyboardShouldPersistTaps to have a middle ground #10628

MaxLap · 2016-10-29T19:38:10Z

Right now, the ScrollView's keyboard hiding behavior is either all or nothing: Hide the keyboard on any tap, or do nothing ever. This PR introduces a third mode to keyboardShouldPersistTaps which is much closer to what I consider should be the default.

In the new behavior, the tap responding is done in the bubbling phase (instead of the capture phase like =true). As a result, a child can handle the tap. If no child does, then the ScrollView will receive the tap and will hide the keyboard. As a result, changing TextInput focus works as a user expects, with a single tap and without keyboard hiding. But taping on Text or on the empty part of the ScrollView hides the keyboard and removes the focus.

You can view the behavior in a monkey patched ScrollView demo on rnplay:
https://rnplay.org/apps/E90UYw
https://rnplay.org/apps/UGzhKA

In order to have a uniform props set, i added 3 values to the keyboardShouldPersistTaps:
'never' and 'always' are the same as false and true.
'handled' is the new behavior.

I don't know stance on backward compatibility of the project. The default behavior isn't changed and remains at 'always' (true), but in my opinion, the goal should be to change the default to be the new 'handled' behavior since that feels the most intuitive.

Note that true and false are still valid options.

facebook-github-bot · 2016-10-29T19:40:30Z

By analyzing the blame information on this pull request, we identified @sahrens and @simonayzman to be potential reviewers.

janicduplessis · 2016-10-29T19:49:19Z

I think we should just make keyboardShouldPersistTaps={false} your new behaviour instead of changing it to 3 values. It will be less of a breaking change in my opinion, it will change the behaviour a little bit but I don't think there are any use cases where you'd want the current behaviour.

Feels more like a bug fix with keyboardShouldPersistTaps={false} than anything else as I'd expect the keyboard to stay opened with touching another input.

cc @mkonicek

MaxLap · 2016-10-29T23:12:40Z

I absolutely agree, i just didn't know what was the stance on background compat. If the concensus is that it should be changed straight away, I'll happily change the PR.

janicduplessis · 2016-10-29T23:32:42Z

@MaxLap 👍 Let's wait a bit see what others think

satya164 · 2016-10-29T23:40:13Z

@janicduplessis I think the current behaviour desirable if your screen filled with touchables. you don't wanna trigger a tap while dismissing the keyboard.

janicduplessis · 2016-10-29T23:49:19Z

Does this actually affects touchables? Looks like it only should only affect TextInputs.

satya164 · 2016-10-30T00:22:13Z

@janicdupless no it doesn't affect touchables. But when you have a screen filled with touchables, you need to tap somewhere to dismiss the keyboard which was triggered due to an text input without triggering a tap on a touchable.

MaxLap · 2016-10-30T01:35:19Z

@janicduplessis @satya164 to clarify, the new behavior basically means if a descendant handles the tap, the ScrollView won't, and therefore won't hide the keyboard. If a descendant button wants to also hide the keyboard, then it can do so itself pretty easily.

MaxLap · 2016-11-10T18:16:39Z

Is there a way to request feedback from other people? It's not so much about reviewing the code, first about if this is something you want in RN.

I think this change would be helpful to many people. I admit that some people may want the current swallow-every-tap behavior, which is why I left it there. I also do think the default behavior should the new one, maybe with some releases in between to let people specify which behavior they want.

I didn't change the documentation yet as I wanted feedback first.

janicduplessis · 2016-11-10T18:55:30Z

Ping @mkonicek

ericvicenti

Overall this change makes a lot of sense.

Should we set the default prop to 'handled'? Or maybe we can do it later once people have had some time to experiment with the 'handled' behavior in their apps.

ericvicenti · 2016-11-11T18:49:26Z

Libraries/Components/ScrollResponder.js

+  scrollResponderHandleStartShouldSetResponder: function(e: Event): boolean {
+    var currentlyFocusedTextInput = TextInputState.currentlyFocusedField();
+
+    if (this.props.keyboardShouldPersistTaps == 'handled' &&


please use === in cases like this

ericvicenti · 2016-11-11T18:51:02Z

Libraries/Components/ScrollResponder.js

@@ -180,7 +187,9 @@ var ScrollResponderMixin = {
  scrollResponderHandleStartShouldSetResponderCapture: function(e: Event): boolean {
    // First see if we want to eat taps while the keyboard is up
    var currentlyFocusedTextInput = TextInputState.currentlyFocusedField();
-    if (!this.props.keyboardShouldPersistTaps &&
+    var keyboardNeverPersistTaps = !this.props.keyboardShouldPersistTaps ||


this could be more consise if you do this on a previous line:

const {keyboardShouldPersistTaps} = this.props;

janicduplessis · 2016-11-11T22:16:06Z

@MaxLap @satya164 I tested this out more in an app and it does affect the interaction with Touchables. If you have a touchable in the screen it will handle the event and thus the keyboard won't be close. Because of this I think we should not change it to the default behavior.

After thinking about it there are use cases for all 3 behaviors

Persist taps 'always': Never dismiss the keyboard
Persist taps 'handled': Dismiss the keyboard only when tapping non-touchable views, useful let's say you have a form that include buttons and want to be able to click then without dismissing keyboard beforehand.
Persist taps 'never': Always dismiss the keyboard even when tapping touchables, useful if you have something like a search bar with a bunch of touchable rows that fill the rest of the screen, you don't want it to trigger the onPress and dismiss the keyboard.

However there is still an issue with the current 'never' behavior (which is I was mostly referring to in my earlier comments) where tapping a different text input while the keyboard is open dismisses the keyboard instead of selecting it. I'll open a new PR to fix this case separately as it doesn't depend on this.

janicduplessis · 2016-11-11T22:24:22Z

With that said I'm not the biggest fan of the string enum + boolean prop, what about we add a deprecation warning for the boolean values or create a new prop altogether and deprecate keyboardShouldPersistTaps?

MaxLap · 2016-11-13T02:10:09Z

I did the changes requested in the review.

I agree that have both the true/false and the "always"/"never" is probably not the best thing. A deprecation is probably a good idea? Is that the final call?

Making the "never" sometimes still persist the keyboard may make the name of that value confusing.

satya164 · 2016-11-13T14:15:32Z

Persist taps 'never': Always dismiss the keyboard even when tapping touchables, useful if you have something like a search bar with a bunch of touchable rows that fill the rest of the screen, you don't want it to trigger the onPress and dismiss the keyboard.

What happens when I tap non-touchable? Shouldn't the keyboard dismiss then too? Why limit this to touchables only?

what about we add a deprecation warning for the boolean values

Yeah, we should deprecate the boolean values and show a warning specifying which value to change to.

janicduplessis · 2016-11-13T20:12:25Z

@satya164 Maybe that wasnt clear but yea it would dismiss the keyboard when tapping non-touchables too.

satya164 · 2016-11-13T20:15:52Z

@janicduplessis great!

MaxLap · 2016-11-14T13:19:19Z

Okay, so for the deprecation. Do I make a new prop or I keep using keyboardShouldPersistTaps? If I keep using it, is there a better way of doing the deprecation than this? It works, but the message starts by saying keyboardShouldPersistTaps is deprecated. Is there an alternative / should I code one or that's good enough?

keyboardShouldPersistTaps: PropTypes.oneOfType(
  [PropTypes.oneOf(['always', 'never', 'handled']),
   deprecatedPropType(PropTypes.bool, 
                      'Values true & false for keyboardShouldPersistTaps ' + 
                      'are deprecated. Use "always" & "never" respectively ' + 
                      'instead or the new "handled".')]),

The resulting message is: keyboardShouldPersistTaps supplied to ScrollView has been deprecated. Values true & false for keyboardShouldPersistTaps are deprecated. Use "always" & "never" respectively instead or the new "handled".

satya164 · 2016-11-14T15:24:54Z

Do I make a new prop or I keep using keyboardShouldPersistTaps

keyboardShouldPersistTaps sounds fine to me @janicduplessis?

Is there an alternative

I think better to check it in componentDidMount,

const { keyboardShouldPersistTaps } = this.props;
if (typeof keyboardShouldPersistTaps === 'boolean') {
  console.warn(`'keyboardShouldPersistTaps={${keyboardShouldPersistTaps}}' is deprecated. Use 'keyboardShouldPersistTaps=${keyboardShouldPersistTaps ? "always" : "never"}' instead`);
}

ericvicenti · 2016-11-14T17:32:54Z

@satya164, fbjs has a warning module, which is considered best practice inside RN:

const warning = require('warning');
...
warning(
  typeof keyboardShouldPersistTaps !== 'boolean',
  `'keyboardShouldPersistTaps={${keyboardShouldPersistTaps}}' is deprecated. Use 'keyboardShouldPersistTaps=${keyboardShouldPersistTaps ? "always" : "never"}' instead`
);

janicduplessis · 2016-11-15T16:13:46Z

@ericvicenti Well it wont actually open / close it, it's just my mental model of it and why I think that never is fine even if it doesnt close it on text inputs.

janicduplessis · 2016-11-15T16:21:23Z

Libraries/Components/ScrollView/ScrollView.js

+     *     is up dismisses the keyboard. When this happens, children won't receive the tap.
+     *   - 'always', the keyboard will not dismiss automatically, and the scroll view will not
+     *     catch taps, but children of the scroll view can catch taps.
+     *   - 'handled', they keyboard will not dismiss automatically when the tap was handled by


Typo: they -> the

janicduplessis · 2016-11-15T16:25:26Z

Libraries/Components/ScrollResponder.js

@@ -474,6 +486,13 @@ var ScrollResponderMixin = {
   * The `keyboardWillShow` is called before input focus.
   */
  componentWillMount: function() {
+    var keyboardShouldPersistTaps = this.props.keyboardShouldPersistTaps;


Nit: lets use destructuring here too for consistency

This adds new possible values to keyboardShouldPersistTaps. 'never' and 'always' are the same as false and true. 'handled' is similar to 'never' (false), but the behavior is done in the bubbling phase of the event instead of the capture phase. So if a descendant component decides to respond to the tap, the ScrollResponder won't receive the event. The default behavior isn't changed and remains at 'always' (true), but in my opinion, the goal should be to change the default to be the new 'handled' behavior since that feels the most intuitive. Note that true and false are still valid options.

The exact change would be to 'always', but handled most likely make more sense and helps spread awareness of that option.

MaxLap · 2016-11-16T00:36:04Z

Did the requested tweaks from the review. Thank you @janicduplessis

I edited the examples to use string values. I replaced the true by 'handled' since that's probably a more sane behavior for them. I have no idea how to test the examples, is anyone setup to do it relatively painlessly?

As a side note, am I wrong in thinking that the props and propTypes that refer to keyboardShouldPersistTaps are useless in UIExplorerPage? (only wrapperProps.keyboardShouldPersistTaps = ... makes sense) If you agree, I'll remove those useless references.

@janicduplessis's reasoning makes sense to me, so I will stick with 'never'.

janicduplessis · 2016-11-16T00:46:48Z

You can run the UIExplorer app from the xcode project in Examples/UIExplorer on iOS or with ./gradlew Examples:UIExplorer:android:app:installDebug with a simulator opened on Android. You will also need to start the packager with ./packager/packager.sh.

Yea, looks like the prop keyboardShouldPersistTaps declared in the propTypes is never used we can remove it.

MaxLap · 2016-11-16T13:16:33Z

Ok, went easier than expected (I'm on windows with android).

The UIExample works great.

I can't get the Movies example to work. It starts, but it can't seem to do its request to rotten tomatoes: undefined is not an object (evaluating 'responseData.total') at line 136 of SearchScreen.js. Because of this, the ListView is never displayed. I doubt there is an issue with my change since it's a single prop.

janicduplessis · 2016-11-16T20:15:17Z

Ok good, as long as uiexplorer works thats fine, anyway your changes don't affect the others.

@ericvicenti Do you want to take care of shipping this since you'll probably want to update fb internal codebase as well to avoid the warning. We should also make sure to communicate this with the release team so they can add something in the release notes about it (not really a breaking change but still).

Summary: When using text inputs inside a ScrollView with `keyboardShouldPersistTaps=false` (default behavior) tapping another text input dismisses the keyboard instead of keeping it open and focusing the new text input which I think is the better and expected behavior. See #10628 for more discussion about that. Note that this affects nothing but the behavior with text inputs unlike #10628. cc satya164 MaxLap ericvicenti Closes #10887 Differential Revision: D4178474 Pulled By: ericvicenti fbshipit-source-id: 0c62ea2fac0017d559d1f8674b0a686a5e1b3d2d

MaxLap · 2016-12-07T20:07:33Z

Anybody willing to make the review on this? Thank you

janicduplessis · 2016-12-07T22:22:19Z

Sorry about the delay, I'll ping Eric about it.

ericvicenti · 2016-12-07T22:23:17Z

Sorry for the delay on the re-review. This looks great and I'm landing it now!

facebook-github-bot · 2016-12-07T22:23:39Z

@ericvicenti has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

MaxLap · 2016-12-08T11:49:39Z

Thank you everyone

Summary: Right now, the ScrollView's keyboard hiding behavior is either all or nothing: Hide the keyboard on any tap, or do nothing ever. This PR introduces a third mode to keyboardShouldPersistTaps which is much closer to what I consider should be the default. In the new behavior, the tap responding is done in the bubbling phase (instead of the capture phase like =true). As a result, a child can handle the tap. If no child does, then the ScrollView will receive the tap and will hide the keyboard. As a result, changing TextInput focus works as a user expects, with a single tap and without keyboard hiding. But taping on Text or on the empty part of the ScrollView hides the keyboard and removes the focus. You can view the behavior in a monkey patched ScrollView demo on rnplay: https://rnplay.org/apps/E90UYw https://rnplay.org/apps/UGzhKA In order to have a uniform props set, i added 3 values to the keyboardShouldPersistTaps: 'never' and 'always' are the same as false and true. 'handled' is the new behavior. I don't Closes #10628 Differential Revision: D4294945 Pulled By: ericvicenti fbshipit-source-id: 1a753014156cac1a23fabfa8e1faa9a768868ef2

Summary: When using text inputs inside a ScrollView with `keyboardShouldPersistTaps=false` (default behavior) tapping another text input dismisses the keyboard instead of keeping it open and focusing the new text input which I think is the better and expected behavior. See facebook#10628 for more discussion about that. Note that this affects nothing but the behavior with text inputs unlike facebook#10628. cc satya164 MaxLap ericvicenti Closes facebook#10887 Differential Revision: D4178474 Pulled By: ericvicenti fbshipit-source-id: 0c62ea2fac0017d559d1f8674b0a686a5e1b3d2d

Summary: Right now, the ScrollView's keyboard hiding behavior is either all or nothing: Hide the keyboard on any tap, or do nothing ever. This PR introduces a third mode to keyboardShouldPersistTaps which is much closer to what I consider should be the default. In the new behavior, the tap responding is done in the bubbling phase (instead of the capture phase like =true). As a result, a child can handle the tap. If no child does, then the ScrollView will receive the tap and will hide the keyboard. As a result, changing TextInput focus works as a user expects, with a single tap and without keyboard hiding. But taping on Text or on the empty part of the ScrollView hides the keyboard and removes the focus. You can view the behavior in a monkey patched ScrollView demo on rnplay: https://rnplay.org/apps/E90UYw https://rnplay.org/apps/UGzhKA In order to have a uniform props set, i added 3 values to the keyboardShouldPersistTaps: 'never' and 'always' are the same as false and true. 'handled' is the new behavior. I don't Closes facebook#10628 Differential Revision: D4294945 Pulled By: ericvicenti fbshipit-source-id: 1a753014156cac1a23fabfa8e1faa9a768868ef2

Summary: Right now, the ScrollView's keyboard hiding behavior is either all or nothing: Hide the keyboard on any tap, or do nothing ever. This PR introduces a third mode to keyboardShouldPersistTaps which is much closer to what I consider should be the default. In the new behavior, the tap responding is done in the bubbling phase (instead of the capture phase like =true). As a result, a child can handle the tap. If no child does, then the ScrollView will receive the tap and will hide the keyboard. As a result, changing TextInput focus works as a user expects, with a single tap and without keyboard hiding. But taping on Text or on the empty part of the ScrollView hides the keyboard and removes the focus. You can view the behavior in a monkey patched ScrollView demo on rnplay: https://rnplay.org/apps/E90UYw https://rnplay.org/apps/UGzhKA In order to have a uniform props set, i added 3 values to the keyboardShouldPersistTaps: 'never' and 'always' are the same as false and true. 'handled' is the new behavior. I don't Closes facebook/react-native#10628 Differential Revision: D4294945 Pulled By: ericvicenti fbshipit-source-id: 1a753014156cac1a23fabfa8e1faa9a768868ef2

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Oct 29, 2016

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 Oct 29, 2016

ericvicenti reviewed Nov 11, 2016

View reviewed changes

hramos added GH Review: needs-revision and removed GH Review: review-needed labels Nov 11, 2016

janicduplessis mentioned this pull request Nov 11, 2016

Don't dismiss keyboard when tapping another text input #10887

Closed

MaxLap force-pushed the master branch from 4930f02 to c762895 Compare November 13, 2016 02:00

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Nov 13, 2016

MaxLap force-pushed the master branch from c762895 to afc7809 Compare November 13, 2016 02:03

janicduplessis reviewed Nov 15, 2016

View reviewed changes

MaxLap added 5 commits November 15, 2016 19:20

Deprecation warning for boolean keyboardShouldPersistTaps

a644977

Adding deprecation details to the docs

f737b10

typo, code style and comment tweaks

7ed8f5d

keyboardShouldPersistTaps true=>'handled' in examples

22b3606

The exact change would be to 'always', but handled most likely make more sense and helps spread awareness of that option.

MaxLap force-pushed the master branch from 4ca4bfe to 22b3606 Compare November 16, 2016 00:20

Removing unused keyboardShouldPersistTaps propType from UIExplorer

c40d7f3

facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Dec 7, 2016

facebook-github-bot closed this in d56530d Dec 8, 2016

charlle mentioned this pull request Jan 10, 2017

Bug fix/update keyboard property FaridSafi/react-native-gifted-chat#332

Merged

mikaello mentioned this pull request Jan 18, 2017

Changed keyboardShouldPersistTaps from boolean prop to string d-a-n/react-native-modal-picker#32

Closed

Reworking keyboardShouldPersistTaps to have a middle ground #10628

Reworking keyboardShouldPersistTaps to have a middle ground #10628

Uh oh!

Conversation

MaxLap commented Oct 29, 2016

Uh oh!

facebook-github-bot commented Oct 29, 2016

Uh oh!

janicduplessis commented Oct 29, 2016

Uh oh!

MaxLap commented Oct 29, 2016

Uh oh!

janicduplessis commented Oct 29, 2016

Uh oh!

satya164 commented Oct 29, 2016

Uh oh!

janicduplessis commented Oct 29, 2016

Uh oh!

satya164 commented Oct 30, 2016

Uh oh!

MaxLap commented Oct 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaxLap commented Nov 10, 2016

Uh oh!

janicduplessis commented Nov 10, 2016

Uh oh!

ericvicenti left a comment

Choose a reason for hiding this comment

Uh oh!

ericvicenti Nov 11, 2016

Choose a reason for hiding this comment

Uh oh!

ericvicenti Nov 11, 2016

Choose a reason for hiding this comment

Uh oh!

janicduplessis commented Nov 11, 2016

Uh oh!

janicduplessis commented Nov 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaxLap commented Nov 13, 2016

Uh oh!

satya164 commented Nov 13, 2016

Uh oh!

janicduplessis commented Nov 13, 2016

Uh oh!

satya164 commented Nov 13, 2016

Uh oh!

MaxLap commented Nov 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

satya164 commented Nov 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericvicenti commented Nov 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janicduplessis commented Nov 15, 2016

Uh oh!

janicduplessis Nov 15, 2016

Choose a reason for hiding this comment

Uh oh!

janicduplessis Nov 15, 2016

Choose a reason for hiding this comment

Uh oh!

MaxLap commented Nov 16, 2016

Uh oh!

janicduplessis commented Nov 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaxLap commented Nov 16, 2016

Uh oh!

janicduplessis commented Nov 16, 2016

Uh oh!

MaxLap commented Dec 7, 2016

Uh oh!

janicduplessis commented Dec 7, 2016

Uh oh!

ericvicenti commented Dec 7, 2016

MaxLap commented Oct 30, 2016 •

edited

Loading

janicduplessis commented Nov 11, 2016 •

edited

Loading

MaxLap commented Nov 14, 2016 •

edited

Loading

satya164 commented Nov 14, 2016 •

edited

Loading

ericvicenti commented Nov 14, 2016 •

edited

Loading

janicduplessis commented Nov 16, 2016 •

edited

Loading