The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Thunderbird 21.0

Status

Thunderbird
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Philip Chee, Assigned: Philip Chee)

Tracking

Trunk
Thunderbird 21.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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

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

Comment 2

4 years ago
Created attachment 704562 [details] [diff] [review]
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.
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Attachment #704562 - Flags: ui-review?(richard.marti)
Attachment #704562 - Flags: review?(mconley)
(Assignee)

Comment 3

4 years ago
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-
Created attachment 704667 [details]
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.
(Assignee)

Comment 7

4 years ago
> 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.
Created attachment 704925 [details]
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.
(Assignee)

Comment 9

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

Comment 10

4 years ago
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

Comment 11

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

Comment 12

4 years ago
Created attachment 707568 [details] [diff] [review]
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);
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+
(Assignee)

Comment 14

4 years ago
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 15

4 years ago
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+
(Assignee)

Comment 16

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 21.0

Comment 17

4 years ago
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 19

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

Comment 20

4 years ago
Backed out.
http://hg.mozilla.org/comm-central/rev/0587280f759e
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 21

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

Comment 22

4 years ago
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
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED

Comment 23

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

Comment 24

4 years ago
> 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

4 years ago
(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"
(Assignee)

Comment 26

4 years ago
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.