Closed Bug 1106779 Opened 5 years ago Closed 5 years ago

Tab switcher doesn't allow access to bottom tab when keyboard is up

Categories

(Firefox for Android :: Theme and Visual Design, defect)

37 Branch
Other
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 39
Tracking Status
firefox34 --- affected
firefox35 --- affected
firefox36 --- affected
firefox37 --- affected
firefox38 --- affected
firefox39 --- verified
firefox40 --- verified
fennec + ---

People

(Reporter: kats, Assigned: jll544, Mentored)

References

Details

(Whiteboard: [good next bug][lang=java])

Attachments

(1 file)

Nexus 4 running nightly, but this issue has been around a long time. I only just figured out the STR. Open 4 or more tabs, for example this bug page 4 times. Make sure the URL bar is visible and put focus in a textarea/textfield so the keyboard comes up. If you get an actionbar dismiss it by clicking the check mark. While the keyboard is still up, hit the tab switcher button. ER: all tabs are visible and selectable. AR: can't scroll down enough to select the bottom tab.
tracking-fennec: --- → ?
Duplicate of this bug: 1107664
Duplicate of this bug: 1109562
Mentor: michael.l.comella
tracking-fennec: ? → +
Whiteboard: [good first bug][lang=java]
I'd like to take this as my first bug. Could anyone help me find what specific area/files I should be looking in? I have fennec building/etc... I just need help navigating the code base!
Hey, Tyler - you've been assigned. I apologize for the delay! I must have missed the email. -_-

You can use the needinfo flags (set below the "Additional comments" box) to give users notifications at the top of Bugzilla and send them periodic emails, which may help avoid having this happen again.

To answer your question, since we want to look dismiss the keyboard when the tab switcher button is pressed, we should look at the listener that is called when the tab switcher button is opened.

Our main java code base is in mobile/android/base. The BrowserToolbar [1] class, and its sub-classes, contain all of the implementation details of the toolbar. Looking at the XML for the phone implementation of this class, ...resources/layout/browser_toolbar.xml [2], we see that the id for this View is "tabs". This ID is found in BrowserToolbar and a listener is attached - look into adding code to the listener to dismiss the keyboard and show the TabsPanel unobstructed when it is opened.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/BrowserToolbar.java
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/browser_toolbar.xml?rev=540bc6af7020#59
Assignee: nobody → tyler.geonetta
Status: NEW → ASSIGNED
Thank you Michael for your assistance.

I did some investigation into this issue and seem to have pin-pointed it. In the toggleTabs() method (BrowserToolbar.java), there IS logic to hide the keyboard before showing the TabsPanel (as you mentioned).

The problems lies in the fact that the logic to calculate the TabsContainer height (getTabContainerHeight()) is called from TabsPanel.show() BEFORE the keyboard is actually dismissed/hidden (before it has actually collapsed, since it does not happen instantaneously).

We can see this behavior by using the following code to wait until after the keyboard has completely closed (~1sec?):

import android.os.*;
private void toggleTabs() {
        if (activity.areTabsShown()) {
            if (activity.hasTabsSideBar())
                activity.hideTabs();
        } else {
            // hide the virtual keyboard
            InputMethodManager imm =
                    (InputMethodManager) activity.getSystemService(Context.INPUT_METHOD_SERVICE);
            boolean hideResult = imm.hideSoftInputFromWindow(tabsButton.getWindowToken(), 0);

            //handler code below is added by me
            Handler handler = new Handler();
            handler.postDelayed(new Runnable() {
              public void run() {
                Tab tab = Tabs.getInstance().getSelectedTab();
                if (tab != null) {
                  if (!tab.isPrivate())
                  activity.showNormalTabs();
                  else
                  activity.showPrivateTabs();
                }
              }
            }, 1000); //1000 milliseconds

            /* notice this code is now called from the above handler
            Tab tab = Tabs.getInstance().getSelectedTab();
            if (tab != null) {
              if (!tab.isPrivate())
              activity.showNormalTabs();
              else
              activity.showPrivateTabs();
            }
            */
        }
    }


Obviously, we don't want to use that arbitrary 1000 ms delay waiting for the keyboard to close... But I can't seem to find any way to check if the keyboard is closed or not from my research (or any events, etc)... Any ideas/direction?
Flags: needinfo?(michael.l.comella)
Hey, Tyler! I apologize for the delay - I won't let it happen again.

Good research!

Note that will require a little exploration because I'm honestly not sure exactly what the solution is here - I hope you don't mind! If you do, let me know and we can find a more clear-cut bug for you.

I noticed InputMethodManager has a hideSoftInputFromWindow method that accepts a `ResultReceiver` instance - i.e. a callback! You can try this. However, skimming the source [1], I'm skeptical whether layout has actually occurred yet, and thus the TabsContainer height will still be improperly set. If that's the case, we can try using the ViewTreeObserver [2] - I imagine hiding the keyboard will trigger a global layout so we can try the onGlobalLayoutObserver. If not, the OnPreDrawListener or OnDrawListener may be appropriate. I don't know the details of Android layout/drawing so I'm not sure of the implications off-hand for which one you use.

If this still doesn't work, combining both solutions seems like it should almost certainly work, but be sure to try this last as it may be excessive. Let me know if you still need help!

By the way, if you want to make sure your mentor (or other user) sees your question, add a needinfo flag (see the box below this comment field).

[1]: http://androidxref.com/4.4.4_r1/xref/frameworks/base/core/java/android/inputmethodservice/InputMethodService.java#401
[2]: https://developer.android.com/reference/android/view/ViewTreeObserver.html#addOnGlobalLayoutListener%28android.view.ViewTreeObserver
Flags: needinfo?(michael.l.comella)
Hi Tyler, are you still planning on working on this bug? If you have any questions please feel free to ask and we can help you out. Thanks!
Flags: needinfo?(tyler.geonetta)
I do intend on working on this, I just got distracted over the holidays...

Anyways, if I remember where I left off -- I tried implementing the ResultReceiver idea that Michael suggested. But for the life of me, I couldn't get it working (gist: https://gist.github.com/anonymous/92ed932fdb58f05f16f6).

I want to try the ViewTreeObserver idea that Michael proposed... but not really sure of exactly how that would work. Ideas?
Flags: needinfo?(tyler.geonetta)
(In reply to Tyler Geonetta from comment #8)
> I do intend on working on this, I just got distracted over the holidays...

No worries, we're all busy! :)

> Anyways, if I remember where I left off -- I tried implementing the
> ResultReceiver idea that Michael suggested. But for the life of me, I
> couldn't get it working (gist:
> https://gist.github.com/anonymous/92ed932fdb58f05f16f6).

What else have you tried here? Was the ResultReceiver getting called? Which result did it receive? It might not work if the soft input doesn't actually get hidden from this call. You can add a few print statements and check the logcat output to check the state (unfortunately, afaik we don't currently have debuggers working with our builds :\).

> I want to try the ViewTreeObserver idea that Michael proposed... but not
> really sure of exactly how that would work. Ideas?

If you'd like to try this, the TabStripView class has many examples (e.g. [1]).

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tabs/TabStripView.java?rev=915b53f26a53#126
Flags: needinfo?(tyler.geonetta)
Unassigning due to inactivity - let me know if you're still working on this Tyler.
Assignee: tyler.geonetta → nobody
Status: ASSIGNED → NEW
Hey, Tyler.

Are you interested in working on this? I've realized this bug might be a bit much for a new contributor, I noticed you were also working on bug 1134927 - do you want to tackle this as a followup to that one?
Flags: needinfo?(tyler.geonetta) → needinfo?(tylertstonge)
Whiteboard: [good first bug][lang=java] → [good next bug][lang=java]
(In reply to Michael Comella (:mcomella) from comment #12)
> Hey, Tyler.
> 
> Are you interested in working on this? I've realized this bug might be a bit
> much for a new contributor, I noticed you were also working on bug 1134927 -
> do you want to tackle this as a followup to that one?

I'm not quite sure what happened to the rest of my comment. I was going to say that I'm not able to replicate this on my Note 4, as soon as I tab to switch tabs the keyboard disappears. I ended up starting on a different bug because of that, but thank you.
Flags: needinfo?(tylertstonge)
Patch attached. 
Boolean returned from hideSoftInputFromWindow() tells us if the keyboard is being hidden. If so, we can listen for the global layout that's triggered after it's gone.
Attachment #8580809 - Flags: review?(michael.l.comella)
Assignee: nobody → jll544
Comment on attachment 8580809 [details] [diff] [review]
bug-1106779.patch

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

Nice work, Jeff!

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa99036546ab
Attachment #8580809 - Flags: review?(michael.l.comella) → review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/d5fe53d4a359
Keywords: checkin-needed
Whiteboard: [good next bug][lang=java] → [good next bug][lang=java][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/d5fe53d4a359
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [good next bug][lang=java][fixed-in-fx-team] → [good next bug][lang=java]
Target Milestone: --- → Firefox 39
Verified fixed:
Device: Nexus 4 (Android 4.4)
Build: Firefox for Android 40.0a1 (2015-04-23) and Firefox for Android 39.0a2 (2015-04-23) 

I am still able to reproduce on 38 Beta 6 and Firefox 37.0.2
You need to log in before you can comment on or make changes to this bug.