react-native-windows icon indicating copy to clipboard operation
react-native-windows copied to clipboard

[Fabric] Implement the onPressOut property for the fabric implementation of TextInput

Open HariniMalothu17 opened this issue 7 months ago • 4 comments

Description

Type of Change

  • New feature (non-breaking change which adds functionality)

Why

Implement the onPressOut property for the fabric implementation of TextInput.

This property was available in RNW Paper via TextInputViewManager.

See https://reactnative.dev/docs/textinput#onpressout for details.

Resolves [https://github.com/microsoft/react-native-windows/issues/13128]

What

Implement the onPressOut property for the fabric implementation of TextInput.

Screenshots

https://github.com/user-attachments/assets/f0903a65-f869-4692-b8c2-52219522bee8

Testing

If you added tests that prove your changes are effective or that your feature works, add a few sentences here detailing the added test scenarios.

Changelog

yes

Add a brief summary of the change to use in the release notes for the next release.

Microsoft Reviewers: Open in CodeFlow

HariniMalothu17 avatar Jun 16 '25 10:06 HariniMalothu17

onPressIn and onPressOut are implemented using a pressable inside the JS in TextInput. https://github.com/facebook/react-native/blob/33aa7b91617bb6e2b9689850e8740c97e01464de/packages/react-native/Libraries/Components/TextInput/TextInput.js#L619

We shouldn't be emitting these events from native.

This looks like onPressIn was implemented incorrectly. Instead we should be removing these native events, and determining why the usePressability was/is not firing the onPressIn/Out on the TextInput.

acoates-ms avatar Jun 17 '25 14:06 acoates-ms

onPressIn and onPressOut are implemented using a pressable inside the JS in TextInput. https://github.com/facebook/react-native/blob/33aa7b91617bb6e2b9689850e8740c97e01464de/packages/react-native/Libraries/Components/TextInput/TextInput.js#L619

We shouldn't be emitting these events from native.

This looks like onPressIn was implemented incorrectly. Instead we should be removing these native events, and determining why the usePressability was/is not firing the onPressIn/Out on the TextInput.

@acoates-ms We can track this as an enhancement task. I've created an issue to follow up. Currently, all touch-related events like onKeyUp, onKeyDown, onFocus, onBlur, and other touch handlers are being emitted from the native side. As part of future improvements, we plan to shift these to be handled through Pressability in JavaScript instead. https://github.com/microsoft/react-native-windows/issues/14798

HariniMalothu17 avatar Jun 18 '25 08:06 HariniMalothu17

onPressIn and onPressOut are implemented using a pressable inside the JS in TextInput. https://github.com/facebook/react-native/blob/33aa7b91617bb6e2b9689850e8740c97e01464de/packages/react-native/Libraries/Components/TextInput/TextInput.js#L619 We shouldn't be emitting these events from native. This looks like onPressIn was implemented incorrectly. Instead we should be removing these native events, and determining why the usePressability was/is not firing the onPressIn/Out on the TextInput.

@acoates-ms We can track this as an enhancement task. I've created an issue to follow up. Currently, all touch-related events like onKeyUp, onKeyDown, onFocus, onBlur, and other touch handlers are being emitted from the native side. As part of future improvements, we plan to shift these to be handled through Pressability in JavaScript instead. #14798

I dont think we should check this in as is. This is code that would all have to be removed to do the proper fix. We should be removing the onPressIn code that was already added, and looking at why pressable isn't working.

acoates-ms avatar Jun 19 '25 03:06 acoates-ms

onPressIn and onPressOut are implemented using a pressable inside the JS in TextInput. https://github.com/facebook/react-native/blob/33aa7b91617bb6e2b9689850e8740c97e01464de/packages/react-native/Libraries/Components/TextInput/TextInput.js#L619

We shouldn't be emitting these events from native.

This looks like onPressIn was implemented incorrectly. Instead we should be removing these native events, and determining why the usePressability was/is not firing the onPressIn/Out on the TextInput.

Is this a change in Fabric? We fire omPressIn and onPressOut in TextInput ViewManager - just because they're defined in Pressable, don't we still need to fire them?

Edit: TextInput has its own onPressIn and onPressOut events that don't require you to wrap it in a Pressable: https://reactnative.dev/docs/textinput#onpressin

jonthysell avatar Jun 19 '25 19:06 jonthysell

onPressIn and onPressOut are implemented using a pressable inside the JS in TextInput. https://github.com/facebook/react-native/blob/33aa7b91617bb6e2b9689850e8740c97e01464de/packages/react-native/Libraries/Components/TextInput/TextInput.js#L619 We shouldn't be emitting these events from native. This looks like onPressIn was implemented incorrectly. Instead we should be removing these native events, and determining why the usePressability was/is not firing the onPressIn/Out on the TextInput.

Is this a change in Fabric? We fire omPressIn and onPressOut in TextInput ViewManager - just because they're defined in Pressable, don't we still need to fire them?

Edit: TextInput has its own onPressIn and onPressOut events that don't require you to wrap it in a Pressable: https://reactnative.dev/docs/textinput#onpressin

onPressIn and onPressOut are implemented using a pressable inside the JS in TextInput. https://github.com/facebook/react-native/blob/33aa7b91617bb6e2b9689850e8740c97e01464de/packages/react-native/Libraries/Components/TextInput/TextInput.js#L619 We shouldn't be emitting these events from native. This looks like onPressIn was implemented incorrectly. Instead we should be removing these native events, and determining why the usePressability was/is not firing the onPressIn/Out on the TextInput.

Is this a change in Fabric? We fire omPressIn and onPressOut in TextInput ViewManager - just because they're defined in Pressable, don't we still need to fire them?

Edit: TextInput has its own onPressIn and onPressOut events that don't require you to wrap it in a Pressable: https://reactnative.dev/docs/textinput#onpressin

@acoates-ms
Since TextInput already supports onPressIn and onPressOut natively (as outlined here: https://reactnative.dev/docs/textinput#onpressin) and doesn't need to be wrapped in a Pressable, are we good to close this PR? Let me know if you have any additional thoughts on whether these events should still be triggered via Pressable.

HariniMalothu17 avatar Jun 25 '25 17:06 HariniMalothu17

@jonthysell @acoates-ms This pr has been open for a long time. Looking for a final signoff on what steps can be taken further

HariniMalothu17 avatar Jul 14 '25 08:07 HariniMalothu17

@jonthysell @acoates-ms This pr has been open for a long time. Looking for a final signoff on what steps can be taken further

Merging this pr for now. Will take this up further in enhancement task

HariniMalothu17 avatar Jul 15 '25 08:07 HariniMalothu17