Closed Bug 398776 (doorhanger) Opened 17 years ago Closed 14 years ago

want mechanism for site-specific notification (doorhanger)

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 4.0b1

People

(Reporter: madhava, Assigned: Gavin)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: popupnotification [doorhanger])

Attachments

(2 files, 5 obsolete files)

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).
Assignee: nobody → enndeakin
cc'ing johnath and dolske as this affects their areas
Flags: blocking-firefox3?
Attached patch initial work (obsolete) (deleted) — Splinter Review
Here is a first run at implementing this. There are still some things to do, but feedback and testing welcome.
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.
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.
Severity: normal → enhancement
Summary: New mechanism for site-specific notification (doorhanger) → want mechanism for site-specific notification (doorhanger)
>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"
Severity: enhancement → normal
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?
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.
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [wanted-firefox3]
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3]
How about entering username/password for FTP login and HTTP Basic/Digest auth? It is a very "site-specific notification".
status1.9.1: --- → ?
Flags: wanted-firefox3.6?
Depends on: 509642
Assignee: enndeakin → gavin.sharp
status1.9.1: ? → ---
Flags: wanted-firefox3.6?
Depends on: 554937
Attached patch MattN's WIP patch (obsolete) (deleted) — Splinter Review
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.
Attachment #283758 - Attachment is obsolete: true
Attachment #283758 - Attachment is obsolete: false
Depends on: 565187
Blocks: 567804
Blocks: 567814
Attached patch WIP patch (obsolete) (deleted) — Splinter Review
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.
Attachment #283758 - Attachment is obsolete: true
Attachment #438202 - Attachment is obsolete: true
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.
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()
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.
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.
(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.
Blocks: FF2SM
Attachment #447711 - Flags: feedback?(dolske)
(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...
Attached patch updated WIP (obsolete) (deleted) — Splinter Review
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).
Attachment #447711 - Attachment is obsolete: true
Attachment #448336 - Flags: feedback?(dolske)
Attachment #447711 - Flags: feedback?(dolske)
Attached patch patch, v1 (obsolete) (deleted) — Splinter Review
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.
Attachment #448336 - Attachment is obsolete: true
Attachment #449058 - Flags: review?(dolske)
Attachment #449058 - Flags: review?(dao)
Attachment #448336 - Flags: feedback?(dolske)
No longer blocks: FF2SM
Builds with this patch applied will show up here: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/gsharp@mozilla.com-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).
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.
ScreenShot,please.
(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 :)
Looks fine! But the icon on the doorhanger is somewhat blurry.
Attached image screenshot (deleted) —
(In reply to comment #24) > Looks fine! But the icon on the doorhanger is somewhat blurry. Yeah, that's bug 565187.
(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
(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.
Blocks: 553455
Blocks: 552965
(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 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"?
Attachment #449058 - Flags: review?(dao) → review+
Forgot to add, pinstripe has geolocation-specific styling in notification.css.
(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.
(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 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.
Attachment #449058 - Flags: review?(dolske) → review+
(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.
(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...
(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?
> Hmm, do we really want button-less notifications? yeah, I don't think we have any plans to use those anymore.
(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.
(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.
Attached patch patch v1.1 (deleted) — Splinter Review
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.
Attachment #449058 - Attachment is obsolete: true
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.
Depends on: 573536
(In reply to comment #42) > Nit: You should use resource://gre/modules/ Landed, with that fixed: http://hg.mozilla.org/mozilla-central/rev/a1faf051bbc9
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a6
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?
(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.
Whiteboard: popupnotification
Depends on: 577927
Blocks: 590836
No longer blocks: 590836
Can a hack in password doorhanger fix bug 101664?
Depends on: 598953
Depends on: 599348
Depends on: 599342
Depends on: 599355
Blocks: 599369
Blocks: 599372
No longer blocks: 599372
Depends on: 599372
No longer blocks: 599369
Depends on: 599369
Depends on: 602827
Depends on: 603188
No longer depends on: 603188
No longer depends on: 602827
Depends on: 610130
Depends on: 617304
No longer depends on: 509642
No longer depends on: 610130
Depends on: 615315
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.
Whiteboard: popupnotification → popupnotification [doorhanger]
Status: RESOLVED → VERIFIED
Depends on: 684534
Depends on: 767842
Depends on: 830559
Depends on: 876394
Depends on: 881331
Depends on: 895971
Depends on: 883326
Depends on: 723951
Depends on: 1231025
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: