688581 - [TABLETUI] Tweak "New Tab" button in landscape mode

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ian Barlow (:ibarlow)]

Attachment: screen shot of current tab menu

Let's add the text "New Tab" next to the plus sign, like in these mocks. http://www.flickr.com/photos/61892693@N03/6050531024/in/set-72157627325688069

Also, looks like somehow a blue divider got added, we should either take that out or make it black

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

What you are calling a blue divider is a scroll bar. I would suspect there is something too wide for its container in the tab bar.

Comment 2

[Matt Brubeck (:mbrubeck)]

(In reply to Kevin Brosnan [:kbrosnan] from comment #1)
> What you are calling a blue divider is a scroll bar. I would suspect there
> is something too wide for its container in the tab bar.

No, there's an actual blue border there that is not a scroll bar.

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: patch

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: screenshot with patch

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 561970 [details] [diff] [review]
patch


>diff --git a/mobile/themes/core/tablet.css b/mobile/themes/core/tablet.css

> #newtab-button[tablet] {
>+  -moz-box-flex: 0;
>   list-style-image: url("images/newtab-default-tablet-hdpi.png");
>+  max-width: 180px;
>+  padding: 0 1em;
>+}
>+
>+#newtab-button[tablet] > .toolbarbutton-text {
>+  color: white;
>+  display: -moz-initial;
>+  list-style-image: url("images/newtab-default-tablet-hdpi.png");
>+  padding: 0 1em;

Why use max-width instead of allowing flex?
Why the dupe list-style-image and padding? Doesn't the label inherit them?

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 561970 [details] [diff] [review]
patch

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

Looks good anyway.

::: mobile/themes/core/honeycomb/browser.css
@@ -329,5 @@
>  .button-control {
>    padding: 0 !important;
>    min-width: @sidebar_width_minimum@ !important;
> -  border-top: @border_width_tiny@ solid @color_divider_border@ !important;
> -  border-bottom: @border_width_tiny@ solid @color_divider_border@ !important;

This affects phone UI as well, right? Maybe it should go on a separate patch?

::: mobile/themes/core/tablet.css
@@ +122,2 @@
>    list-style-image: url("images/newtab-default-tablet-hdpi.png");
> +  max-width: 180px;

Maybe width 200px with no padding? Isn't the selected state of this button supposed to fill the whole width of the tabs pane?

@@ +126,5 @@
> +
> +#newtab-button[tablet] > .toolbarbutton-text {
> +  color: white;
> +  display: -moz-initial;
> +  list-style-image: url("images/newtab-default-tablet-hdpi.png");

Do you actually need to set list-style-image here?

Comment 7

[Matt Brubeck (:mbrubeck)]

(In reply to Lucas Rocha (:lucasr) from comment #6)
> Comment on attachment 561970 [details] [diff] [review] [diff] [details] [review]
> > -  border-top: @border_width_tiny@ solid @color_divider_border@ !important;
> > -  border-bottom: @border_width_tiny@ solid @color_divider_border@ !important;
> 
> This affects phone UI as well, right? Maybe it should go on a separate patch?

The blue dividers actually look ugly in the honeycomb phone UI too.  This shouldn't affect any normal users, but I think it's correct to remove them.

> Maybe width 200px with no padding? Isn't the selected state of this button
> supposed to fill the whole width of the tabs pane?

If I let the button fill up the whole width (using "width:200px" or flex=1) then I couldn't get the text to sit next to the icon.  The toolbarbutton XBL uses the "flex=1" attribute for its <label>, which you can't override with CSS.  :(

I'll see if I can do something better with padding and text-align.

> Do you actually need to set list-style-image here?

No, I left this in accidentally.  Removed.

Comment 8

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

Okay, this works fine without setting flex=0.  I also tested this in LTR, and with a longer string (to make sure it won't break in other locales).

Comment 9

[Wesley Johnston (:wesj)]

Just an FYI, I had that same problem in the portrait tabs menu. Worked around by changing from a toolbarbutton to a button. We should really just go fix the toolkit toolbarbutton binding... or override it.

Comment 10

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b412df0df6b

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2b412df0df6b

Comment 12

[Cristian Nicolae (:xti)]

Verified fixed on:
Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110926
Firefox/9.0a1 Fennec/9.0a1
Device: Acer ICONIA A500
OS: Android 3.1