838211 - Work - Info app bar theming, part 1 (color and button style changes)

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Theme, defect, P1)

Description

[Jim Mathies [:jimm]]

This is partially done, but it needs work. The background color seems off, and the buttons don't look right.

Comment 1

[Matt Brubeck (:mbrubeck)]

Bug 837436 drops the old Android-themed notification widget, replacing it with the standard desktop widget (plus some basic Metro theming inherited from platform.css).  We'll still need some additional theme changes to fully adapt that desktop widget to Metro.

Comment 2

[Matt Brubeck (:mbrubeck)]

We'll need a basic design spec or mockup for how this should be themed (background color, font, button style, "close" icon).

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: screenshot: current look with desktop widget

For reference, here's what this looks like now (with bug 837436 fixed).

Comment 4

[Stephen Horlander [:shorlander]]

Attachment: InfoBars - Design Spec - i01

First Design Spec, might need some iteration and clarification.

Comment 5

[Stephen Horlander [:shorlander]]

Attachment: InfoBar Close Icon

Comment 6

[Stephen Horlander [:shorlander]]

Attachment: InfoBar Arrow

Comment 7

[Stephen Horlander [:shorlander]]

Attachment: InfoBar Password Icon

Comment 8

[Stephen Horlander [:shorlander]]

Attachment: InfoBar Geolocation Icon

Comment 9

[Matt Brubeck (:mbrubeck)]

Attachment: WIP

Some questions that came up while I was working on this patch:

1) Should we use these buttons styles for dialogs too?  Currently our default button style is white with black text and black 2px border.  It would actually just as easy (or easier) to change all our buttons at once than to override them just for the info bars.

2) Will the first button in the info bar always be the "default" one?

3) How tall is the shadow above the info bar?

Comment 10

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 2

Uses the new styles for all buttons, not just in notifications.  Depends on bug 841587.  Only a few of the styles from the spec are implemented so far.

Comment 11

[Stephen Horlander [:shorlander]]

(In reply to Matt Brubeck (:mbrubeck) from comment #9)
> Created attachment 713736 [details] [diff] [review]
> WIP
> 
> Some questions that came up while I was working on this patch:
> 
> 1) Should we use these buttons styles for dialogs too?  Currently our
> default button style is white with black text and black 2px border.  It
> would actually just as easy (or easier) to change all our buttons at once
> than to override them just for the info bars.

Yes, we should make all push buttons consistent.

> 2) Will the first button in the info bar always be the "default" one?

Left most being the default is typical but I have seen counter examples. All of our infobars that I can think of that apply to Metro follow that convention.

> 3) How tall is the shadow above the info bar?

Fades out completely at 10px

Comment 12

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 3

Finished adding all the button states.  To do: Add the assets, tweak the spacing, and get the notification border+shadow to look right.

Comment 13

[Matt Brubeck (:mbrubeck)]

(In reply to Stephen Horlander from comment #11)
> > 2) Will the first button in the info bar always be the "default" one?
> 
> Left most being the default is typical but I have seen counter examples. All
> of our infobars that I can think of that apply to Metro follow that
> convention.

For bug 841587, we need to decide whether "first button = default" is a rule that we should write into our code, or just a guideline that may have exceptions.  The one possible exception I've seen in our stories is the "blocked popup" bar -- see bug 831947 comment 1.

Comment 14

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 4

Comment 15

[Frank Yan (:fryn)]

Attachment: Info app bar opening animation - design

Click the gray toolbar on the bottom to toggle the info app bar visibility.

Comment 16

[Matt Brubeck (:mbrubeck)]

We are splitting this work into two phases.  For this initial bug we will land the color/background changes.  In bug 845348, as part of the overall NewUI work (bug 845137), we will move the bar to the bottom and add the arrow graphic.

Comment 17

[Matt Brubeck (:mbrubeck)]

Attachment: part 1: colors and images

This includes the basic colors, backgrounds, images, and padding/margins from the design spec.  It does not include the shadow or the arrow image, which we will add later as part of bug 845348.

I'll post a separate patch for the string changes.

Comment 18

[Matt Brubeck (:mbrubeck)]

Attachment: part 2: password manager strings

Changes the button labels, and gets rid of the "Not Now" button.

Comment 19

[Matt Brubeck (:mbrubeck)]

Attachment: part 1: colors and images (v2)

Fixed some issues for notifications that have no images.  (We should eventually add images for all our built-in notifications, but we don't have all of them yet.)

Comment 20

[Stephen Horlander [:shorlander]]

Attachment: InfoBars - Design Spec - i02

Messed up the color value for the top/bottom border: Was hsla(0,0%,0%,.7) should have been hsla(0,0%,0%,.07)

Comment 21

[Matt Brubeck (:mbrubeck)]

Attachment: part 3: ContentPermissionPrompt strings

This changes the strings for the geolocation permission prompt to match the design spec.  The biggest change is that we now show explicit "Always for this Site" and "Never for this Site" buttons in the prompt, rather than implicitly setting these when you choose Allow or Deny five times in a row.

This also affects other content permissions that use this prompt (offline storage, desktop notifications, and web app installation -- though I don't think the latter two are actually enabled for Metro).

Lastly, it fixes a silly bug where we were passing {accessKey: null} and it was getting intepreted as {accessKey: "n"}.

Comment 22

[Frank Yan (:fryn)]

Comment on attachment 718767 [details] [diff] [review]
part 1: colors and images (v2)

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

::: browser/metro/base/content/browser.js
@@ +1255,5 @@
>          }
>          else {
>            var buttons = [
>              {
> +              isDefault: false,

What is the purpose of this?

Comment 23

[Matt Brubeck (:mbrubeck)]

(In reply to Frank Yan (:fryn) from comment #22)
> >            var buttons = [
> >              {
> > +              isDefault: false,
> 
> What is the purpose of this?

By default, the first button in the notification bar is marked as .notification-button-default, which we use to turn it orange.  For the popup blocker bar, we don't want any of the buttons to have default styling, so we turn it off here.  For more context, see bug 841587 and bug 831947 comment 2.

Comment 24

[Matt Brubeck (:mbrubeck)]

Attachment: screenshot: geolocation prompt (after patches)

Here's a screenshot of how the geolocation prompt looks with these patches applied.  And the link below has several more showing how other notification bars, and also how the new button styles look in dialogs and flyouts:
http://people.mozilla.com/~mbrubeck/notification/

Please let me know if any changes are needed, and if so then I will make them in this bug or file follow-up bugs as appropriate.

In particular, some of the strings from the design spec attachment 719299 [details] are different than the ones in stories (e.g. bug 831949); which ones should I prefer?

Comment 25

[Asa Dotzler [:asa]]

(In reply to Matt Brubeck (:mbrubeck) from comment #24)
> Created attachment 720921 [details]
> screenshot: geolocation prompt (after patches)

Looks great! 

> In particular, some of the strings from the design spec attachment 719299 [details]
> [details] are different than the ones in stories (e.g. bug 831949); which
> ones should I prefer?

Go with the strings in shorlander's mock-ups.

Comment 26

[Stephen Horlander [:shorlander]]

Comment on attachment 720921 [details]
screenshot: geolocation prompt (after patches)

Looks good! Found some alignment issues and the blurry button issue we talked about on IRC:

- Space between the icon and the message should be 12px (it is 27px)
- Button should be 32px tall
- Space between buttons should be 20px (currently 25px and 23px)
- Start|End button padding should be 16px

Thank you!

Comment 27

[Matt Brubeck (:mbrubeck)]

Attachment: part 1: colors and images (v3)

Updated to address shorlander's ui-review comments -- just needed some minor CSS tweaks to properly override some of the default theme styles.  Carrying r=fryn.

Comment 28

[Matt Brubeck (:mbrubeck)]

Attachment: updated screenshot

Comment 29

[Stephen Horlander [:shorlander]]

Comment on attachment 721735 [details]
updated screenshot

> .button-default,
> .notification-button-default {
>  background: linear-gradient(to bottom, hsl(35, 100%, 50%), hsl(30, 100%, 50%));
>  border-color: hsl(35, 100%, 48%);
>  color: white;
>}

Looks good, thank you! Would you mind changing the border-color here to hsl(30, 100%, 48%) please? I put the wrong value in the spec image. Thanks!

Comment 30

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/7a20fb6025b3
https://hg.mozilla.org/mozilla-central/rev/b35b2a98eb6d
https://hg.mozilla.org/mozilla-central/rev/855a01a68826