Update Modern theme for recent changes in web notifications

RESOLVED FIXED in seamonkey2.42

Status

SeaMonkey
Themes
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

unspecified
seamonkey2.42

SeaMonkey Tracking Flags

(seamonkey2.41 fixed, seamonkey2.42 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

2 years ago
Created attachment 8684269 [details] [diff] [review]
Proposed patch
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #8684269 - Flags: review?(philip.chee)

Comment 2

2 years ago
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;
+}
Attachment #8684269 - Flags: review?(philip.chee) → review-
(Assignee)

Comment 3

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

Comment 4

2 years ago
Created attachment 8690243 [details] [diff] [review]
Updated for review comments
Attachment #8684269 - Attachment is obsolete: true
Attachment #8690243 - Flags: review?(philip.chee)

Updated

2 years ago
Attachment #8690243 - Flags: review?(philip.chee) → review+
(Assignee)

Comment 5

2 years ago
Pushed comm-central changeset 2671737ba397.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-seamonkey2.42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.42
(Assignee)

Comment 6

2 years ago
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
Attachment #8690243 - Flags: approval-comm-beta?

Updated

2 years ago
Attachment #8690243 - Flags: approval-comm-beta? → approval-comm-beta+
(Assignee)

Comment 7

2 years ago
Created attachment 8702021 [details] [diff] [review]
Fix for long titles

If the alert title is longer than the message then this can cause the settings icon to get misplaced.
Attachment #8702021 - Flags: review?(philip.chee)
Attachment #8702021 - Flags: approval-comm-beta?
Attachment #8702021 - Flags: approval-comm-aurora?

Updated

2 years ago
Attachment #8702021 - Flags: review?(philip.chee)
Attachment #8702021 - Flags: review+
Attachment #8702021 - Flags: approval-comm-beta?
Attachment #8702021 - Flags: approval-comm-beta+
Attachment #8702021 - Flags: approval-comm-aurora?
Attachment #8702021 - Flags: approval-comm-aurora+
(Assignee)

Comment 8

2 years ago
Pushed comm-central changeset 911f59a17fb7.
Pushed comm-aurora changeset 8056593ec381.
Pushed comm-beta changset 5f51c3ea23b2 (both patches folded).
status-seamonkey2.41: affected → fixed
(In reply to neil@parkwaycc.co.uk 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
...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
You need to log in before you can comment on or make changes to this bug.