From c2dde6a1a2c288adb8520d7e306540273261d55f Mon Sep 17 00:00:00 2001 From: Eric Rozell Date: Fri, 7 May 2021 11:27:59 -0400 Subject: [PATCH 1/6] Emit TextInput onChangeText before onSelectionChange 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 #7740 --- .../Views/TextInputViewManager.cpp | 37 +++++-------------- 1 file changed, 10 insertions(+), 27 deletions(-) diff --git a/vnext/Microsoft.ReactNative/Views/TextInputViewManager.cpp b/vnext/Microsoft.ReactNative/Views/TextInputViewManager.cpp index ba338c8af0c..523cb5ff27e 100644 --- a/vnext/Microsoft.ReactNative/Views/TextInputViewManager.cpp +++ b/vnext/Microsoft.ReactNative/Views/TextInputViewManager.cpp @@ -154,7 +154,6 @@ class TextInputShadowNode : public ShadowNodeBase { private: xaml::Controls::TextBox::TextChanging_revoker m_textBoxTextChangingRevoker{}; - xaml::Controls::TextBox::TextChanged_revoker m_textBoxTextChangedRevoker{}; xaml::Controls::TextBox::SelectionChanged_revoker m_textBoxSelectionChangedRevoker{}; xaml::Controls::TextBox::ContextMenuOpening_revoker m_textBoxContextMenuOpeningRevoker{}; @@ -177,9 +176,6 @@ void TextInputShadowNode::createView(const winrt::Microsoft::ReactNative::JSValu } void TextInputShadowNode::dispatchTextInputChangeEvent(winrt::hstring newText) { - if (!m_initialUpdateComplete) { - return; - } m_nativeEventCount++; folly::dynamic eventData = folly::dynamic::object("target", m_tag)("text", HstringToDynamic(newText))("eventCount", m_nativeEventCount); @@ -203,36 +199,23 @@ void TextInputShadowNode::registerEvents() { // TextChanging is used to drop the Javascript response of 'A' and expect // another TextChanged event with correct event count. if (m_isTextBox) { + m_passwordBoxPasswordChangedRevoker = {}; m_passwordBoxPasswordChangingRevoker = {}; - m_textBoxTextChangingRevoker = - control.as().TextChanging(winrt::auto_revoke, [=](auto &&, auto &&) { - if (m_initialUpdateComplete) - m_nativeEventCount++; - }); + auto textBox = control.as(); + m_textBoxTextChangingRevoker = textBox.TextChanging( + winrt::auto_revoke, [=](auto &&, auto &&) { dispatchTextInputChangeEvent(textBox.Text()); }); } else { m_textBoxTextChangingRevoker = {}; - + auto passwordBox = control.as(); if (control.try_as()) { - m_passwordBoxPasswordChangingRevoker = - control.as().PasswordChanging(winrt::auto_revoke, [=](auto &&, auto &&) { - if (m_initialUpdateComplete) - m_nativeEventCount++; - }); + m_passwordBoxPasswordChangingRevoker = passwordBox.PasswordChanging( + winrt::auto_revoke, [=](auto &&, auto &&) { dispatchTextInputChangeEvent(passwordBox.Password()); }); + } else { + m_passwordBoxPasswordChangedRevoker = passwordBox.PasswordChanged( + winrt::auto_revoke, [=](auto &&, auto &&) { dispatchTextInputChangeEvent(passwordBox.Password()); }); } } - if (m_isTextBox) { - m_passwordBoxPasswordChangedRevoker = {}; - auto textBox = control.as(); - m_textBoxTextChangedRevoker = textBox.TextChanged( - winrt::auto_revoke, [=](auto &&, auto &&) { dispatchTextInputChangeEvent(textBox.Text()); }); - } else { - m_textBoxTextChangedRevoker = {}; - auto passwordBox = control.as(); - m_passwordBoxPasswordChangedRevoker = passwordBox.PasswordChanged( - winrt::auto_revoke, [=](auto &&, auto &&) { dispatchTextInputChangeEvent(passwordBox.Password()); }); - } - if (m_isTextBox) { m_passwordBoxContextMenuOpeningRevoker = {}; auto textBox = control.as(); From f46670c1380ee8515a51031029bbe33c679a9fb4 Mon Sep 17 00:00:00 2001 From: Eric Rozell Date: Fri, 7 May 2021 12:07:38 -0400 Subject: [PATCH 2/6] Change files --- ...ative-windows-60609fec-a316-43d0-b789-deb31e09b2f7.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/react-native-windows-60609fec-a316-43d0-b789-deb31e09b2f7.json diff --git a/change/react-native-windows-60609fec-a316-43d0-b789-deb31e09b2f7.json b/change/react-native-windows-60609fec-a316-43d0-b789-deb31e09b2f7.json new file mode 100644 index 00000000000..03bde7d0c45 --- /dev/null +++ b/change/react-native-windows-60609fec-a316-43d0-b789-deb31e09b2f7.json @@ -0,0 +1,7 @@ +{ + "type": "prerelease", + "comment": "Emit TextInput onChangeText before onSelectionChange", + "packageName": "react-native-windows", + "email": "erozell@outlook.com", + "dependentChangeType": "patch" +} From 8034cae259302e51d044386ecdfccc896dd03bc3 Mon Sep 17 00:00:00 2001 From: Eric Rozell Date: Fri, 7 May 2021 13:30:17 -0400 Subject: [PATCH 3/6] Applying yarn format --- vnext/Microsoft.ReactNative/Views/TextInputViewManager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vnext/Microsoft.ReactNative/Views/TextInputViewManager.cpp b/vnext/Microsoft.ReactNative/Views/TextInputViewManager.cpp index 523cb5ff27e..5e0f27ba239 100644 --- a/vnext/Microsoft.ReactNative/Views/TextInputViewManager.cpp +++ b/vnext/Microsoft.ReactNative/Views/TextInputViewManager.cpp @@ -203,7 +203,7 @@ void TextInputShadowNode::registerEvents() { m_passwordBoxPasswordChangingRevoker = {}; auto textBox = control.as(); m_textBoxTextChangingRevoker = textBox.TextChanging( - winrt::auto_revoke, [=](auto &&, auto &&) { dispatchTextInputChangeEvent(textBox.Text()); }); + winrt::auto_revoke, [=](auto &&, auto &&) { dispatchTextInputChangeEvent(textBox.Text()); }); } else { m_textBoxTextChangingRevoker = {}; auto passwordBox = control.as(); From 31ff4d2c54c56edeb5e6771deefd27c41bd353a5 Mon Sep 17 00:00:00 2001 From: Eric Rozell Date: Tue, 11 May 2021 19:36:15 -0400 Subject: [PATCH 4/6] Adds E2E test for order of events --- .../test/LegacyTextInputTest.test.ts | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/packages/e2e-test-app/test/LegacyTextInputTest.test.ts b/packages/e2e-test-app/test/LegacyTextInputTest.test.ts index 7b5a3ba0be2..034dd8f8448 100644 --- a/packages/e2e-test-app/test/LegacyTextInputTest.test.ts +++ b/packages/e2e-test-app/test/LegacyTextInputTest.test.ts @@ -74,6 +74,15 @@ describe('LegacyTextInputTest', () => { expect(await textInput.getText()).toBe('abc\rdef'); }); + + test('TextInput onChange before onSelectionChange', async () => { + const textInput = await textInputField(); + await textInput.setValue('a'); + await assertLogContainsInOrder( + 'onChange text: a', + 'onSelectionChange range: 1,1', + ); + }); }); async function textInputField() { @@ -101,3 +110,28 @@ async function assertLogContains(text: string) { }, ); } + +async function assertLogContainsInOrder(...expectedLines: string[]) { + const textLogComponent = await $('~textinput-log'); + + await browser.waitUntil( + async () => { + const loggedText = await textLogComponent.getText(); + const actualLines = loggedText.split('\n'); + let previousIndex = Number.MAX_VALUE; + for (const line of expectedLines) { + const index = actualLines.findIndex(l => l === line) + if (index === -1 || index > previousIndex) { + return false; + } + + previousIndex = index; + } + + return true; + }, + { + timeoutMsg: `"${await textLogComponent.getText()}" did not contain lines "${expectedLines.join(', ')}"`, + }, + ); +} From 830b6938bf4c416ff4d774b5fa6e4e1742db349c Mon Sep 17 00:00:00 2001 From: Eric Rozell Date: Tue, 11 May 2021 20:41:04 -0400 Subject: [PATCH 5/6] Adding back inadvertently removed guard --- vnext/Microsoft.ReactNative/Views/TextInputViewManager.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/vnext/Microsoft.ReactNative/Views/TextInputViewManager.cpp b/vnext/Microsoft.ReactNative/Views/TextInputViewManager.cpp index 5e0f27ba239..f13e35d094a 100644 --- a/vnext/Microsoft.ReactNative/Views/TextInputViewManager.cpp +++ b/vnext/Microsoft.ReactNative/Views/TextInputViewManager.cpp @@ -176,6 +176,9 @@ void TextInputShadowNode::createView(const winrt::Microsoft::ReactNative::JSValu } void TextInputShadowNode::dispatchTextInputChangeEvent(winrt::hstring newText) { + if (!m_initialUpdateComplete) { + return; + } m_nativeEventCount++; folly::dynamic eventData = folly::dynamic::object("target", m_tag)("text", HstringToDynamic(newText))("eventCount", m_nativeEventCount); From fb70fba8368ba478153134e85413150b99509502 Mon Sep 17 00:00:00 2001 From: Eric Rozell Date: Wed, 12 May 2021 14:25:53 -0400 Subject: [PATCH 6/6] yarn lint:fix --- packages/e2e-test-app/test/LegacyTextInputTest.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/e2e-test-app/test/LegacyTextInputTest.test.ts b/packages/e2e-test-app/test/LegacyTextInputTest.test.ts index 034dd8f8448..bdfe824da2e 100644 --- a/packages/e2e-test-app/test/LegacyTextInputTest.test.ts +++ b/packages/e2e-test-app/test/LegacyTextInputTest.test.ts @@ -120,7 +120,7 @@ async function assertLogContainsInOrder(...expectedLines: string[]) { const actualLines = loggedText.split('\n'); let previousIndex = Number.MAX_VALUE; for (const line of expectedLines) { - const index = actualLines.findIndex(l => l === line) + const index = actualLines.findIndex(l => l === line); if (index === -1 || index > previousIndex) { return false; } @@ -131,7 +131,9 @@ async function assertLogContainsInOrder(...expectedLines: string[]) { return true; }, { - timeoutMsg: `"${await textLogComponent.getText()}" did not contain lines "${expectedLines.join(', ')}"`, + timeoutMsg: `"${await textLogComponent.getText()}" did not contain lines "${expectedLines.join( + ', ', + )}"`, }, ); }