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

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, enhancement)

Description

[Kurt Pruenner]

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
Popups"-whitelist.

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.

Comment 1

[Jason Bassford]

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.

Comment 2

[Jason Bassford]

*** Bug 201631 has been marked as a duplicate of this bug. ***

Comment 3

[Dan M]

See bug 199705 comment 1 for a way to get at the URL of the blocked popup window
from the front-end event handler.

Comment 4

[Jesse Ruderman]

See also bug 176564, same bug for Firebird.

Comment 5

[Jesse Ruderman]

*** Bug 212201 has been marked as a duplicate of this bug. ***

Comment 6

[Alfonso Martinez]

*** Bug 228755 has been marked as a duplicate of this bug. ***

Comment 7

[Michiel van Leeuwen (email: mvl+moz@)]

taking

Comment 8

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v1

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

Comment 9

[Dan M]

Comment on attachment 139814 [details] [diff] [review]
patch v1

I like it but for these things:

(1)

>+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)
  parent.removeChild(parent.firstChild)

(2)

>+  <popupset>
>+    <popup id="popupBlockerMenu">
>+      <menu label="&popupIconMenu.showpopupstext;">
>+        <menupopup id="showPopupsMenu" oncommand="StatusbarPopupCommand(event.target);"
>+                   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.

Comment 10

[Dan M]

Attachment: double stuf contextual menu (see comment 9)

Comment 11

[Michiel van Leeuwen (email: mvl+moz@)]

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 :)

Comment 12

[Dan M]

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 -> http://wp.netscape.com/ex/shak/home_popups/popup_03.html?X
                       http://wp.netscape.com/ex/shak/home_popups/gotcha.html

perhaps

Open #1 wp.netscape.com/.../popup_03.html
Open #2 wp.netscape.com/.../gotcha.html

Comment 13

[Alex Bishop]

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.

Comment 14

[Ben Goodger (use ben at mozilla dot org for email)]

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 15

[Dan M]

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.

Comment 16

[Michiel van Leeuwen (email: mvl+moz@)]

I don't mind renaming Options to Features, but i got the name from
http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.cpp#3122
which comes from
http://lxr.mozilla.org/seamonkey/source/dom/public/idl/base/nsIDOMWindowInternal.idl#154

So if other mozilla codes names it Options, shouldn't i use it to, for consistency?

Comment 17

[Dan M]

Right, I forgot about that. You borrowed from an ancient parameter name. Most
window.open 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.

Comment 18

[Dan M]

"throughout the Mozilla codebase": see nsWindowWatcher (which itself has an
exception, the internal method WinHasOption).

Comment 19

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v2

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
one...)

Comment 20

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: screenshot of the menus

The screenshot (obviously modified to show two menus in one screenshot, but you
get the idea)

Comment 21

[Alex Bishop]

> 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 22

[Dan M]

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

extensions/cookie/resources/content/cookieNavigatorOverlay.xul
...
>         <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.

Comment 23

[jag (Peter Annema)]

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

I must say that "Show http://blabla.com/jksadfh232%25jfaa" 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
http://blabla.com/jksadfh232%25jfaa" or "Show blocked
http://blabla.com/jksadfh232%25jfaa"? Then again, that may be a bit too word-y,
and maybe most URIs will be recognizable as popup URIs, so perhaps "Show
http://blabla.com/jksadfh232%25jfaa" 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 = event.target;
>+  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");
>+    window.open(uri, "", features);

Just inline the getAttribute here.

Comment 24

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v3

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 :)

Comment 25

[Michiel van Leeuwen (email: mvl+moz@)]

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 :(

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 140695 [details] [diff] [review]
patch v3

That was jag, not jst :-)

Comment 27

[jag (Peter Annema)]

Comment on attachment 140695 [details] [diff] [review]
patch v3

sr=jag

Comment 28

[Michiel van Leeuwen (email: mvl+moz@)]

Checked in. Let the popups come back!

Comment 29

[Neil Pryde]

Good to see another feature copied from MultiZilla. Now we can remove our icon
and fix some of your errors :-)

Comment 30

[Johnny Stenback (:jst)]

Reopening. This bug caused bug 235457.

Comment 31

[Johnny Stenback (:jst)]

Attachment: This part of the fix for this bug was backed out.

Comment 32

[Alfonso Martinez]

*** Bug 238277 has been marked as a duplicate of this bug. ***

Comment 33

[HJ]

(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
released.

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!

Comment 34

[Stefan Borggraefe]

*** Bug 240376 has been marked as a duplicate of this bug. ***

Comment 35

[Boris 'pi' Piwinger]

Changing summary to make this bug easier to find.

pi

Comment 36

[Boris 'pi' Piwinger]

*** Bug 219663 has been marked as a duplicate of this bug. ***

Comment 37

[Boris 'pi' Piwinger]

Could this here have caused the regression in bug 210664.

pi

Comment 38

[Michiel van Leeuwen (email: mvl+moz@)]

higly unlikely, because this was backed out.

Comment 39

[Michiel van Leeuwen (email: mvl+moz@)]

Checked back in again.

Comment 40

[Boris Zbarsky [:bzbarsky]]

This doesn't work for something like:

<body onload="document.foo.submit()">
  <form name="foo" target="_blank" method="post" action="whatever">
    <!-- form elements here -->
  </form>
</body>

right?  Is there a bug filed on making that work?

Comment 41

[Ewok]

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.

Comment 42

[Michiel van Leeuwen (email: mvl+moz@)]

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

Comment 43

[Boris Zbarsky [:bzbarsky]]

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 "window.open and then load" setup with a "open window and load in one
call" setup) we should be able to fix the issue I raised.

Comment 44

[Christian :Biesinger (don't email me, ping me on IRC)]

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?

Comment 45

[Dan M]

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?

Comment 46

[Boris 'pi' Piwinger]

I feel lost. The functionality is gone, so the bug is no longer fixed. Is there
a new bug to add the desired behavior?

pi

Comment 47

[Michiel van Leeuwen (email: mvl+moz@)]

Actually, there is. Comment 39 says this is checked in again. So, new builds
will have it.

Comment 48

[Boris 'pi' Piwinger]

I see, but it did not make it into 1.7, right? If so, could this be added to 1.7.1?

pi

Comment 49

[Rick Bull]

Attachment: JavaScript-generated-document testcase

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?

Comment 50

[Gabe Jones]

(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 http://quote.bloomberg.com, as well.  Have you
filed/is there a bug for this?  I have searched but found none.

Comment 51

[Rick Bull]

Gabe, I've filed bug 259623 regarding the JavaScript bug.