641892 - Support showing multiple popup notification icons at the same time

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Margaret Leibovic]

Right now we can only show one popup notification icon at a time, which causes a problem if there are multiple notification on a page, such as the password manager notification and the geolocation notification.

Also, this prevents us from creating multiple persistent indications, like in this mockup: https://bug634065.bugzilla.mozilla.org/attachment.cgi?id=513594.

Comment 1

[:Margaret Leibovic]

Attachment: wip

The current PopupNotifications API makes this tricky. This patch works, but I'm not sure I like the way I handled the case where the iconBox is the notification anchor. I modified the anchorElement getter to use #default-notification-icon as the anchor if it exists, but it seems bad to place a dependency on that ID in PopupNotifications.jsm. However, it will still fall back to using the iconBox if that element doesn't exist, so this patch shouldn't break anything.

Comment 2

[:Margaret Leibovic]

Comment on attachment 547430 [details] [diff] [review]
wip

>-.notification-anchor-icon {
>+.notification-anchor-icon:not([showing]) {
>   display: none;
>   -moz-user-focus: normal;
> }

Oops, this won't work. I'll change it to:

.notification-anchor-icon {
  -moz-user-focus: normal;
}

.notification-anchor-icon:not([showing]) {
  display: none;
}

Comment 3

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Does this bug stop now?

Comment 4

[:Margaret Leibovic]

(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #3)
> Does this bug stop now?

No, this bug still exists. Unfortunately, I haven't had the time to work on it in a while. Also, at this point, I'm not sure what the design for this is supposed to look like.

Jared has been doing some work near this area of the location bar in bug 588270, so I'm cc'ing him. However, I remember this bug being kind of tricky, so I don't know if he'd have time to work on it either.

Comment 5

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: proposed patch

What about this patch? This will work as we hope.
I don't have Mac/Linux environment. So I can't adjust styles...

Comment 6

[:Margaret Leibovic]

Comment on attachment 612804 [details] [diff] [review]
proposed patch

Nice, this is looking good. The changes in the patch look like they make sense to me, but it's been a while since I've worked with this code, and I remember there were often tricky edge cases to watch out for in here.

I made a build with this patch applied to test it out, and it's working pretty well. However, I found that if I visit a page with multiple notifications immediately after launching the browser, I get into a weird state where the notifications are combined into one panel (I will upload a screenshot). I also found that sometimes clicking on the default icon doesn't open the arrow panel as it should.

In case it helps, I made a testcase that shows a geolocation notification and an indexedDB notification: http://people.mozilla.com/~mleibovic/test/multiple-icon.html. We'll probably also want to try to add testcases for multiple notifications to browser_popupNotification.js: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_popupNotification.js. I also bet generating notifications from a privileged scratchpad could be a good way to play around with testing this.

Comment 7

[:Margaret Leibovic]

Attachment: incorrectly combined notifications

Comment 8

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Comment on attachment 612804 [details] [diff] [review]
proposed patch

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

(In reply to Margaret Leibovic [:margaret] from comment #6)
> I made a build with this patch applied to test it out, and it's working
> pretty well. However, I found that if I visit a page with multiple
> notifications immediately after launching the browser, I get into a weird
> state where the notifications are combined into one panel (I will upload a
> screenshot). I also found that sometimes clicking on the default icon
> doesn't open the arrow panel as it should.

Umm... I think:

::: toolkit/content/PopupNotifications.jsm
@@ +493,1 @@
>        });

The checking |n.anchorElement == anchorElement| is removed from this part. It'll causes your reported bug behavior. 

|PopupNotifications.prototype._refreshPanel()| is make popup notification panels from |notificationsToShow| array.
If no checking |n.anchorElement == anchorElement|, _refreshPanel() will make some notifications without considering anchor element.

And I think that, the current implementation of _refreshPanel() is not clearly. This implementation may be useful for making many same icons(e.g. showing 2~3 geolocation icons) from array without build-in xul:image.notification-anchor-icon .
But the current PopupNotifications design need to set xul:image.notification-anchor-icon before the fact.

It has not a consistency between overall design and local design. We may need to clean up them.

Comment 9

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v2

Comment 10

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v2.1

Sorry, patch v2 was rejected in applying.
This v2.1 is rebased on latest mozilla-central.

Comment 11

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Comment on attachment 615028 [details] [diff] [review]
patch v2.1

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

This patch is some test failed.
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=17eb4722039f (Thank you, Nakano-san.)

I can fix to be failed "browser_customize_popupNotification.js" soon.
I write a my fixing idea about this fail.

It may be difficult to fix "browser_bug553455.js".
I have not get the plan to fix it yet... The timing to show popup is too severe. I still have not been able to debug it fully.

Fail "browser_popupNotification.js" is related to showing popup. Maybe, this is caused with same bug of fail "browser_bug553455.js". I have not researched yet.
It may need to change all logic..... :(

::: toolkit/content/PopupNotifications.jsm
@@ +455,5 @@
> +        // it is different from the notification of |anchorElement|.
> +        let oldAnchorElement = this._currentAnchorElement;
> +        for (let n of this._currentNotifications) {
> +          if (n.anchorElement == oldAnchorElement) {
> +            this._fireCallback(n, "dismissed");

This part should be check |n.anchorElement != null|. n.anchorElement sometimes returns null.

Comment 12

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v3

I change the part of getting anchorElement.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 615779 [details] [diff] [review]
patch v3

Did you forget to request review on this patch?

Comment 14

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Comment on attachment 615779 [details] [diff] [review]
patch v3

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

(In reply to Jared Wein [:jaws] from comment #13)
> Comment on attachment 615779 [details] [diff] [review]
> patch v3
> 
> Did you forget to request review on this patch?

I waited this test:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=e93a0987a49a
(Thank you, Nakano-san)

This patch v3 fails the failed test which is added by this patch.
I try to fix it.

Comment 15

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v4

This version fix the added test & add the style for Linux.
The style for Mac is not adjusted because I don't have Mac and Mac Theme has had "margin: 0 2px" already.

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 616111 [details] [diff] [review]
patch v4

> .notification-anchor-icon {
>   display: none;
>   -moz-user-focus: normal;
> }
> 
>+.notification-anchor-icon[showing] {
>   display: -moz-box;
> }

.notification-anchor-icon {
  -moz-user-focus: normal;
}

.notification-anchor-icon:not([showing]) {
  display: none;
}

Comment 17

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Dão Gottwald [:dao] from comment #16)
> Comment on attachment 616111 [details] [diff] [review]
> patch v4
> 
> > .notification-anchor-icon {
> >   display: none;
> >   -moz-user-focus: normal;
> > }
> > 
> >+.notification-anchor-icon[showing] {
> >   display: -moz-box;
> > }
> 
> .notification-anchor-icon {
>   -moz-user-focus: normal;
> }
> 
> .notification-anchor-icon:not([showing]) {
>   display: none;
> }

Is your review meaning this ?:

.notification-anchor-icon {
  -moz-user-focus: normal;
}

.notification-anchor-icon:not([showing]) {
  display: none;
}

.notification-anchor-icon[showing] {
  display: -moz-box;
}

Comment 18

[Dão Gottwald [:dao]]

(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #17)
> (In reply to Dão Gottwald [:dao] from comment #16)
> > Comment on attachment 616111 [details] [diff] [review]
> > patch v4
> > 
> > > .notification-anchor-icon {
> > >   display: none;
> > >   -moz-user-focus: normal;
> > > }
> > > 
> > >+.notification-anchor-icon[showing] {
> > >   display: -moz-box;
> > > }
> > 
> > .notification-anchor-icon {
> >   -moz-user-focus: normal;
> > }
> > 
> > .notification-anchor-icon:not([showing]) {
> >   display: none;
> > }
> 
> Is your review meaning this ?:
> 
> .notification-anchor-icon {
>   -moz-user-focus: normal;
> }
> 
> .notification-anchor-icon:not([showing]) {
>   display: none;
> }
> 
> .notification-anchor-icon[showing] {
>   display: -moz-box;
> }

No, I meant just what I wrote.

Comment 19

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

I forgot to remove "showing" attribute from icon element when user switch a tab.
I'll fix it & add its test into a next patch.

Comment 20

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v5

Comment 21

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #20)
> Created attachment 616425 [details] [diff] [review]
> patch v5

This works as well but this have not passed tests which is added with this bug yet. I'm trying to fix them now.
I'm sure that basic logic is completed.

Comment 22

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v6

This patch fails tests.
However, this patch pass tests if we fix this patch as following:
  // Test multiple notification are shown
  { // Test #21

    onShown: function (popup) {
+     window.setTimeout(function(){
      checkPopup(popup, this.notifyObj2);

      // check notifyObj1 anchor icon is showing
      isnot(document.getElementById("default-notification-icon").boxObject.width, 0,
            "default anchor should be visible");
      // check notifyObj2 anchor icon is showing
      isnot(document.getElementById("geo-notification-icon").boxObject.width, 0,
            "geo anchor should be visible");

      dismissNotification(popup);
+     }, 2000);
    },

So this failing will be not caused by this patch changeset. It is a latent bug of PopupNotifications.jsm or its test codes. We should fix it. But I have not found the bug point.
Please review this patch, and What should we do about this bug?

Comment 23

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #22)
> Created attachment 616566 [details] [diff] [review]
> patch v6
> 
> This patch fails tests.

Test Result (abstract):
==========================
TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_popupNotification.js | [Test #22] popup shown
TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_popupNotification.js | [Test #22] checking popup
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_popupNotification.js | shown callback was triggered
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_popupNotification.js | only one notification displayed - Got 0, expected 1
Stack trace:
    JS frame :: chrome://mochikit/content/browser-test.js :: test_is :: line 418
    JS frame :: chrome://mochitests/content/browser/browser/base/content/test/browser_popupNotification.js :: checkPopup :: line 733
    JS frame :: chrome://mochitests/content/browser/browser/base/content/test/browser_popupNotification.js :: <TOP_LEVEL> :: line 659
    JS frame :: chrome://mochitests/content/browser/browser/base/content/test/browser_popupNotification.js :: <TOP_LEVEL> :: line 97
    JS frame :: chrome://mochitests/content/browser/browser/base/content/test/browser_popupNotification.js :: <TOP_LEVEL> :: line 137

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_popupNotification.js | an unexpected uncaught JS exception reported through window.onerror - TypeError: notification is undefined at chrome://mochitests/content/browser/browser/base/content/test/browser_popupNotification.js:740
Stack trace:
    JS frame :: chrome://mochikit/content/tests/SimpleTest/SimpleTest.js :: simpletestOnerror :: line 983
    native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_popupNotification.js | [Test #22] added listeners; panel state: false
==========================

This part is |checkPopup(popup, this.notifyObj2);| of:

==========================
// Test multiple notification are shown
    onShown: function (popup) {
      checkPopup(popup, this.notifyObj2);

      // check notifyObj1 anchor icon is showing
      isnot(document.getElementById("default-notification-icon").boxObject.width, 0, "default anchor should be visible");
      // check notifyObj2 anchor icon is showing
      isnot(document.getElementById("geo-notification-icon").boxObject.width, 0, "geo anchor should be visible");
      dismissNotification(popup);
    },
==========================

Comment 24

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

I missed that I can set multiple test.onHidden, at browser/base/content/test/browser_popupNotification.js. So I have set multiple test.onHidden, the pach v6 above tests are passed. (The failing tests are caused by calling same test.onHidden multiple times)

I'm making a next patch.

Comment 25

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v7

This patch passes tests on my local machine!

Comment 26

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v7.1

Comment 27

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Result of try-server test of Patch v7.1:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=88f855dea1c4
(Thank you, Nakano-san!)
(Some failed test is not caused by this patch)

I'm sure that this patch works complete. Please review this. And I'm Sorry for requesting many review, Margaret.

Comment 28

[:Margaret Leibovic]

Comment on attachment 616863 [details] [diff] [review]
patch v7.1

Sorry for the delayed review! I wanted to find time to look at this more closely. This looks good to me, and a build with the patch was working well. Thanks for adding tests! I just have a few nits:

>diff --git a/browser/base/content/test/browser_popupNotification.js b/browser/base/content/test/browser_popupNotification.js

>@@ -637,16 +637,94 @@ var tests = [

>+  // Test multiple notification are shown

s/notification/notification icons/

>+  // Test multiple notification icons are removed when it switch the tab

English grammar nit :) I think this comment should be:
// Test that multiple notification icons are removed when switching tabs

>diff --git a/toolkit/content/PopupNotifications.jsm b/toolkit/content/PopupNotifications.jsm

>@@ -77,25 +80,28 @@ Notification.prototype = {

>+    // Use a default anchor icon if it's available
>+    if (!anchorElement) {
>+      anchorElement = iconBox.querySelector("#default-notification-icon") || iconBox;
>+    }

Nit: Remove the braces on this if statement to match the rest of the file.

>@@ -534,16 +548,34 @@ PopupNotifications.prototype = {

>+  _hideIcons: function PopupNotifications_hideIcons() {
>+    let icons = this.iconBox.querySelectorAll(ICON_SELECTOR);
>+    for (let icon of icons) {
>+      if (icon.hasAttribute(ICON_ATTRIBUTE_SHOWING)) {
>+        icon.removeAttribute(ICON_ATTRIBUTE_SHOWING);
>+      }

You can get rid of this hasAttribute check, since calling removeAttribute won't cause any problems if the element doesn't have the attribute, and it's not more expensive than calling hasAttribute.

Comment 29

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v8

Thank you for your review, Margaret.
I modified parts that you pointed.

Comment 30

[:Margaret Leibovic]

Comment on attachment 620612 [details] [diff] [review]
patch v8

Thanks!

Comment 31

[:Margaret Leibovic]

I'm not sure if you have commit access, so I'm marking this as checkin-needed.

Comment 32

[:Gavin Sharp [email: [e-mail]]]

SeaMonkey will probably need to be adjusted to account for these changes.

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 620612 [details] [diff] [review]
patch v8

>diff --git a/toolkit/content/PopupNotifications.jsm b/toolkit/content/PopupNotifications.jsm

>+const ICON_SELECTOR = ".notification-anchor-icon";
>+const ICON_ATTRIBUTE_SHOWING = "showing";

uber-style nit: I don't think factoring these strings out into constants helps with clarity or maintainability, and it makes it harder to see what the code is doing without having to look up the constant value, so I think you should get rid of these.

Comment 34

[[e-mail]]

(In reply to Gavin Sharp from comment #32)
> SeaMonkey will probably need to be adjusted to account for these changes.
Thanks for the heads-up.

(In reply to Gavin Sharp from comment #33)
> >+const ICON_ATTRIBUTE_SHOWING = "showing";
> uber-style nit: I don't think factoring these strings out into constants
> helps with clarity or maintainability, and it makes it harder to see what
> the code is doing without having to look up the constant value, so I think
> you should get rid of these.
It might be even clearer still if you used the hidden property, although I don't know whether you would want to initially hide the icons in script or in markup.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aeea5b83cf89

Comment 36

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/aeea5b83cf89