Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Touch/Mouse Input Will Not Leak To Child Components of ScrollView After Focus is in TextInput (Since 0.62) #5867

Closed
NickGerleman opened this issue Aug 28, 2020 · 12 comments
Closed

Touch/Mouse Input Will Not Leak To Child Components of ScrollView After Focus is in TextInput (Since 0.62) #5867

NickGerleman opened this issue Aug 28, 2020 · 12 comments
Assignees
@NickGerleman
Labels
Area: Mouse bug Partner: Facebook Partner: Stream (label may be applied by bot based on author) Partner: Xbox (label may be applied by bot based on author) Upstream
Milestone
0.64

Comments

@NickGerleman
Copy link
Collaborator

From @KAnder425

I've got a repro in the playground. It's in my repo at KAnder425@93a31c0

To repro you must put your focus back in the Text Input after bringing up the Popup. The first touch event after that will be ignored (see ("I'm touched" counter).

I also have a gif of the repro attached.
PopupRepro

The key pieces are;

  • Popup must be isLightDismissEnable (false)
  • The Touchable must be in a ScrollView
  • Focus must be in a TextInput outside of the Popup (or maybe just outside the Touchable, IDK)

I did notice that the same behavior (eg. swallowing a touch event) occurs more broadly in our app as well as in RNTester, because it really doesn't depend on the Popup. You can repro this in the FlatList example in RNTester if you just put focus into a TextInput before clicking on the FlatList elements.

For now, I've been able to workaround this in our app by setting the 'keyboardShouldPersistTaps' property (either 'always' | 'handled') on the ScrollView that contains the TextInput, but I'd prefer a solution in RNW. agree it's quite tricky
@NickGerleman NickGerleman self-assigned this Aug 28, 2020
@ghost ghost added the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Aug 28, 2020
@NickGerleman
Copy link
Collaborator Author

Confirmed the regression started with ae37f8e which was the upgrade to 0.62.

In the above case, the ScrollView sees onResponderGrant instead of the TouchableHighlight. The responder state machine is controlled by upstream JS in RN, depending on touchStart, touchEnd, and seemingly some other factors. Confirmed that in the repro case, we still correctly send touchStart from native using the react tag for TouchableHighlight instead of the ScrollView.

@NickGerleman
Copy link
Collaborator Author

ScrollResponder has some logic to take precedent over nested views if their is a TextInput focused and keyboardPersistTaps is undefined or never https://github.com/facebook/react-native/blob/0.62-stable/Libraries/Components/ScrollResponder.js#L153-L243 This is what's triggering the logic. Debugging to figure out why we weren't hitting it before.

@NickGerleman
Copy link
Collaborator Author

We saw the behavior change because the 0.62 merge fixed the implementation of TextInputState.currentlyFocusedField(), which triggers upstream logic around it that only really makes sense on phones. We should change this to only eat input in the case where a soft-keyboard is up, since that's the intent.

@NickGerleman NickGerleman changed the title Clicks Will Sometimes Get Eaten After Selecting a TextInput Since 0.62 Touch/Mouse Input Will Not Leak To Child Components of ScrollView After Focus is in TextInput (Since 0.62) Aug 28, 2020
NickGerleman added a commit to NickGerleman/react-native that referenced this issue Aug 28, 2020
@NickGerleman
Prevent ScrollView From Stealing Responder Capture When Using Physica…
aff76af 
…l Keyboard

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.

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.
@NickGerleman NickGerleman mentioned this issue Aug 28, 2020
Closed
@chrisglein chrisglein added Area: Mouse bug and removed Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) labels Aug 31, 2020
@chrisglein chrisglein added this to the 0.64 milestone Aug 31, 2020
NickGerleman added a commit to NickGerleman/react-native-windows that referenced this issue Sep 1, 2020
@NickGerleman
Prevent Clicks/Taps From TextInput to Touchables in ScrollView From B…
a9b57a9 
…eing Eaten

See microsoft#5867

This backports the upstream change made in facebook/react-native#29798 to older versions of react-native Windows. Not going to bother to add the patching to the master branch, since we will pull in the fix automatically before 0.64.

Validates the issue no longer repros after the change in the multicolumn flatlist example.
@NickGerleman NickGerleman mentioned this issue Sep 1, 2020
Closed
NickGerleman added a commit to NickGerleman/react-native-windows that referenced this issue Sep 1, 2020
@NickGerleman
Prevent Clicks/Taps From TextInput to Touchables in ScrollView From B…
15b118d 
…eing Eaten

See microsoft#5867

This backports the upstream change made in facebook/react-native#29798 to older versions of react-native-windows. Not going to bother to add the patching to the master branch, since we will pull in the fix automatically before 0.64.

Validates the issue no longer repros after the change in the multicolumn flatlist example.
@NickGerleman NickGerleman mentioned this issue Sep 1, 2020
Closed
@NickGerleman
Copy link
Collaborator Author

Need to also check if this impacts NetUI. Looking through everything again, I think it may run into the same problem, since TextInputState should be correctly wired now.

@NickGerleman NickGerleman reopened this Sep 18, 2020
@NickGerleman
Copy link
Collaborator Author

This is a bit worse than previously known. @DrewHiggins reached out and it was realized that this bug interacts badly with ref.focus also setting currently focused element on TextInputState. This means that programmatically focusing other components can cause clicks to be dropped in a ScrollView as well.

@NickGerleman NickGerleman added the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Oct 22, 2020
@NickGerleman
Copy link
Collaborator Author

Going to try to upstream again soon, but this one might be worth bringing a fix back to 0.63 for if it sticks upstream, since it's impacting multiple partners. Though both have workarounds.

@chrisglein chrisglein added Request Backport to 0.63 and removed Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) labels Oct 26, 2020
@yagoldin
Copy link

yagoldin commented Nov 5, 2020

Adam Gorman(adamgor) and I are also encountering this issue.

@NickGerleman
Copy link
Collaborator Author

Chatted offline. Will hear back on whether the keyboardShouldPersistTaps="always" workaround is okay until we have an upstream fix.

@NickGerleman NickGerleman added Partner: Xbox (label may be applied by bot based on author) Partner: Stream (label may be applied by bot based on author) and removed Partner: Office labels Nov 8, 2020
@NickGerleman
Copy link
Collaborator Author

Removing "Partner: Office" since Stellar is no longer effected, but Garrison + Stream are both effected. Garrison has a workaround they're happy with, and there wasn't concern of a delayed upstream release. Stream still validating the workaround.

@NickGerleman
Copy link
Collaborator Author

Facebook also impacted, and Stream was having some issues getting the workaround applied.

Going back to the stance of us potentially wanting to be a bit aggressive and bring this back to 0.63, since this seems to be impacting many teams.

@NickGerleman NickGerleman added Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) Request Backport to 0.63 labels Nov 12, 2020
@asklar asklar removed the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Nov 16, 2020
@asklar
Copy link
Member

asklar commented Nov 16, 2020 •
edited
Loading

Fix is in PR to RN core, pretty bad UX; waiting to bake the fix in FB before backporting to 0.63

@rectified95
Copy link
Contributor

Adding link to PR referenced above: facebook/react-native#30374

@NickGerleman NickGerleman mentioned this issue Dec 4, 2020
Closed
NickGerleman added a commit to NickGerleman/react-native-windows that referenced this issue Dec 7, 2020
@NickGerleman
Fix Taps/Clicks Being Eaten in 0.63
20b77c5 
Fixes microsoft#5867

Original PR in core: facebook/react-native#30374

This has been requested by enough folks to make sense to backport to 0.63. The upstream PR has stayed in FB master for a few weeks, and will be cherry-picked into 0.64 upstream, so this change only exists in 0.63.
@NickGerleman NickGerleman mentioned this issue Dec 7, 2020
Merged
ghost pushed a commit that referenced this issue Dec 9, 2020
@NickGerleman
Fix Taps/Clicks Being Eaten in 0.63 (#6701)
3eaab0d 
* Fix Taps/Clicks Being Eaten in 0.63

Fixes #5867

Original PR in core: facebook/react-native#30374

This has been requested by enough folks to make sense to backport to 0.63. The upstream PR has stayed in FB master for a few weeks, and will be cherry-picked into 0.64 upstream, so this change only exists in 0.63.

* Change files
philippeauriach pushed a commit to philippeauriach/react-native that referenced this issue May 5, 2021
@NickGerleman @philippeauriach
Prevent ScrollView From Stealing Responder Capture When Using Physica…
10ac4a6 
…l Keyboard (facebook#29798)

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

Pull Request resolved: facebook#29798

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.

Reviewed By: kacieb

Differential Revision: D23426786

Pulled By: TheSavior

fbshipit-source-id: 7138ef0bc4508aaec5531f455b022b105b5d858a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@NickGerleman NickGerleman

Labels
Area: Mouse bug Partner: Facebook Partner: Stream (label may be applied by bot based on author) Partner: Xbox (label may be applied by bot based on author) Upstream
Projects
None yet
Milestone
0.64
6 participants
@NickGerleman @rectified95 @asklar @chrisglein @Khalef1 @yagoldin