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

Status: RESOLVED FIXED

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

Description

[alex_mayorga]

Attachment: Screenshot_2014-12-03-21-12-04.png

The menu is tiny and don't show all items, please see attached.

Comment 1

[Aaron Train [:aaronmt]]

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?)

Comment 2

[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.

Comment 3

[Aaron Train [:aaronmt]]

Another Kindle quirk, hm

Comment 4

[Richard Newman [:rnewman]]

This used to work, so my guess is a newtablet regression.

Comment 5

[Lucas Rocha (:lucasr)]

Mike, can you have a look?

Comment 6

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Aaron, do you have a Kindle Fire on which to repo (and maybe for me to send some patches your way)?

Comment 7

[Aaron Train [:aaronmt]]

I don't have one. MV maybe?

Comment 8

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

I'll investigate MV - Richard, in the worst case, do you have one?

Comment 9

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

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.

Comment 10

[alex_mayorga]

I could test on the device where I initially found the bug if you give me an apk.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Thanks - I'll let you know when I get around to authoring some patches.

Comment 12

[Richard Newman [:rnewman]]

And yes, I have a Fire HD (or HDX?) if you need me to test.

Comment 14

[Tim Nguyen :ntim]

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.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

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.

Comment 16

[Tim Nguyen :ntim]

(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 !

Comment 17

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

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

Comment 18

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

What about this build, Tim?

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

By the way, thanks for your help! :)

Comment 19

[Tim Nguyen :ntim]

(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)

Comment 20

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

(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.

Comment 21

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

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.

Comment 22

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

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.

Comment 23

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

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

Comment 24

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

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());

Comment 25

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Or Alex, what happens when you run the build in comment 24 and open the toolbar menu?

Comment 26

[Tim Nguyen :ntim]

(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).

Comment 27

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Attachment: Set internal container height as height of MenuPopup

Solution via http://stackoverflow.com/a/7698709

Comment 28

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Thanks for your help, Tim and Alex! :)

Comment 29

[Martyn Haigh (:mhaigh)]

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.

Comment 30

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/88f17b6cc0cb

Comment 31

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

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

Comment 32

[Tim Nguyen :ntim]

Thanks for fixing this !

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/88f17b6cc0cb

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/166eac7ff25c
https://hg.mozilla.org/releases/mozilla-beta/rev/e4e2855e992c