Skip to content

Let children onFocus onBlur events in TouchableWithoutFeedback pass through to parent #4078

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
Merged
kaiguo merged 4 commits into from
Feb 11, 2020
Merged

Let children onFocus onBlur events in TouchableWithoutFeedback pass through to parent #4078

kaiguo merged 4 commits into from
Feb 11, 2020

Conversation

@kaiguo
Copy link
Contributor

@kaiguo kaiguo commented Feb 11, 2020 •
edited
Loading

Fixes #3827.

TextInput's onFocus and onBlur callbacks are not functioning, this is because we wrapped RCTTextInput in a TouchableWithoutFeedback, and onFocus onBlur were overridden by TouchableWithoutFeedback.

To fix this we should let OVERRIDE_PROPS do the work and just copy the rest of the children props to parent.

Microsoft Reviewers: Open in CodeFlow

@kaiguo kaiguo requested a review from a team as a code owner February 11, 2020 20:06
@ghost ghost added the vnext label Feb 11, 2020
ddalp
ddalp reviewed Feb 11, 2020
View reviewed changes
packages/E2ETest/wdio/test/testInput.spec.ts
TextInputTestPage.clickMultilineTextInput();
assert.ok(TextInputTestPage.getTextInputPrevText().includes('onBlur'));
});

Copy link
Contributor

@ddalp ddalp Feb 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for adding test coverage! #Resolved

ddalp
ddalp reviewed Feb 11, 2020
View reviewed changes
vnext/src/Libraries/Components/Touchable/TouchableWithoutFeedback.windows.js
onResponderRelease: this.touchableHandleResponderRelease,
onResponderTerminate: this.touchableHandleResponderTerminate,
onKeyUp: this._onKeyUp,
onKeyDown: this._onKeyDown,
Copy link
Contributor

@ddalp ddalp Feb 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

There are more events removed in RN61 TouchableWithoutFeedBack.js, for example: onKeyUp and onKeyDown, should we remove those events too? #Resolved

Copy link
Contributor Author

@kaiguo kaiguo Feb 11, 2020

Choose a reason for hiding this comment

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

I think we should, but looks like ReactNative is not doing it? https://github.com/facebook/react-native/blob/master/Libraries/Components/Touchable/TouchableWithoutFeedback.js#L75-L94

Removing it here and adding it to the override list basically means if there's no event handler set on Touchable, use the callbacks on children to handle the events, which I think makes sense for keyUp/Down events, but I'm not sure why they are not in ReactNative.

Copy link
Contributor Author

@kaiguo kaiguo Feb 11, 2020

Choose a reason for hiding this comment

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

Oh, is keyUp/Down windows only?

Copy link
Contributor

@ddalp ddalp Feb 11, 2020

Choose a reason for hiding this comment

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

I think so. Search for topKeyDown definition.

In reply to: 377886796 [](ancestors = 377886796)

Copy link
Contributor

@kmelmon kmelmon Feb 11, 2020

Choose a reason for hiding this comment

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

yes keyUp/keyDown are windows-specific. See #2623.

In reply to: 377889221 [](ancestors = 377889221,377886796)

Copy link
Contributor Author

@kaiguo kaiguo Feb 11, 2020

Choose a reason for hiding this comment

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

Looks like we are re-routing the events to onPress so we probably shouldn't remove them.

https://github.com/microsoft/react-native-windows/blob/master/vnext/src/Libraries/Components/Touchable/TouchableHighlight.windows.js#L453-L474

Copy link
Contributor Author

@kaiguo kaiguo Feb 11, 2020 •
edited by ddalp
Loading

Choose a reason for hiding this comment

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

@ddalp just checked there's no keyDown/Up events on TextInput, there's only onKeyPress. We are sending out the onPressed event in _onKeyUp callback, so I think we should keep it as is.

Also added a test to verify keyPress event on TextInput. #Resolved

ddalp
ddalp approved these changes Feb 11, 2020
View reviewed changes
Copy link
Contributor

@ddalp ddalp left a comment

Choose a reason for hiding this comment

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

:shipit:

@kaiguo kaiguo merged commit 13fb9b9 into microsoft:master Feb 11, 2020
@kaiguo kaiguo deleted the touchable-focus-blur-events branch February 11, 2020 23:55
@jagorrin
Copy link
Contributor

jagorrin commented Feb 19, 2020

When can I expect this fix to be included in a release?

@kmelmon
Copy link
Contributor

kmelmon commented Feb 19, 2020

@jagorrin The fix is available now, but the published release it's part of is in beta (0.61-beta). Version 61 will likely stay in beta for at least another week. Do you need this to be part of a previous release?

@jagorrin
Copy link
Contributor

jagorrin commented Feb 19, 2020 •
edited
Loading

@kmelmon Would it be possible for this to be cherry-picked into a new 0.60-vnext release? It would be great to be able to have this fix without having to update to the beta version of 0.61 to get it.

If so, I have two other fixes that are in the same spot - available only in the 0.61-beta releases, but would be great to have in a new 0.60-vnext release:

@kmelmon
Copy link
Contributor

kmelmon commented Feb 20, 2020 •
edited
Loading

@jagorrin all that's required is to submit the cherry-picks as PRs into the 0.60-stable branch. Once checked in, our CI loop will run and publish to npm. If you can prepare the PRs, I'd be happy to review and approve.

@jagorrin jagorrin mentioned this pull request Feb 20, 2020
Closed
kmelmon pushed a commit that referenced this pull request Feb 21, 2020
@jagorrin @kaiguo
Let children onFocus onBlur events in TouchableWithoutFeedback pass t…
d4ee9dc 
…hrough to parent (#4078) (#4150)

* Cherry-pick fix for 4078

* Remove duplicate keys

* Remove onFocus and onBlur events

Co-authored-by: Kai Guo <guokai.ok@gmail.com>
@jagorrin jagorrin mentioned this pull request Feb 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marlenecota marlenecota Awaiting requested review from marlenecota

@jonthysell jonthysell Awaiting requested review from jonthysell

@acoates-ms acoates-ms Awaiting requested review from acoates-ms

@kmelmon kmelmon Awaiting requested review from kmelmon

1 more reviewer

@ddalp ddalp ddalp approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

TextInput's onFocus and onBlur functions are not being executed

4 participants

@kaiguo @jagorrin @kmelmon @ddalp