Closed Bug 933422 Opened 6 years ago Closed 6 years ago

Hiding keyboard when showing search suggestions briefly shows background

Categories

(Firefox for Android :: Awesomescreen, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 29
Tracking Status
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 --- affected
firefox29 --- verified
fennec 27+ ---

People

(Reporter: Margaret, Assigned: bnicholson)

Details

Attachments

(1 file, 2 obsolete files)

This looks like some glitch that happens when the keyboard disappears. You can see this if you edit a bookmark with a new profile (since there are only 4 bookmarks in that case).
Assignee: nobody → sriram
tracking-fennec: ? → 26+
I speculate this to be the screen corruption as a result of the window being smaller, and there is nothing to show behind (earlier we had LayerView).
We don't have any background there, and that's the problem.
Option 1: Add a window background, when keyboard is shown. -- hard to keep track of and doing it.
Option 2: Don't shrink the window for keyboard -- affects content pages too.
(In reply to Sriram Ramasubramanian [:sriram] from comment #2)
> We don't have any background there, and that's the problem.
> Option 1: Add a window background, when keyboard is shown. -- hard to keep
> track of and doing it.

Why would it be hard exactly? Isn't it just about setting a background in onConfigurationChanged using something like:

http://developer.android.com/reference/android/content/res/Configuration.html#keyboardHidden
Status: NEW → ASSIGNED
If I were to add a window background when keyboard shows, and remove it when the keyboard is hidden, isn't it as good as doing it in HomePager's show and hide?
keyboardHidden cannot capture soft keyboard changes: http://stackoverflow.com/a/6279959/1052787
(In reply to Sriram Ramasubramanian [:sriram] from comment #5)
> keyboardHidden cannot capture soft keyboard changes:
> http://stackoverflow.com/a/6279959/1052787(In reply to Sriram Ramasubramanian [:sriram] from comment #4)

Have you actually tried it? It might be a device or version specific bug. 

> If I were to add a window background when keyboard shows, and remove it when
> the keyboard is hidden, isn't it as good as doing it in HomePager's show and
> hide?

Yeah, I'd be ok with that. I do wonder how bad the overdraw will be as a result.
bug 920791 might be doing similar magic to know when the keyboard is open/dismissed.
Status: ASSIGNED → NEW
This bug might be fixed by bug 935604. Can we retest?
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
(In reply to Mark Finkle (:mfinkle) from comment #8)
> This bug might be fixed by bug 935604. Can we retest?

I just updated to the latest Nightly, but I still see this happen.

In fact, it's a bit weirder now. I see the same bookmark item show up many times in a row, with some empty space between them.

Still testing on Nexus 4, 4.3.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
After discussing with mcomella, the other bug wouldn't fix it.
Ideally there should be a background when the IME is shown, so that the empty portion being IME when it is hidden, isn't corrupted.

But, there is no way to see if the IME is about to be shown or not. There are two options here:
1. Show/hide window background when about:home is shown/hidden.
2. Show/hide widnow background when the TextView (url-bar) receives/loses focus.
Assignee: sriram → michael.l.comella
(In reply to Sriram Ramasubramanian [:sriram] from comment #10)
> After discussing with mcomella, the other bug wouldn't fix it.
> Ideally there should be a background when the IME is shown, so that the
> empty portion being IME when it is hidden, isn't corrupted.
>
> But, there is no way to see if the IME is about to be shown or not. There
> are two options here:
> 1. Show/hide window background when about:home is shown/hidden.

I tried this and it didn't fix the bug (see the attached patch). Honestly, I
have no idea why.

> 2. Show/hide widnow background when the TextView (url-bar) receives/loses
> focus.

Since #1 did not work, I don't think this will work either.

I'll keep investigating.
Note that this behavior is also present in the browser search screen (STR: Open BrowserSearch, type a query that does not match any history/bookmarks, hit back to drop keyboard).
Changing the "Window animation scale" in Developer Options to a value > 1x causes bug to go away (while values < 1x make the effect more noticeable) - it might be helpful to see what Android is drawing differently when this option is enabled.
The issue is also not reproduceable with the "Show GPU overdraw" option enabled.

Adding 'android:background="@android:color/white"' to the 'home_pager_container', the 'bookmarks_list', and FrameLayout containing 'bookmarks_list' did not fix the issue either - I wager the issue is more complicated than our initial "just draw a background!" idea.
(In reply to Michael Comella (:mcomella) from comment #12)
> Note that this behavior is also present in the browser search screen (STR:
> Open BrowserSearch, type a query that does not match any history/bookmarks,
> hit back to drop keyboard).

Actually, you don't need a query that doesn't match any history/bookmarks - this appears even when the list is not short.
I can't repro this on my Galaxy Nexus (though I can repro it on the Browser Search screen there). I also could not repro on devices I tested on with Android < 4.

The device I can repro on is a Nexus 4.
My best idea for discovering the cause for this bug is to attempt to repro this in a separate application. I tried doing this with a ListView [1], but to no avail. I wonder if the use of fragments isn't related.

[1]: https://github.com/mcomella/AndySandbox/tree/933422_listview
Handing this back to Sriram because I'm not actively working on this and I believe he'll be back soon.
Assignee: michael.l.comella → nobody
Assignee: nobody → sriram
Missed the 26 cycle, track 27?
Assignee: sriram → bnicholson
As written, this bug no longer seems to be present. There's a similar glitch that happens for search suggestions where the background is briefly shown when hiding the keyboard, so I'll morph the bug into that. But that has been around for awhile (probably since Sriram removed window backgrounds), and is not a recent regression.
tracking-fennec: 26+ → ?
Summary: With short bookmark list, items are drawn twice when a bookmark is edited → Hiding keyboard when showing search suggestions briefly shows background
tracking-fennec: ? → 27+
Status: REOPENED → ASSIGNED
The window resize is happening in the first place because of android:windowSoftInputMode="adjustResize" (http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AndroidManifest.xml.in#89).

We generally want this on to prevent focused views from being hidden by the keyboard. In the browser search fragment, however, the only focusable input is the URL bar at the top of the screen, so this flag is unnecessary. Disabling window resize, we can draw the entire view even when the keyboard is visible, preventing the glitch when the keyboard is hidden.
Attachment #8336951 - Attachment is obsolete: true
Attachment #8363346 - Flags: review?(michael.l.comella)
Comment on attachment 8363346 [details] [diff] [review]
Don't resize the search window when showing the keyboard

Review of attachment 8363346 [details] [diff] [review]:
-----------------------------------------------------------------

I noticed this removes the annoying behavior where the scroll height of the ListView is changed when the keyboard is hidden so I wonder if we shouldn't change this value when the HomePager is shown and dismissed (rather than in BrowserSearch), to remove the same change-the-scroll behavior there. Plus, it may solve our mysteriously missing bookmarks issue if it decides to reappear.

What do you think?

::: mobile/android/base/home/BrowserSearch.java
@@ +182,5 @@
> +        // Adjusting the window size when showing the keyboard results in the underlying
> +        // activity being painted when the keyboard is hidden (bug 933422). This can be
> +        // prevented by not resizing the window.
> +        mWindow = activity.getWindow();
> +        mWindow.setSoftInputMode(WindowManager.LayoutParams.SOFT_INPUT_ADJUST_NOTHING);

If this doesn't get moved out to the HomePager, shouldn't we call this just before the fragment is displayed to the user (i.e. onStart(), I believe, but that might be just after - the docs are ambiguous)?

@@ +194,5 @@
>          mUrlOpenListener = null;
>          mSearchListener = null;
>          mEditSuggestionListener = null;
> +
> +        mWindow.setSoftInputMode(WindowManager.LayoutParams.SOFT_INPUT_ADJUST_RESIZE);

I think it would be better to restore this to what it initially was, rather than assuming it was `ADJUST_RESIZE`, however, I can't find any way to dynamically retrieve the soft input mode.
Attachment #8363346 - Flags: review?(michael.l.comella) → feedback+
(In reply to Michael Comella (:mcomella) from comment #22)
> I noticed this removes the annoying behavior where the scroll height of the
> ListView is changed when the keyboard is hidden so I wonder if we shouldn't
> change this value when the HomePager is shown and dismissed (rather than in
> BrowserSearch), to remove the same change-the-scroll behavior there. Plus,
> it may solve our mysteriously missing bookmarks issue if it decides to
> reappear.
> 
> What do you think?

I had the same thought myself when writing this patch, but there are views in HomePager that rely on keyboard resizing to work properly. For example, the pin site list doesn't resize with the keyboard or hide when scrolling, so the keyboard ends up covering the list. We could tweak this further to maybe hide the keyboard on touch like we do with the BrowserSearch list, but I figured it's not worth the added complexity since HomePager isn't even broken.

In general, this fix is kind of a hack, so I wanted to contain it as much as possible. HomePager is quite a bit more complicated than BrowserSearch, so using this flag too broadly could cause some edge casey bugs to crop up.

> If this doesn't get moved out to the HomePager, shouldn't we call this just
> before the fragment is displayed to the user (i.e. onStart(), I believe, but
> that might be just after - the docs are ambiguous)?

Yeah, onStart is probably better than onAttach. Patch updated.

> I think it would be better to restore this to what it initially was, rather
> than assuming it was `ADJUST_RESIZE`, however, I can't find any way to
> dynamically retrieve the soft input mode.

Thought the same thing myself :) As you said, there's no "getSoftInputMode", so there's no obvious way to store the existing value. On the upside, it's pretty unlikely we'll be using anything other than adjustResize in the future.
Attachment #8363346 - Attachment is obsolete: true
Attachment #8364068 - Flags: review?(michael.l.comella)
Ack, just saw that the WindowManager import was not in the right alphabetical position. Fixed this locally.
Comment on attachment 8364068 [details] [diff] [review]
Don't resize the search window when showing the keyboard

Review of attachment 8364068 [details] [diff] [review]:
-----------------------------------------------------------------

Do you want to add a comment to [1] to update the `setSoftInputMode` call in `BrowserSearch.onStop` if the attribute is changed?

Otherwise, lgtm.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/WebAppManifestFragment.xml.frag.in#4
Attachment #8364068 - Flags: review?(michael.l.comella) → review+
(In reply to Michael Comella (:mcomella) from comment #25)
> Do you want to add a comment to [1] to update the `setSoftInputMode` call in
> `BrowserSearch.onStop` if the attribute is changed?

Done.

https://hg.mozilla.org/integration/fx-team/rev/a3582e029c13
Ack #2: put comment in the wrong place.

Follow-up: https://hg.mozilla.org/integration/fx-team/rev/4200ecc6b162
https://hg.mozilla.org/mozilla-central/rev/a3582e029c13
https://hg.mozilla.org/mozilla-central/rev/4200ecc6b162
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
I think this is too late for 27.
Status: RESOLVED → VERIFIED
I agree. Not sure this should even be tracking a specific version actually; this is a long-standing bug, and its impact is very minor (see comment 20).
You need to log in before you can comment on or make changes to this bug.