1227711 - Add a box-shadow to the XUL alerts

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Comment 1

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

Per the spec at https://mozilla.invisionapp.com/share/3A4A895XZ#/screens/104465677 we should have a 30px box-shadow on the notifications.

Unfortunately, I'm having trouble getting mouse events to pass through the box-shadow, and the shadows are overlapping other notifications, meaning that it blocks access to older notifications. As a result, I've set the shadow to have a 10px blur radius.

Comment 2

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

Attachment: Patch

Comment 3

[Matthew N. [:MattN]]

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

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

::: toolkit/components/alerts/resources/content/alert.js
@@ +9,5 @@
>  const NS_ALERT_LEFT = 2;
>  const NS_ALERT_TOP = 4;
>  
> +const WINDOW_MARGIN = 0;
> +const WINDOW_SHADOW_SPREAD = 10;

Should WINDOW_SHADOW_SPREAD be 10 on non-Windows too?

::: toolkit/themes/windows/global/alerts/alert.css
@@ +20,5 @@
>    border: 1px solid ThreeDShadow;
>    border-radius: 1px;
>    background-color: -moz-Dialog;
>    color: -moz-DialogText;
> +  box-shadow: 0 2px 10px rgba(0,0,0,0.59);

I don't really want to churn on this if we're going to uplift it so I think we should get ui-review on 10px first.

Comment 4

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

Attachment: Screenshot of patch

Stephen, can you give ui-r on the adjusted box-shadow here? (The alignment of the close button is handled by bug 1227717).

Comment 5

[Stephen Horlander [:shorlander]]

Comment on attachment 8691595 [details]
Screenshot of patch

Looks good to me. Thanks!

Comment 6

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

Attachment: Patch v2

Thanks for the ui-review shorlander. The patch now uses AppCosntants to only apply the shadow adjustments on Windows, and keeps the window margin on non-Windows.

I also fixed the positioning of the notification to have the correct offset from the corner of the screen as pointed out in today's status meeting. This places the first notification 10 pixels higher than what is shown in the already attached screenshot.

Comment 7

[Matthew N. [:MattN]]

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

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

Btw. notifications seems a little tight together (external vertical spacing) on OS X (not changed by this patch). I've asked phlsa to spec the spacing between notifications.

::: toolkit/themes/windows/global/alerts/alert.css
@@ +20,5 @@
>    border: 1px solid ThreeDShadow;
>    border-radius: 1px;
>    background-color: -moz-Dialog;
>    color: -moz-DialogText;
> +  box-shadow: 0 2px 10px rgba(0,0,0,0.59);

This is good in the Windows stylesheet since we get the shadow for free on OS X already and Linux doesn't have transparency yet to support this.

Comment 8

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

https://hg.mozilla.org/integration/fx-team/rev/10a9cf7c54fadd9073807e863725ee1b32449ba6
Bug 1227711 - Add a box-shadow to the XUL alerts. ui-r=shorlander r=MattN

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/10a9cf7c54fa