1673958 - Defective implementation of the MozElements.NotificationBox

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect, P2)

Description

[Alessandro Castellani [:aleca]]

While working on another bug, i noticed a defective behaviour when triggering multiple notifications in the mail front-end.
The notifications don't stack on top of each other and are not ordered by priority, instead they are added one after another as the markup generates a new legacy-stack for every notification.

This is due to a mistake during the first de-xbl conversion, and that's totally my fault as I did that.
https://searchfox.org/comm-central/rev/aa6ec7ca8813e82b76168e76f6cd706ed226bdcd/mail/base/content/mail3PaneWindowCommands.js#32

Example:

  get notificationbox() {
    delete this.notificationbox;

    let newNotificationBox = new MozElements.NotificationBox(element => {
      element.setAttribute("flex", "1");
      element.setAttribute("notificationside", "bottom");
      document.getElementById("messenger-notification-footer").append(element);
    });

    return (this.notificationbox = newNotificationBox);
  }

First there's a risk of too much recursion since we're using the notificationbox getter and then deleting inside itself before generating a new NotificationBox element.
Also, since we're generating a new element at every call, we will never be able to properly stack notifications as we create a new container every time.

I think this was never reported, or even noticed, since I suspect we never show more than 1 notification at a time, therefore extremely rare to reproduce.

Comment 1

[Alessandro Castellani [:aleca]]

Attachment: 1673958-notifications.diff

Some much needed clean up and fixes.

Here's a try run to see how many things I broke: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=cda74cf2abe060d18dcf0bc68878d0264936b42d

Comment 2

[Magnus Melin [:mkmelin] (away, back June 30)]

Comment on attachment 9184651 [details] [diff] [review]
1673958-notifications.diff

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

LGTM, r=mkmelin
Unfortunate things are not more centralized, but it is what it is.

Comment 3

[Alessandro Castellani [:aleca]]

Unfortunate things are not more centralized, but it is what it is.

Indeed.
Do you think would be worth exploring the possibility of centralizing these notifications in a follow up bug?
Maybe a CE?

Comment 4

[Magnus Melin [:mkmelin] (away, back June 30)]

Not sure it's very doable.

Comment 5

[Alessandro Castellani [:aleca]]

Any update from Philipp?
If he's super busy, is there someone else that can review Calendar code?

Comment 6

[Magnus Melin [:mkmelin] (away, back June 30)]

Comment on attachment 9184651 [details] [diff] [review]
1673958-notifications.diff

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

Yes, Geoff is back, and eagerly awaiting reviews, I'm sure ;)

Comment 7

[Geoff Lankow (:darktrojan)]

Comment on attachment 9184651 [details] [diff] [review]
1673958-notifications.diff

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

This makes sense. I always wondered why using notifications was so wordy.

There's implications for all add-ons with notifications. If this is going back to 78 (probably should?) I think every gNotification should have a shim notificationbox property to keep current add-on code working. By next ESR we should have an API sorted out so it wouldn't matter any more.

r- just to counteract Magnus's r+. :-)

::: calendar/lightning/content/lightning-utils.js
@@ +18,5 @@
> +XPCOMUtils.defineLazyGetter(this, "gNotification", () => {
> +  return new MozElements.NotificationBox(element => {
> +    document.getElementById("no-identity-notification").append(element);
> +  });
> +});

You don't want to add to `this` here, because this file is used in a bunch of different windows including the main window.

Comment 8

[Magnus Melin [:mkmelin] (away, back June 30)]

We could do this for trunk only also. It's a problem, but not a huge one (unnoticed for over a year on release).

Comment 9

[Alessandro Castellani [:aleca]]

Attachment: 1673958-notifications.diff

Thanks for the detailed review.
I updated all the notification variable to match what they actually area and what type of notification container they interact with. In this way, we won't have a bunch of random gNotification all around TB that we can't distinguish.

I also found other small CSS problems, and some wrong parameters we were passing when appending the notification (eg. "null" as a string instead of null).

If we decide to uplift this for 78, I prefer to create a patch variation with the shim variables, and keep trunk clean.

Try run to see how many things I broke: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=e8e45dd12e999ba636459c8caa540a796dab0147

Comment 10

[Alessandro Castellani [:aleca]]

Another try-run after the backout fixing all those broken mochitests: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=7115b04fa52709cc37ed7aef0b1ffe6310650238

Comment 11

[Magnus Melin [:mkmelin] (away, back June 30)]

Comment on attachment 9188470 [details] [diff] [review]
1673958-notifications.diff

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

::: calendar/lightning/content/lightning-item-iframe.js
@@ +2647,1 @@
>    if (notification != null) {

would make this a falsy comparison while your're here

Comment 12

[Alessandro Castellani [:aleca]]

Attachment: 1673958-notifications.diff

Patch updated and new try-run launched since the previous one picked the outdated build: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=b922a139914614fe455f44ed2d117455445c3659

Comment 13

[Alessandro Castellani [:aleca]]

Attachment: 1673958-notifications.diff

Patch updated to fix those bct3 failures.
Another try-run launched, which seems to be stuck: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=dc4284e1209cc1cd1b2a7967d9917cb65b4cfff1

I'll launch another one if this fails for whatever reason.
Ah, it's running, let's see if I broke something else.

Comment 14

[Geoff Lankow (:darktrojan)]

Comment on attachment 9188727 [details] [diff] [review]
1673958-notifications.diff

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

::: mail/components/im/content/chat-conversation-info.js
@@ +113,5 @@
>  
>        if (Services.prefs.getBoolPref("chat.otr.enable")) {
>          let otrButton = this.querySelector(".otr-button");
>          otrButton.addEventListener("command", this.otrButtonClicked);
> +        OTRUI.setNotificationBox(gNotification);

Do you still want to call this one gNotification? It's a bit confusing.

::: mail/extensions/openpgp/content/ui/enigmailMsgComposeOverlay.js
@@ +3086,5 @@
>     */
>    async notifyUser(priority, msgText, messageId, detailsText) {
>      let notif = document.getElementById("attachmentNotificationBox");
>      if (!notif) {
> +      notif = gNotification;

This should be gComposeNotification, I think.

Comment 15

[Alessandro Castellani [:aleca]]

Attachment: 1673958-notifications.diff

Good catch, I missed those.
Also that Enigmail notifyUser() was using the wrong selector to attach a notification, so I fixed it.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7b9df4584eda
Fix defective implementation of the MozElements.NotificationBox. r=mkmelin, r=darktrojan

Comment 17

[Kai Engert [:KaiE:]]

Does TB78 need this? If yes, please request 84 beta approval.

Comment 18

[Alessandro Castellani [:aleca]]

What do you think we should do Magnus? Beta ad 78 uplift?
We do risk to break add-ons using notifications in those areas.

Comment 19

[Magnus Melin [:mkmelin] (away, back June 30)]

It wasn't ever reported by user AFAIK, so maybe let it just ride the trains and skip uplifts.