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

RESOLVED FIXED in Firefox 22

Status

Firefox for Metro
Theme
P1
normal
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: jimm, Assigned: mbrubeck)

Tracking

Trunk
Firefox 22
All
Windows 8.1
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=work)

Attachments

(10 attachments, 9 obsolete attachments)

3.04 KB, image/png
Details
173 bytes, image/png
Details
1001 bytes, image/png
Details
1.12 KB, image/png
Details
1.32 KB, text/html
Details
4.86 KB, patch
fryn
: review+
Details | Diff | Splinter Review
1.35 MB, image/png
Details
7.07 KB, patch
fryn
: review+
Details | Diff | Splinter Review
27.81 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
53.70 KB, image/png
shorlander
: ui-review+
Details
(Reporter)

Description

5 years ago
This is partially done, but it needs work. The background color seems off, and the buttons don't look right.
(Reporter)

Updated

5 years ago
Blocks: 831947
(Reporter)

Updated

5 years ago
Blocks: 831949
(Assignee)

Comment 1

5 years ago
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.
Depends on: 837436
(Assignee)

Comment 2

5 years ago
We'll need a basic design spec or mockup for how this should be themed (background color, font, button style, "close" icon).
Flags: needinfo?(shorlander)
Keywords: uiwanted
Hardware: x86_64 → All
Summary: Notification bar theming → Work - Notification bar theming
Whiteboard: feature=work
(Assignee)

Comment 3

5 years ago
Created attachment 710307 [details]
screenshot: current look with desktop widget

For reference, here's what this looks like now (with bug 837436 fixed).
Attachment #710307 - Attachment is patch: false
Attachment #710307 - Attachment mime type: text/plain → image/png
(Assignee)

Updated

5 years ago
Blocks: 838734

Updated

5 years ago
Blocks: 838497
QA Contact: shorlander

Updated

5 years ago
Priority: -- → P1

Updated

5 years ago
Summary: Work - Notification bar theming → Work - Info app bar theming
Created attachment 713273 [details]
InfoBars - Design Spec - i01

First Design Spec, might need some iteration and clarification.
Flags: needinfo?(shorlander)
Created attachment 713274 [details]
InfoBar Close Icon
Created attachment 713275 [details]
InfoBar Arrow
Created attachment 713276 [details]
InfoBar Password Icon
Created attachment 713277 [details]
InfoBar Geolocation Icon
(Assignee)

Comment 9

5 years ago
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.

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

3) How tall is the shadow above the info bar?
Assignee: nobody → mbrubeck
Status: NEW → UNCONFIRMED
Ever confirmed: false
Flags: needinfo?(shorlander)
(Assignee)

Updated

5 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Updated

5 years ago
Depends on: 841587
(Assignee)

Comment 10

5 years ago
Created attachment 714147 [details] [diff] [review]
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.
Attachment #713736 - Attachment is obsolete: true
(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
Flags: needinfo?(shorlander)
(Assignee)

Updated

5 years ago
Keywords: uiwanted
(Assignee)

Comment 12

5 years ago
Created attachment 714453 [details] [diff] [review]
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.
Attachment #714147 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
(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.
(Assignee)

Comment 14

5 years ago
Created attachment 716059 [details] [diff] [review]
WIP 4
Attachment #710307 - Attachment is obsolete: true
Attachment #714453 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 835623

Comment 15

5 years ago
Created attachment 718083 [details]
Info app bar opening animation - design

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

Updated

5 years ago
Blocks: 845348
(Assignee)

Comment 16

5 years ago
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.
Summary: Work - Info app bar theming → Work - Info app bar theming, part 1 (color and button style changes)
(Assignee)

Comment 17

5 years ago
Created attachment 718691 [details] [diff] [review]
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.
Attachment #716059 - Attachment is obsolete: true
Attachment #718691 - Flags: review?(fyan)
(Assignee)

Comment 18

5 years ago
Created attachment 718722 [details] [diff] [review]
part 2: password manager strings

Changes the button labels, and gets rid of the "Not Now" button.
Attachment #718722 - Flags: review?(fyan)
(Assignee)

Comment 19

5 years ago
Created attachment 718767 [details] [diff] [review]
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.)
Attachment #718691 - Attachment is obsolete: true
Attachment #718691 - Flags: review?(fyan)
Attachment #718767 - Flags: review?(fyan)
Created attachment 719299 [details]
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)
Attachment #713273 - Attachment is obsolete: true
(Assignee)

Comment 21

5 years ago
Created attachment 720163 [details] [diff] [review]
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"}.
Attachment #720163 - Flags: review?(fyan)

Comment 22

5 years ago
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?
Attachment #718767 - Flags: review?(fyan) → review+

Updated

5 years ago
Attachment #718722 - Flags: review?(fyan) → review+

Updated

5 years ago
Attachment #720163 - Flags: review?(fyan) → review+
(Assignee)

Comment 23

5 years ago
(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.
(Assignee)

Comment 24

5 years ago
Created attachment 720921 [details]
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?
Attachment #720921 - Flags: ui-review?(shorlander)
Attachment #720921 - Flags: feedback?(asa)

Updated

5 years ago
Attachment #720921 - Attachment is patch: false
Attachment #720921 - Attachment mime type: text/plain → image/png

Comment 25

5 years ago
(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.
(Assignee)

Updated

5 years ago
Attachment #720921 - Flags: feedback?(asa)
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!
Attachment #720921 - Flags: ui-review?(shorlander) → ui-review-

Updated

5 years ago
No longer depends on: 835623
(Assignee)

Comment 27

5 years ago
Created attachment 721734 [details] [diff] [review]
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.
Attachment #718767 - Attachment is obsolete: true
Attachment #721734 - Flags: review+
(Assignee)

Comment 28

5 years ago
Created attachment 721735 [details]
updated screenshot
Attachment #720921 - Attachment is obsolete: true
Attachment #721735 - Flags: ui-review?
(Assignee)

Updated

5 years ago
Attachment #721735 - Flags: ui-review? → ui-review?(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!
Attachment #721735 - Flags: ui-review?(shorlander) → ui-review+
(Assignee)

Comment 30

5 years ago
https://hg.mozilla.org/mozilla-central/rev/7a20fb6025b3
https://hg.mozilla.org/mozilla-central/rev/b35b2a98eb6d
https://hg.mozilla.org/mozilla-central/rev/855a01a68826
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.