398776 - (doorhanger) want mechanism for site-specific notification (doorhanger)

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Madhava Enros [:madhava]]

Mockups:
http://people.mozilla.com/~madhava/files/favicon/mockups_2007-07-27/
(all_notifications.png and fullmenu_option1.png being the most useful)

Two existing issues with the current notification bar for messages and warnings are (1) that they cover only the content area and are therefore spoofable, and (2) that user-decisions must be made in response to them immediately because, as soon as they are dismissed, there is no obvious way to get back to the relevant option.

The intention of this design is to remedy both of those issues while, at the same time, to associate messages/warnings from Firefox about a specific site more closely with the site-identity artifact in the browser -- in other words, the favicon or, when appropriate, the larry-extended favicon.  Issue (1) is improved by the fact that the doorhanger covers some chrome.  Issue (2), above, is improved by the association of the notification with the favicon (the notification grows from and shrinks back into the favicon) -- options related to the decisions made in notifications show up, and can be changed later, in the favicon menu (see mockups).

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

cc'ing johnath and dolske as this affects their areas

Comment 2

[Neil Deakin]

Attachment: initial work

Here is a first run at implementing this. There are still some things to do, but feedback and testing welcome.

Comment 3

[Jesse Ruderman]

The new notifications for popups and passwords overlap with more chrome and content than is acceptable, IMO.  The reason we used notification bars rather than dialogs was to let users ignore the question if they want to, and the new design breaks that.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Replacing the word "New" from beginning of bug summary, so this won't look 
like a brand new bug in each email that gets sent out.  

Comment 5

[Alex Faaborg [:faaborg] (Firefox UX)]

>The reason we used notification bars rather
>than dialogs was to let users ignore the question if they want to, and the new
>design breaks that.

Here is the rational behind the current design:

1) Streamlined close
Clicking anywhere outside of the door hanger closes it, so you can dismiss the notification/question/interface without having to move your mouse cursor, or your hand off of the mouse itself.

2) Less jarring motion
Shifting the content area is visually jarring, and overlapping the content area is unacceptable.  The only other great solution I've seen is the Safari find bar scroll trick.

3) Modal behavior of users
In practice I think that user's feel compelled to answer any question asked to them in a notification bar, even if the answer is clicking the close button to dismiss the bar.  So while it is certainly easy to simply ignore notifications, user's may still interact with them as if they were modal dialogs.

4) Supporting undo
If you close the notification bar, there is no way to get it back.  If you dismiss a door hanger notification, you can get back to it by clicking on the favicon/identity menu, and selecting the appropriate item, which has the same icon shown in the notification.

5) Spoof-ability
I personally care more about the above 4 reasons, but I should also note that this design has the advantage of putting trusted messages more clearly in chrome and out of content.  This is now more relevant because we are going beyond pop-up blocking and into password management, malware and phishing protection (badware are modal messages and appear in red https://bugzilla.mozilla.org/attachment.cgi?id=265071).  We are worried about spoofed notification bars that say things like "Firefox recommends that you download and install this malware protection suite: evil.exe" and "For Firefox to remember your password for paypal.com, please enter it here"

Comment 6

[Jesse Ruderman]

Shifting content down is better than overlapping content, IMO.  If it's jarring, that's because we have unnecessary and buggy animation.  How about keeping the current shape of notification bars but making them be "visually attached" to chrome?

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Not blocking, but definitely wanted.

> Shifting content down is better than overlapping content, IMO.  If it's
> jarring, that's because we have unnecessary and buggy animation.  How about

I disagree. The priority of these notifications can be set so that they fade away when someone clicks on the page, or after a timeout, or force a response to the question. As with the <notificationmessages> there will be options on frequently occuring notifications to not show notifications and just take a default action.

Comment 8

[O. Atsushi (Torisugari)]

How about entering username/password for FTP login and HTTP Basic/Digest auth?

It is a very "site-specific notification".

Comment 10

[Justin Dolske [:Dolske]]

Attachment: MattN's WIP patch

This is a snapshop of MattN's work from last summer, around the time his internship ended. He was in the middle of moving things to the Larry dropdown from a independent button (thinking was this might go in before the "site button"), so may be partially broken.

Comment 11

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

Attachment: WIP patch

Here's a dump of my latest WIP. It introduces a new javascript module, PopupNotifications.jsm, which is in charge of managing the new-style popup notifications, with an API similar to the current notificationbox API. It also includes the changes required to make the geolocation notification use this new API (and related test changes).

The latest mockups from Alex changes the goal of this new system a fair bit from what was around when this bug was filed. The current plan is to use this style of notifications only for requests of "sensitive information" from the page - e.g. geolocation, HTTP auth passwords, "Account Manager" functionality (http://hacks.mozilla.org/2010/05/getting-involved-with-account-manager/), etc.

The unfinished parts are:
- styling of the panels on Linux/Windows (besides my sucking at it, not having easy access to builds on those platforms has hampered my efforts somewhat)
- figuring out how exactly the post-dismissal re-activation should be handled - at the moment the notifications are removed when dismissed. The location bar "geo" button that's in Alex's latest mockup is present and somewhat functional as an anchor for the notifications, but I'm not happy with how it's working, and there's some anchorID stuff in this patch that needs to either be fleshed out or removed.

Apart from that, I think this is at the point where it would benefit from some early review passes. Not sure who exactly should review it yet, though.

Comment 12

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

Some explanations of the geolocation test changes:
- The new geolocation notification doesn't include a one-time "don't tell" option. The available options are "share location", "always share location", "never share location", and the implicit "ignore request" from dismissing the popup. That required that I disable a couple of tests for Firefox, since it's the only user of this new API (the test is still valid in e.g. SeaMonkey)

- I also had to rework some of the geolocation_common code to first check for new-style notifications before trying to interact with the old-style ones

- geolocation.html (used by test_manyWindows.html was kind of busted - it duplicated a bunch of code from geolocation_common unnecessarily, and also wasn't quite behaving the way it was supposed to (the way the setTimeouts were run meant that most of the stuff it did didn't have any effect - it's more of a crash test than a functional test). I simplified it quite a bit.

Comment 13

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

Frank, Dolske and I had briefly discussed doing work to expose HTTP auth prompts using this API - here's how I think that could work:
- create a new popupnotification[type="auth"] binding that includes the password fields etc.
- add a new show()-like method to the PopupNotifications object (showAuth?) that creates these kinds of notifications (potentially with different options for callbacks etc.), but that stores/tracks them using the same existing mechanism as show()

Comment 14

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

I think the "remember password" notification is implementable on top of this patch, if you ignore the anchoring issues (every notification type is currently anchored to the same "iconBox" with a single hardcoded icon in it). Might even help sort out what the solution to those issues should be.

I kind of forgot about no-interaction notifications which had also been discussed - for that we can just remove the requirement that you pass in a message or mainAction to show(), I think.

Comment 15

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

Oh, and that patch is on top of attachment 440596 [details] [diff] [review], which is needed to make the menu-buttons not-completely-ugly. Still hoping for a better solution to that bug, though.

Comment 16

[Robert Kaiser]

(In reply to comment #12)
> That required that I disable a couple of tests for Firefox, since it's
> the only user of this new API (the test is still valid in e.g. SeaMonkey)

I had hoped we'd have a platform-generic "interface"/module as a base for those notifications in some way, as I'm pretty sure Thunderbird will want something in that style as well, and we could hook in messages from the platform as well if this would live there in some way.

SeaMonkey for one will try to port this fast, and then those geolocation tests you disable will probably be obsolete, right?

Also, how visible is it to users that they can "just dismiss" this doorhanger? With notification bars, we learned that the "x" on the right side wasn't enough for users and we needed explicit "not now" buttons to make the path to ignoring clear.

Comment 17

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

(In reply to comment #16)
> I had hoped we'd have a platform-generic "interface"/module as a base for those
> notifications in some way

I don't want to block on that. It can happen later on, once the API settles down a bit. The module is relatively browser/-agnostic so moving it to toolkit later shouldn't be a problem.

> Also, how visible is it to users that they can "just dismiss" this doorhanger?
> With notification bars, we learned that the "x" on the right side wasn't enough
> for users and we needed explicit "not now" buttons to make the path to ignoring
> clear.

This is a valid concern. With bug 552982, these could probably have titlebar "x" close buttons like normal windows, but I'm not sure that's the look we want to go for...

Comment 18

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

Attachment: updated WIP

I came up with what seems to be a workable solutation for the anchorID stuff, I think. The PopupNotifications object is responsible for hiding/showing the iconBox, if passed in, and notifications can have optional anchorIDs that indicate which element (typically a child of the icon box, but not necessarily) they should be anchored to.

This is pretty close to being ready for review, I think (still need styling on Linux/Windows but that should be easier this week when I'm back in Toronto).

Comment 19

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

Attachment: patch, v1

Finished up some basic styling for Windows/Linux. It's still rather ugly, but I think it's probably land-able on trunk. Also made a minor test tweak to fix failures on non-Mac, and some other minor changes. The tests are somewhat flaky on Linux, and potentially on other platforms as well. I'll need to keep an eye on them.

The stashing of dismissed notifications is the only major part missing from this patch. I've already got a followup patch prepared that does that, but I'll submit it separately.

I've pushed this to try, I'll link to the test builds here once they're done.

Comment 20

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

Builds with this patch applied will show up here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-54702b8ddb7f/

A simple test page that will let you see the new geolocation notification is at http://gavinsharp.com/tmp/geo.html . Google Maps also has geolocation support (click the little blue button).

Comment 21

[flying sheep]

It would be nice to include an option to use the notification techniques already present in gnome _and_ kde.

there already is the addon PlasmaNotify for kde, but a native solution would be way better.

Comment 22

[O. Atsushi (Torisugari)]

ScreenShot,please.

Comment 23

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

(In reply to comment #21)
> It would be nice to include an option to use the notification techniques
> already present in gnome _and_ kde.

These notifications are webpage-specific, and aren't something you'd want to see system-wide (or when you're not using Firefox). We have other notification APIs that handle that kind of notification (nsIAlertsService), and the debate about how much those should delegate to system APIs on various platforms should be (and has been!) had somewhere other than in this bug :)

Comment 24

[Ria Klaassen (not reading all bugmail)]

Looks fine! But the icon on the doorhanger is somewhat blurry.

Comment 25

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

Attachment: screenshot

Comment 26

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

(In reply to comment #24)
> Looks fine! But the icon on the doorhanger is somewhat blurry.

Yeah, that's bug 565187.

Comment 27

[flying sheep]

(In reply to comment #23)
> These notifications are webpage-specific, and aren't something you'd want to see system-wide
oh, sorry, i just followed the link from https://wiki.mozilla.org/Firefox/Projects/UX_Priorities_3.7 , where a link entitled “notifications” points to the doorhanger notifications. i think somebody should fix this page to contain unambiguous links to both kinds of notifications

Comment 28

[O. Atsushi (Torisugari)]

(In reply to comment #25)
> Created an attachment (id=449369) [details]
> screenshot

Thanks.

Then, after pushing a "Share Location" button, no visible UI remains?

1)
Is it possible for users to distinguish a tab is whether or not they have told their location, after some amount of web surfing? Some guys sometimes keep tons of tabs open for a week or so. It's nice to have an ability to grasp their privacy status at a glance. 

URLbar won't show the log-in name. So if I had 2 log-in accounts, say "Jack" and "Betty", browsing for a couple of  hours would make me forget which I'd logged in as. As a result, I'd have no  clue whether I should behave as a boy or a girl and surly fail sock-puppeting. It's a huge security risk.

2)
Another (and similar) point is, is it possible to turn off the feature during web browsing on the same site? In HTTP auth term, is it possible to "log off" without closing the tab? If I understand correctly, reloading the same URI will result in reusing the username & password. So nature call will do serious damage to either usability or privacy: being away from the browser with the tab close or with it still open. There exists no alternative "Just log out".

This also applies geo-location. While I travels to the Mars, I may want to know weather forecasts both there and home.  So, in the future, mobile browser should be able to switch back and forth between 2 location-profiles: registered one (at my house) and dynamic one (GPS).  And preferably a fake location, e.g. at the bottom of the pacific ocean.

Anyway, I think, "inactive" status of door-hanger is more  important than "active" one. And yes, this bug is focusing on polishing popping-up of the active notification, and nobody has touched "inactive" cases. But that was fair enough. The username-password dialog is completely independent from other UIs and there's no room for improving how to display static status. On the other hand, door-hanger is on browser's chrome. Maybe "inactive" one is a little too ambitious for now, but I believe a seamless design gluing "active" and "inactive" door-hangers will make more and more senses in the future.

Comment 29

[Dão Gottwald [:dao]]

(In reply to comment #12)
> - The new geolocation notification doesn't include a one-time "don't tell"
> option. The available options are "share location", "always share location",
> "never share location", and the implicit "ignore request" from dismissing the
> popup.

On google maps, dismissing the popup doesn't stop google's spinning geolocation knob, whereas the notification bar's "Don't share" button did.

For the record I'll point out that I like the unobtrusiveness of the notification bar and dislike the modal-dialog-like popup, but I guess this ship has sailed.

Comment 30

[Dão Gottwald [:dao]]

Comment on attachment 449058 [details] [diff] [review]
patch, v1

>+popupnotification:not([buttonlabel]) .popup-notification-menubutton {
>+  display: none;

Looks like the selector could be .popup-notification-menubutton:not([label])

>+          <box id="notification-popup-box" hidden="true">
>+            <hbox align="center">
>+              <image id="geo-notification-icon"/>
>+            </hbox>
>+          </box>

Isn't the box already horizontal, i.e. couldn't you spare the hbox? Otherwise use orient="horizontal"?

Comment 31

[Dão Gottwald [:dao]]

Forgot to add, pinstripe has geolocation-specific styling in notification.css.

Comment 32

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

(In reply to comment #29)
> On google maps, dismissing the popup doesn't stop google's spinning geolocation
> knob, whereas the notification bar's "Don't share" button did.

Yeah, this is a known downside of not being able to "cancel" the request. The upside is that with a followup patch, you'll be able to respond to the request again at some later point from the identity-icon dropdown, even after having dismissed the popup... I'm not really sure that's the right tradeoff, and I share some of your reservations about the interaction differences between these and the notification bars, but I still think these are worth experimenting with a bit further, to get wider feedback.

Comment 33

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

(In reply to comment #31)
> Forgot to add, pinstripe has geolocation-specific styling in notification.css.

Hmm, it was added for geo, but it's not necessarily geo-specific (would apply to any notification with a text-link). I'm kind of tempted to just leave it, but I suppose the odds of someone else relying on it are pretty low (and they can always use custom styling if they hack the notification in the same way, I guess).

Comment 34

[Justin Dolske [:Dolske]]

Comment on attachment 449058 [details] [diff] [review]
patch, v1

>+function Notification(id, message, anchorID, mainAction, secondaryActions,
>+                      browser, parent) {

Parent seems a little odd for some reason... owner? notifications? eh. parent?


>+function PopupNotifications(window, panel, tabbrowser, iconBox) {

Remove the window arg, and just derive it from tabbrowser.ownerDocument.defaultView? Alternatively, remove tabbrowser and have window find the first <tabbrowser>? [This also eliminates the odd possibility of having a window that's not a parent of the tabbrowser.]


>+  function updateFromListeners() {
>+    // setTimeout(..., 0) needed, otherwise openPopup from "activate" event
>+    // handler results in the popup being hidden again for some reason...
>+    self.window.setTimeout(function () {
>+      self._update();
>+    }, 0);

Hmm, that's kind of annoying. Did you look into this at all? I wonder if activate is being fired before the window focuses, such that it focuses after the popup and triggers its removal?


>+  getNotification: function PopupNotifications_getNotification(id, browser) {
>+    let notifications = this._getNotificationsForBrowser(browser || this.tabbrowser.selectedBrowser);
>+    for (let i = 0; i < notifications.length; i++) {
>+      let n = notifications[i];
>+      if (n.id == id)
>+        return n;
>+    }

Maybe:

  let n = null;
  notifications.some(function(x) x.id == id && (n = x))
  return n;


>+  show: function PopupNotifications_show(browser, id, message, anchorID,
>+                                         mainAction, secondaryActions, options) {

Make anchorID be required, and remove the iconBox fallback? Both faaborg and limi really didn't like the idea of having a generic notifications button...

Should an icon be specified here, or are we requiring that it be handled through CSS? (I think I prefer CSS)


>+    if (mainAction && isInvalidAction(mainAction))
>+      throw "PopupNotifications_show: invalid mainAction";

Hmm, do we really want button-less notifications? Seems like if the user can't do anything about the notification, it just shouldn't be shown. And then there's the question of how the user removes the notification once they've read it, since there's no button. :)


>+    let existingIndex = -1;
>+    function sameID(n, i) {
>+      if (n.id == id) {
>+        existingIndex = i;
>+        return true;
>+      }
>+      return false;
>+    }
>+
>+    // If there's an existing notification with this ID, remove it
>+    if (notifications.some(sameID))
>+      this._remove(notifications[existingIndex]);

Similar trick as above would be a lot shorter...

  let n = null;
  notifications.some(function(x) x.id == id && (n = x))
  if (n)
    this._remove(n);


>+    let fm = Cc["@mozilla.org/focus-manager;1"].getService(Ci.nsIFocusManager);

Hmm, worth adding this to Services.jsm?


>+    if (browser == this.tabbrowser.selectedBrowser && fm.activeWindow == this.window) {
>+      // show panel now
>+      this._update(notification.anchorElement);

Seems odd to pass in the anchorElement here, I'd expect _update() to just show the first available notification in its list, when called.


>+    } else {
>+      // Otherwise, update() will display the notification the next time the
>+      // relevant tab/window is selected.
>+
>+      // Notify observers that we're not showing the popup (useful for testing)
>+      this._notify("backgroundShow");

Not sure if it's terribly useful (yet), but it would be nice to have these notifications have a subject relevant to what they're notifying about (eg, in this case, the notification itself).


>+  get panelOpen() {

isPanelOpen()?


>+  locationChange: function PopupNotifications_locationChange() {

Would be nice if we had a generic way to register for location changes (w/o the full overhead of a web progress listener)...


>+  _refreshPanel: function PopupNotifications_refreshPanel(notificationsToShow) {
...
>+    notificationsToShow.forEach(function (n) {

Geolocation never has more than one notification for a panel... I assume this is mostly leftover code from the old menu-style notification form? Worth keeping or not? (Ditto for elsewhere in this file)

>+  _update: function PopupNotifications_update(anchor) {
...
>+    if (notificationsToShow.length > 0) {
>+      this._showPanel(notificationsToShow, anchorElement);
>+    } else {
>+      // Notify observers that we're not showing the popup (useful for testing)
>+      this._notify("updateNotShowing");
>+
>+      this._hidePanel();
>+
>+      if (this.iconBox)
>+        this.iconBox.hidden = true;
>+    }

If there are notifications but none are to be shown, I don't think you want to hide the icon box.

>+  _popupHidden: function PopupNotifications_popupHidden(event) {

onPopupHidden?


>+  _buttonCommand: function PopupNotifications_buttonCommand(event) {
>+    // Need to find the associated notification object, which is a bit tricky
>+    // since it isn't associated with the button directly - this is kind of
>+    // gross and very dependent on the structure of the popupnotification
>+    // binding's content.

Hrm. The attribute inheritance is a bit ugly too... Maybe do what the plugin-problem UI does, and force a style flush, and then make use of XBL properties and/or do some magic with setting oncommand with a closure to needed info?

>+    event.stopPropagation();

Why is this needed?

>+  dump: function (s) {
>+    this.window.dump("[PopupNotifications] " + s + "\n");
>+  },
>+
>+  dumpJSStack: function dumpJSStack() {
>+    this.dump("JS Stack:");
>+    var caller = Components.stack;
>+    while (caller) {
>+      this.dump("\t" + caller.name);
>+      caller = caller.caller;
>+    }
>+  }

Remove?


>+    // XXX
>+    // browserBundle.GetStringFromName("geolocation.learnMore")
>+    //var formatter = Cc["@mozilla.org/toolkit/URLFormatterService;1"].getService(Ci.nsIURLFormatter);
>+    //link.href = formatter.formatURLPref("browser.geolocation.warning.infoURL");

Hmm. Seems like we should probably keep this, maybe file a followup for adding it back? Probably need a way for the caller to pass in an extra snipped of XUL to be appended to a notification, since we rebuild them... :/

>+            Services.perms.add(requestingURI, "geo", Ci.nsIPermissionManager.ALLOW_ACTION);

Is there a followup bug to add support for undoing this via doorhangers?


>--- a/dom/tests/mochitest/geolocation/Makefile.in
...
>+ifndef MOZ_PHOENIX
>+_TEST_FILES += test_cancelCurrent.html \
>+               test_cancelWatch.html \
>+               $(NULL)
>+endif

Err, ifndef? Shouldn't that be ifdef? 

Let's call this r+ even though there's a lot of minor nits. Oh, I didn't really look at the tests, rs on that unless you want me to look at 'em.

Comment 35

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

(In reply to comment #34)
> Should an icon be specified here, or are we requiring that it be handled
> through CSS? (I think I prefer CSS)

Yeah, CSS is much more theme-friendly.

> >+    if (mainAction && isInvalidAction(mainAction))
> >+      throw "PopupNotifications_show: invalid mainAction";
> 
> Hmm, do we really want button-less notifications? Seems like if the user can't
> do anything about the notification, it just shouldn't be shown. And then
> there's the question of how the user removes the notification once they've read
> it, since there's no button. :)

Some of Alex's original mockups had them (e.g. http://people.mozilla.com/~faaborg/files/20090821-notification/newNotification-i1.png ). They're not particularly hard to support so I think it doesn't hurt to leave them. Right now notifications are removed when dismissed, so the dismissal wouldn't be an issue. After bug 572967, we can make button-less notifications auto-remove themselves (rather than just being dismissed).

> >+      this._update(notification.anchorElement);
> 
> Seems odd to pass in the anchorElement here, I'd expect _update() to just show
> the first available notification in its list, when called.

We want it to show this notification specifically, though, not just whichever happens to be first in the list.

> Geolocation never has more than one notification for a panel... I assume this
> is mostly leftover code from the old menu-style notification form? Worth
> keeping or not? (Ditto for elsewhere in this file)

Yeah, I'll look into simplyfying this a bit (maybe in a followup).

> If there are notifications but none are to be shown, I don't think you want to
> hide the icon box.

As it is, it's impossible for there to be unshowable notifications, since the only time we call _update() with an anchor argument is when we've just added a notification for that anchor. That will change in bug 572967, though, so that patch handles that case.

> >+            Services.perms.add(requestingURI, "geo", Ci.nsIPermissionManager.ALLOW_ACTION);
> 
> Is there a followup bug to add support for undoing this via doorhangers?

Filed bug 572972.

> >--- a/dom/tests/mochitest/geolocation/Makefile.in
> ...
> >+ifndef MOZ_PHOENIX
> >+_TEST_FILES += test_cancelCurrent.html \
> >+               test_cancelWatch.html \
> >+               $(NULL)
> >+endif
> 
> Err, ifndef? Shouldn't that be ifdef?

No, it's right as is - those tests test the "cancel request" button, which Firefox no longer has...

Will address the rest of the comments tomorrow.

Comment 36

[Robert Kaiser]

(In reply to comment #34)
> >+function PopupNotifications(window, panel, tabbrowser, iconBox) {
> 
> Remove the window arg, and just derive it from
> tabbrowser.ownerDocument.defaultView? Alternatively, remove tabbrowser and have
> window find the first <tabbrowser>? [This also eliminates the odd possibility
> of having a window that's not a parent of the tabbrowser.]

It sounds to me like deriving the window from the tabbrowser is better, as tabbrowser.ownerDocument.defaultView should always exist IIRC, but some crazy extensions could insert another <tabbrowser> into the window...

Comment 37

[Justin Dolske [:Dolske]]

(In reply to comment #35)

> > Hmm, do we really want button-less notifications?
> 
> Some of Alex's original mockups had them

Those were for indicating that a new doorhanger menuitem had been added, without making the user chose an action immediately (button click or dismissing the dialog).

EG, for geolocation an always-approve site would show one briefly to indicate that geolocation was active, or password manager might show one when remember-password was available but autocomplete=off was in play.

So, I don't think these are needed any more.

> > >+      this._update(notification.anchorElement);
> > 
> > Seems odd to pass in the anchorElement here
> 
> We want it to show this notification specifically, though, not just whichever
> happens to be first in the list.

Shouldn't it be _update(notification), then?

Comment 38

[Alex Faaborg [:faaborg] (Firefox UX)]

> Hmm, do we really want button-less notifications?

yeah, I don't think we have any plans to use those anymore.

Comment 39

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

(In reply to comment #37)
> Shouldn't it be _update(notification), then?

_update takes an anchor because in bug 572967 we need to be able to update based on which anchor was clicked. I kind of forgot about that when I split bug 572967's patch out, but easier to just leave it as-is given that bug 572967 should be landing soon hopefully.

Comment 40

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

(In reply to comment #34)
> >+    event.stopPropagation();
> 
> Why is this needed?

Needed in the menupopup case to avoid the command event bubbling up to the button listener. Not needed in the button case, so I removed it.

Comment 41

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

Attachment: patch v1.1

This is with Dao and Dolske's comments addressed, except for these from Dolske's review:
1) investigate need for setTimeout in activate handler
2) make anchorID be required
3) remove no-button-notification support
4) add focusmanager to services.jsm (MDC doc?)
5) make notify pass notification object
6) remove support for multiple notifications
7) fixup command triggering to use closures rather than attributes/JS props on the notification object
8) adding back the "more info" link

I investigated 7) briefly and ran into issues.
I will file followups for 1), 4), 6), 7) and 8).
I think 2), 3) and 5) are probably not worth doing at this point (2 and 3 aren't a real burden to support, 5 has no current use case but I may end up doing in a followup to improve tests).
(2 and 6 also would require me to make some test changes that I don't want to block this landing on)

I need to do some final testing and sanity checking, but this is what I plan to land when I get a chance.

Comment 42

[Robert Kaiser]

Comment on attachment 452426 [details] [diff] [review]
patch v1.1

>+  Cu.import("resource:///modules/PopupNotifications.jsm", tmp);

Nit: You should use resource://gre/modules/ when importing toolkit/platform modules, so that this doesn't break in XULRunner configurations where application (resource:///) and platform (resource://gre/) are in different locations.

Comment 43

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

(In reply to comment #42)
> Nit: You should use resource://gre/modules/

Landed, with that fixed:
http://hg.mozilla.org/mozilla-central/rev/a1faf051bbc9

Comment 44

[Robert Kaiser]

Comment on attachment 452426 [details] [diff] [review]
patch v1.1

>+    // XXX
>+    // browserBundle.GetStringFromName("geolocation.learnMore")
>+    //var formatter = Cc["@mozilla.org/toolkit/URLFormatterService;1"].getService(Ci.nsIURLFormatter);
>+    //link.href = formatter.formatURLPref("browser.geolocation.warning.infoURL");

Oh, is there a followup filed for this?

Comment 45

[Robert Kaiser]

(In reply to comment #44)
> Oh, is there a followup filed for this?

Oh, just found bug 573536 in the actually checked-in patch, sorry for the bugmail.

Comment 46

[alex_mayorga]

Can a hack in password doorhanger fix bug 101664?

Comment 47

[Eric Shepherd [:sheppy]]

Documented the PopupNotifications.jsm code module here:

https://developer.mozilla.org/en/JavaScript_code_modules/PopupNotifications.jsm

Added a document showing how to use it here:

https://developer.mozilla.org/en/Using_popup_notifications

Linked to from the JS code modules and Fx4 pages.