1222491 - Update Modern theme for recent changes in web notifications

Status: RESOLVED FIXED

(SeaMonkey :: Themes, defect)

Description

[[e-mail]]

The layout of the alert was rearranged and some additional controls were added.

I think the alert changes landed in Gecko 44 but feel free to correct me on that.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Comment 2

[Philip Chee]

Comment on attachment 8684269 [details] [diff] [review]
Proposed patch

nb: I used the following demo to test:
https://developer.cdn.mozilla.net/media/uploads/demos/e/l/elfoxero/c17223c414d8ddafb7808972b5617d9e/html5-notifications_1400214081_demo_package/index.html

>  .alertBox {
>    border: 1px solid #7B969C;
>    border-radius: 3px;
>    background-color: #C7D0D9;
>  }
The window already has a border (border: ridge #5486DA 4px).
Having another 1px box with a radius just inside it looks weird.

>  .alertTitle {
>    font-weight: bold;
>    font-size: 110%;
> +  padding: 8px;
I think 8px looks excessive. Perhaps 6px or 4px?

Ulgy UX due to Missing CSS e.g.:

#alertSettings > .button-box > .button-menu-dropmarker,
#alertSettings > .button-box > .box-inherit > .button-text {
  display: none;
} 

#alertSettings {
  -moz-appearance: none;
  background-color: transparent;
  border-width: 0px;
--------------------------------------------------------------------

(Maybe scale this image smaller or find a better "gear" image? Perhaps in a followup bug?)
#alertSettings > .button-box > .box-inherit > .button-icon {
  height: 16px;
  width: 16px;
} 

Also note the following patches not ported yet:

Bug 1218780 - Notifications shouldn't use a hand cursor

-#alertNotification[clickable="true"] {
-  cursor: pointer;
-}
-
-label {
-  cursor: inherit;
-}
-

Bug 1218781 - Padding on the right side of notifications is missing when an icon is supplied

+#alertTextLabel {
+  padding-inline-end: 8px;
+}

Comment 3

[[e-mail]]

(In reply to Philip Chee from comment #2)
> (From update of attachment 8684269 [details] [diff] [review])
> >  .alertBox {
> >    border: 1px solid #7B969C;
> >    border-radius: 3px;
> >    background-color: #C7D0D9;
> >  }
> The window already has a border (border: ridge #5486DA 4px).
Added by you in bug 870413 porting the CSS added by you in bug 404580.
> Having another 1px box with a radius just inside it looks weird.
Added by you in bug 818017... not that I can talk, it all has r=me.

> Also note the following patches not ported yet:
> 
> Bug 1218780 - Notifications shouldn't use a hand cursor
Not convinced.

> Bug 1218781 - Padding on the right side of notifications is missing when an
> icon is supplied
Doesn't appear to apply, padding is set on the .alertTextBox instead.

Comment 4

[[e-mail]]

Attachment: Updated for review comments

Comment 5

[[e-mail]]

Pushed comm-central changeset 2671737ba397.

Comment 6

[[e-mail]]

Comment on attachment 8690243 [details] [diff] [review]
Updated for review comments

[Approval Request Comment]
Regression caused by (bug #): Various
User impact if declined: Alerts don't work properly in Modern
Testing completed (on m-c, etc.): Landed on c-c before uplift
Risk to taking this patch (and alternatives if risky): Low
String changes made by this patch: None

Comment 7

[[e-mail]]

Attachment: Fix for long titles

If the alert title is longer than the message then this can cause the settings icon to get misplaced.

Comment 8

[[e-mail]]

Pushed comm-central changeset 911f59a17fb7.
Pushed comm-aurora changeset 8056593ec381.
Pushed comm-beta changset 5f51c3ea23b2 (both patches folded).

Comment 9

[Tony Mechelynck [:tonymec]]

(In reply to [e-mail] from comment #8)
> Pushed comm-central changeset 911f59a17fb7.
> Pushed comm-aurora changeset 8056593ec381.
> Pushed comm-beta changset 5f51c3ea23b2 (both patches folded).

To make it clickable: comm-beta changeset 5f51c3ea23b2

Comment 10

[Tony Mechelynck [:tonymec]]

...and two of them aren't correctly resolved (central is OK)
https://hg.mozilla.org/releases/comm-aurora/rev/8056593ec381
https://hg.mozilla.org/releases/comm-beta/rev/5f51c3ea23b2