The default bug view has changed. See this FAQ.

Allow multiple messages in browsermessage

VERIFIED FIXED in Firefox 2 alpha2

Status

()

Firefox
Tabbed Browser
P1
normal
VERIFIED FIXED
13 years ago
10 years ago

People

(Reporter: Marcello Scacchetti, Assigned: Neil Deakin)

Tracking

(Blocks: 1 bug, {verified1.8.1})

unspecified
Firefox 2 alpha2
verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox2 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: SWAG: 6d, URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

13 years ago
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.
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/
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.
Status: UNCONFIRMED → NEW
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

Updated

11 years ago
Status: NEW → ASSIGNED
Flags: blocking-firefox2? → blocking-firefox2+
Blocks: 327047

Updated

11 years ago
Priority: -- → P4
Target Milestone: --- → Firefox 2 alpha2
Blocks: 226735
No longer blocks: 327047

Updated

11 years ago
Blocks: 331739

Updated

11 years ago
Priority: P4 → P3
Whiteboard: SWAG: 6d

Updated

11 years ago
Assignee: mconnor → enndeakin
Status: ASSIGNED → NEW
Depends on: 324536
Priority: P3 → P1
(Assignee)

Comment 4

11 years ago
Created attachment 217452 [details] [diff] [review]
Support multiple notifications

Adds support for multiple sliding notifications.

For more info see http://wiki.mozilla.org/XUL:NotificationBox
Attachment #217452 - Flags: review?(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
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.
(Assignee)

Comment 7

11 years ago
Created attachment 218842 [details] [diff] [review]
Address review comments
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+
(Assignee)

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 9

11 years ago
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
(Assignee)

Comment 10

11 years ago
checked in on 1.8 branch

Updated

11 years ago
Keywords: fixed1.8.1

Comment 11

11 years ago
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
(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

11 years ago
Created attachment 220357 [details]
Notification bar close icon alignment "fix"

Comment 14

11 years ago
(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

Depends on: 336439
Did this cause bug 336439?

Updated

11 years ago
Depends on: 340710
Verified fixed with Windows, Mac and Linux with 2.0.b1rc3 builds
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1 → verified1.8.1

Comment 17

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

Comment 18

11 years ago
I stand corrected. We're creating the right accessible but not firing the AlertActive event for the right DOM node. See bug 347913.
(Assignee)

Comment 19

10 years ago
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.