Skip to content
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

Refactor Legacy Text Input to use PlatformTextInputSession #1974

Open
wants to merge 17 commits into
base: jb-main
Choose a base branch
from

Conversation

ASalavei
Copy link

@ASalavei ASalavei commented Apr 1, 2025

Refactor LegacyPlatformTextInputServiceAdapter to use PlatformTextInputSession API instead of PlatformTextInputService.

Fixes https://youtrack.jetbrains.com/issue/CMP-7729/Keyboard-showing-up-when-preventing-it
Fixes https://youtrack.jetbrains.com/issue/CMP-7771/Keyboard-no-longer-show-up-if-it-was-dismiss-once-before-when-using-TextField-with-TextFieldState-on-iOS

Release Notes

Fixes - iOS

  • Fix the keyboard reappearing after it was dismissed via LocalSoftwareKeyboardController in BasicTextField(TextFieldState)

Fixes - Multiple Platforms

  • Fix InterceptPlatformTextInput for the legacy TextField

Breaking Changes - Multiple Platforms

  • A custom implementation for deprecated LocalTextInputService is no longer supported

@ASalavei ASalavei requested review from m-sasha and igordmn April 1, 2025 13:47
@ASalavei ASalavei marked this pull request as ready for review April 1, 2025 13:47
@ASalavei ASalavei requested a review from mazunin-v-jb April 1, 2025 13:47
@ASalavei
Copy link
Author

ASalavei commented Apr 1, 2025

Please take a look. Also we have to decide if we want to see this fix it in the 1.8.0 release

@ASalavei ASalavei marked this pull request as draft April 1, 2025 14:32
@ASalavei ASalavei changed the title Move Legacy Text Input to PlatformTextInputSession Refactor Legacy Text Input to use PlatformTextInputSession Apr 2, 2025
@m-sasha m-sasha self-requested a review April 2, 2025 08:40
Copy link
Member

@m-sasha m-sasha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, it's a bit strange to delegate legacy input service to new input service which delegates back to the legacy input service on iOS/web. But if it's deemed necessary, then ok.

@ASalavei
Copy link
Author

ASalavei commented Apr 2, 2025

As discussed, it's a bit strange to delegate legacy input service to new input service which delegates back to the legacy input service on iOS/web. But if it's deemed necessary, then ok.

@m-sasha , not exactly. The MR basically drops usage of the PlatformTextInputService API and redirects calls to the new PlatformTextInputSession API. PlatformTextInputService won't be used anymore for all targets and can be removed. I have to discuss this change with @igordmn.

ASalavei added 2 commits April 4, 2025 14:14
# Conflicts:
#	compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/platform/UIKitTextInputService.uikit.kt
Copy link

@mazunin-v-jb mazunin-v-jb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm too

@mazunin-v-jb mazunin-v-jb self-requested a review April 4, 2025 14:33
@mazunin-v-jb
Copy link

However, it breaks keyboard behavior

val keyboard = LocalSoftwareKeyboardController.current
keyboard?.hide()

This code does nothing in your implementation, probably something is missing from the old realization.

@ASalavei
Copy link
Author

ASalavei commented Apr 7, 2025

However, it breaks keyboard behavior

@mazunin-v-jb , thanks for checking. Fixed.

@ASalavei ASalavei marked this pull request as ready for review April 7, 2025 09:56
@ASalavei ASalavei requested a review from m-sasha April 7, 2025 16:20
@ASalavei ASalavei requested a review from Schahen April 7, 2025 16:21
@ASalavei ASalavei requested a review from m-sasha April 11, 2025 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants