679472 - Make Fennecomb Action Bar pretty

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ian Barlow (:ibarlow)]

Attachment: Action bar icons

Let's make the action bar look like this http://www.flickr.com/photos/61892693@N03/6050531024/sizes/l/in/photostream/ and this http://www.flickr.com/photos/61892693@N03/6050531112/sizes/l/in/photostream/

Some key things to note:
* The action bar is a slightly transparent (95%) white. Unlike our phone browser it always sits at the top of the page and content can flow underneath it. 
* In landscape mode, the bar is pushed to the right to accommodate the tab menu.
* In portrait mode, it runs the full width of the screen, and has an additional "tabs" button to the left of the awesomebar
* The styling of the corresponding menus (tabs, awesomebar, bookmarks, more menu) will follow soon as separate bugs

Detailed specs: http://cl.ly/3R02173b460C281I0N0x

Visual assets (icons) are attached to this bug

Comment 1

[Wesley Johnston (:wesj)]

Attachment: WIP

WIP of just the location bar changes. This is the beginning of the Honeycomb theme though, so I've had to bring in some of the Gingerbread one, and probably broke some strange things in the process (I think dialogs are white... ish). I've put a test build on:

http://dl.dropbox.com/u/72157/fennec-honeycomb1.apk ( http://bit.ly/r5jXqE )

Comment 2

[Ian Barlow (:ibarlow)]

Hi Wes, thanks for posting. I have some spacing and sizing comments for you here, regarding the URL bar http://cl.ly/401R3f2r1c2b3q1l0X43

Comment 3

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

Ian - The "Enter Address or Search" is in a placeholder color. It is not the normal text color. Try using a real webpage to get the real text color.

Placeholder text is normally lighter and grayer than the standard text color.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Patch. I think this is ready for review, but I'd like you to look at it again first Ian? The build I liked above has been updated. I'll attach a diff with the gingerbread theme files for comparison.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Diff with gingerbread theme

Comment 6

[Ian Barlow (:ibarlow)]

Attachment: A couple more icons for the action bar

Hi Wes, this is looking good. I have a few more comments here, having seen the bar in action now: http://cl.ly/1r0h413Y1d0d1K3Z2M0N

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch v1

I had to create a few images to get the special urlbar underline working here. Talked to Ian about him remaking them if he wants, but these are probably fine (they're just lines). So I figure, lets start reviewing this. I'll another diff with gingerbread up in a minute.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Diff with gingerbread theme

Gingerbread diff

Comment 9

[Ian Barlow (:ibarlow)]

Thanks Wes, this looks good. I have some additional pixely tweaks I would like us to make, but I will file them as separate bugs. 
Ian

Comment 10

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

Comment on attachment 554921 [details] [diff] [review]
Patch v1


>diff --git a/mobile/chrome/content/browser-ui.js b/mobile/chrome/content/browser-ui.js

>+    let tab = Browser.getTabForBrowser(aBrowser);
>+
>     if (caption) {
>       this._title.value = caption;
>       this._title.classList.remove("placeholder");
>+      tab.chromeTab.setAttribute("label", caption);
>     } else {
>       this._title.value = this._title.getAttribute("placeholder");
>       this._title.classList.add("placeholder");
>+      tab.chromeTab.removeAttribute("label");
>     }

This is the wrong place to do this IMO. I think the tabs.xml binding should handle this using it's own "DOMTitleChanged" message listener. Feel free to just drop this from the current patch and add it later, unless you want to move it now.

>diff --git a/mobile/chrome/content/browser.js b/mobile/chrome/content/browser.js

>+    let cmd = document.getElementById("cmd_showTabs");
>+    cmd.setAttribute("label", this._tabs.length-1);

this._tabs.length - 1

>+    let cmd = document.getElementById("cmd_showTabs");
>+    cmd.setAttribute("label", this._tabs.length-1);


this._tabs.length - 1

r- for the tabs.xml change

CSS review happening in the diff patch cause it's easier to handle there

Comment 11

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

Comment on attachment 554922 [details] [diff] [review]
Diff with gingerbread theme

>+++ mobile/themes/core/honeycomb/browser.css	2011-08-22 11:56:02.100211414 -0700

> /* URL bar ----------------------------------------------------------------- */
> #urlbar-container {

>+  font: "Droid Sans";

"font" should not be needed and if it is use the full font list we use in platforms.css - this is not android only.

>+#tool-back2 {
>+  -moz-margin-end: 1px;
>+}

pixels?

>--- mobile/themes/core/gingerbread/platform.css	2011-08-19 14:35:39.776543889 -0700

> ::-moz-selection {
>-  background-color: @color_background_highlight@;
>+  background-color: @color_selection@;

I don't necessarily like that we are changing manifest constants in honeycomb

> }
> 
> menu,
> menuitem {
>   padding: 0 !important;
>   margin: 0 !important;
> }
>@@ -228,16 +228,21 @@ spinbuttons {
>   background-image: url("chrome://browser/skin/images/toggle-on.png");
> }
> 
> .spinbuttons-up {
>   list-style-image: url("chrome://browser/skin/images/arrowup-16.png");
> }
> 
> /* toolbar buttons --------------------------------------------------------- */
>+toolbar {
>+  -moz-appearance: none;
>+  background-color: @color_background_default@;
>+}
>+
> toolbarbutton {
>   min-width: @touch_button_large@ !important; /* primary button size */
>   min-height: @touch_button_large@ !important; /* primary button size */
>   -moz-appearance: none !important;
>   margin: 0;
>   padding: @padding_xsmall@;
> }
> 
>@@ -631,39 +636,39 @@ dialog {
> .panel-arrow[side="right"] {
>   list-style-image: url("chrome://browser/skin/images/arrowbox-horiz.png");
>   margin-left: -@margin_snormal@;
> }
> 
> /*.panel-row-header ------------------------------------------------------------ */
> .panel-row-header {
>   border-bottom: @border_width_xxlarge@ solid @color_background_active@;
>-  background-color: @color_background_default@ !important;
>+  background-color: @color_background_active@ !important;

You probably guessed that I don't like this change either. "*_default" on froyo and gingerbread, but "*_active" on honeycomb? You basically changed "*_default" to be transparent so you need to change all the non-transparent backgrounds to a new name? Or is the color really different? I might have given the transparent background a different name, so we didn't need to change all these "*_default" to "*_active"

> .panel-row-button:hover:active {
>-  background: @color_background_active@;
>+  background: @color_background_default@;

Clearly the active tab should have the active background!


>--- mobile/themes/core/gingerbread/defines.inc	2011-08-11 17:37:52.787005717 -0700

>-%define color_background_active #525252
>-%define color_background_default #000
>-%define color_text_default #fff
>+%define color_background_active #aaa
>+%define color_background_default rgba(255,255,255,0.95)

default background is now white everywhere? I thought honeycomb still used black backgrounds for many things. If we want to make the toolbar background white, then let's make a color_background_toolbar or color_background_actionbar

default background is now semi-transparent everywhere? I worry about the performance impact, but will wait for talos.

active background is for things like the selected tab. are the unselected tabs now white? even in the preferences panel?

>+%define color_text_default #222222

default text color is no longer white everywhere?

>-%define color_background_highlight #febc2b
>+%define color_background_highlight #a7c5f4
> %define color_background_highlight_overlay rgba(254, 188, 43, 0.8)

update this too, for web content

> %define color_text_highlight #000
>+%define color_selection #c0e49a

Why add color_selection? Why not color_background_highlight? Why are they different now?

> %ifdef ANDROID
>-%define font_xlarge 5.08mozmm
>-%define font_xnormal 2.75mozmm
>-%define font_normal 2.54mozmm
>-%define font_snormal 2.33mozmm
>-%define font_small 1.91mozmm
>-%define font_xsmall 1.69mozmm
>-%define font_tiny 1.48mozmm
>-%define font_xtiny 1.27mozmm
>-
>-%define touch_row 7.41mozmm
>-%define touch_button_xlarge 7.62mozmm
>-%define touch_button_large 6.77mozmm
>-%define touch_button_small 5.93mozmm
>+%define font_xlarge 6.08mozmm
>+%define font_xnormal 3.75mozmm
>+%define font_normal 3.54mozmm
>+%define font_snormal 3mozmm
>+%define font_small 2.91mozmm
>+%define font_xsmall 2.69mozmm
>+%define font_tiny 2.48mozmm
>+%define font_xtiny 2.27mozmm
>+
>+%define touch_row 10.41mozmm
>+%define touch_button_xlarge 10.62mozmm
>+%define touch_button_large 9.77mozmm
>+%define touch_button_small 8.93mozmm

What is the reason for these changes? Why are font sizes bigger for tablets? Is this part of the Android UI guidelines?

>-%define sidebar_width_minimum 8.47mozmm
>+%define sidebar_width_minimum 121px

Hmmm

>-%define font_snormal 22px
>+%define font_snormal 20px

If this is a good change we should do it on froyo and gingerbread too

>-%define sidebar_width_minimum 80px
>+%define sidebar_width_minimum 122px

Same here

r-, need some clarification on the changes

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Sorry about the color problems. This mostly works, but isn't quite right, especially on desktop. I used some hackyness -moz-translate with em units hackiness to get the tabs-button to draw the text inside the button. I think I'll probably just put together a special button widget for that instead, because the font-sizes we have setup on desktop don't seem to correlate very well with the font-sizes we have on device.

I also have some small problems with the star button if I don't bump up touch_button_xlarge a mm. I'll attach a diff for you to look at in the meantime mfinkle.

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Diff with gingerbread theme

Comment 14

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Whoops. Talked to mfinkle yesterday and the font and touch size changes are ok with him and UX for now.

Comment 15

[Wesley Johnston (:wesj)]

Attachment: Diff with gingerbread theme

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Patch v3

I just removed the documenttab label stuff for now. That was mostly leftover from my fist version of this, which rethemed the tabbar.

Comment 17

[Wesley Johnston (:wesj)]

Attachment: Diff with gingerbread theme

Comment 18

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

Comment on attachment 555773 [details] [diff] [review]
Patch v3

looks good to me.

I'd like to see a patch that backports the new, more specific color_background_* flags to gingerbread too. No need to worry about froyo.

Comment 19

[Wesley Johnston (:wesj)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/8dc1b204359a

Comment 20

[Matt Brubeck (:mbrubeck)]

Attachment: followup

The <spacer> is showing up in non-tablet mode and messing up the urlbar slightly.

Comment 21

[Matt Brubeck (:mbrubeck)]

Pushed follow-up:
http://hg.mozilla.org/integration/mozilla-inbound/rev/9f11fc87eaa6

Comment 22

[Ed Morley [:emorley]]

Follow-up: http://hg.mozilla.org/mozilla-central/rev/9f11fc87eaa6

Comment 23

[Ed Morley [:emorley]]

Main part: http://hg.mozilla.org/mozilla-central/rev/8dc1b204359a

Comment 24

[Aaron Train [:aaronmt]]

Recently acquired a Galaxy Tab 10.1, checking out this theme in 09/02 Nightly - should this be what I am seeing based on this bug in particular? http://i.imgur.com/nQo2E.jpg

Comment 25

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

(In reply to Aaron Train [:aaronmt] from comment #24)
> Recently acquired a Galaxy Tab 10.1, checking out this theme in 09/02
> Nightly - should this be what I am seeing based on this bug in particular?
> http://i.imgur.com/nQo2E.jpg

Yes, that's it for now. We have a few other bugs filed (and landed) to tweak it further.

Comment 26

[Aaron Train [:aaronmt]]

Verified Fixed based on comment #25
Mozilla/5.0 (Android; Linux armv7l; rv:9.0a1) Gecko/20110902 Firefox/9.0a1 Fennec/9.0a1