796111 - Adjust design of nsIAlertService windows to match notification design

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

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

Attachment: Design of toast notifications

The toast notification design can be updated to match the design of the web notification toasts without much structural change.

See the attachment for the design by Stephen.

Comment 1

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

Attachment: Patch

Comment 2

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

Requesting tracking-fx17 since the toast notifications are a critical part of the Social API.

Comment 3

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

Attachment: Screenshot of patch

This is a screenshot of the patch. Note that I did not include the "X" in the upper-right corner, as that doesn't already exist for nsIAlertService notifications.

Comment 4

[Dão Gottwald [:dao]]

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

> .alertBox {
>-  border: 1px solid threedshadow;
>+  border: 1px solid #dadada;
>+  border-bottom-color: #969696;

These border colors seem quite different from the mockup.

>   border-radius: 3px;
>   background-color: -moz-Dialog;
>-  min-height: 50px;
>-  padding: 8px;

What's the effect of removing the padding when there's more text?

>+  font: message-box;

What's the effect of "font: message-box" here? I would expect it to be a no-op.

>+  min-width: 255px;
>+  max-width: 255px;

Just use 'width'?

How do alert boxes with vertical orientation look after your changes?

Bunch of hardcoded colors that assume the default theme -- you can overlay native colors with translucent ones e.g. in order to get the gradient, or target the default theme explicitly via @media.

Comment 5

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

Attachment: Patch v2

Moved some of the colors to a @media(-moz-windows-default-theme) media query and used -moz-dialog with translucent colors for non-default theme.

The colors are not the exact same as the mockup since the mockup shows a box-shadow that I was unable to get to work. Having a darker bottom-border color will still create the appearance of a shadow on the alert.

If the text is too large to fit, a scrollbar will appear on the text area. I think this is acceptable.

I talked with Stephen on IRC and he said to kill the vertical orientation, since it doesn't provide any extra value but makes it harder to come up with a nice visual design.

2:53 PM <jaws> shorlander: do we need a vertical orientation for our toast notifications?
2:53 PM <jaws> i don't think it's necessary
2:53 PM <shorlander> jaws: vertical orientation?
2:53 PM <shorlander> I don't know when that would ever be needed :)
2:53 PM <jaws> yeah, if the windows taskbar is on the side of the screen we show it vertically
2:54 PM <jaws> the image is on top of the text
2:54 PM <jaws> ok, i'd like to kill it with your approval
2:55 PM <shorlander> [KILLTHEM]

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 667210 [details] [diff] [review]
Patch v2

(In reply to Jared Wein [:jaws] from comment #5)
> The colors are not the exact same as the mockup since the mockup shows a
> box-shadow that I was unable to get to work. Having a darker bottom-border
> color will still create the appearance of a shadow on the alert.

The box shadow isn't significantly offset at the bottom, so the darker bottom border doesn't seem like a good approximation. In fact, the threedshadow border on all sides is probably a closer approximation...

> If the text is too large to fit, a scrollbar will appear on the text area. I
> think this is acceptable.

All text should be visible at once. The alert isn't interactive (it closes when clicked) and the user doesn't really have the time to scroll it anyway.

>     if (gOrigin & NS_ALERT_TOP) {
>       document.documentElement.pack = "end";
>       alertBox.setAttribute("origin", "top");
>     } else {
>       alertBox.setAttribute("origin", "bottom");
>     }
>   }
> 
>-  alertBox.orient = (gOrigin & NS_ALERT_HORIZONTAL) ? "vertical" : "horizontal";

The way "pack" is set above only makes sense in combination with "orient".

>+@media (-moz-windows-default-theme) {

>+  .alertImageBox {
>+    -moz-border-end: 1px solid #dadada;
>+    background-image: linear-gradient(#f6f6f6, #eeee);
>+  }

#eeee isn't valid.

Is this even needed? Why specifically target the default theme here rather than using a generic overlay to get the desired effect?

>+  .alertTextBox {
>+    background-image: linear-gradient(#f2f3f7, #dce2e2);

Ditto, a generic overlay would be preferable. I suspect the colors you're hardcoding here don't make much sense for the XP default theme and maybe not for Windows 8 default theme either.

Comment 7

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

Attachment: Patch v3

This patch fixes the issues that were pointed out in the previous review.

Here are two screenshots of the patch applied:
Download completed (normal size): http://screencast.com/t/drKiJ75FZc9p
Oversize example: http://screencast.com/t/2GtmjuG3IT

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 667834 [details] [diff] [review]
Patch v3

>+  let alertTextBox = document.getElementById("alertTextBox");
>+  let alertImageBox = document.getElementById("alertImageBox");
>+  alertTextBox.style.minHeight = alertTextBox.scrollHeight + "px";
>+  alertImageBox.style.minHeight = alertTextBox.scrollHeight + "px";

alertTextBox is a vbox that should have overflow:visible as its initial style, so I don't understand how it would overflow.

gnomestripe/global/alerts/alert.css still depends on the orient attribute.

Comment 9

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

Attachment: Patch v4

Thanks for catching gnomestripe. I updated gnomestripe to follow the same design as winstripe, and also removed a hardcoded value for active hyperlinks and replaced it with -moz-activehyperlinktext.

I was able to remove the minHeight setting on the alertTextBox, but the minHeight is needed on the alertImageBox to get the window sized correctly by sizeToContent().

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 668188 [details] [diff] [review]
Patch v4

>--- a/toolkit/themes/gnomestripe/global/alerts/alert.css
>+++ b/toolkit/themes/gnomestripe/global/alerts/alert.css

> .alertBox {
>-  border: 1px solid ThreeDShadow;
>+  border: 1px solid threedshadow;
>+  border-radius: 3px;

Is this radius rendered reasonably when the window manager isn't compositing?

(In reply to Jared Wein [:jaws] from comment #9)
> I was able to remove the minHeight setting on the alertTextBox, but the
> minHeight is needed on the alertImageBox to get the window sized correctly
> by sizeToContent().

So how exactly do things look without the min-height set? I'm trying to understand the layout here. Eventually this should also be documented in alert.js, if it turns out to be really needed.

Comment 11

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

(In reply to Dão Gottwald [:dao] from comment #10)
> Comment on attachment 668188 [details] [diff] [review]
> Patch v4
> 
> >--- a/toolkit/themes/gnomestripe/global/alerts/alert.css
> >+++ b/toolkit/themes/gnomestripe/global/alerts/alert.css
> 
> > .alertBox {
> >-  border: 1px solid ThreeDShadow;
> >+  border: 1px solid threedshadow;
> >+  border-radius: 3px;
> 
> Is this radius rendered reasonably when the window manager isn't compositing?

This border-radius should probably be removed for gnomestripe.
 
> (In reply to Jared Wein [:jaws] from comment #9)
> > I was able to remove the minHeight setting on the alertTextBox, but the
> > minHeight is needed on the alertImageBox to get the window sized correctly
> > by sizeToContent().
> 
> So how exactly do things look without the min-height set? I'm trying to
> understand the layout here. Eventually this should also be documented in
> alert.js, if it turns out to be really needed.

This is what it looks like without the minHeight on alertImageBox: http://screencast.com/t/H0vCyAG89. Note that the last line is clipped. The height of the window seems to come from the height of the alertImageBox, I'm guessing this has to do with alertImageBox being an hbox, whereas alertTextBox is a vbox.

Comment 12

[William Chen [:wchen]]

Comment on attachment 668188 [details] [diff] [review]
Patch v4

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

::: toolkit/components/alerts/resources/content/alert.js
@@ -57,5 @@
> -  // Make sure that the contents are fixed at the window edge facing the
> -  // screen's center so that the window looks like "sliding in" and not
> -  // like "unfolding". The default packing of "start" only works for
> -  // vertical-bottom and horizontal-right positions, so we change it here.
> -  if (gOrigin & NS_ALERT_HORIZONTAL) {

We should also get rid of the definition for NS_ALERT_HORIZONTAL in widget/LookAndFeel.h because we only use the value for these XUL alerts.

Comment 13

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

William, I can remove it in this bug or a follow-up, but it will make the NS_ALERT_LEFT and NS_ALERT_TOP enum values awkward since they will be missing the |1| bit.

Dao, is there anything else that I should change? I can make the requested changes to the patch before landing.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 668188 [details] [diff] [review]
Patch v4

(In reply to Jared Wein [:jaws] from comment #11)
> This border-radius should probably be removed for gnomestripe.

Ok, please do so.

> This is what it looks like without the minHeight on alertImageBox:
> http://screencast.com/t/H0vCyAG89. Note that the last line is clipped. The
> height of the window seems to come from the height of the alertImageBox, I'm
> guessing this has to do with alertImageBox being an hbox, whereas
> alertTextBox is a vbox.

I don't see why the hbox/vbox setup would cause this. Anyway, I guess this is good enough for now and can be investigated further separately.

Comment 15

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 668188 [details] [diff] [review]
Patch v4

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5551c001a21

[Approval Request Comment]
Bug caused by (feature/regressing bug #): need to update alerts for social api
User impact if declined: less than pretty notifications for social api
Testing completed (on m-c, etc.): tested locally, and just landed on m-i
Risk to taking this patch (and alternatives if risky): none expected
String or UUID changes made by this patch: none

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a5551c001a21

Comment 17

[Alex Keybl [:akeybl]]

Comment on attachment 668188 [details] [diff] [review]
Patch v4

[Triage Comment]
Please land early Monday to make the next merge.

Comment 18

[Justin Wood (:Callek)]

applying 796111.patch
patching file toolkit/components/alerts/resources/content/alert.js
Hunk #1 FAILED at 36
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/alerts/resources/content/alert.js.rej
patching file toolkit/components/alerts/resources/content/alert.xul
Hunk #1 FAILED at 13
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/alerts/resources/content/alert.xul.rej
patching file toolkit/themes/winstripe/global/alerts/alert.css
Hunk #1 FAILED at 8
1 out of 1 hunks FAILED -- saving rejects to file toolkit/themes/winstripe/global/alerts/alert.css.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 796111.patch

not yet unwrapping .rej's

Comment 19

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

(In reply to Justin Wood (:Callek) from comment #18)
> errors during apply, please fix and refresh 796111.patch

This patch is dependent on bug 786125 which is awaiting aurora-approval.

Comment 20

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

Comment on attachment 668188 [details] [diff] [review]
Patch v4

Flagging Dave for superreview to get his OK on uplifting this and bug 786125 to mozilla-beta (fx17).

Comment 21

[Alex Keybl [:akeybl]]

Comment on attachment 668188 [details] [diff] [review]
Patch v4

[Triage Comment]
jaws and I discussed - worst case scenario here is that we regress alerts for a single beta. Let's land early to give time for feedback.

Comment 22

[Justin Dolske [:Dolske]]

Comment on attachment 668188 [details] [diff] [review]
Patch v4

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

Stealing sr, as this is trying to land in time for the first beta. This seems pretty unlikely to have addon compat issues, so let's get it in.

Comment 23

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

Landed on beta-17:
https://hg.mozilla.org/releases/mozilla-beta/rev/defe56708537

Comment 24

[u279076]

Is there something QA can do to verify this fix, apart from a quick visual inspection of notifications?

Comment 25

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

A quick visual inspection of notifications is sufficient.

Comment 26

[Paul Silaghi, QA [:pauly]]

I've tested this with Notification Tester Addon https://bugzilla.mozilla.org/attachment.cgi?id=672897

The notifications look ok on Win and Linux. The only problem is that I don't see any notifications on Mac - bug 810293

Comment 27

[u279076]

I think it's safe enough to call this verified since Windows and Linux covers the majority of our user base. I wouldn't wait for bug 810293 to be fixed to mark this bug verified. We can always reopen if something is found to have regressed.