Prevent ScrollView From Stealing Responder Capture When Using Physical Keyboard

[NickGerleman]

Summary

Fixes microsoft/react-native-windows#5867

ScrollResponder has logic so that the first tap exiting out of a soft keyboard is captured instead of leaking to its children. This state is checked by testing if TextInputState.currentlyFocusedInput() is non-null. This also fires in cases a soft keyboard is not present (e.g. on Desktop where a physical keyboard is in use). This presents to users as clicks/taps not being registered when moving from a TextInput to something esle.

Instead of checking TextInputState to see if the softKeyboard is open, check this.keyboardWillOpenTo, which is tied to keyboard open and close events.

Changelog

[General] [Fixed] - Prevent ScrollView From Stealing Responder Capture When Using Physical Keyboard

Test Plan

Validated that on react-native-windows, ScrollView will capture responder events when tapped and a soft-keyboard is open, but will not capture events when clicking from a TextView to a child of a ScrollView and no soft keyboard is open.

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,208,857	-12
android	hermes	armeabi-v7a	6,858,033	-20
android	hermes	x86	7,643,485	-24
android	hermes	x86_64	7,534,437	-24
android	jsc	arm64-v8a	9,367,594	0
android	jsc	armeabi-v7a	9,008,898	8
android	jsc	x86	9,230,310	0
android	jsc	x86_64	9,807,462	4

Base commit: 07a597a

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 07a597a

[elicwhite]

I am probably the right reviewer for this, but I can't reason about the behavior of this particular change well enough to have confidence in landing this as is. I haven't seen or thought about keyboardWillOpenTo before. Can you help me understand the lifecycle of that?

If we land this and it has regressions, we probably wouldn't learn immediately (which would be preferred) but rather start seeing strange reports over the next week or two. I'd feel more comfortable if you can help validate this change on mobile, not just desktop.

[NickGerleman]

@TheSavior keyboardWillOpenTo corresponds to the event fired in keybaordWillShow and keyboardDidShow, and is cleared in keyboardWillHide and keyboardDidHide. It is seemingly used to query soft keyboard dimensions when doing some measurements. It should be set from the time a keyboard has been triggered to be shown, to the point where it is triggered to be hiden. This is where we register the event handlers, in the bottom of the file.

 scrollResponderKeyboardWillShow: function(e: KeyboardEvent) {
    this.keyboardWillOpenTo = e;
    this.props.onKeyboardWillShow && this.props.onKeyboardWillShow(e);
  },

  scrollResponderKeyboardWillHide: function(e: KeyboardEvent) {
    this.keyboardWillOpenTo = null;
    this.props.onKeyboardWillHide && this.props.onKeyboardWillHide(e);
  },

  scrollResponderKeyboardDidShow: function(e: KeyboardEvent) {
    // TODO(7693961): The event for DidShow is not available on iOS yet.
    // Use the one from WillShow and do not assign.
    if (e) {
      this.keyboardWillOpenTo = e;
    }
    this.props.onKeyboardDidShow && this.props.onKeyboardDidShow(e);
  },

  scrollResponderKeyboardDidHide: function(e: KeyboardEvent) {
    this.keyboardWillOpenTo = null;
    this.props.onKeyboardDidHide && this.props.onKeyboardDidHide(e);
  },

Can try to get a mobile dev environment set up (had a feeling this day would come eventually). react-native-windows currently fired keyboardDidShow and keyboardDidHide, which should be similar to the other platforms.

[elicwhite]

The comment at the top of this method says:

/**
   * There are times when the scroll view wants to become the responder
   * (meaning respond to the next immediate `touchStart/touchEnd`), in a way
   * that *doesn't* give priority to nested views (hence the capture phase):
   *
   * - Currently animating.
   * - Tapping anywhere that is not a text input, while the keyboard is
   *   up (which should dismiss the keyboard).
   *
   * Invoke this from an `onStartShouldSetResponderCapture` event.
   */

Would this change only work for the "while the keyboard is up" case? What about the "currently animating" case? (FWIW, I don't know what case that is...)

[NickGerleman]

I believe that's covered by this chunk above.

    // The scroll view should receive taps instead of its descendants if:
    // * it is already animating/decelerating
    if (this.scrollResponderIsAnimating()) {
      return true;
    }

Getting an iOS env set up so I can validate ScrollView still eats first input when dismissing soft keyboard on mobile.

[NickGerleman]

The change seems to be working as intended on iOS as well. As a reference, FlatListExample creates a FlatList explicitly setting keyboardShouldPersistTaps to always. This means that tapping inside the ScrollView will never deactivate the soft keyboard, Touchables are activated on first tap, and focus is always left in the TextInput. See https://imgur.com/nidrqQV

This is in contrast to the MultiColumn FlatList example, which doesn't specialize keyboardShouldPersistTaps. The behavior before and after this change will eat the first input to dismiss the soft keyboard. See https://imgur.com/1hleIlj

Things get a bit interesting if we we change the simulator to emulate a connected physical keyboard for the same MultiColumn FlatList example. Previous behavior here was to behave similarly to soft keyboard, where the first tap would be eaten. This is what we don't want, since there isn't a soft keyboard up. After the change, touchables are correctly activated on first tap. See https://imgur.com/FcxaMLU

Interestingly we don't blur the TextInput on iOS when tapping a touchable in this scenario. I thought this was a little bit strange, but discovered the same behavior when using a hardware keyboard in the builtin Files app, where tapping to switch between tabs doesn't blur the Textinput. We do still blur the input on scroll, which matches native behavior with the hardware keyboard as well.

[NickGerleman]

Going to get an Android dev environment set up Monday morning to test against it as well. Was pleasantly surprised how painless it was to set up the iOS one to test Core 🙂. I expect to see similar behavior though, where we no longer eat taps to dismiss keyboard when there isn't a keyboard up.

[NickGerleman]

Confirmed we see the same behavior on Android, where we will eat the first tap to dismiss the soft keyboard when one is present, but will no longer eat the tap when no soft keyboard is present. https://imgur.com/J9IATcM

Retaining TextInput focus when using a physical keyboard and interacting with other UI is consistent with the built in native Messages app on Android.

As an aside, keyboardWillOpenTo also confused me as a name. I was wondering if there was some naming convention this was related to, but based on your questions about it, I'm wondering if it might be better to rename it to something like keyboardShownEvent. It's also currently untyped (ScrollResponder isn't flow-strict). Can make those changes if you would like.

[elicwhite]

Those sound like good changes to make in a follow up PR. Let's keep this one small.

[facebook-github-bot]

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

[react-native-bot]

This pull request was successfully merged by @NickGerleman in 93e7a7a.

^{When will my fix make it into a release? | Upcoming Releases}

[elicwhite]

This had to get reverted. It broke search results being tappable on Marketplace. I don't have a repro from the team at this point

[NickGerleman]

Oh wow that's not good. Thank you for the heads up.

I wonder if we're somehow seeing keyboardWillShow/KeyboardDidShow without a corresponding keyboardWillHide/keyboardDidHide. Taking another look at the description of keyboard events, I completely missed the documented behavior of:

If you set android:windowSoftInputMode to adjustNothing, no events will be available on Android"

"adjustNothing" isn't documented in current Android docs (wondering if it went away), but this would mess with this change a bit if it still exists, since it means there are some cases where we wouldn't discover the keyboard is open. It's also per-activity, which makes me wonder if there are cases behavior can change while we're still listening to events.

It seems like there is some more cases off the happy path I need to think about. E.g.

• Are there cases where we can force the keyboard to be up outside of a TextInput, where ScrollResponder wouldn't be able to dismiss it, but we previously wouldn't capture
• Are there cases where we might have been controlling a native TextInput outside of the root view with the keyboard up, where RN doesn't know about TextInputState and previously assumes the soft keyboard wasn't up
• Are there still any cases (documented or undocumented) where we won't see keyboard events
• Are there states we can transition between that cause us to see only part of the open/close lifecycle

[NickGerleman]

I've been trying to dig into this more, and I'm realizing there are a decent number of scenarios where we might pop the soft keyboard, but not have previously known about it in TextInputState. E.g. a brownfield app with focus in a native TextInput outside of RN, or a custom editable text component that doesn't set it. Would be curious to know if Marketplace falls into one of those.

If I can confirm there aren't still cases where we never see keyboard events, I wonder if the fix could be as simple as checking both event driven keyboard open state AND if there's a currently focused TextInput. I.e. stop eating taps in the known case where we shouldn't, but don't introduce new possibilities where we could eat them (where the soft keyboard is open but we TextInputState doesn't know about it).

[elicwhite]

I believe the text input in the navigation is a native control, while the search results are a FlatList. I believe the issue was that the results weren't tappable.

[elicwhite]

Additional info from the task internally:

(iOS) enter Marketplace tab-click search icon-> enter type ahead -> type a few keys with soft keyboard -> click one of search suggestions.

expected:
navigate to search result page.

actual behavior:
no response from the system, logging confirmed that RN product code doesn't receive the onPress event.

[NickGerleman]

Thanks. I think I know what went wrong here. I'm going to do a little bit more digging and validation, but will try to have a different PR out soon.

[elicwhite]

Apparently this was an issue on both iOS and Android.

[NickGerleman]

Yeah, I think the main thing I missed (that's now a bit obvious in retrospect) is that there are cases where a soft keyboard is open that TextInputState wouldn't know about. So the issue looks like:

1. Enter a control with a native TextInput (not an RN one)
2. Soft keyboard pops up without changing TextInputState in RN
3. The ScrollView now captures the responder chain because it sees a soft keyboard up, where it used to not capture because it was checking TextInputState
4. ScrollView's responder doesn't do anything, since it nornally would try to blur the currently focused RN TextInput to dismiss the keyboard

So, basically, we end up capturing in an additional state that we don't actually have logic to get out of. I think the safer option would be to keep the existing behavior of not capturing when TextInputState doesn't know about what's focued, but also add the soft keyboard check.