268590 - Allow multiple messages in browsermessage

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect, P1)

Description

[Marcello Scacchetti]

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
3.

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.

Comment 1

[Gervase Markham [:gerv]]

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:
Firefox:     http://www.mozilla.org/projects/firefox/
Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html
Seamonkey:   http://www.mozilla.org/projects/seamonkey/

Comment 2

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

This does indeed happen. I'd expect this to be a duplicate of an existing bug
report, though.

Comment 3

[Phil Ringnalda (:philor)]

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.

Comment 4

[Neil Deakin]

Attachment: Support multiple notifications

Adds support for multiple sliding notifications.

For more info see http://wiki.mozilla.org/XUL:NotificationBox

Comment 5

[Mike Connor [:mconnor]]

Comment on attachment 217452 [details] [diff] [review]
Support multiple notifications

ptions",
-                             "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)
       this._displayPageReportFirstTime();
 
     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;
     gFindBar.closeFindBar();
   }
   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("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
-                                                "vbox");
-            vbox.setAttribute("flex", "1");
-            vbox.appendChild(this._createMessage("top"));
-            vbox.appendChild(b);
-            vbox.appendChild(this._createMessage("bottom"));
+            var nbox = document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
+                                                "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

Comment 6

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

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.

Comment 7

[Neil Deakin]

Attachment: Address review comments

Comment 8

[Mike Connor [: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!

Comment 9

[Steffen Wilberg]

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.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/locales/en-US/chrome/browser/browser.properties&rev=1.24&mark=76#75

Comment 10

[Neil Deakin]

checked in on 1.8 branch

Comment 11

[Bryan]

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:

http://www.cnn.com/2006/SHOWBIZ/Movies/04/27/film.summerpreview.ap/index.html

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.

Bryan

Comment 12

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

(In reply to comment #11)
> http://www.cnn.com/2006/SHOWBIZ/Movies/04/27/film.summerpreview.ap/index.html
> 
> 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.

Comment 13

[Bryan]

Attachment: Notification bar close icon alignment "fix"

Comment 14

[Bryan]

(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 https://bugzilla.mozilla.org/attachment.cgi?id=220357 ?  I assume I'll need a new bug for this.  I believe there is one but I can't seem to find it.

Bryan

Comment 15

[Boris Zbarsky [:bzbarsky]]

Did this cause bug 336439?

Comment 16

[Tracy Walker [:tracy]]

Verified fixed with Windows, Mac and Linux with 2.0.b1rc3 builds

Comment 17

[Aaron Leventhal]

This widget is not accessible and we're just noticing the problem now. There is no nsIAccessibleProvider implementation.

Comment 18

[Aaron Leventhal]

I stand corrected. We're creating the right accessible but not firing the AlertActive event for the right DOM node. See bug 347913.

Comment 19

[Neil Deakin]

Bug 377693 has notification widget tests