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

Fixing Android keyboard adjustResize not working with status bar tran… #2311

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nikhiltekwani09
Copy link

@nikhiltekwani09 nikhiltekwani09 commented Aug 21, 2024

…slucent

Description

In android, the adjustResize soft input mode is not working as expected when status bar is set as translucent.
react-native version: 0.74.3
react-native-screens version: 3.34.0

Fixes #2308

Changes

  • Fixed an issue with window insets where the keyboard (IME) height was not being accurately calculated.
  • Replaced the old handling of WindowInsetsCompat.Type.statusBars() with a new implementation that includes the keyboard inset (WindowInsetsCompat.Type.ime()).
  • This ensures correct bottom insets handling when the keyboard is open by taking the maximum of the system bars and IME insets.

Example where issue is reproducible - https://github.com/1880akshay/RN_NewArch_TextInput/tree/android-keyboard
The change which caused the issue - 52f13c6

@kirillzyusko
Copy link
Contributor

@nikhiltekwani09 I don't think it's a great fix. Adding keyboard insets (i. e. keyboard height) as bottom padding doesn't sound right to me (but it's my personal opinion - I'm not a maintainer of RNS, but I added the code that you are modifying).

Another question - why do we add keyboard insets only to Android R? Prior to that version everything works fine?

I also opened a separate issue in another repository: Expensify/react-native-live-markdown#346

The issue may be different but maybe it can give you some insights into where to search for the problem additionally!

@nikhiltekwani09
Copy link
Author

@kirillzyusko the things were working perfectly prior to android R
The commit that generated the issue: 52f13c6

And I have considered the keyboard height using WindowInsetsCompat.Type.ime() as this gives a non 0 value only when the keyboard is opened and we want the height to decrease equivalent to value of keyboard height when it is opened

@kirillzyusko
Copy link
Contributor

Well, my opinion is that it's not a correct approach, because prior to 52f13c6 we were using only getSystemWindowInsetBottom, right?

If we open a documentation we'll clearly see, that new approach is to use getInsets(WindowInsetsCompat.Type.statusBars()):

image

Now we add ime insets, which I think is not a correct solution, because systemWindowInsetBottom didn't include .ime() insets, but in this PR we start to mix different insets and apply them to a decorView 🤷‍♂️ (so these changes do not look to me as a backward compatible).

Here is the PR that introduced new API for Android 30+: #1868

But again, it's only my opinion - I would wait for a review from maintainers to see what they say 👀

@nikhiltekwani09
Copy link
Author

Yes but that is always returning a 0 value and earlier it was returning the keyboard height
Do you suggest any other workaround to fix the issue?

@kirillzyusko
Copy link
Contributor

@nikhiltekwani09 well, I checked and it looks like systemWindowInsetBottom indeed includes ime insets. I'm still slightly concerned because the documentation says the opposite (that it includes StatusBar only). And also curious why it happens on Fabric only?

@nikhiltekwani09
Copy link
Author

Yes that is why I manually added ime insets. Also the issue is happening without fabric as well.

@kirillzyusko
Copy link
Contributor

@nikhiltekwani09 oh, yeah, I see it now. I think potentially a better option would be to combine insets together without involving math operations:

val windowInsets = defaultInsets.getInsets(WindowInsetsCompat.Type.systemBars() or WindowInsetsCompat.Type.ime())
WindowInsetsCompat
    .Builder()
    .setInsets(
        WindowInsetsCompat.Type.systemBars(),
        Insets.of(
            windowInsets.left,
            0,
            windowInsets.right,
          windowInsets.bottom,
        ),
    ).build()

But this is really strange, that Android claims to use systemBars() as a replacement for systemWindowInset*, but systemWindowInset* includes ime insets and systemBars() are not 🤷‍♂️

        /**
         * @return All system bars. Includes {@link #statusBars()}, {@link #captionBar()} as well as
         * {@link #navigationBars()}, but not {@link #ime()}.
         */
        @InsetsType
        public static int systemBars() {
            return STATUS_BARS | NAVIGATION_BARS | CAPTION_BAR;
        }

Anyway I would wait for a review from SWM team 😅

@nikhiltekwani09
Copy link
Author

Yes I would also wait for the comments from the maintainers now

@nikhiltekwani09
Copy link
Author

Can we please merge this PR

@tboba tboba requested review from tboba and removed request for tboba September 11, 2024 09:31
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.

Android keyboard adjustResize not working with status bar translucent
2 participants