-
Notifications
You must be signed in to change notification settings - Fork 82
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Fix IME candidate popup position for BasicTextField2 #1794
Conversation
Two reviewers due to the addition of |
0803741
to
c15e398
Compare
...ation/foundation/src/commonMain/kotlin/androidx/compose/foundation/text/TextFieldDelegate.kt
Outdated
Show resolved
Hide resolved
@ExperimentalComposeUiApi | ||
fun updateSelectionState(newState: TextFieldValue) = Unit | ||
|
||
@ExperimentalComposeUiApi | ||
fun notifyFocusedRect(rect: Rect) = Unit |
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.
It's not only desktop change, but I see only Desktop related tests. How does it work for iOS? Web?
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.
It should be a desktop-only change. The other platforms don't implement the new method.
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.
What about web? It seems that they need to implement something like this.
cc @Schahen to check if web should be affected or not
cc @mazunin-v-jb to confirm that iOS doesn't need this functionality in any form
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.
iOS will need that, but for now, iOS doesn't use that method. There's no need to implement it here, I'll do it later myself
...se/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/PlatformTextInputSession.skiko.kt
Show resolved
Hide resolved
…tField2 (PlatformTextInputSession.platformSpecificTextInputSession).
c15e398
to
804fa1c
Compare
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.
Checked the snippet on Windows - works as well
PlatformTextInputSession.platformSpecificTextInputSession now calls
DesktopTextInputService.notifyFocusedRect
to tell the system where to place the IME candidate window for languages such as Chinese.Fixes https://youtrack.jetbrains.com/issue/CMP-7467
Testing
WindowTypingLocationTest
.This should be tested by QA
Release Notes
Fixes - Desktop
TextField(TextFieldState)
(akaBasicTextField2
).