Emit TextInput onChangeText before onSelectionChange #7742
Conversation
Previously, TextInputViewManager subscribed to two events to implement `onChangeText`: 1. TextChanging - used to increment a native counter to ensure JS changes to text do not overwrite native changes to text. 2. TextChanged - used to emit the `onChangeText` event and surprisingly also incremented the native counter. Additionally, the view manager subscribed to SelectionChanged to emit the `onSelectionChange` event. In XAML, the sequence of native events is as follows: 1. TextChanging 2. SelectionChanged 3. TextChanged Since we emit the event that ultimately triggers `onChangeText` via the TextChanged native event, this means that we actually receive the cursor position change before the text change. This is different from other React Native platforms and makes it more difficult to implement behaviors based off the se`onSelectionChange` event. One concern with this change is if there are any repercussions of eliminating the two phased approach to the native counter and firing of the event. Here is why I don't believe this will create any issues: 1. Previously, we incremented `m_nativeEventCount` twice, once in `TextChanging` and once in `TextChanged`. Reducing this to incrementing only once will not introduce any bugs, as JavaScript does not increment the `mostRecentEventCount` prop, so the only requirement is that a native change to the TextBox content increases the count. 2. Previously, we emitted the event in `TextChanged`. It appears that the `TextBox.Text()` property is already updated at the time that `TextChanging` is called, whether the `Text()` property was set externally or it was updated internally by a key event. The only difference between the two events is that `TextChanged` is called after the text is rendered (which may occur asynchronously from the actual text change). This is the reason we needed to introduce the dependency on `TextChanging` in the first place, as `TextChanging` is guaranteed to be called synchronously with the value change (and could not be interrupted by, e.g., an update from JS). 3. Is it a problem that JavaScript receives the text changed event before it actually renders? This is a good question, but I can't think of any scenarios where it's important that the TextBox has actually rendered the change before emitting the event, and since JS runs asynchronously from the UI thread, it's likely that by the time any UI changes / side effects triggered by JS make their way back to the UI thread, the text will have been rendered. Fixes microsoft#7740
|
Any ideas on how we could test this sort of change? |
It would probably be pretty trivial to create a test with an event counter, simulate typing a character and assert that the event count for onSelectionChange is less than the event count for onChange. I'm not sure how WinAppDriver works, but if there's a way to simulate typing a character (that would also move the cursor forward), then I should be able to add an e2e test for this. |
|
We are able to simulate typing into a TextInput. See below for an example: |
|
Make sure this doesn't break the recently fixed scenario in #7658 |
I don't believe it does for the reasons I documented. This really shouldn't change anything other than the order of events, since I don't believe we should have had any dependencies on the |
|
@NickGerleman I added an E2E test for this. Please take another look. |
|
@rozele you will have to run |
NickGerleman
added
the
AutoMerge
|
Hello @NickGerleman! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit 3 participants
Previously, TextInputViewManager subscribed to two events to implement
onChangeText:onChangeTextevent and surprisingly also incremented the native counter.Additionally, the view manager subscribed to SelectionChanged to emit the
onSelectionChangeevent.In XAML, the sequence of native events is as follows:
Since we emit the event that ultimately triggers
onChangeTextvia the TextChanged native event, this means that we actually receive the cursor position change before the text change. This is different from other React Native platforms and makes it more difficult to implement behaviors based off the seonSelectionChangeevent.One concern with this change is if there are any repercussions of eliminating the two phased approach to the native counter and firing of the event. Here is why I don't believe this will create any issues:
Previously, we incremented
m_nativeEventCounttwice, once inTextChangingand once inTextChanged. Reducing this to incrementing only once will not introduce any bugs, as JavaScript does not increment themostRecentEventCountprop, so the only requirement is that a native change to the TextBox content increases the count.Previously, we emitted the event in
TextChanged. It appears that theTextBox.Text()property is already updated at the time thatTextChangingis called, whether theText()property was set externally or it was updated internally by a key event. The only difference between the two events is thatTextChangedis called after the text is rendered (which may occur asynchronously from the actual text change). This is the reason we needed to introduce the dependency onTextChangingin the first place, asTextChangingis guaranteed to be called synchronously with the value change (and could not be interrupted by, e.g., an update from JS).Is it a problem that JavaScript receives the text changed event before it actually renders? This is a good question, but I can't think of any scenarios where it's important that the TextBox has actually rendered the change before emitting the event, and since JS runs asynchronously from the UI thread, it's likely that by the time any UI changes / side effects triggered by JS make their way back to the UI thread, the text will have been rendered.
Fixes #7740
Microsoft Reviewers: Open in CodeFlow