572967 - don't remove popup notifications on dismissal (allow them to be re-activated from their anchor icon)

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

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

This is a followup to bug 398776 - the new popup notifications added in that patch disappear when dismissed. We should instead keep them around and allow users to re-display them by clicking their anchor icon.

Comment 1

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

Attachment: WIP

This is on top of the patch for bug 398776.

Comment 2

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

Attachment: patch

The idea is that we no longer remove notifications when they are dismissed, but instead mark them as "dismissed". dismissed notifications are not displayed, but can be un-dismissed by clicking on their anchor (which now also sticks around post-dismissal). I also made a few changes to enable keyboard accessibility of the anchor icons (though need to investigate further there, since tabbing "into" the popup isn't possible).

Comment 3

[Justin Dolske [:Dolske]]

Comment on attachment 457453 [details] [diff] [review]
patch

>+++ b/browser/base/content/browser.js
...
> XPCOMUtils.defineLazyGetter(this, "PopupNotifications", function () {
>   let tmp = {};
>   Cu.import("resource://gre/modules/PopupNotifications.jsm", tmp);
>-  return new tmp.PopupNotifications(gBrowser,
>-                                    document.getElementById("notification-popup"),
>-                                    document.getElementById("notification-popup-box"));
>+  try {
>+    return new tmp.PopupNotifications(gBrowser,
>+                                      document.getElementById("notification-popup"),
>+                                      document.getElementById("notification-popup-box"));
>+  } catch (ex) {
>+    Components.utils.reportError(ex);
>+  }
> });

Hmm, does this mean if the |new| call throws the first time, it'll be a hard failure? Or does defineLazyGetter() handle getting a reval of |undefined|(?).


>+++ b/browser/themes/gnomestripe/browser/browser.css
...
>+.notification-anchor-icon:focus {
>+  outline: 1px dotted -moz-DialogText;
>+}

Dotted focus rings in chrome? That seems odd, even for just Linux?


>+++ b/toolkit/content/PopupNotifications.jsm
...
>+    this.panel.ownerDocument.commandDispatcher.advanceFocusIntoSubtree(this.panel);

I'm not sure I understand what this is doing... Setting the initial focus in the panel contents?


>+  _onIconBoxCommand: function PopupNotifications_onIconBoxCommand(event) {
...
>+    // Mark notifications anchored to this anchor as un-dismissed
>+    this._currentNotifications.forEach(function (n) {
>+      if (n.anchorElement == anchor)
>+        n.dismissed = false;
>+    });
>+
>+    // ...and then show them.
>+    this._update(anchor);

Err, are you trying to show multiple doorhangers at once? Clicking the icon should just show the topmost one, AIUI?

Comment 4

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

(In reply to comment #3)
> Hmm, does this mean if the |new| call throws the first time, it'll be a hard
> failure? Or does defineLazyGetter() handle getting a reval of |undefined|(?).

Yes, it will be a hard failure (so this is not a functional change). I added this because otherwise exceptions thrown within PopupNotifications.jsm during development weren't being reported :(

> >+.notification-anchor-icon:focus {
> >+  outline: 1px dotted -moz-DialogText;
> >+}
> 
> Dotted focus rings in chrome? That seems odd, even for just Linux?

I just copied the styling directly from what we do for Larry.

> >+    this.panel.ownerDocument.commandDispatcher.advanceFocusIntoSubtree(this.panel);
> 
> I'm not sure I understand what this is doing... Setting the initial focus in
> the panel contents?

Oops, thought I'd removed this (maybe I forgot to qrefresh). That was the idea, but this doesn't work quite right. I'm going to file a followup and talk to accessibility guys to ensure that keyboard accessibility is sufficient.

> Err, are you trying to show multiple doorhangers at once? Clicking the icon
> should just show the topmost one, AIUI?

It will show all notifications anchored to the given element - as it is, multiple notifications can re-use the same anchor element (and therefore be shown at the same time). There are no current callers making use of that ability, but I don't know that it's worth disallowing (and even if we did, this code would need to stay as it is, apart from s/them/it/ in the comment).

Comment 5

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

Attachment: patch without focus stuff

Comment 6

[Justin Dolske [:Dolske]]

Comment on attachment 458560 [details] [diff] [review]
patch without focus stuff

Sorry for being slow to follow up here. :/

>+  try {
>+    return new tmp.PopupNotifications(gBrowser,
>+                                      document.getElementById("notification-popup"),
>+                                      document.getElementById("notification-popup-box"));
>+  } catch (ex) {
>+    Components.utils.reportError(ex);
>+  }

What I was trying to ask above, poorly, was why the |catch| doesn't rethrow the error, since it's now suppressing it entirely.

But I guess things are hosed anyway, because it doesn't look like defineLazyGetter() can recover if |aLambda| throws -- it's already deleted the getter.

Comment 7

[Dietrich Ayala (:dietrich)]

posthumous-blocking+, since some of these notifications can contain privacy preference decisions that users might want to reconsider.

Comment 8

[aicitman]

I'm not a c++ guru, but from what I understood from the code, if you don't press anything, the icon will stay there forever. Am I correct

Comment 9

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

Until you navigate to a different page, yes. I need to investigate what will happen if the geolocation request times out.

Comment 10

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

Attachment: patch+test fix

I landed this earlier, but forgot to include a local test change. Here's an updated patch that's ready to land.

Comment 11

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

I filed bug 584825 for the timed-out-request followup.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 463255 [details] [diff] [review]
patch+test fix

>+.notification-anchor-icon:focus {
>+  outline: 1px dotted -moz-DialogText;
>+}

this should be using :-moz-focusring

Comment 13

[Dão Gottwald [:dao]]

(In reply to comment #12)
> Comment on attachment 463255 [details] [diff] [review]
> patch+test fix
> 
> >+.notification-anchor-icon:focus {
> >+  outline: 1px dotted -moz-DialogText;
> >+}
> 
> this should be using :-moz-focusring

landed with this fix

http://hg.mozilla.org/mozilla-central/rev/bb6a68816c12