Closed Bug 1107386 Opened 9 years ago Closed 9 years ago

New tablet UI - Browser menu is clipped and unusable on the Kindle Fire

Categories

(Firefox for Android Graveyard :: General, defect, P1)

Other
Android
defect

Tracking

(firefox36 fixed, firefox37 fixed, firefox38 fixed, fennec36+)

RESOLVED FIXED
Firefox 38
Tracking Status
firefox36 --- fixed
firefox37 --- fixed
firefox38 --- fixed
fennec 36+ ---

People

(Reporter: alex_mayorga, Assigned: mcomella)

References

Details

Attachments

(2 files)

The menu is tiny and don't show all items, please see attached.
What device and android version is this and channel are you running? I'm not able to reproduce on my Nexus 7 (2013, 5.0, Nightly 12/04). Do you have specific steps (e.g, did you do a device rotation and open the menu and rotate again while open or something?)
tracking-fennec: --- → ?
Keywords: steps-wanted
Flags: needinfo?(alex_mayorga)
This is a Kindle Fire, more precisely GL Layers! EGL? EGL+ GL Context? GL Context+ GL Layers+ AdapterDescription: 'Model: KFJWI, Product: Kindle Fire, Manufacturer: Amazon, Hardware: bowser, OpenGL: Imagination Technologies -- PowerVR SGX 544 -- OpenGL ES 2.0 build 1.9.RC2@2130229' Stagefright? Stagefright- Amazon KFJWI Android/jem/jem:4.0.4/IMM76D/8.5.1_user_5156220:user/release-keys

There are no steps, the menu looks like this regardless of landscape or portrait.
Flags: needinfo?(alex_mayorga)
Another Kindle quirk, hm
Blocks: kindle
Keywords: steps-wanted
Summary: "Hamburger" menu is clipped and unusable → "Hamburger" menu is clipped and unusable on the Kindle Fire
Summary: "Hamburger" menu is clipped and unusable on the Kindle Fire → Browser menu is clipped and unusable on the Kindle Fire
This used to work, so my guess is a newtablet regression.
Mike, can you have a look?
Assignee: nobody → michael.l.comella
Priority: -- → P1
Aaron, do you have a Kindle Fire on which to repo (and maybe for me to send some patches your way)?
Flags: needinfo?(aaron.train)
I don't have one. MV maybe?
Flags: needinfo?(aaron.train)
I'll investigate MV - Richard, in the worst case, do you have one?
Flags: needinfo?(rnewman)
Found a Kindle Fire here (1st or 2nd gen, not sure) but it did not run the tablet UI (in fact, there were no toolbar animations so I think it was GB) - we're going to need something newer here.
I could test on the device where I initially found the bug if you give me an apk.
Thanks - I'll let you know when I get around to authoring some patches.
And yes, I have a Fire HD (or HDX?) if you need me to test.
Flags: needinfo?(rnewman)
tracking-fennec: ? → 36+
Confirmed on my device (7 inch) as well.

I think it might also be related to the size of the screen, the Kindle Fire is a small tablet.
Summary: Browser menu is clipped and unusable on the Kindle Fire → New tablet UI - Browser menu is clipped and unusable on the Kindle Fire
Status: NEW → ASSIGNED
Hey, Alex (or Tim!).

Can you run a few tests for me?

  1) Can you still reproduce on the latest Nightly?
  2) If you click the tabs panel button (the number of tabs you have open) and click the newly uncovered menu button, does that menu appear clipped or is it fully visible?
  3) If you install my test build (https://people.mozilla.com/~mcomella/apks/mcomella-1107386_01.apk), is the issue still reproduceable? Note: I disabled the tab strip on purpose.
Flags: needinfo?(alex_mayorga)
(In reply to Michael Comella (:mcomella) from comment #15)
> Hey, Alex (or Tim!).
> 
> Can you run a few tests for me?
> 
>   1) Can you still reproduce on the latest Nightly?
Yes, I can still reproduce on the latest Nightly.

>   2) If you click the tabs panel button (the number of tabs you have open)
> and click the newly uncovered menu button, does that menu appear clipped or
> is it fully visible?
That menu appears fine.

>   3) If you install my test build
> (https://people.mozilla.com/~mcomella/apks/mcomella-1107386_01.apk), is the
> issue still reproduceable? Note: I disabled the tab strip on purpose.
Works great in that build !
Then I think the issue here is that this Kindle device assumes the location of the overflow menu - we put it in a non-standard location because the tab strip occupies the top part of the screen. This is why I disabled it in my test.

Looking at Chrome, I think they're using the new Toolbar API [1] (in support library v7) so that might be one possible solution if we upgrade to there.

Otherwise, we can try moving the location of the menu on Kindle devices to the typically position at the bottom of the tab strip.

[1]: https://developer.android.com/reference/android/widget/Toolbar.html
Flags: needinfo?(alex_mayorga)
What about this build, Tim?

https://people.mozilla.com/~mcomella/apks/mcomella-1107386_02.apk

By the way, thanks for your help! :)
Flags: needinfo?(ntim007)
(In reply to Michael Comella (:mcomella) from comment #18)
> What about this build, Tim?
> 
> https://people.mozilla.com/~mcomella/apks/mcomella-1107386_02.apk
So the menu is at the right size, but not the right position (top left corner of the screen)
Flags: needinfo?(ntim007)
(In reply to Michael Comella (:mcomella) from comment #17)
> Then I think the issue here is that this Kindle device assumes the location
> of the overflow menu

In my build in comment 19, I anchored the menu to the tab strip (i.e. menu is at the top of the screen) and it worked correctly, which further verifies my hypothesis.

After speaking with :antlam in our tablet meeting, we decided to single out this particular device and place the menu at the top of the screen - the menu will be in an incorrect position (i.e. unpolished), but at least it will work - as far as I know, we don't officially support Kindle devices anyway.
After talking with Finkle on IRC, I'm first going to try to do a dynamic diagnosis - if we find the menu is oddly sized, move it to the top of the screen - before singling out this device.
I just noticed that the original screenshot shows the correct width of the popup menu but not the correct height - I wonder if it'd be better to set a minimum height instead. At the very least, it'd be simpler to fix in code.
Actually, right before showing the popup, in showAsDropDown [1], we call:

 // Set a height, so that the popup will not be displayed below the bottom of the screen.
 setHeight(mPopupMinHeight);

mPopupMinHeight is 64dp: most devices are probably using this a minimum value while Kindle is probably using this as an exact value. Oops!

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/menu/MenuPopup.java?rev=182716b666bf#69
Tim, how does this work?

https://people.mozilla.com/~mcomella/apks/mcomella-1107386_03.apk

---

I think the comment:

  // Set a height, so that the popup will not be displayed below the bottom of the screen.

is a reference to this solution:

  http://stackoverflow.com/a/7698709

Option 2 measures the height of the contained View and sets that as the height of the popup. So I did that:

-> setHeight(mPanel.getHeight());
Flags: needinfo?(ntim007)
Or Alex, what happens when you run the build in comment 24 and open the toolbar menu?
Flags: needinfo?(alex_mayorga)
(In reply to Michael Comella (:mcomella) from comment #24)
> Tim, how does this work?
> 
> https://people.mozilla.com/~mcomella/apks/mcomella-1107386_03.apk
Sorry for the delay, I was a bit busy.
Anyways, this works great ! (right size & position).
Flags: needinfo?(ntim007)
Flags: needinfo?(alex_mayorga)
Thanks for your help, Tim and Alex! :)
Comment on attachment 8553411 [details] [diff] [review]
Set internal container height as height of MenuPopup

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

Looks good - ship it.
Attachment #8553411 - Flags: review?(mhaigh) → review+
Comment on attachment 8553411 [details] [diff] [review]
Set internal container height as height of MenuPopup

Approval Request Comment
[Feature/regressing bug #]:
  New tablet UI (bug 1014156)

[User impact if declined]:
  Users on one particular Kindle device will be unable to open the browser menu and thus be unabled to access some part of the browser's features

[Describe test coverage new/current, TreeHerder]:
  Current: there are tests to open and use the browser menu but they do not test if the menu is of the appropriate size (i.e. as long as the menu is scrollable, we won't notice a change in size). The tests do not run on this particular Kindle device.
  New: none

[Risks and why]:
  Low - we replace setHeight(minimumHeight) with setHeight(actualHeight). In the worst case, we get the size wrong and the menu is larger than it needs to be (on all devices), but at least the functionality is accessible to all devices.

[String/UUID change made/needed]: None
Attachment #8553411 - Flags: approval-mozilla-beta?
Attachment #8553411 - Flags: approval-mozilla-aurora?
Whiteboard: [fixed-in-fx-team]
Thanks for fixing this !
https://hg.mozilla.org/mozilla-central/rev/88f17b6cc0cb
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
Attachment #8553411 - Flags: approval-mozilla-beta?
Attachment #8553411 - Flags: approval-mozilla-beta+
Attachment #8553411 - Flags: approval-mozilla-aurora?
Attachment #8553411 - Flags: approval-mozilla-aurora+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: