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

RESOLVED FIXED in Firefox 4.0b4

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Gavin, Assigned: Gavin)

Tracking

Trunk
Firefox 4.0b4
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: popupnotification)

Attachments

(1 attachment, 3 obsolete attachments)

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.
Posted patch WIP (obsolete) — Splinter Review
This is on top of the patch for bug 398776.
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Whiteboard: popupnotification
Posted patch patch (obsolete) — Splinter Review
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).
Attachment #452179 - Attachment is obsolete: true
Attachment #457453 - Flags: review?(dolske)
blocking2.0: --- → ?
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?
Attachment #457453 - Flags: review?(dolske)
(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).
Posted patch patch without focus stuff (obsolete) — Splinter Review
Attachment #457453 - Attachment is obsolete: true
Attachment #458560 - Flags: review?(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.
Attachment #458560 - Flags: review?(dolske) → review+
Attachment #458560 - Flags: approval2.0+
posthumous-blocking+, since some of these notifications can contain privacy preference decisions that users might want to reconsider.
blocking2.0: ? → betaN+

Comment 8

9 years ago
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
Until you navigate to a different page, yes. I need to investigate what will happen if the geolocation request times out.
I landed this earlier, but forgot to include a local test change. Here's an updated patch that's ready to land.
Attachment #458560 - Attachment is obsolete: true
I filed bug 584825 for the timed-out-request followup.
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
(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
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b4
You need to log in before you can comment on or make changes to this bug.