Closed Bug 198846 Opened 21 years ago Closed 20 years ago

Add a way to manually show a single blocked pop-up window (unblock using right click)


(SeaMonkey :: UI Design, enhancement)

Not set


(Not tracked)



(Reporter: Bugzilla2-Briareos, Assigned: mvl)




(5 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3) Gecko/20030312
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3) Gecko/20030312

Currently, when you click on the "!" icon in the status bar, you are given the
option (via the Popup Manager) to add a whole site to the "Allowed

The problem here is that for most sites, you don't really know whether the
suppressed popup was part of the site's navigation or just some annoying ad
popup, or (even worse) one of those funny popups that spawn even more popups as
fast as they can, thus making the browser unusable.

Probably the easiest way to find out what's really going on would be to have the
"!" icon open the first suppressed popup instead of the Popup Manager, as you
can easily add the site to the whitelist via the context menu in the popup, and
just opening a single popup would avoid the nastyness of such "popup bombs; the
"!" icon should probably stay in the status bar as long as there are more
suppressed popups that could be shown.

Reproducible: Always

Steps to Reproduce:
1. Visit a site spawning a popup.
2. Notice the "!" icon in the status bar.
3. Scratch your head what to do now.

Actual Results:  
You don't know what kind of popup was suppressed.

Expected Results:  
You should be able to look at a single (or as so many times as you click on the
"!" icon, if there's more) popup and base your choice of adding the site to the
whitelist or not on that.
OS: Windows 2000 → All
Hardware: PC → All
I haven't found this bug elsewhere so I'm going to go ahead and confirm it.

Personally, I think that this should be implemented via a context menu for the
popup icon on the status bar.  You could right-click and "Allow this popup" -
which would display the current popup being blocked, but still not whitelist it.
 (Future occurances of the specific popup, as well as all others on the site,
would keep being blocked.)  Of course, "Add to whitelist" should also be part of
the context menu.  (Another option could be "Show popup details", showing, among
other things, the link that's being called.  That way you could get information
about the popup prior to actually viewing it.)

I don't agree, however, that the current left-click behaviour should change.
Ever confirmed: true
*** Bug 201631 has been marked as a duplicate of this bug. ***
See bug 199705 comment 1 for a way to get at the URL of the blocked popup window
from the front-end event handler.
See also bug 176564, same bug for Firebird.
*** Bug 212201 has been marked as a duplicate of this bug. ***
*** Bug 228755 has been marked as a duplicate of this bug. ***
Assignee: blake → mvl
Attached patch patch v1 (obsolete) — Splinter Review
This patch does two things: add the options string to the popup blocked event,
and  adds a contect menu to the statusbar icon which allows to open the blocked
Attachment #139814 - Flags: superreview?(jst)
Attachment #139814 - Flags: review?(danm-moz)
Comment on attachment 139814 [details] [diff] [review]
patch v1

I like it but for these things:


>+function StatusbarPopupShowing(event) {
>+  var parent = document.getElementById("showPopupsMenu");
>+  var children = parent.childNodes;
>+  for (var i = 0; i < children.length; i++) {
>+    parent.removeChild(children[i]);
>+  }

this has gotta be more efficient as
while (parent.firstChild)


>+  <popupset>
>+    <popup id="popupBlockerMenu">
>+      <menu label="&popupIconMenu.showpopupstext;">
>+        <menupopup id="showPopupsMenu" oncommand="StatusbarPopupCommand(;"
>+                   onpopupshowing="return StatusbarPopupShowing(event)" />
>+      </menu>
>+    </popup>
>+  </popupset>

I'd prefer the contextual menu to simply contain a list of popups as
first-level menu items. We can always reconsider if someday we want to add more
menu items. I'm attaching a png of the double stuf contextual menu and cc:ing
the UI owners for a second opinion.

Also please keep line lengths under 80 characters. That makes it prettier for
those of us who still actually prefer snappy '70s-era character-based editors
to toilsome mouse-driven ones.

That's it! Pretty minor gripes, really. It's almost there, I think.
Attachment #139945 - Attachment description: double stuf contextual (see comment 9) → double stuf contextual menu (see comment 9)
The reason for the second level menu was that a list of just some urls doesn't
tell you what it will do. So if you want a single level menu, there must be some
header or anything. The main menu entry was a nice and easy header :)
I see your point. I'd still find it easier to navigate without the extra
indirection. Here's hoping for a ruling from one of the UI guys. Another
thought, and it's just a thought: instead of

Show Blocked Popups ->


Open #1
Open #2
Context menus should show the user a small number of commands that directly
relate to the item that spawned the menu for quick access. Submenus
significantly slow down the access time and hide items, so they should be
avoided (not to mention that they can be rather unpredictably placed - imagine
what where the menu would appear if you moved your Mozilla window so that its
right edge was near the left of the screen).

Also, the items for opening blocked popups should really be duplicated in the
Tools > Popup Manager submenu, where they're more discoverable.
If the third parameter of the open() method is known as the "window features"
shouldn't the parameter on nsIDOMPopupBlockedEvent and terminology used
throughout this patch be known as "features" rather than "options"? 

As a developer familiar with Mozilla code, I read this patch and was curious
initially as to what "options" were... there was no documentation in the
interface and the term is a non-standard one as I have said. 

Oh and the UI is awful, but it's Seamonkey. Surprise surprise. 
Comment on attachment 139814 [details] [diff] [review]
patch v1

Ben is one of those UI owners we've been wanting to hear from, so there you
have it. Not much in the guidance direction, but at least there's disapproval

I don't know what Ben has planned for Firebird so from my perspective, this
patch is quite workable and I think r=able with comments 9 and 13 addressed. In
particular, as Alex mentioned, it's widely considered good practice that
context menus not have submenus, and that they be a secondary means of getting
to the functionality they represent.

So for UI happiness, lose the submenu from the context menu, and, hooboy, find
a way to mention the capability in the main menu where it's part of the "now
what?" feature set. I suppose I'll suggest, with every expectation of being
smacked down, a third-level Tools -> Popup Manager submenu.
Attachment #139814 - Flags: review?(danm-moz) → review-
I don't mind renaming Options to Features, but i got the name from
which comes from

So if other mozilla codes names it Options, shouldn't i use it to, for consistency?
Right, I forgot about that. You borrowed from an ancient parameter name. Most references generally refer to open's third parameter as "features,"
not "options." And it's generally called "features" throughout the Mozilla
codebase. I agree with Ben that you should at least name your
nsIDOMPopupBlockedEvent new member variable and parameter "features." If you
want, you could optionally also change its name in nsGlobalWindow.* and
nsIDOMWindowInternal. That's not a hint to do it; I'm ambivalent on that one.
"throughout the Mozilla codebase": see nsWindowWatcher (which itself has an
exception, the internal method WinHasOption).
Attached patch patch v2 (obsolete) — Splinter Review
Updated patch.
Renamed Options to Features.
Made the contect menu single level, with "Show http://domain/path"
duplicated the menu into tools->popup managers, but not as a third level, but
below a separator.
Screenshots coming. (when you need a real site with popups, you cant find
Attachment #139814 - Attachment is obsolete: true
Attached image screenshot of the menus
The screenshot (obviously modified to show two menus in one screenshot, but you
get the idea)
Attachment #139814 - Flags: superreview?(jst)
Attachment #140433 - Flags: review?(danm-moz)
> I suppose I'll suggest, with every expectation of being
> smacked down, a third-level Tools -> Popup Manager submenu.

Did I mention that more than one level of submenus is bad too?
Comment on attachment 140433 [details] [diff] [review]
patch v2

Thanks for the smackage, Alex. I like it. The patch. Two small things:

>+function createShowPopupsMenu(parent) {
>+  var children = parent.childNodes;

You're not using |children| any longer; you can ditch that line.

Also please add a comment here saying something about how

>         <menuitem id="ManagePopups" label="&cookieManagePopups.label;"
>                   accesskey="&cookieManagePopups.accesskey;"
>                   oncommand="OpenManagePopups('');"
>                   hidden="true"/>
>+        <menuseparator id="popupMenuSeparator" />

<!--  items are added programmatically; see StatusbarPopupShowing -->

in case some poor wretch tries to add more items between you and your
separator. It's no bigs but I figure the mixture of hard and soft items
deserves a note.
Attachment #140433 - Flags: review?(danm-moz) → review+
Attachment #140433 - Flags: superreview?(jst)
Comment on attachment 140433 [details] [diff] [review]
patch v2

I like this second patch.

Picking a nit: don't put spaces between " and > in your XUL, i.e. |"...">| and
|"..."/>| instead of |"..." >| and |"..." />|, unless the file's style is like

I must say that "Show" in the popup manager
submenu doesn't exactly make it clear to me what it's going to show and why
it's under that submenu. Perhaps "Show popup" or "Show blocked"? Then again, that may be a bit too word-y,
and maybe most URIs will be recognizable as popup URIs, so perhaps "Show" will be okay.

>Index: content/events/src/nsDOMEvent.cpp
>RCS file: /cvsroot/mozilla/content/events/src/nsDOMEvent.cpp,v
>retrieving revision 1.164
>diff -u -6 -p -d -r1.164 nsDOMEvent.cpp
>--- content/events/src/nsDOMEvent.cpp	1 Feb 2004 10:09:00 -0000	1.164
>+++ content/events/src/nsDOMEvent.cpp	2 Feb 2004 21:12:40 -0000
>@@ -1389,12 +1391,23 @@ NS_IMETHODIMP nsDOMEvent::GetPopupWindow
>     nsPopupBlockedEvent* event = NS_STATIC_CAST(nsPopupBlockedEvent*, mEvent);
>     *aPopupWindowURI = event->mPopupWindowURI;
>     NS_IF_ADDREF(*aPopupWindowURI);
>     return NS_OK;
>   }
>   *aPopupWindowURI = 0;
>+  return NS_OK;  // Don't throw an exception
>+/* readonly attribute DOMString popupFeatures; */
>+NS_IMETHODIMP nsDOMEvent::GetPopupWindowFeatures(nsAString &aPopupWindowFeatures)
>+  if (mEvent->eventStructType == NS_POPUPBLOCKED_EVENT) {
>+    nsPopupBlockedEvent* event = NS_STATIC_CAST(nsPopupBlockedEvent*, mEvent);
>+    aPopupWindowFeatures = event->mPopupWindowFeatures;
>+    return NS_OK;
>+  }
>   return NS_OK;  // Don't throw an exception
> }

You only really need one return NS_OK here, remove the one inside the block.
Alternatively add a line to clear |aPopupWindowFeatures| before the last return
if you want to do like the other code does.

>Index: xpfe/browser/resources/content/navigator.js
>RCS file: /cvsroot/mozilla/xpfe/browser/resources/content/navigator.js,v
>retrieving revision 1.523
>diff -u -6 -p -d -r1.523 navigator.js
>--- xpfe/browser/resources/content/navigator.js	22 Jan 2004 01:47:58 -0000	1.523
>+++ xpfe/browser/resources/content/navigator.js	2 Feb 2004 21:12:41 -0000
>@@ -2172,12 +2178,55 @@ function StatusbarViewPopupManager() {
>   }
>   catch(ex) { }
>   // open whitelist with site prefilled to unblock
>   window.openDialog("chrome://communicator/content/popupManager.xul", "",
>                       "chrome,resizable=yes", hostPort);
>+function StatusbarPopupShowing(event) {

It's not statusbar specific anymore. Perhaps a more generic name?

>+//  var parent = document.getElementById("popupBlockerMenu");

Remove the above line.

>+  var parent =;
>+  var browser = getBrowser().selectedBrowser;      
>+  var separator = document.getElementById("popupMenuSeparator");

Initially I thought the separator couldn't be null here, but it looks like part
of the code (XUL, JS) lives in navigator which is always built, and part of the
code lives in cookie, which is optionally built. So when I disable the cookie
extension I don't get image blocking and popup blocking either? Ugh. Maybe I'm
just missing something. I hope.

Anyway, if anyone's ever totally bored I'd like to see the cookie dependent
code moved out of navigator into the cookie extension.

>+function createShowPopupsMenu(parent) {
>+  var children = parent.childNodes;

You can remove this, like danm said.

>+  while (parent.lastChild && parent.lastChild.getAttribute("uri"))
>+    parent.removeChild(parent.lastChild);

Nice, I was going to suggest lastChild instead of firstChild. Use
|hasAttribute| instead of |getAttribute|.

>+  var browser = getBrowser().selectedBrowser;      
>+  for (i = 0; i < browser.popupUrls.length; i++) {

Add |var| before |i = 0|.

>+    var menuitem = document.createElement("menuitem");
>+    menuitem.setAttribute("label", gNavigatorBundle.getString('popupMenuShow') +
>+                                   " " + browser.popupUrls[i].spec);

Big i18n no-no. Use "Show %S" instead in the .properties file and replace
getString with formatString.

>+    menuitem.setAttribute("uri", browser.popupUrls[i].spec);
>+    menuitem.setAttribute("features", browser.popupFeatures[i]);
>+    parent.appendChild(menuitem);
>+  }
>+  return true;
>+function StatusbarPopupCommand(target) {
>+  var uri = target.getAttribute("uri");
>+  if (uri) {
>+    var features = target.getAttribute("features");
>+, "", features);

Just inline the getAttribute here.
Attachment #140433 - Flags: superreview?(jst) → superreview-
Attached patch patch v3Splinter Review
Patch updated to jst's comments.

> Anyway, if anyone's ever totally bored I'd like to see the cookie dependent
> code moved out of navigator into the cookie extension.

I'd rather see it move to its own directory somewhere, or just be intergral
part of navigator. But it being part of cookies makes no sense at all. Popup
blocker code that is there should move out of it. Too bad it depends on
nsIPermissionManager which still lives in cookies.....
Anyway, this is not the bug to discuss that :)
Attachment #140434 - Attachment is obsolete: true
Attachment #140695 - Flags: superreview?(jst)
Attachment #140433 - Attachment is obsolete: true
Comment on attachment 140434 [details]
screenshot of the menus

This image is still valid. I checked the wrong checkbox, and obsoleted this
instead of the patch. I suck :(
Attachment #140434 - Attachment is obsolete: false
Comment on attachment 140695 [details] [diff] [review]
patch v3

That was jag, not jst :-)
Attachment #140695 - Flags: superreview?(jst) → superreview?(jag-mozbugs)
Attachment #140695 - Flags: superreview?(jag-mozbugs) → superreview?(jag)
Comment on attachment 140695 [details] [diff] [review]
patch v3

Attachment #140695 - Flags: superreview?(jag)
Attachment #140695 - Flags: superreview+
Attachment #140695 - Flags: review?(danm-moz)
Attachment #140695 - Flags: review?(danm-moz) → review+
Checked in. Let the popups come back!
Closed: 21 years ago
Resolution: --- → FIXED
Good to see another feature copied from MultiZilla. Now we can remove our icon
and fix some of your errors :-)
Reopening. This bug caused bug 235457.
Resolution: FIXED → ---
*** Bug 238277 has been marked as a duplicate of this bug. ***
(In reply to comment #29)
> Good to see another feature copied from MultiZilla. Now we can remove our icon
> and fix some of your errors :-)

Damnit Neil, I'm pissed. This bug could have been fixed before mozilla 1.7b was

Crap, just inform the mozilla developers next time, like I asked you to do so,
they don't care about MultiZilla, however, security is a serious issue for all
of us, with or without MultiZila installed!
*** Bug 240376 has been marked as a duplicate of this bug. ***
Changing summary to make this bug easier to find.

Summary: Add a way to manually show a single blocked pop-up window → Add a way to manually show a single blocked pop-up window (unblock using right click)
Depends on: 235457
*** Bug 219663 has been marked as a duplicate of this bug. ***
Could this here have caused the regression in bug 210664.

higly unlikely, because this was backed out.
Checked back in again.
Closed: 21 years ago20 years ago
Resolution: --- → FIXED
This doesn't work for something like:

<body onload="">
  <form name="foo" target="_blank" method="post" action="whatever">
    <!-- form elements here -->

right?  Is there a bug filed on making that work?
just built firefox and dont see any of this functionality at all, right clicking
the blocked icon does nothing, double clicking brings up the same old window.
bz: ther is no bug filed that i know of. But you are right, it will fail. And i
don't see an easy way of fixing it :(

Ewok: that's because this is a seamonkey bug, and a seamonkey patch. For
firefox, see bug 176564
mvl, I don't see an obvious easy way of fixing it either, to be truthful... 
File a bug, cc me and danm.  We really need to fix the way new windows are
opened for target="_blank" anyway.  If we do it right (i.e. if we replace the
current " and then load" setup with a "open window and load in one
call" setup) we should be able to fix the issue I raised.
I'd like to repeat here what I said on irc...
I had problems finding this functionality, and I even knew that it existed. I
was looking for it in the "add exception" dialog, it didn't occur to me to
right-click the button (I guess that's because statusbar buttons don't usually
have contextmenus. yes I know that the offline button has a contextmenu as well).

can we add it to the dialog as well, somehow?
And I thought this was one of those stealth features that nobody wanted...

Agreed, UI mavens generally consider a context menu to be a shortcut, not the
main UI. But what's wrong with the Tools -> Popup Manager submenu?
I feel lost. The functionality is gone, so the bug is no longer fixed. Is there
a new bug to add the desired behavior?

Actually, there is. Comment 39 says this is checked in again. So, new builds
will have it.
I see, but it did not make it into 1.7, right? If so, could this be added to 1.7.1?

I've noticed that a pop-up window that is generated with the JavaScript
document.write() method doesn't work. Instead it just shows the original
document. See the attached testcase for an example. I think my code is valid,
and the file works as expected if you allow pop-up from the site. Perhaps this
has something to do with what Boris was saying in comment #43?
(In reply to comment #49)
> I've noticed that a pop-up window that is generated with the JavaScript
> document.write() method doesn't work. Instead it just shows the original
> document.

I just came across this on, as well.  Have you
filed/is there a bug for this?  I have searched but found none.
Gabe, I've filed bug 259623 regarding the JavaScript bug.
Depends on: 259623
No longer depends on: 259623
Product: Core → Mozilla Application Suite
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.