Skip to content

fix(Android): fix formSheet not adjusting for keyboard when it contains input with autofocus #2911

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

Merged
merged 8 commits into from
May 20, 2025

Conversation

kligarski
Copy link
Contributor

@kligarski kligarski commented May 8, 2025

Description

Sometimes, formSheet that contains input with autoFocus would not account for the keyboard and would remain behind it. This is because SheetDelegate is set as a OnApplyWindowInsetsListener when fragment is resumed - this happens after finishing formSheet entering animations (animators impact fragment lifecycle). Sometimes, IME insets were applied before this handler was set. Now, we can use WindowInsetsAnimationCallback added in this PR. Its onPrepare method is called before onApplyWindowInsets therefore making it possible to set OnApplyWindowInsetsListener in time.

Changes

  • change insetsObserverProxy's listeners to HashSet (we might try to add the same listener multiple times)
  • override onPrepare method in WindowInsetsAnimationCallback
  • change visibility of SheetDelegate to internal

Test code and steps to reproduce

Open Test2895 screen in the example app, open & close the formSheet (multiple times).

Checklist

  • Included code example that can be used to test this change
  • Ensured that CI passes

@kligarski kligarski requested a review from kkafar May 8, 2025 13:51
@kligarski kligarski force-pushed the @kligarski/form-sheet-autofocus-2 branch from 2dda31a to b78afc5 Compare May 9, 2025 12:07
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

I see that it improves the situation, however, I'm not convinced taht onPrepare is good palce to do so. My initial impression is that registering observer proxy in onAttachedToWindow would make more sense. Can you check, whether it's feasible timing-wise to do so?

Copy link
Contributor Author

@kligarski kligarski left a comment

Choose a reason for hiding this comment

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

onAttachedToWindow works as well.

@kligarski kligarski requested a review from kkafar May 9, 2025 14:17
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

Answering your question here.

Comment on lines 178 to 191
// This code currently does not work (changing maxHeight requires relayout)
// but it creates visual glitch when closing formSheet with keyboard still open.
// Decided to temporarily comment it out.
// More details: https://github.com/software-mansion/react-native-screens/pull/2911

// val newMaxHeight =
// if (behavior.maxHeight - keyboardState.height > 1) {
// behavior.maxHeight - keyboardState.height
// } else {
// behavior.maxHeight
// }

val newMaxHeight = behavior.maxHeight

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decided to comment out this part because it does not work currently (even before this PR, sheet would extend to previous maxHeight) but it causes visual glitch when closing the formSheet.

Before After
with_newMaxHeight.mp4
without_newMaxHeight.mp4

Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

I see one issue, however: you say that we can rely on WindowInsetsAnimationCallback. I say: it is supported since API level 30, while min sdk is lower than that. How does the current apporach look on e.g. SDK 29?

@kligarski
Copy link
Contributor Author

I see one issue, however: you say that we can rely on WindowInsetsAnimationCallback. I say: it is supported since API level 30, while min sdk is lower than that. How does the current apporach look on e.g. SDK 29?

As we're using ViewCompat.setWindowInsetsAnimationCallback to register the callback, it handles the case when using API 21 <= ... < 30 for us.

    static void setCallback(@NonNull View view, @Nullable Callback callback) {
        if (Build.VERSION.SDK_INT >= 30) {
            Impl30.setCallback(view, callback);
        } else if (Build.VERSION.SDK_INT >= 21) {
            Impl21.setCallback(view, callback);
        }
        // Do nothing pre 21
    }

But I now noticed that in the docs, there is a note about using API < 30:

Prior to API 30, if an OnApplyWindowInsetsListener is used on the same view, be sure to always use the ViewCompat version of setOnApplyWindowInsetsListener, otherwise the listener will be overridden by this method.

I'll try to make sure that everything works on API < 30 as well, because we're sometimes overriding onApplyWindowInsets instead of using setOnApplyWindowInsetsListener.

@kligarski
Copy link
Contributor Author

kligarski commented May 19, 2025

With setOnApplyWindowInsetsListener, we set the listener on Screen which does not override onApplyWindowInsets so we're good here. I also didn't find any case where we would use View.setOnApplyWindowInsetsListener instead of ViewCompat version on the Screen class. The animation looks on API 29 as good as on 36.

api_29.mov

@kligarski kligarski requested a review from kkafar May 19, 2025 12:00
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

Once #2925 (comment) this is answered we can proceed

@kligarski kligarski requested a review from kkafar May 20, 2025 11:17
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

Looks good now!

@kligarski kligarski merged commit 54acad4 into main May 20, 2025
5 checks passed
@kligarski kligarski deleted the @kligarski/form-sheet-autofocus-2 branch May 20, 2025 11:37
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.

2 participants