1230212 - Implement updated design spec for XUL alerts

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Screenshot of some of the measurements

See https://mozilla.invisionapp.com/share/4G55MBTKR#/screens

Comment 1

[Edwin Wong [:edwong]]

phlsa: let's go with 80x80 px

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

This updates to the new spec, as well as fixes the spacing for non-web notifications such as the Update Available notification that Firefox shows on its own (fixed by the addition of the `:not([hasOrigin])` selector).

I also fixed the color of the gear to match the color of the close button, which is greyscale on Windows 8 and 10+. I didn't add a section to keep it blue-ish on other versions of Windows, but I can if you'd like.

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8695430 [details] [diff] [review]
Patch

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

::: toolkit/themes/shared/alert-common.css
@@ +53,5 @@
>    }
>  }
>  
>  #alertImage {
> +  width: 80px;

I thought we agree to make this a maximum to not upscale? Also, why don't we have a max-height anymore? What's stopping me from making an image that's 80px wide but 1000px high?

@@ +70,5 @@
>    width: 255px;
>  }
>  
> +#alertBox:not([hasImage]) > box > #alertTextBox {
> +  padding-inline-start: 8px;

Where did this number come from? I see 8px in the spec for the title without a favicon but this is for the body text.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

Interdiff:
> diff --git a/toolkit/themes/shared/alert-common.css b/toolkit/themes/shared/alert-common.css
> --- a/toolkit/themes/shared/alert-common.css
> +++ b/toolkit/themes/shared/alert-common.css
> @@ -50,16 +50,20 @@
>  }
> 
>  #alertImage {
>    width: 80px;
> +  height: 80px;
> +  max-width: 80px;
> +  max-height: 80px;
> +  object-fit: scale-down;
>    margin: 0 7px 7px;
>  }
> 
>  .alertTextBox {

(In reply to Matthew N. [:MattN] from comment #3)
> I thought we agree to make this a maximum to not upscale? Also, why don't we
> have a max-height anymore? What's stopping me from making an image that's
> 80px wide but 1000px high?

Yes, that's right. I added the above changes to make sure that the box is always 80x80 and that we won't distort the image when drawing in the box.

> 
> @@ +70,5 @@
> >    width: 255px;
> >  }
> >  
> > +#alertBox:not([hasImage]) > box > #alertTextBox {
> > +  padding-inline-start: 8px;
> 
> Where did this number come from? I see 8px in the spec for the title without
> a favicon but this is for the body text.

This was an eye test where I had to adjust the numbers pixel by pixel to get it to look right.

Comment 5

[Lina Butler (ex-Mozilla)]

Comment on attachment 8698120 [details] [diff] [review]
Patch v1.1

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

Looks good!

::: toolkit/themes/shared/alert-common.css
@@ +67,2 @@
>    /* The text box width is increased to make up for the lack of image when one
> +     is not provided. 333px is the text box width when a picture is present,

Where does the 333px come in?

@@ +74,5 @@
>    width: 255px;
>  }
>  
> +#alertBox:not([hasImage]) > box > #alertTextBox {
> +  padding-inline-start: 8px;

Neat, I didn't know about this property. Are you using it because we might have RTL text?

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Kit Cambridge [:kitcambridge] from comment #5)
> Comment on attachment 8698120 [details] [diff] [review]
> Patch v1.1
> 
> Review of attachment 8698120 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good!
> 
> ::: toolkit/themes/shared/alert-common.css
> @@ +67,2 @@
> >    /* The text box width is increased to make up for the lack of image when one
> > +     is not provided. 333px is the text box width when a picture is present,
> 
> Where does the 333px come in?

Ah, I forgot to update that comment. It should be 349px in that line, which is explained in the following line.
 
> @@ +74,5 @@
> >    width: 255px;
> >  }
> >  
> > +#alertBox:not([hasImage]) > box > #alertTextBox {
> > +  padding-inline-start: 8px;
> 
> Neat, I didn't know about this property. Are you using it because we might
> have RTL text?

Yeah, this is to help us support RTL text, which should be done by default.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/6a2ad662612370d248530465d690785c20abcf0b
Bug 1230212 - Implement updated design spec for XUL alerts. r=kitcambridge

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6a2ad6626123

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8698120 [details] [diff] [review]
Patch v1.1

Approval Request Comment
[Feature/regressing bug #]: implemented finished design for push notifications shipping in firefox 44
[User impact if declined]: push notifications will change UI designs after 44 ships, and 44 will ship with a less-than-ideal UI that has improper margins, spacing, and extra noise
[Describe test coverage new/current, TreeHerder]: visual UI changes, no tests, on m-c for > 3 days now
[Risks and why]: none expected, CSS and SVG changes only
[String/UUID change made/needed]: none

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for Aurora45 and Beta44

Approval Request Comment
[Feature/regressing bug #]: implemented finished design for push notifications shipping in firefox 44
[User impact if declined]: push notifications will change UI designs after 44 ships, and 44 will ship with a less-than-ideal UI that has improper margins, spacing, and extra noise
[Describe test coverage new/current, TreeHerder]: visual UI changes, no tests, on m-c for > 3 days now
[Risks and why]: none expected, CSS and SVG changes only
[String/UUID change made/needed]: none

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8700673 [details] [diff] [review]
Patch for Aurora45 and Beta44

Visual fit and finish for push notifications, Beta44+, Aurora45+

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/64b098725084

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6c52c5091ef2

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/64b098725084