Skip to content

[Fabric] Implement the onPressIn property for the fabric implementation of TextInput #14480

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.

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

HariniMalothu17
Copy link

@HariniMalothu17 HariniMalothu17 commented Apr 3, 2025

Description

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

Why

Implement the onPressIn property for the fabric implementation of TextInput.
This property was available in RNW Paper via TextInputViewManager

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

Resolves #13127

What

Implemented the onPressIn property for the fabric implementation of TextInput.

Screenshots

Recording.2025-04-04.025511.mp4

Testing

Tested using playground-composition App,
Added an alert prompt to the onPressIn event when the TextInput is pressed.
Tested using RNTesterFabric app
image

Changelog

Yes

Added a brief summary of onPressIn to use in the release notes for the next release.

Microsoft Reviewers: Open in CodeFlow

@HariniMalothu17 HariniMalothu17 requested a review from a team as a code owner April 3, 2025 22:00
@HariniMalothu17 HariniMalothu17 changed the title User/hmalothu/implement on press in Implement the onPressIn property for the fabric implementation of TextInput Apr 3, 2025
@HariniMalothu17 HariniMalothu17 marked this pull request as draft April 3, 2025 22:18
@HariniMalothu17 HariniMalothu17 marked this pull request as ready for review April 3, 2025 22:24
@HariniMalothu17 HariniMalothu17 self-assigned this Apr 3, 2025
@HariniMalothu17 HariniMalothu17 changed the title Implement the onPressIn property for the fabric implementation of TextInput [Fabric] Implement the onPressIn property for the fabric implementation of TextInput Apr 3, 2025
@HariniMalothu17 HariniMalothu17 added API: Completion Area: TextInput New Architecture Broad category for issues that apply to the RN "new" architecture of Turbo Modules + Fabric Parity: Fabric vs. Paper RNW Fabric does not look or behave like RNW Paper enhancement labels Apr 3, 2025
@TatianaKapos
Copy link
Contributor

This is great! Can we also add an e2etest to make this change more stable in the future?

Instructions here: https://github.com/microsoft/react-native-windows/wiki/E2E-Testing-(Fabric)
Example PRs: https://github.com/microsoft/react-native-windows/pull/12746/files#diff-4e06b21930c2eef3b60c891c6816afb6379337dadef22b8f33e6667ecbba2f53 and https://github.com/microsoft/react-native-windows/pull/12771/files#diff-589ea5c45c3c5c3bf5e8ef865c8e86cc1c42719a07bbc316dc3e3d879954aa20

It also looks like rntester already has a TextInput you can refer too!

Copy link
Contributor

@jonthysell jonthysell left a comment

Choose a reason for hiding this comment

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

I think more than TextInput will require an onPressIn event and parts of this needs to be implemented up in HostPlatformViewEventEmitter rather than directly here.

For reference WindowsTextInputEventEmitter inherits from ViewEventEmitter which inherits from HostPlatformViewEventEmitter which inherits from BaseViewEventEmitter. HostPlatformViewEventEmitter is where we implement eventcode that multiple components will need.

@@ -66,5 +66,12 @@ void WindowsTextInputEventEmitter::onContentSizeChange(OnContentSizeChange event
return textInputMetricsContentSizePayload(runtime, event);
});
}
void WindowsTextInputEventEmitter::onPressIn(OnPressIn event) const {
Copy link
Contributor

Choose a reason for hiding this comment

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

The default version of this function should be implemented in HostPlatformViewEventEmitter and should dispatch to "pressIn".

Then, you should override that function here in WindowsTextInputEventEmitter to instead dispatch the event to "textInputPressIn".

See other comments for details.

Copy link
Author

Choose a reason for hiding this comment

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

Have updated the code .Please refer to the latest commit

@@ -41,11 +41,16 @@ class WindowsTextInputEventEmitter : public ViewEventEmitter {
facebook::react::Size contentSize;
};

struct OnPressIn {
Copy link
Contributor

Choose a reason for hiding this comment

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

This struct should instead be something like PressEvent and put it up in the HostPlatformViewEventEmitter for re-use elsewhere.

See https://reactnative.dev/docs/pressevent for the other fields the event should have in JS - I'm not sure if some (all?) of these need to be implemented in native code (some may be added in JS). You might need to update your test to display the event and verify what's present.

See other comments for details.

Copy link
Author

Choose a reason for hiding this comment

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

Have updated the code .Please refer to the latest commit

void onChange(OnChange value) const;
void onSelectionChange(const OnSelectionChange &value) const;
void onSubmitEditing(OnSubmitEditing value) const;
void onKeyPress(OnKeyPress value) const;
void onContentSizeChange(OnContentSizeChange value) const;
void onPressIn(OnPressIn value) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be implemented as a virtual fucntion in HostPlatformViewEventEmitter for re-use elsewhere.

See other comments for details.

Copy link
Author

Choose a reason for hiding this comment

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

Have updated the code .Please refer to the latest commit

@@ -659,6 +659,14 @@ void WindowsTextInputComponentView::OnPointerPressed(
auto hr = m_textServices->TxSendMessage(msg, static_cast<WPARAM>(wParam), static_cast<LPARAM>(lParam), &lresult);
args.Handled(hr != S_FALSE);
}

// Implements OnPressIn event
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably okay here for now, but it might need to be moved up into a more common place too, like in ComponentView.h

Copy link
Author

Choose a reason for hiding this comment

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

The updates have been made in the same file for now as it is implemented under PointerPressed

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) label Apr 9, 2025
@jonthysell jonthysell self-requested a review April 10, 2025 23:10
@anupriya13
Copy link
Contributor

anupriya13 commented Apr 14, 2025

@HariniMalothu17 @TatianaKapos I have added E2EFabricTestApp test cases here for TextInput #14461

@anupriya13 anupriya13 self-requested a review April 14, 2025 04:28
@TatianaKapos
Copy link
Contributor

@anupriya13 @HariniMalothu17 we will need to bring those changes in and make sure this PR changes that test.

@anupriya13
Copy link
Contributor

@anupriya13 @HariniMalothu17 we will need to bring those changes in and make sure this PR changes that test.

Fixing #14461 might some time. @HariniMalothu17 please add a new test case for this one.

Copy link
Contributor

@anupriya13 anupriya13 left a comment

Choose a reason for hiding this comment

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

Please add E2EFabric test case

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) label Apr 16, 2025
@@ -67,4 +67,20 @@ void WindowsTextInputEventEmitter::onContentSizeChange(OnContentSizeChange event
});
}

void WindowsTextInputEventEmitter::onPressIn(const PressEvent &event) const {
dispatchEvent("textInputPressIn", [event](jsi::Runtime &runtime) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Try making this event = std::move(event). Otherwise you are capturing a reference to an object that is going to go away. The dispatchEvent method will push things to the JS thread.

Copy link
Author

Choose a reason for hiding this comment

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

Updated

test('TextInput triggers onPressIn and updates state text', async () => {
// Find the TextInput component and trigger the onPressIn event
const input = await app.findElementByTestID('textinput-press');
await input.click(); // This triggers the onPressIn event
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes you'll need to search in the example search bar to make sure this component is visible before clicking. I would add this function (

const searchBox = async (input: string) => {
) to TextInputComponentTest. This method allows you to pass in a string with a few keys for the sample title and it will search in the search bar for it.

This should fix the click issue

Copy link
Author

Choose a reason for hiding this comment

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

Updated.Please refer to the latest commit

@HariniMalothu17 HariniMalothu17 marked this pull request as draft April 24, 2025 15:23
@HariniMalothu17 HariniMalothu17 marked this pull request as ready for review April 24, 2025 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API: Completion Area: TextInput enhancement New Architecture Broad category for issues that apply to the RN "new" architecture of Turbo Modules + Fabric Parity: Fabric vs. Paper RNW Fabric does not look or behave like RNW Paper
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement onPressIn property for TextInput for fabric
7 participants