688802 - [TABLETUI] Get consistent styling across our menus and dialogs

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ian Barlow (:ibarlow)]

Marked up screenshots coming shortly

Comment 1

[Ian Barlow (:ibarlow)]

Attachment: mockup of comment

This should get our menus looking a little more consistent. Comments are marked up in the included PNG, but here is a high level overview:

* All menu rows should be 10.48mm tall
* We should use the "awesomebar page title" font size for almost everything
* Add 0.3mm rounded corners to menus

The mockups goes into more detail -- have a look, let me know if there are any questions / concerns

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Tweak app menu to match planned design

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Tweak context popups to match planned design

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: Tweak browser prompts to match planned design

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: Tweak awesome panels to match planned design

This one should go first.

Comment 6

[Lucas Rocha (:lucasr)]

FYI: I'm still working on patches for the site info and bookmark popups. But the attached patches are self-contained and can be reviewed now.

Comment 7

[Lucas Rocha (:lucasr)]

Attachment: Screen

Comment 8

[Lucas Rocha (:lucasr)]

Attachment: Screenshot of context popup

Comment 9

[Lucas Rocha (:lucasr)]

Attachment: Screenshot of awesome panels

Comment 10

[Lucas Rocha (:lucasr)]

Attachment: Screenshot of app menu

Comment 11

[Wesley Johnston (:wesj)]

Comment on attachment 562717 [details] [diff] [review]
Tweak app menu to match planned design

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

::: mobile/themes/core/honeycomb/browser.css
@@ +1665,5 @@
>    -moz-box-align: center;
>    border-top: @border_width_tiny@ solid @color_button_border@;
>    border-bottom: none;
>    background-color: transparent;
> +  font-size: @font_snormal@ !important;

I wonder if we should just do this for all richlistitems. Seems like I've seen it somewhere else...

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 562718 [details] [diff] [review]
Tweak context popups to match planned design

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

I think we can make this a little simpler...

::: mobile/themes/core/honeycomb/browser.css
@@ +1276,5 @@
>  
>  /* Override richlistbox and richlistitem styles */
>  #context-header {
> +  border-bottom: @border_width_xxlarge@ solid @color_divider_border@;
> +  margin: 0px @margin_large@ 0px;

No need for the 0px on the end

@@ +1283,5 @@
> +}
> +
> +#context-header > label {
> +  text-align: center;
> +  padding: @padding_xlarge@ 0px;

Can you try using -moz-box-align: center on the #content-header to see if we can avoid this (maybe -moz-box-pack: center to center these if thats what we want)? I don't think the titles are supposed to be centered though...

@@ +1297,5 @@
>    -moz-box-align: center;
>    padding: 0px @padding_large@;
>    border: none;
>    border-top: @border_width_tiny@ solid transparent;
> +  font-size: @font_snormal@;

This makes me wonder about the richlistitems again... we use them everywhere though. So changing them means testing... everything. Maybe better in a follow up that we can easily track whatever else it fits.

Comment 13

[Wesley Johnston (:wesj)]

Comment on attachment 562722 [details] [diff] [review]
Tweak awesome panels to match planned design

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

This looks pretty good. I'm looking at using transforms to position these things in another bug, so I'm not sure we need the negative margin. Can we drop it for now.

Comment 14

[Lucas Rocha (:lucasr)]

Attachment: Tweak context popups to match planned design

I had tried to use -moz-box-align:center on #context-header but it doesn't work, to be my surprise. I removed the incorrect text-align: center from the header.

The reason I didn't apply style changes to richlistitem directly is that it cascades to too many parts of the UI and I felt it was a bit too risky to change it here.

Comment 15

[Lucas Rocha (:lucasr)]

Attachment: Tweak awesome panels to match planned design

Removed the negative margin. Keeping the review+.

Comment 16

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

Be careful with some changes. Just make sure the changes don't cascade poorly into other UI. Bug 682412 changed the touch_row height for example.

Let's not get too focused on making pixel perfect consistency to mockups. We reuse concepts and widgets throughout Firefox. This is a good thing. Creating CSS for commonly used widgets not only helps us internally, it helps add-on developers create UIs that match well with Firefox.

Comment 17

[Wesley Johnston (:wesj)]

Comment on attachment 563411 [details] [diff] [review]
Tweak context popups to match planned design

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

Sorry. I'm probably being really picky here after checking in a lot of rough "this sorta looks right stuff" myself. But I want one more run through this. I'd actually feel like we're close enough now, we should start getting extra picky or this last 10% is just going to drag on for too long.

::: mobile/themes/core/honeycomb/browser.css
@@ +1287,5 @@
> +}
> +
> +#context-header > label {
> +  padding: @padding_xlarge@ 0px;
> +  margin: 0px;

All labels actually have a margin-start (and margin-end maybe?) set on them in platform.css. If we can override it here (this apparently isn't enough to do so), these lines will line up better with the border on the header.

Ian's mockups actually have some padding on the inside of the labels as well (although his lines between rich list items seem to all be slightly different lengths, I'm guessing either my imagination, or not correct).

Comment 18

[Lucas Rocha (:lucasr)]

Attachment: Tweak context popups to match planned design

This patch makes context popups look much more like the mockups.

Comment 19

[Lucas Rocha (:lucasr)]

Attachment: Screenshot of context popup

Comment 20

[Wesley Johnston (:wesj)]

Comment on attachment 564534 [details] [diff] [review]
Tweak context popups to match planned design

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

Looks fantastic.

Comment 21

[Wesley Johnston (:wesj)]

Comment on attachment 562719 [details] [diff] [review]
Tweak browser prompts to match planned design

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

This still needs the text-align: center removed. I'd probably be happy with that alone, but I think it also needs some padding added to the left side so that the text and border-left edges are not aligned.

Comment 22

[Lucas Rocha (:lucasr)]

Attachment: Tweak browser prompts to match planned design

I had already fixed it locally but forgot to update the patch here.

Comment 23

[Lucas Rocha (:lucasr)]

Attachment: Screenshot of browser prompt

Comment 24

[Lucas Rocha (:lucasr)]

Pushed all approved patches:
http://hg.mozilla.org/integration/mozilla-inbound/rev/343233066e64
http://hg.mozilla.org/integration/mozilla-inbound/rev/a858c168dcc9
http://hg.mozilla.org/integration/mozilla-inbound/rev/62aaeeef2ba1
http://hg.mozilla.org/integration/mozilla-inbound/rev/78b48c64903c

Reported bug 694049 and bug 694050 to track the remaining issues on bookmark popups and site menu, respectively.

Comment 25

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/343233066e64
https://hg.mozilla.org/mozilla-central/rev/a858c168dcc9
https://hg.mozilla.org/mozilla-central/rev/62aaeeef2ba1
https://hg.mozilla.org/mozilla-central/rev/78b48c64903c