Skip to content

Conversation

@protikbiswas100
Copy link
Contributor

@protikbiswas100 protikbiswas100 commented Oct 23, 2025

Description

TextInput regression between Paper and Fabric: Manual set value clearTextOnSubmit-style behavior not working

Type of Change

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

Why

The issue is that JavaScript receives onSubmitEditing with eventCount N, but when it immediately calls setValue(''), native has already moved to N+1, so the JS command gets rejected. This causes mismatch between JS and Native side

Resolves [https://github.com//issues/15168]

What

The issue is that JavaScript receives onSubmitEditing with eventCount N, but when it immediately calls setValue('') native has already moved to N+1, so the JS command gets rejected. To fix the issue the solution taken is don't increment the event count until AFTER the JavaScript has had a chance to respond to the submit event.

Screenshots

Before

15168-before.mp4

After

15168-after.mp4

Testing

yarn build passed successfully, added a sample textInput field in RNTester-Fabric app to repro the issue.

Ran E2ETestAppFabric Unit Test
build

Changelog

Should this change be included in the release notes: yes

Added synchroinization logic betwwen JS and native layer onClearSubmit is not working

Microsoft Reviewers: Open in CodeFlow
Microsoft Reviewers: Open in CodeFlow

@protikbiswas100 protikbiswas100 requested a review from a team as a code owner October 23, 2025 05:25
@protikbiswas100
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@protikbiswas100
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a regression in TextInput behavior between Paper and Fabric architectures where manual value clearing on submit (clearTextOnSubmit-style behavior) was not working correctly. The issue occurred when JavaScript immediately updated the text value during onSubmitEditing, causing synchronization problems between native and JS state.

Key Changes:

  • Modified event count validation to accept immediate JS updates by changing the condition from eventCount >= m_nativeEventCount to eventCount >= m_nativeEventCount - 1
  • Added state synchronization when clearing text to ensure React state stays consistent with native TextInput state

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
vnext/Microsoft.ReactNative/Fabric/Composition/TextInput/WindowsTextInputComponentView.cpp Modified event count check and added state update logic when clearing text on submit
change/react-native-windows-daac39ef-4292-4f66-81ae-708b1c0f6676.json Added change file for versioning

@anupriya13
Copy link
Contributor

anupriya13 commented Oct 23, 2025

In the video when you pressed enter in last text input why did it have the value "backslash" instead of empty?

@protikbiswas100
Copy link
Contributor Author

In the video when you pressed enter in last text input why did it have the value "backslash" instead of empty?

I pressed back slash key by mistake after pressing enter from my keyboard

@protikbiswas100
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@anupriya13
Copy link
Contributor

In the video when you pressed enter in last text input why did it have the value "backslash" instead of empty?

I pressed back slash key by mistake after pressing enter from my keyboard

Okay can you please re-record and attach correct before and after videos?

@anupriya13
Copy link
Contributor

anupriya13 commented Oct 23, 2025

Description Summary missing in the end.
"Add a brief summary of the change to use in the release notes for the next release."

@anupriya13
Copy link
Contributor

E2ETestAppFabric Unit Test verification?

@protikbiswas100
Copy link
Contributor Author

In the video when you pressed enter in last text input why did it have the value "backslash" instead of empty?

I pressed back slash key by mistake after pressing enter from my keyboard

Okay can you please re-record and attach correct before and after videos?

reuploaded before and after videos

@protikbiswas100
Copy link
Contributor Author

E2ETestAppFabric Unit Test verification

Added in next commit

@protikbiswas100
Copy link
Contributor Author

Description Summary missing in the end. "Add a brief summary of the change to use in the release notes for the next release."

It's already there in description

@protikbiswas100
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@protikbiswas100
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@protikbiswas100
Copy link
Contributor Author

E2ETestAppFabric Unit Test verification

Added in next commit

Just curios @protikbiswas100. Why not reply directly to the comment where the question was asked? I mean it'd be easier to reply and follow. What's the advantage of copying the comment to a blockquote and then replying. I find it hard to follow but if there're any merits in this I'd be happy to learn.

I cannot reply to the comments in the same thread, unless it comes from the review, the conversations that generate with comments I can only quote reply them

@protikbiswas100
Copy link
Contributor Author

Description Summary missing in the end. "Add a brief summary of the change to use in the release notes for the next release."

It's already there in description

I think it's needed for a tool to parse and post it as part of the release notes. The detailed PR description ≠ one bullet point of this change in release notes.

Added

@protikbiswas100
Copy link
Contributor Author

protikbiswas100 commented Nov 3, 2025

Description Summary missing in the end. "Add a brief summary of the change to use in the release notes for the next release."

Added it, also we should not add this line "Add a brief summary of the change to use in the release notes for the next release." if it is Yes, spoke with @sundaramramaswamy regarding this, you can look at the PRs from Redmond team about how a chnagelog file should be for yes #15013

@protikbiswas100
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@protikbiswas100
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@protikbiswas100
Copy link
Contributor Author

What changes were made to the codebase to solve the bug, add the functionality, etc. that you specified above.

Please drop this line from the description. (1) is template string; I see it there despite commenting on this on a previous PR. Please have the discipline to remove template strings that are noise from the PR description.

TextInput regression between Paper and Fabric: Manual set value clearTextOnSubmit-style behavior not working

Both the first line of description and Why sections have this same line. Is the duplication needed? This doesn't convey why this regression happened or how the PR fixes it.

Modified to if (eventCount >= m_nativeEventCount - 1) to allow immediate JS side changes.

Code diff already tells me this, but this line still doesn't convey what the change does. Please briefly write the why and then the how in the description.

I changed the why and what now they are different

@protikbiswas100
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

onSubmitEditingArgs.eventCount = m_nativeEventCount;
emitter->onSubmitEditing(onSubmitEditingArgs);

// Increment after emitting to allow JS to respond with matching count
Copy link
Contributor

Choose a reason for hiding this comment

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

The JS is run on a separate thread. So if the JS thread is busy at this time, there JS will not have responded by this point.
You can probably reproduce this by pausing the JS thread in the debugger while stepping through this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can keep a buffer of 2 between native and JS event counts, consider below example scenario

Native event N: User types, increment to N+1
Native event N+1: User presses Enter, emit onSubmitEditing with eventCount N+1, increment to N+2
JS thread might be busy here
Native event N+2: Maybe another keystroke, increment to N+3
Now JS thread processes onSubmitEditing and calls setValue('') with eventCount N+1
Native receives setValue('') with eventCount N+1
Check: N+3 - N+1 = 2 <= 2 -> Accepted

Why 2 events tolerance?

1 event behind: Common due to normal async delay
2 events behind: Covers cases where JS thread was busy and one additional user action occurred
More than 2: Likely indicates stale commands that should be rejected for safety

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you debug how this works in Android? The event count is a synchronization requirements. Accepting events out of order may cause input to be lost.

@protikbiswas100
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@protikbiswas100
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@protikbiswas100
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants