[Fabric] Add onKeyPress for TextInput and enable legacyTextInputTests #12771
Conversation
| "IsKeyboardFocusable": false, | ||
| "LocalizedControlType": "text", | ||
| "Name": "onKeyPress key: a | ||
| onChange text: a |
There was a problem hiding this comment.
The text log is now being saved in the snapshot!
TatianaKapos
changed the title
vnext/Microsoft.ReactNative/Fabric/Composition/TextInput/WindowsTextInputComponentView.cpp
Outdated
Show resolved
Hide resolved
vnext/Microsoft.ReactNative/Fabric/Composition/TextInput/WindowsTextInputComponentView.cpp
Outdated
Show resolved
Hide resolved
|
Video with Backspace edgecase! Playground-composition_5YKeR0mLAn.mp4 |
| onChange text: ab | ||
| onSelectionChange range: 2,2 | ||
| onSelectionChange range: 1,1 | ||
| onChange text: a |
There was a problem hiding this comment.
Issue created for this: #12780, when the root issue of this gets solved we can remove the extra check for onKeyPress or add a check for onChange
| // save the last key pressed for onKeyPress | ||
| wchar_t key[2] = L" "; | ||
| key[0] = static_cast<wchar_t>(args.KeyCode()); | ||
| m_lastKeyPressed = ::Microsoft::Common::Unicode::Utf16ToUtf8(key, 1); |
There was a problem hiding this comment.
Any reason we dont just fire the onKeyPress event here?
There was a problem hiding this comment.
I originally had it there, but then onKeyPress would fire before onChange which is the opposite of what Paper does.
There was a problem hiding this comment.
Hmm.. I just checked in expo snack. On web and iOS onKeyPress fires before onChange. On android onChange fires first.
So it doesn't look like the order is properly defined right now.
I think I'd go with the simpler code. Assuming that it is simpler to just fire the event in OnCharacterReceived/OnKeyDown
There was a problem hiding this comment.
If we don't care about matching Paper exactly for the order of events, I can switch it back! It does make the code a lot more simpler. And actually reading the specs for TextInput, it looks like it should fire before (https://reactnative.dev/docs/textinput#onkeypress) so Paper may have a bug.
@jonthysell Would it be helpful to have a running document of intentional API differences between Paper/Fabric? I feel like it may make switching to Fabric easier but just an idea!
Description
Adds onKeyPress event to Fabric's TextInput and enables some legacy textInput tests
Type of Change
Why
Component Parity
What
We used to be able to get the Text out of a Text component on Paper but that currently doesn't work with Fabric (and may not be what we want). Changed LegacyTextInput to use snapshots instead.
Screenshots
Paper
ApplicationFrameHost_fNUflIy6yT.mp4
Fabric
Playground-composition_HSKn16woYF.mp4
Testing
tested locally and events showing up in legacytests
Changelog
no