-
Notifications
You must be signed in to change notification settings - Fork 63
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
Fix IME window insets and view offset when keyboard appears #1199
Conversation
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/uikit/KeyboardOverlapHeight.uikit.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/uikit/KeyboardOverlapHeight.uikit.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/uikit/KeyboardOverlapHeight.uikit.kt
Outdated
Show resolved
Hide resolved
# Conflicts: # compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/scene/ComposeSceneMediator.uikit.kt # compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/scene/UIViewComposeSceneLayer.uikit.kt
Updates: Keyboard overlapping logic was updated. Also lots of bugs connected with platform layers where fixed. PTAL. |
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/uikit/KeyboardOverlapHeight.uikit.kt
Outdated
Show resolved
Hide resolved
...ui/ui/src/uikitMain/kotlin/androidx/compose/ui/uikit/KeyboardSceneDisplayParameters.uikit.kt
Outdated
Show resolved
Hide resolved
...ui/ui/src/uikitMain/kotlin/androidx/compose/ui/uikit/KeyboardSceneDisplayParameters.uikit.kt
Outdated
Show resolved
Hide resolved
...ui/ui/src/uikitMain/kotlin/androidx/compose/ui/uikit/KeyboardSceneDisplayParameters.uikit.kt
Outdated
Show resolved
Hide resolved
...ui/ui/src/uikitMain/kotlin/androidx/compose/ui/uikit/KeyboardSceneDisplayParameters.uikit.kt
Outdated
Show resolved
Hide resolved
...ui/ui/src/uikitMain/kotlin/androidx/compose/ui/uikit/KeyboardSceneDisplayParameters.uikit.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/window/ComposeContainer.uikit.kt
Show resolved
Hide resolved
...e/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/window/ComposeSceneKeyboardOffsetManager.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/uikit/SoftwareKeyboardState.uikit.kt
Outdated
Show resolved
Hide resolved
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.
After discussion with @ASalavei we found that the problem is in incorrect coordinates conversion, so it must be fixed instead of introducing a new offset for selection handles.
...on/src/uikitMain/kotlin/androidx/compose/foundation/text/selection/SelectionHandles.uikit.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/uikit/SoftwareKeyboardState.uikit.kt
Outdated
Show resolved
Hide resolved
...e/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/window/ComposeSceneKeyboardOffsetManager.kt
Outdated
Show resolved
Hide resolved
# Conflicts: # compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/scene/ComposeSceneMediator.uikit.kt
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.
Just a couple of nitpicks, but looks good overall.
Could you please also add a couple of unit tests to avoid regressions?
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/platform/PlatformInsets.uikit.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/scene/ComposeSceneMediator.uikit.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/scene/ComposeSceneMediator.uikit.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/uikit/KeyboardOverlapHeight.uikit.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/window/ComposeContainer.uikit.kt
Outdated
Show resolved
Hide resolved
…election handlers location
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.
In general, the code LGTM. But it requires second approval from team mates
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/scene/ComposeSceneMediator.uikit.kt
Outdated
Show resolved
Hide resolved
...on/src/uikitMain/kotlin/androidx/compose/foundation/text/selection/SelectionHandles.uikit.kt
Outdated
Show resolved
Hide resolved
# Conflicts: # compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/window/IntermediateTextInputUIView.uikit.kt
animationView.setFrame(animationTargetFrame) | ||
}, | ||
completion = { isFinished -> | ||
keyboardDisplayLink.invalidate() |
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.
Should we also set keyboardAnimationListener
to nil
here? Unlike usual target:selector:
APIs, CADisplayLink
strongly retains its target. Could that lead to a retain-cycle that GC can't break?
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.
Thanks, that's make sense! I added nullification code.
dispatch_async(dispatch_get_main_queue()) { | ||
keyboardDisplayLink.addToRunLoop(NSRunLoop.mainRunLoop(), NSDefaultRunLoopMode) |
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.
Can that potentially trigger race condition if for some reason animation at 208
is immediately cancelled for some reason? Its completion handler will execute with displayLink being invalidated, but then the displayLink will be added to runloop after.
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.
Thanks for spotting. I checked various scenarios and it looks like everything works pretty fine. Invalidated display ling can be added successfully to run loop with no further effect.
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.
Two mildly suspicious places, but otherwise LGTM.
Proposed Changes
UIViewComposeSceneLayer
. Untie it from the view controller life cycle.ImeWindowInsets
class andState<Float>
to calculate and pass insets for each individual compose window.Fixes JetBrains/compose-multiplatform#4016
Fixes JetBrains/compose-multiplatform#4618