Closed Bug 679472 Opened 9 years ago Closed 9 years ago

Make Fennecomb Action Bar pretty

Categories

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

Firefox 8
All
Android
defect
Not set
blocker

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 9

People

(Reporter: ibarlow, Assigned: wesj)

References

Details

Attachments

(5 files, 9 obsolete files)

Attached file 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
Assignee: nobody → wjohnston
Attached patch WIP (obsolete) — Splinter Review
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 )
Hi Wes, thanks for posting. I have some spacing and sizing comments for you here, regarding the URL bar http://cl.ly/401R3f2r1c2b3q1l0X43
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.
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #554285 - Attachment is obsolete: true
Attached patch Diff with gingerbread theme (obsolete) — Splinter Review
Hi Wes, this is looking good. I have a few more comments here, having seen the bar in action now: http://cl.ly/1r0h413Y1d0d1K3Z2M0N
Blocks: 655762
Hardware: x86 → All
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #554526 - Attachment is obsolete: true
Attachment #554921 - Flags: review?(mark.finkle)
Attached patch Diff with gingerbread theme (obsolete) — Splinter Review
Gingerbread diff
Attachment #554527 - Attachment is obsolete: true
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 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
Attachment #554921 - Flags: review?(mark.finkle) → review-
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
Attachment #554922 - Flags: review-
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #554921 - Attachment is obsolete: true
Attached patch Diff with gingerbread theme (obsolete) — Splinter Review
Attachment #554922 - Attachment is obsolete: true
Attached patch Patch v2 (obsolete) — Splinter Review
Whoops. Talked to mfinkle yesterday and the font and touch size changes are ok with him and UX for now.
Attachment #555599 - Attachment is obsolete: true
Attachment #555759 - Flags: review?(mark.finkle)
Attached patch Diff with gingerbread theme (obsolete) — Splinter Review
Attachment #555600 - Attachment is obsolete: true
Attached patch Patch v3Splinter Review
I just removed the documenttab label stuff for now. That was mostly leftover from my fist version of this, which rethemed the tabbar.
Attachment #555759 - Attachment is obsolete: true
Attachment #555759 - Flags: review?(mark.finkle)
Attachment #555773 - Flags: review?(mark.finkle)
Attachment #555760 - Attachment is obsolete: true
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.
Attachment #555773 - Flags: review?(mark.finkle) → review+
Attached patch followupSplinter Review
The <spacer> is showing up in non-tablet mode and messing up the urlbar slightly.
Attachment #555806 - Flags: review?(wjohnston)
Attachment #555806 - Flags: review?(wjohnston) → review+
Follow-up: http://hg.mozilla.org/mozilla-central/rev/9f11fc87eaa6
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 9
Depends on: 682291
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
(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.
Verified Fixed based on comment #25
Mozilla/5.0 (Android; Linux armv7l; rv:9.0a1) Gecko/20110902 Firefox/9.0a1 Fennec/9.0a1
Status: RESOLVED → VERIFIED
Depends on: 692828
You need to log in before you can comment on or make changes to this bug.