Fix keyboard crashes in BrowserFragment #5033
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is the new recommended way to show and hide the keyboard according to the documentation: https://developer.android.com/develop/ui/views/touch-and-input/keyboard-input/visibility#ShowReliably Quote: - (Recommended) Use WindowInsetsControllerCompat. This object displays the soft keyboard during Activity.onCreate() as shown in the following code snippet. The call is guaranteed to be scheduled after the window is focused. We decided to enforce the view passed as an EditText as we couldn't imagine a reason for any other type as of now.
We're only changing the functions we saw break right now. We'll follow up with a task to look into other places where we show/hide the keyboard
We should discourage the use of the the extension functions that probably are not reliable in favour of our new extension functions
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @mikescamell and the rest of your teammates on |
joshliebe
reviewed
Sep 18, 2024
common/common-utils/src/main/java/com/duckduckgo/common/utils/extensions/FragmentExtensions.kt
Outdated
Show resolved
Hide resolved
joshliebe
reviewed
Sep 18, 2024
common/common-utils/src/main/java/com/duckduckgo/common/utils/extensions/FragmentExtensions.kt
Outdated
Show resolved
Hide resolved
malmstein
requested changes
Sep 18, 2024
app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt
Outdated
Show resolved
Hide resolved
malmstein
approved these changes
Sep 19, 2024
There was a problem hiding this comment.
Looks good! I let you agree with @joshliebe in the outstanding question, but I don’t consider that a blocker.
joshliebe
requested changes
Sep 19, 2024
There was a problem hiding this comment.
I’m gonna have to disagree with @malmstein on this one, blocking on: #5033 (comment)
joshliebe
reviewed
Sep 19, 2024
joshliebe
approved these changes
Sep 19, 2024
cmonfortep
added a commit
that referenced
this pull request
Sep 26, 2024
Task/Issue URL: https://app.asana.com/0/1202552961248957/1208407851630430/f ### Description After we did #5033 the omnibar is taking focus more than usual, triggering onFocus changes in some unexpected scenarios (e.g: if we send the app to background, etc.) After taking a look, the logic to show/hide keyboard was changed, and we now, for both methods, execute `editText.requestFocus()`. That seems correct for showing, but unsure for hiding. ### Steps to test this PR _Feature 1_ - just smoke test ### UI changes | Before | After | | ------ | ----- | !(Upload before screenshot)|(Upload after screenshot)|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Task/Issue URL: https://app.asana.com/0/414730916066338/1208339084893367/f
Description
We saw some keyboard crashes due to some recent refactoring of the Omnibar.
The Omnibar was previously solely bound in onActivityCreated.
It was never nulled out in
onDestroyViewso the Fragment held on to it, and when the delay concluded it would happily try to call show/hideKeyboard using the Omnibar without issue.Now, we’re using the FragmentViewBindingDelegate for the Omnibar, which gets nulled out in onDestroy, so when the delay concluded we tried calling show/hideKeyboard on a null object.
To fix this we’re introducing new extension functions that follow the recommended way to show and hide the keyboard.
Steps to test this PR
Run the following tests locally they should all pass
whenProtectionsAreDisabledRequestAreNotBlockedwhenProtectionsAreDisabledSurrogatesAreNotLoadedwhenProtectionsAreEnabledHttpsUpgradedCorrectlywhenProtectionsAreEnabledRequestBlockedCorrectlywhenProtectionsAreEnabledSurrogatesAreLoadedwhenProtectionsAreFingerprintProtectedOmnibar Keyboard
UI changes
N/A