Closed Bug 268590 Opened 16 years ago Closed 15 years ago

Allow multiple messages in browsermessage


(Firefox :: Tabbed Browser, defect, P1)




Firefox 2 alpha2


(Reporter: marcello.scacchetti, Assigned: enndeakin)


(Blocks 1 open bug, )


(Keywords: verified1.8.1, Whiteboard: SWAG: 6d)


(2 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

If you go to a web page requiring plugin installation and having popups, you
will first see the bar asking to install missing plugin and then appears the bar
about the blocked popup overwriting the plugin installation notification.

Reproducible: Always
Steps to Reproduce:
1. install a clean firefox 1.0 without plugins
2. go to the listed webpage

Actual Results:  
The first time i can only see the popup block notification loosing plugin
installation bar

Expected Results:  
Something like have the plugin installation bar under the popup block bar so if
i press X on popup notification i can still see the plugin installation request,
or add another bar under the plugin installation request so i can see both at
the same time.
This is an automated message, with ID "auto-resolve01".

This bug has had no comments for a long time. Statistically, we have found that
bug reports that have not been confirmed by a second user after three months are
highly unlikely to be the source of a fix to the code.

While your input is very important to us, our resources are limited and so we
are asking for your help in focussing our efforts. If you can still reproduce
this problem in the latest version of the product (see below for how to obtain a
copy) or, for feature requests, if it's not present in the latest version and
you still believe we should implement it, please visit the URL of this bug
(given at the top of this mail) and add a comment to that effect, giving more
reproduction information if you have it.

If it is not a problem any longer, you need take no action. If this bug is not
changed in any way in the next two weeks, it will be automatically resolved.
Thank you for your help in this matter.

The latest beta releases can be obtained from:
This does indeed happen. I'd expect this to be a duplicate of an existing bug
report, though.
Assignee: firefox → nobody
OS: Windows 2000 → All
Hardware: PC → All
I'd expect it to be a duplicate, too, but I can't find another, and mconnor says he wants to fix this early in the 2.0 cycle, so this one will do nicely.
Component: General → Tabbed Browser
Ever confirmed: true
Flags: blocking-aviary2?
QA Contact: general → tabbed.browser
Summary: popup block notification bar overwrite plugin installation bar → Allow multiple messages in browsermessage
Assignee: nobody → mconnor
Flags: blocking-firefox2? → blocking-firefox2+
Blocks: 327047
Priority: -- → P4
Target Milestone: --- → Firefox 2 alpha2
Blocks: 331739
Priority: P4 → P3
Whiteboard: SWAG: 6d
Assignee: mconnor → enndeakin
Depends on: 324536
Priority: P3 → P1
Attached patch Support multiple notifications (obsolete) — Splinter Review
Adds support for multiple sliding notifications.

For more info see
Attachment #217452 - Flags: review?(mconnor)
Comment on attachment 217452 [details] [diff] [review]
Support multiple notifications

-                             "top", true, popupButtonAccesskey);
+        var na = gBrowser.getNotificationBoxForBrowser(gBrowser.selectedBrowser);
+        var notification = na.getItemWithValue("popup-blocked");
+        if (notification) {
+          notification.label = message;
+        }
+        else {
+          var buttons = [{
+            label: popupButtonText,
+            accessKey: popupButtonAccesskey,
+            popup: "blockedPopupOptions",
+            callback: null
+          }];
+          const priority = na.PRIORITY_WARNING_MEDIUM;
+          na.appendItem(message, "popup-blocked", "chrome://browser/skin/Info.png",
+                        priority, buttons);
+        }

can you replace na with notificationBox here?  We're trying to encourage clearer variable names throughout browser and toolkit...

@@ -510,17 +528,17 @@ const gPopupBlockerObserver = {
     // If the info message is showing at the top of the window, and the user has never
     // hidden the message before, show an info box telling the user where the info
     // will be displayed.
     if (showMessage && firstTime)
     gPrefService.setBoolPref("privacy.popups.showBrowserMessage", !showMessage);
-    gBrowser.hideMessage(null, "top");
+    gBrowser.selectedBrowser.notification.removeCurrentMessage();

missed a caller!

         else {
+          // XXXben - use regular software install warnings for now until we can
+          // properly differentiate themes. It's likely in fact that themes won't
+          // be blocked so this code path will only be reached for extensions.
+          notificationName = "xpinstall"
+          messageKey = "xpinstallWarning";
+          buttonKey = "xpinstallWarningButton";
+          buttonAccesskeyKey = "xpinstallWarningButton.accesskey";

so you don't need these set here, just use them in the getString call directly, especially since you're not declaring these vars

+    }
+    else
+      window.openDialog("chrome://browser/content/preferences/permissions.xul",
+                        "_blank", "resizable,dialog=no,centerscreen", params);
+      return false;

nit: indentation on return false

@@ -2433,32 +2451,31 @@ function toggleAffectedChrome(aHide)
   navToolbox.hidden = aHide;
   if (aHide)
     gChromeState = {};
     var sidebar = document.getElementById("sidebar-box");
     gChromeState.sidebarOpen = !sidebar.hidden;
     gSidebarCommand = sidebar.getAttribute("sidebarcommand");
-    var message = gBrowser.getMessageForBrowser(gBrowser.selectedBrowser, "top");
+    var na = gBrowser.getNotificationBoxForBrowser(gBrowser.selectedBrowser);
     gChromeState.messageOpen = !message.hidden;
     message.hidden = aHide;
I'm guessing you mean s/message/na/ for all three lines here?  Call it notificationBox instead here and all following instances
     var statusbar = document.getElementById("status-bar");
     gChromeState.statusbarOpen = !statusbar.hidden;
     statusbar.hidden = aHide;
     var findBar = document.getElementById("FindToolbar");
     gChromeState.findOpen = !findBar.hidden;
   else {
     if (gChromeState.messageOpen) {
-      var message = gBrowser.getMessageForBrowser(gBrowser.selectedBrowser, "top");
-      message.hidden = aHide;
+      gBrowser.getNotificationBoxForBrowser(gBrowser.selectedBrowser).hidden = aHide;

hmm, you're hiding the whole notification box here? that'll hide the <browser/> which is bad.

+function pluginsMissing()
+  // get the urls of missing plugins
+  var tabbrowser = getBrowser();
+  var missingPluginsArray = tabbrowser.mCurrentTab.missingPlugins;
+  if (missingPluginsArray) {
+    window.openDialog("chrome://mozapps/content/plugins/pluginInstallerWizard.xul",
+      "PFSWindow", "modal,chrome,resizable=yes", {plugins: missingPluginsArray, tab: tabbrowser.mCurrentTab});

nit, line up args better, I guess that'll be three lines

             // Add the Message and the Browser to the box
-            var vbox = document.createElementNS("",
-                                                "vbox");
-            vbox.setAttribute("flex", "1");
-            vbox.appendChild(this._createMessage("top"));
-            vbox.appendChild(b);
-            vbox.appendChild(this._createMessage("bottom"));
+            var nbox = document.createElementNS("",
+                                                "notificationbox");
+            nbox.setAttribute("flex", "1");
+            nbox.appendChild(b);
             b.setAttribute("flex", "1");
-            this.mPanelContainer.appendChild(vbox);
+            this.mPanelContainer.appendChild(nbox);

let's call this notificationBox as well (I kept seeing hbox instead of nbox here!)

Ok, as a general note, I think that for the sake of making callers saner, we should do a s/Item/Notification here (i.e. this.allNotifications is much more clear)

die, generic variable and property names, die!

+      <property name="allItems" readonly="true"
+                onget="return this.getElementsByTagName('notification');"/>

you're probably going to need a property to get to the <xul:stack> for the toggleAffectedChrome call in browser.js, so just get childNodes from that?  Should be a lot faster...

+      <method name="appendItem">
+        <parameter name="label"/>
+        <parameter name="value"/>
+        <parameter name="image"/>
+        <parameter name="priority"/>
+        <parameter name="buttons"/>
+        <body>
+          <![CDATA[
+            if (priority < this.PRIORITY_INFO_LOW)
+              priority = this.PRIORITY_INFO_LOW;
+            else if (priority > this.PRIORITY_CRITICAL_BLOCK)
+              priority = this.PRIORITY_CRITICAL_BLOCK;

I'd much rather just throw is someone passes something bogus,

+            var notifications = this.allItems;
+            var insertPos = null;
+            for (var n = notifications.length - 1; n >= 0; n--) {
+              if (notifications[n].priority < priority)
+                break;
+              insertPos = notifications[n];
+            }

so, this means that the first message of a specific priority will be the frontmost, which I think is ok, since if they're equal, let's leave the first one up there instead of extra visual changes.  document this behaviour places.

general nitpicky stuff:
* use the a prefix for params, we tend to do that fairly well in widgets
* need to file a followup on figuring out appropriate priorities for the existing callers
* why is the priority ranging from -4 to 5?  Part of the 10 point scale is that its easier to mentally decide how important something is on a scale of one to ten?  I'm guessing the idea is to do something sane if someone calls this wrong, but I think if you don't pass a priority, it should throw (doing 1-10 would mean that!)
* need to file a second followup for making sure styling is right on major platforms, and colors are what we want

pretty good, overall, I like the overall approach here, but definitely need to see another round
Attachment #217452 - Flags: review?(mconnor) → review-
Maybe getNotificationBoxForBrowser could just be getNotificationBox() with an optional argument for which browser, defaulting to the current browser? Seems like most callers would only care about the current browser.
Attachment #217452 - Attachment is obsolete: true
Attachment #218842 - Flags: review?(mconnor)
Comment on attachment 218842 [details] [diff] [review]
Address review comments

Ok, let's get this on trunk first, and we'll land later this week if everything bakes well.  Thanks!
Attachment #218842 - Flags: review?(mconnor) → review+
Closed: 15 years ago
Resolution: --- → FIXED
Comment on attachment 218842 [details] [diff] [review]
Address review comments

>Index: browser/base/content/browser.js
>@@ -559,92 +574,96 @@ const gXPInstallObserver = {
>-            messageString = browserBundle.getString("xpinstallDisabledMessage");
>+            messageString = browserBundle.getFormattedString("xpinstallDisabledMessage",
>+                                                             [brandShortName, host]);
xpinstallDisabledMessage doesn't contain %S, so this change is at least unnecessary.
checked in on 1.8 branch
Keywords: fixed1.8.1
Browser notifications seem to be "flaky" now.  I don't know if it's related to this patch or not but navigating to this page I get some very weird behaviour:

Click next on the image and the notification bar disappears although I haven't clicked [x] to close it.  Reloading the page doesn't trigger the browser notification bar to appear although it starts to appear (you can actually see it begin to load and then roll close" and I'm pretty sure the pop-up is being pushed.  If I have multiple tabs open and switch between them I can get it to reappear.  Never really saw this before this landed.

(In reply to comment #11)
> Click next on the image and the notification bar disappears although I haven't
> clicked [x] to close it.  Reloading the page doesn't trigger the browser

The target seems a little larger than the icon (on OS X), but not too terribly much larger.
(In reply to comment #12)
> The target seems a little larger than the icon (on OS X), but not too terribly
> much larger.

Mike, what does this mean?

Also, can we get the close button moved a little to the right?  It looks much better when aligned with the scrollbar as seen in the attachment ?  I assume I'll need a new bug for this.  I believe there is one but I can't seem to find it.


Depends on: 336439
Did this cause bug 336439?
Depends on: 340710
Verified fixed with Windows, Mac and Linux with 2.0.b1rc3 builds
This widget is not accessible and we're just noticing the problem now. There is no nsIAccessibleProvider implementation.
I stand corrected. We're creating the right accessible but not firing the AlertActive event for the right DOM node. See bug 347913.
Bug 377693 has notification widget tests
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.