1100021 - New tablet toolbar buttons are broken on devices with hardware menu buttons

Status: VERIFIED FIXED

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

Description

[Kohei Yoshino [:kohei]]

Attachment: Screenshot_2014-11-15-15-41-54.png

See the attached screenshot. The reload, star, menu buttons are missing. The tab counter button is misplaced.

The latest x86 Nightly build on Galaxy Tab 3.

Comment 1

[Kohei Yoshino [:kohei]]

The Android version is 4.4.2 KitKat.

Comment 2

[Lucas Rocha (:lucasr)]

Looks like one of these cases where our UI is getting confused about whether or not the device is considered a 'tablet'?

Comment 3

[Kohei Yoshino [:kohei]]

Umm, I don't know but the older UI looks good to me on the same 10.1-inch tablet.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

We have an x86 Galaxy Tab 3 in the office - I'll check it out when I get in.

Comment 5

[desiradaniel2007]

I am unable to reproduce on my 10-inch ASUS Memopad, so it could be tablet-specific, yes.

Comment 6

[Aaron Train [:aaronmt]]

I see this on my Galaxy Tab 3 as well.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I can confirm that I see this issue on my x86 Galaxy Tab 3. If I had to guess, it's because there is a hardware menu key so the software menu button is missing (which is a configuration I have not tested).

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Tablet w/o soft menu key

Anthony, what do you think of the margin on the right of the tabs panel button?

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Correct new tablet toolbar layout for devices with hardware menu key

My patch which dynamically adds the same margin to both the menu button and tabs panel button, waiting on antlam's confirmation.

Comment 10

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8524941 [details]
Tablet w/o soft menu key

Ah, thats why.. (hardware menu) Nice catch!

This looks ok to me! thanks Mike

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Correct new tablet toolbar layout for devices with hardware menu key

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 8525612 [details] [diff] [review]
Correct new tablet toolbar layout for devices with hardware menu key

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

Looks good.

::: mobile/android/base/toolbar/BrowserToolbarNewTablet.java
@@ +63,5 @@
> +        final RelativeLayout.LayoutParams lp =
> +               (RelativeLayout.LayoutParams) rightmostActionBarView.getLayoutParams();
> +        lp.rightMargin = lp.rightMargin +
> +                res.getDimensionPixelOffset(R.dimen.new_tablet_browser_toolbar_menu_right_margin);
> +        rightmostActionBarView.setLayoutParams(lp);

Given that you're changing the current LayoutParams instance, you can just call rightmostActionBarrequestLayout.requestLayout() instead of setLayoutParams() here.

nit: factor out this code into a separate method called resolveRightmostMargin() or something.

Comment 13

[Lucas Rocha (:lucasr)]

I wonder if you could avoid the dynamic margin code by just setting padding = 6dp on the toolbar container instead?

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Correct new tablet toolbar layout for devices with hardware menu key

(In reply to Lucas Rocha (:lucasr) from comment #13)
> I wonder if you could avoid the dynamic margin code by just setting padding
> = 6dp on the toolbar container instead?

Good point. We can't do this statically because we have to set it on the parent
container in gecko_app and we still have old tablet to worry about.

This patch sets the padding dynamically, which still cleans up the logic.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Correct new tablet toolbar layout for devices with hardware menu key

Removed an excess comment.

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/34ad0732d16d

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Ignore comment 16: closed tree.

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/75599db3d71d

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/75599db3d71d

Comment 20

[Kohei Yoshino [:kohei]]

Forgot to say: the issue is gone, the current Nightly looks pretty. Thanks <3