831737 - Re-write and simplify newmailalert.{js|xul|css} (Backport changes from SeaMonkey Bug 404580)

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Philip Chee]

From SeaMonkey Bug 404580:

> Currently this is a direct copy of Thunderbird's XUL newmailalert which was
> based on a older implementation of the tookit alert.xul. The fade-in/
> fade-out animation should be simplified using css3 animations as in the
> current toolkit alert.xul.

> 2. Re-write and simplify newmailalert.{js|xul|css}
> 2.1 Rebase the code based on the latest toolkit alert.
> 2.2 Use CSS3 animation to simplify the code.
> 2.3 Position the close button using the new stack right and top attributes.

Also see toolkit alert Bug 786125 - Alert showing/hiding animation is janky (nsIAlertsService)

Comment 1

[[e-mail]]

Alternatively, move the (suite fork of) newmailalert.* to mailnews/base/content and package it appropriately.

Comment 2

[Philip Chee]

Attachment: Patch v1.0 Proposed fix.

> Alternatively, move the (suite fork of) newmailalert.* to mailnews/base/content
> and package it appropriately.

1. Unfork newmailalert.{js|xul} and move to mailnews/

2. Remove ancient workarounds for bugs that probably don't exist any more.

> -/*
> - * See bug 332160. Apply these rules on #alertContainer instead of on
> - * #newMailAlertNotification like on windows.
> - */
> -#alertContainer {
> +#newMailAlertNotification {
I assume that the GTK+ widget bug should have been fixed by now.

> -  background-color: -moz-Dialog;
> -  color: -moz-DialogText;
> -}
Already inherited from global.css so is redundant.

> -#closeButton > .toolbarbutton-icon {
> -  -moz-margin-end: 0px; /* override toolkit's default value */
> -}
> -
Fixed in Tookit Bug 479439 - toolbarbutton-icon shouldn't have a margin if the toolbarbutton doesn't have a label.


> diff --git a/suite/mailnews/newmailalert.css b/mailnews/base/content/newmailalert.css
> copy from suite/mailnews/newmailalert.css
> copy to mailnews/base/content/newmailalert.css
> diff --git a/suite/mailnews/newmailalert.js b/mailnews/base/content/newmailalert.js
> copy from suite/mailnews/newmailalert.js
> copy to mailnews/base/content/newmailalert.js
> diff --git a/suite/mailnews/newmailalert.xul b/mailnews/base/content/newmailalert.xul
> copy from suite/mailnews/newmailalert.xul
> copy to mailnews/base/content/newmailalert.xul
> diff --git a/mail/base/content/newmailalert.js b/mail/base/content/newmailalert.js

Summary:

Re-write and simplify newmailalert.{js|xul|css}:
1. Rebase the code based on the latest toolkit alert.
2. Use CSS3 animation to simplify the code.
3. Position the close button using the new stack right and top attributes.

For a diff of what I've changed in these files see: SM Bug 404580 and:
http://hg.mozilla.org/comm-central/rev/3b6458d65543#l9.1

I can't test Gnomestripe so asking TB UX people (paenglab, bwinton) for ui-review.

Comment 3

[Philip Chee]

Comment on attachment 704562 [details] [diff] [review]
Patch v1.0 Proposed fix.

> I can't test Gnomestripe so asking TB UX people (paenglab, bwinton) for
> ui-review.
For some reason I can't set more than one ui-review? so adding bwinton to feedback?

Comment 4

[Richard Marti (:Paenglab)]

Comment on attachment 704562 [details] [diff] [review]
Patch v1.0 Proposed fix.

I like the new alert appears a little bit away from screen edge and not like the actual fully on the edge.

ui-r- because the opacity animation for the appearance is to slow. A half second after the alert is fully visible it vanishes immediately. A faster animation and longer open time (actual alerts.totalOpenTime:3000) would be better. 

Also more space on the left and right of the close button is needed to look better. I attach a screenshot about this.

Comment 5

[Richard Marti (:Paenglab)]

Attachment: comparison

On top the new alert and on bottom the old.

The close button has now 1px space on the right to the border. 3px like the top gap would be better. On the left the button is to close to the text. Maybe when you set set the #alertTextBox's -moz-padding-end from 16px to 20px it would look better.

Comment 6

[Richard Marti (:Paenglab)]

I forgot to say, on Linux (Mint) I haven't seen the alert. The system wide notification was always shown.

Comment 7

[Philip Chee]

> I like the new alert appears a little bit away from screen edge and not like the
> actual fully on the edge.
Originally mscott spent a lot of time getting the alert flush with the edge of the screen as possible. But I'm now following the latest toolkit alert.xul which moves the alert a little bit away from the screen. And besides mscott isn't around any more to complain ;)

> I forgot to say, on Linux (Mint) I haven't seen the alert. The system wide
> notification was always shown.
Reading through the unix code the xul alert is only shown if libnotify isn't installed in your Linux.

Comment 8

[Richard Marti (:Paenglab)]

Attachment: Alert under Linux Mint

(In reply to Philip Chee from comment #7)
> Reading through the unix code the xul alert is only shown if libnotify isn't
> installed in your Linux.

Removed libnotify and the alert appears :).

The alert doesn't show the border because of the global.css rule -moz-appearance: window;. Changing #newMailAlertNotification to -moz-appearance: none; shows the borders.

Again good visible is the close button too much at the edge.

Comment 9

[Philip Chee]

> The alert doesn't show the border because of the global.css rule -moz-appearance:
> window;. Changing #newMailAlertNotification to -moz-appearance: none; shows the
> borders.
Thanks for testing!

> Again good visible is the close button too much at the edge.
All fixed locally.

Comment 10

[Philip Chee]

Bug 436089 made some changes to the Thunderbird version of newmailalert.js which we are replacing. We will need to fix the SeaMonkey version (which this patch moves to shared /mailnews/

Comment 11

[:aceman]

Bug 436089 is not yet landed (and I don't know how long it will take for the final review). If you can land this one sooner I have no problem with it and will make a patch for the shared newmailalert.js.

Comment 12

[Philip Chee]

Attachment: Patch v1.1 fix ui-review issues.

Changes since the last patch:

1. Increase alerts.totalOpenTime to 10 seconds. Decrease fade in to 2s. Add a 2s fade out.
2. gnomestripe: Add -moz-appearance: none; to #newMailAlertNotification
to override the global.css rule { -moz-appearance: window; }.
3. Increase -moz-padding-end of #alertTextBox from 16px to 20px.
4. Increase the padding of the close button.
5. showAlert(): return early if there is no mail to show.
6. Move some common prefs to /mailnews/

> +pref("mail.biff.alert.show_preview", true);
> +pref("mail.biff.alert.show_subject", true);
> +pref("mail.biff.alert.show_sender",  true);
> +pref("mail.biff.alert.preview_length", 40);

Comment 13

[Richard Marti (:Paenglab)]

Comment on attachment 707568 [details] [diff] [review]
Patch v1.1 fix ui-review issues.

This looks good now. Thanks.

Comment 14

[Philip Chee]

Comment on attachment 707568 [details] [diff] [review]
Patch v1.1 fix ui-review issues.

I'll take whoever gets to review this first.

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 707568 [details] [diff] [review]
Patch v1.1 fix ui-review issues.

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

This looks good. r=mkmelin
(on linux you can test it by renaming libnotify.so)

::: suite/mailnews/newmailalert.js
@@ +120,3 @@
>  function showAlert()
>  {
> +  if (!document.getElementById('folderSummaryInfo').hasMessages) {

tiny nit: prefer " over '

Comment 16

[Philip Chee]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/a3767eefc685

> ::: suite/mailnews/newmailalert.js
> @@ +120,3 @@
>>  function showAlert()
>>  {
>> +  if (!document.getElementById('folderSummaryInfo').hasMessages) {
> 
> tiny nit: prefer " over '
Fixed on check-in. Thanks to all!

Comment 17

[:aceman]

What about mconley's pending review request? :)

Comment 18

[Mike Conley (:mconley) (:⚙️)]

(In reply to :aceman from comment #17)
> What about mconley's pending review request? :)

I'm on a work week, and unlikely to get to this until next week.

Comment 19

[:aceman]

Comment on attachment 707568 [details] [diff] [review]
Patch v1.1 fix ui-review issues.

The question was more about if the review is still needed when they already checked it in :) But according to comment 14, this was an alternative request and mkmelin got to it first.

Comment 20

[Philip Chee]

Backed out.
http://hg.mozilla.org/comm-central/rev/0587280f759e

Comment 21

[Philip Chee]

https://tbpl.mozilla.org/php/getParsedLog.php?id=19412283&tree=Thunderbird-Trunk#error0

TEST-START | /home/cltbld/talos-slave/test/build/mozmill/notification/test-notification.js | test_revert_to_newmailalert
Test Failure: Timeout waiting for window to close!
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/notification/test-notification.js | test-notification.js::test_revert_to_newmailalert
TEST-START | /home/cltbld/talos-slave/test/build/mozmill/notification/test-notification.js | setupTest
TEST-PASS | /home/cltbld/talos-slave/test/build/mozmill/notification/test-notification.js | test-notification.js::setupTest
TEST-START | /home/cltbld/talos-slave/test/build/mozmill/notification/test-notification.js | test_new_mail_received_causes_notification
Test Failure: Did not show alert notification.
--DOMWINDOW == 24 (0xc20df40) [serial = 27] [outer = (nil)] [url = chrome://messenger/content/msgAccountCentral.xul]
WARNING: NS_ENSURE_SUCCESS(rv, false) failed with result 0x8000FFFF: file ../../../../../mozilla/content/base/src/nsContentUtils.cpp, line 2985
WARNING: NS_ENSURE_TRUE(pusher.Push(aBoundElement)) failed: file ../../../../../mozilla/content/xbl/src/nsXBLProtoImplMethod.cpp, line 324
++DOMWINDOW == 25 (0xb660328) [serial = 31] [outer = 0xacf3210]
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/notification/test-notification.js | test-notification.js::test_new_mail_received_causes_notification

Comment 22

[Philip Chee]

Pushed patch again to comm-central:
http://hg.mozilla.org/comm-central/rev/919bcd851b6f
Push test fixes to comm-central:
http://hg.mozilla.org/comm-central/rev/5240c8bf5b11

Comment 23

[Joe Sabash [:JoeS1]]

It seems that maybe since this checkin although its still there in configedit
mail.biff.alert.preview_length
has no effect on the length of the newmail popup.
alerts.totalOpenTime does affect a change in persistence.

This means that folks that modified the original pref will have to fish around for the new pref.

Maybe we should remove the old pref altogether.
Or migrate that value to the new pref

I haven't regression tested this to pinpoint this bug.
Just got annoyed by the new longer display time.

Comment 24

[Philip Chee]

> mail.biff.alert.preview_length has no effect on the length of the newmail popup.
This only affects *nix.
Do you have libnotify installed? See Comment 8

> This means that folks that modified the original pref will have to fish around for
> the new pref.
I'm not sure what you mean. I didn't change the logic for mail.biff.alert.preview_length

Comment 25

[Joe Sabash [:JoeS1]]

(In reply to Philip Chee from comment #24)
> > mail.biff.alert.preview_length has no effect on the length of the newmail popup.
> This only affects *nix.
> Do you have libnotify installed? See Comment 8

No, I'm on windows 

> I'm not sure what you mean. I didn't change the logic for
> mail.biff.alert.preview_length

mail.biff.alert.preview_length worked on older TB/win2k (esr10)
I started playing with the prefs when I noticed  a much longer display time on trunk. I guess its the slide + fade overhead.
No matter, there are enough prefs there to adjust to taste.
Not very discoverable though to connect new mail notification to "alerts prefs"

Comment 26

[Philip Chee]

As far as I can tell, mail.biff.alert.preview_length only affects the length (number of characters) of the preview message text and not the duration of the alert.

Previously the default popup duration was set to 3000ms but was increased to 10s in this bug after some discussion see Comment 4 and Comment 12 above. Also the pref has always been "alerts.totalOpenTime" so the discoverability hasn't changed one bit.