1547202 - Blank tray pop-ups of new message notifications for non-Inbox folders

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Marco A.G.Pinto [:marcoagpinto]]

The add-ons are broken (Enigmail+ImportExportTool) and when it checks for messages at start, the pop-up tray icon window is blank.

I believe the tray icon window issue happens since beta 66 or 65.

Comment 1

[Jorg K (CEST = GMT+2)]

I think this should be: When it checks for messages at start, the pop-up tray icon window is blank.
Some add-ons may be broken in TB 67 beta, that's expected.

Comment 2

[Jorg K (CEST = GMT+2)]

Hello Alice, could you try finding the regression for us. This one is not so easy. The problem is that the new e-mail notification in the Windows systray (sometimes?) shows up blank.

So I guess the STR could be:
Send yourself an e-mail, quit TB before it gets retrieved.
Start (another version of) TB and watch the new mail notification.

I've noticed blank new mail notification windows when starting various local build, and I've seen it for a while, I'd say at least since the start of 2019.

Comment 3

[Alice0775 White]

Attachment: image.png

I cannot reproduce the issue on latest Daily with new profile.
The notification popup seems to be as expected (see screenshot).

Build ID 20190425094146
User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Thunderbird/68.0a1

Comment 4

[Alice0775 White]

(In reply to Alice0775 White from comment #3)

Created attachment 9060963 [details]
image.png

I cannot reproduce the issue on latest Daily with new profile.
The notification popup seems to be as expected (see screenshot).

Build ID 20190425094146
User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0)
Gecko/20100101 Thunderbird/68.0a1

Please ignore, Maybe I misunderstood the str.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 2019-04-26 15_33_28-.png

Well, your screenshot shows a non-empty notification, but I've just received and empty one. I'm still not sure whether that was a SPAM message and the emptiness is expected.

Comment 6

[Alice0775 White]

I do not know exact STR and expected reaults.

STR1:

1. Send a message from another application
2. Start Thunderbird and select [unrelated folder] (i.e, other than inbox)
3. Wait for a while

Actual Results and Expected Results:
Sound played. Tray Icon appears. Notification popup appears with out any problem

STR2:

1. Send a message from another application
2. Start Thunderbird and select [inbox folder]
3. Wait for a while

Actual Results:
Good build : Sound played. Tray Icon appears. But no notification popup appears(52esr) or the popup appears and disappears quickly(60esr).
Bad Build: Sound played. Tray Icon appears. Notification popup appears, but subject is blank.

Expected Results:

Regression window
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=d4e98be4fc0b4bbc5f65149e874048b600daf21e&tochange=bd1dfa52d616bce5e6024b8a6c2524f0658e6205
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cfdbecb2e2305dba0ccac931aa91423135039d2f&tochange=5fb87d2312ea4881026ecaac6488fbdc2e4a7725

Comment 7

[Jorg K (CEST = GMT+2)]

Thank you so much!!

Comment 8

[WaltS48 [:walts48]]

On Linux I set "mail.biff.use_system_alert;false" because I wanted to be able to click an email in the notification and have it open in Thunderbird if I was using another application like Firefox or SMPlayer.

Using Thunderbird 67.0b1 and 68.0a1 on Linux I mainly see an empty notification when my Gmail IMAP account gets new mail from Bugzilla. Messages to the Inbox appear in the notification.

Comment 9

[Jorg K (CEST = GMT+2)]

Yes, the notifications for new messages in the inbox seem to work, but not for other folders if new messages appear there.

Comment 10

[WaltS48 [:walts48]]

I updated to 67.0b2 on Linux Mint 19 and now I hear my alert sound but don't see any notification at all for mail received in my Bugzilla folder.

I've tested with "mail.biff.use_system_alert;false" and set to true to use native notifications on Linux.

Comment 11

[Magnus Melin [:mkmelin]]

Attachment: bug1547202_blank_new.patch

Some improvements and bug fix - not sure it fixes this particular case.

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 9068000 [details] [diff] [review]
bug1547202_blank_new.patch

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

::: mail/base/content/foldersummary.js
@@ +153,1 @@
>              decodeURIComponent(escape(msgHdr.getStringProperty("preview")));

Would it make sense not to evaluate `msgHdr.getProperty("preview")` twice. So do:
let preview;
if (this.showPreview && (preview = msgHdr.getProperty("preview"))) {

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 9068000 [details] [diff] [review]
bug1547202_blank_new.patch

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

At least the last hunk I'm commenting on here should make a change. I'll do a beta build and start using it, so I'll see whether the issue goes away:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=17709f49c6cbd8d7102b132195564312febf1bb0

::: mail/base/content/foldersummary.js
@@ +110,5 @@
>  
>        foundNewMsg = true;
> +      if (this.children.length >= this.maxMsgHdrsInPopup) {
> +        return foundNewMsg;
> +      }

Does that fix the issue of huge notifications? I though seeing all summaries/previews was quite useful ;-)

@@ +211,5 @@
> +      let node = event.target;
> +      while (node.hasChildNodes()) {
> +        node.lastChild.remove();
> +      }
> +    });

What's this change for?

::: mailnews/base/content/newmailalert.js
@@ +51,5 @@
>    // This is really the root folder and we have to walk through the list to
>    // find the real folder that has new mail in it...:(
>    let allFolders = rootFolder.descendants;
>    var folderSummaryInfoEl = document.getElementById("folderSummaryInfo");
> +  folderSummaryInfoEl.maxMsgHdrsInPopup = gNumNewMsgsToShowInAlert;

Wow, that typo/copy&paste error is pretty fatal since we have:
`for (let i = 0; i < this.maxMsgHdrsInPopup && i < numMsgKeys.value; i++)`.

Comment 14

[Jorg K (CEST = GMT+2)]

That was busted, try again:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4fd33037db7ca87daed06fcd730b233b5c057a6e

Comment 15

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #13)

Does that fix the issue of huge notifications? I though seeing all
summaries/previews was quite useful ;-)

It does yes. Maybe we should indeed increase the number we're showing.

What's this change for?

The event handlers should have been part of the object. It worked earlier, but you'd have to always remember to set the event handler inline in the markup, or break yourself. So better have the object handle them automatically.

Wow, that typo/copy&paste error is pretty fatal since we have:
for (let i = 0; i < this.maxMsgHdrsInPopup && i < numMsgKeys.value; i++).

Yep, I think the changes here are correct and needed, but not necessarily fixing the issue at hand. Maybe we should still get it landed. It shouldn't hurt anything.

Comment 16

[Magnus Melin [:mkmelin]]

Attachment: bug1547202_blank_new.patch

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 9068281 [details] [diff] [review]
bug1547202_blank_new.patch

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

I'm running with a beta with this patch. Let's see whether it helps. Given that we're busted, there's no rush. Meanwhile ...

::: mail/base/content/foldersummary.js
@@ +108,5 @@
>          return false;
>        }
>  
>        foundNewMsg = true;
> +      if (this.children.length >= this.maxMsgHdrsInPopup) {

... can you check where this is set and maybe increase it to 5 or so.

Comment 18

[Jorg K (CEST = GMT+2)]

I'm running this in the beta now and I when the notifications used to be blank before, they no longer are. So I believe this is fixed.

Comment 19

[Magnus Melin [:mkmelin]]

Attachment: bug1547202_blank_new.patch

Made it 6 msg that we show.

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 9068291 [details] [diff] [review]
bug1547202_blank_new.patch

OK. However, I cheered too early, I've just received a blank notification again :-(

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ca6c4600f029
fixes for message numbers in newmailalert.js and foldersummary.js. r=jorgk

Comment 22

[Jorg K (CEST = GMT+2)]

Coming to think of it, the alert might have never worked correctly. I think in TB 60 we got notifications like: "XXX received 2 new messages", but the preview of only one message was shown, so the notification only ever had one item and thus a fixed size. De-XBL made it work better and the notification is now bigger according to the number of messages and shows multiple previews.

Comment 23

[Jorg K (CEST = GMT+2)]

I have a reliable way to reproduce this: Pop account, move bugmail to a bugmail folder with a filter, comment on bug 1343157 (Invalid bugs).

Mail arrives, filter moves it, notification is blank.

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: debug-patch.patch

I added some debugging: parseFolder() isn't even called. You can convince yourself that it's working properly by hovering the folder and looking at the tooltip which is produced here:

mail/base/content/foldersummary.js
233 if (tooltipnode.parseFolder(msgFolder, null, asyncResults)) {

With the debug I see:
=== prefillAlertInfo: before let folder of fixIterator
=== prefillAlertInfo: looking at 0 AAA Chat
=== prefillAlertInfo: looking at 0 From Inbox
=== prefillAlertInfo: looking at 00 Raspberry Pi
=== prefillAlertInfo: looking at Good ones
=== prefillAlertInfo: looking at 0000 AAA SMIME
[snip]
followed by a long list of folders that belong to this account I'm checking. The folder where the message was moved do, a local folder which is part of the account wasn't checked.

As I said, I doubt that it ever worked. Maybe in the past, some stale old "new" message was notified (EDIT: or messages which arrived at the same time but weren't moved by a filter) so the notification wasn't empty, but I don't think the message from the filter target was notified. Or perhaps it's a timing issue. But for POP3 I think the e-mail is stored directly in the target folder without ever hitting the inbox at least if you run it before junk classification.

Comment 25

[Jorg K (CEST = GMT+2)]

Marco, you are the reporter here. What is your setup that produces the blank messages? Do you have a filter that moves new messages into a (local) folder which is not part of the account?

Comment 26

[Richard Marti (:Paenglab)]

I see this sometimes too:
I have a POP- and a IMAP account. Normally I am in the IMAP account and when a new mail arrives in the POP account the notification is shown. Then I switch to the POP account and the notification arrives a second time. And this second time it is sometime blank. This is my working profile many years old.

Comment 27

[Jorg K (CEST = GMT+2)]

Richard: Any filters involved? Only sometimes blank?

I've just tried this in TB 60. Message is moved and no notification is displayed.

Alice: Can you try this again with these STR:
On your account, IMAP or POP, create a filter to move messages where From contains [e-mail] to a local folder. Then comment on bug 1343157 to create a bugmail. I think you don't need to wait, you can just "Get Messages".

In TB 60, I don't see any notification, hence also no blank one. In TB 68 or trunk, I get the empty notification. Can you please check where that behaviour changed.

Comment 28

[Jorg K (CEST = GMT+2)]

Richard: We really need a reproducible case. Can you make your observation reproducible? I found a reproducible case, but there might be others.

Comment 29

[Richard Marti (:Paenglab)]

No filter involved, still on the inbox. But different to the screenshot here where is no title, no subject. Only the notification with the icon on the left. This may be a profile issue as it looks different to the screenshot here. And I have this already since a long time (before TB 60).

Comment 30

[Alice0775 White]

(In reply to Jorg K (GMT+2) from comment #27)

Richard: Any filters involved? Only sometimes blank?

I've just tried this in TB 60. Message is moved and no notification is
displayed.

Alice: Can you try this again with these STR:
On your account, IMAP or POP, create a filter to move messages where From
contains [e-mail] to a local folder. Then comment on
bug 1343157 to create a bugmail. I think you don't need to wait, you can
just "Get Messages".

In TB 60, I don't see any notification, hence also no blank one. In TB 68 or
trunk, I get the empty notification. Can you please check where that
behaviour changed.

TB 52 : no notification sound, no notification popup
TB 60 : no notification sound, no notification popup
TB 69.0a1: no notification sound, no notification popup

Comment 31

[Jorg K (CEST = GMT+2)]

Attachment: 1547202-fix-empty-notification.patch

Clearly the de-XBL guys broke this :-(

Comment 32

[:aceman]

Comment on attachment 9069182 [details] [diff] [review]
1547202-fix-empty-notification.patch

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

::: mailnews/base/content/newmailalert.js
@@ +109,5 @@
>  
>  // If the user initiated the alert, show it right away, otherwise start opening the alert with
>  // the fade effect.
>  function showAlert() {
> +  if (!document.getElementById("folderSummaryInfo").hasMessages()) {

Maybe it would be better to make 'hasMessages' a getter as it was previously (see https://searchfox.org/comm-central/source/suite/mailnews/content/mailWidgets.xml#1860) instead of a function, when it just returns a bool.

Comment 33

[Magnus Melin [:mkmelin]]

Comment on attachment 9069182 [details] [diff] [review]
1547202-fix-empty-notification.patch

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

Great! 
I'd do it like in the patch. hasMessages isn't really a property, and having it be a function matches with e.g. hasChildNodes()

Comment 34

[Jorg K (CEST = GMT+2)]

I don't care as long as the annoying empty notifications stop. Interesting by-product: Now we know that messages moved "away" by a filter won't be notified by design.

Comment 35

[Marco A.G.Pinto [:marcoagpinto]]

(In reply to Jorg K (GMT+2) from comment #25)

Marco, you are the reporter here. What is your setup that produces the blank messages? Do you have a filter that moves new messages into a (local) folder which is not part of the account?

@Jörg

This happens in the 14'' laptop where I have beta + daily and I believe I have the old Telepac account set there which no longer exists and also the test address of Wayne, so, the message it detects as new are in Wayne's test address when I run the daily or beta... but it has been weeks (?) since I last ran the beta/daily... I can't remember.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/28e2abca5938
fix empty notification by retrieving .hasMessages() properly. r=mkmelin

Comment 37

[Jorg K (CEST = GMT+2)]

TB 68 beta:
https://hg.mozilla.org/releases/comm-beta/rev/7ceba6091b884e3ab318165d8ed89ead6499e22e
https://hg.mozilla.org/releases/comm-beta/rev/81e4c33847be70dbd83926bd263a528a0d91273a