Closed Bug 831737 Opened 7 years ago Closed 7 years ago

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

Categories

(Thunderbird :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 21.0

People

(Reporter: philip.chee, Assigned: philip.chee)

References

Details

Attachments

(3 files, 1 obsolete file)

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)
Alternatively, move the (suite fork of) newmailalert.* to mailnews/base/content and package it appropriately.
Attached patch Patch v1.0 Proposed fix. (obsolete) — Splinter Review
> 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.
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Attachment #704562 - Flags: ui-review?(richard.marti)
Attachment #704562 - Flags: review?(mconley)
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?
Attachment #704562 - Flags: feedback?(bwinton)
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.
Attachment #704562 - Flags: ui-review?(richard.marti) → ui-review-
Attached image 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.
I forgot to say, on Linux (Mint) I haven't seen the alert. The system wide notification was always shown.
> 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.
Attached image 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.
> 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.
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/
Depends on: 436089
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.
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);
Attachment #704562 - Attachment is obsolete: true
Attachment #704562 - Flags: review?(mconley)
Attachment #704562 - Flags: feedback?(bwinton)
Attachment #707568 - Flags: ui-review?(richard.marti)
Attachment #707568 - Flags: review?(mconley)
Comment on attachment 707568 [details] [diff] [review]
Patch v1.1 fix ui-review issues.

This looks good now. Thanks.
Attachment #707568 - Flags: ui-review?(richard.marti) → ui-review+
Comment on attachment 707568 [details] [diff] [review]
Patch v1.1 fix ui-review issues.

I'll take whoever gets to review this first.
Attachment #707568 - Flags: review?(mkmelin+mozilla)
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 '
Attachment #707568 - Flags: review?(mkmelin+mozilla) → review+
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!
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 21.0
What about mconley's pending review request? :)
Blocks: 436089
No longer depends on: 436089
(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 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.
Attachment #707568 - Flags: review?(mconley)
Backed out.
http://hg.mozilla.org/comm-central/rev/0587280f759e
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
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
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
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.
> 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
(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"
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.
You need to log in before you can comment on or make changes to this bug.