-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[Fabric] Introduce autoCapitalize property for TextInput #13266
Conversation
vnext/Microsoft.ReactNative/Fabric/Composition/TextInput/WindowsTextInputComponentView.cpp
Show resolved
Hide resolved
vnext/Microsoft.ReactNative/Fabric/Composition/TextInput/WindowsTextInputComponentView.cpp
Show resolved
Hide resolved
} | ||
} | ||
|
||
m_textServices->TxSetText(wstrText.c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_textServices->TxSetText(wstrText.c_str()); | |
winrt::check_hresult(m_textServices->TxSetText(wstrText.c_str())); |
return true; | ||
} else if (autoCapitalizeType == "words") { | ||
BSTR bstr = nullptr; | ||
winrt::hresult hr = m_textServices->TxGetText(&bstr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
winrt::hresult hr = m_textServices->TxGetText(&bstr); | |
winrt::check_hresult(m_textServices->TxGetText(&bstr)); |
SysFreeString(bstr); | ||
} else if (autoCapitalizeType == "sentences") { | ||
BSTR bstr = nullptr; | ||
winrt::hresult hr = m_textServices->TxGetText(&bstr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
winrt::hresult hr = m_textServices->TxGetText(&bstr); | |
winrt::check_hresult(m_textServices->TxGetText(&bstr)); |
changelog should be yes and the comment could just be your PR title |
@@ -0,0 +1,7 @@ | |||
{ | |||
"type": "none", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"type": "none", | |
"type": "prerelease", |
"comment": "Implemented autoCapitalize prop for TextInput", | ||
"packageName": "react-native-windows", | ||
"email": "14967941+danielayala94@users.noreply.github.com", | ||
"dependentChangeType": "none" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"dependentChangeType": "none" | |
"dependentChangeType": "patch" |
} else if (capitalizationType == "sentences") { // DOUBLE CHECK THIS LOGIC | ||
bool newSentence = true; | ||
|
||
// Patterns to look at to declare newSentence as true: first character, or "? ", or "! ", or ". ". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does a new line count as a new Sentence? I would test this out on a multiline TextInput (mostly because I always miss that edgecase lol)!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also does this work if you copy/paste text into the TextInput? I'm not sure if it's supposed to work on that case (check expo snack) but I would make sure it doesn't cause an exception!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capitalization rules doesn't apply to characters inserted in the middle of the string, except if autoCapitalize='characters'
.
Testing the multiline scenario is a good observation, let me check what iOS/Android do and add new TextInputs to the E2E test app.
None - Do not autocapitalize anything. | ||
*/ | ||
|
||
BSTR bstr = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a winrt/wil version for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, wil::unique_bstr
. Initially I wanted to install WIL nupkg but it fails due to a conflict with another package, so to avoid delaying the work I opted to do manual allocation, as other parts of the code already do.
vnext/Microsoft.ReactNative/Fabric/Composition/TextInput/WindowsTextInputComponentView.cpp
Show resolved
Hide resolved
vnext/Microsoft.ReactNative/Fabric/Composition/TextInput/WindowsTextInputComponentView.cpp
Show resolved
Hide resolved
} else if (capitalizationType == "words") { | ||
bool nextWord = true; | ||
|
||
for (auto &ch : wstrText) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple questions here:
- do we need a empty string (not nullptr) check in the beginning?
- How do you deal with special characters in this loop? What if the string is just one special character?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should pretty much never try to do this kind of logic. The localization complexity of text is crazy.
You could maybe use EM_FINDWORDBREAK to move between words.
vnext/Microsoft.ReactNative/Fabric/Composition/TextInput/WindowsTextInputComponentView.cpp
Show resolved
Hide resolved
m_textServices->TxSetText(wstrText.c_str()); | ||
} | ||
|
||
SysFreeString(bstr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe if you use a wil wrapper you don't need to handle memory yourself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is correct, unfortunately I haven't been able to install WIL yet.
if (autoCapitalizeType == "characters") { | ||
return true; | ||
} else if (autoCapitalizeType == "words") { | ||
BSTR bstr = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wil/winrt wrapper? and brace initialization here?
// If autoCapitalizeType == "none" or different from characters/words/sentences, return false. | ||
// NOTE: TxGetText always returns a '\r' at the end of the string. See comments below for potential implications. | ||
bool WindowsTextInputComponentView::shouldAutoCapitalize() { | ||
std::string_view autoCapitalizeType = windowsTextInputProps().autoCapitalize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
brace initialization?
winrt::hresult hr = m_textServices->TxGetText(&bstr); | ||
|
||
if (SUCCEEDED(hr)) { | ||
std::wstring wstrText = std::wstring(bstr, SysStringLen(bstr)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
brace initialization?
} | ||
} | ||
|
||
SysFreeString(bstr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wil wrapper might be needed here.
WPARAM wParam; | ||
|
||
// Auto-capitalize the key when applicable | ||
if (shouldAutoCapitalize()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attempting to just do this on CharRecieved will miss a bunch of cases.
What if the text is set from JS? (Might be worth seeing what happens with a controlled text case in android/iOS).
What if the text is pasted in?
IME input?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AutoCorrectProc (https://learn.microsoft.com/en-us/windows/win32/api/richedit/nc-richedit-autocorrectproc) might be of some interest. Its possibly a better way to modify the text.
|
||
// Auto-capitalize the key when applicable | ||
if (shouldAutoCapitalize()) { | ||
wParam = static_cast<WPARAM>(std::towupper(static_cast<wchar_t>(args.KeyCode()))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is keycode always a wchar? -- What does this do with surrogate pairs?
void WindowsTextInputComponentView::autoCapitalizeOnUpdateProps(const std::string &capitalizationType) noexcept { | ||
/* | ||
Possible values are: | ||
Characters - All characters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For characters at least, we should just use EM_SETEDITSTYLE and SES_UPPERCASE - to have RichEdit do this properly.
Generally trying to do all this with Get/SetText is probably not the way to go about this. -- It scales pretty badly if the app has a large text field, and the interactions with other parts of the text stack are likely to cause other issues. |
I'm going to review the ideas proposed in this PR, and likely propose a new design for this feature in another PR. |
Description
Type of Change
New feature.
Why
Enables TextInput controls to autocapitalize letters, depending on the selected mode.
characters
: all characters are capitalized.words
: every first letter of a word is capitalized.sentences
: only the first letter of a sentence is capitalized.none
: no autocapitalization is performed.Resolves #13120
What
Introduced logic to update the TextInput during updateProps(), and added a new check in OnCharacterReceived() to determine if the character should be capitalized.
Screenshots
AutoCapitalize.mp4
Testing
Changelog
No
Microsoft Reviewers: Open in CodeFlow