Closed Bug 688802 Opened 8 years ago Closed 8 years ago

[TABLETUI] Get consistent styling across our menus and dialogs

Categories

(Firefox for Android Graveyard :: General, defect)

Firefox 9
x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 10

People

(Reporter: ibarlow, Unassigned)

References

Details

Attachments

(9 files, 6 obsolete files)

374.93 KB, image/png
Details
1.49 KB, patch
wesj
: review+
Details | Diff | Splinter Review
125.47 KB, image/png
Details
112.54 KB, image/png
Details
1.77 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
1.83 KB, patch
wesj
: review+
Details | Diff | Splinter Review
50.98 KB, image/png
Details
1.52 KB, patch
wesj
: review+
Details | Diff | Splinter Review
23.85 KB, image/png
Details
Marked up screenshots coming shortly
Attached image 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
Blocks: 688489
Attachment #562718 - Flags: review?(wjohnston)
Attachment #562719 - Flags: review?(wjohnston)
This one should go first.
Attachment #562722 - Flags: review?(wjohnston)
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.
Attached image Screen (obsolete) —
Attached image Screenshot of context popup (obsolete) —
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...
Attachment #562717 - Flags: review?(wjohnston) → review+
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.
Attachment #562718 - Flags: review?(wjohnston) → review-
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.
Attachment #562722 - Flags: review?(wjohnston) → review+
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.
Attachment #562718 - Attachment is obsolete: true
Attachment #563411 - Flags: review?(wjohnston)
Removed the negative margin. Keeping the review+.
Attachment #562722 - Attachment is obsolete: true
Attachment #563413 - Flags: review+
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 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).
This patch makes context popups look much more like the mockups.
Attachment #563411 - Attachment is obsolete: true
Attachment #563411 - Flags: review?(wjohnston)
Attachment #564534 - Flags: review?(wjohnston)
Attachment #562725 - Attachment is obsolete: true
Comment on attachment 564534 [details] [diff] [review]
Tweak context popups to match planned design

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

Looks fantastic.
Attachment #564534 - Flags: review?(wjohnston) → review+
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.
Attachment #562719 - Flags: review?(wjohnston) → review-
I had already fixed it locally but forgot to update the patch here.
Attachment #562719 - Attachment is obsolete: true
Attachment #566154 - Flags: review?(wjohnston)
Attachment #562723 - Attachment is obsolete: true
Attachment #566154 - Flags: review?(wjohnston) → review+
You need to log in before you can comment on or make changes to this bug.