166442 - make popup window blocking UI less hidden

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, enhancement)

Description

[Dan M]

Mozilla's current popup window blocking UI has been criticized as being too
hidden, too unfriendly, too broad. I suggest making popup blocking a function of
the offending website's domain. So, two changes:

1) Add a checkbox in the chrome of popup windows allowing the user to disallow
the website from posing another popup.
2) Add a set of management dialogs exactly like Tools -> Image Manager.

I think I hear someone saying "what no! not another manager!" Well that's my
suggestion. For imagery of item (1) see this bug's first attachment. For imagery
of item (2) drag your mouse up to the Tools menu.

Comment 1

[Dan M]

Attachment: screen shot of initial comment's point (1)

Comment 2

[Dan M]

Attachment: *preliminary* patch implementing initial comment's point (1)

It's hacky in spots but it works.

I'm actually not particularly interested in implementing point (2). It'd be
mostly cribbing from the Image Manager code, so not very difficult. Anyone
interested?

Comment 3

[Dan M]

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

Comment 4

[timeless]

can you add a tooltip to the checkbox so that people can find out what "this
site" is?

Comment 5

[Dan M]

Attachment: implementation of site-by-site popup window management

Then here it is. It's a complete implementation with all the UI and even
timeless' tooltip. It wants (more) testing but I've been running with it and I
claim it works. Barring any bugs, which we know there aren't any of.

Comment 6

[Doron Rosenberg (IBM)]

So will this be on by default, or are you going to use the current popup blocker
pref to turn this one?

I have fooled around on something similar and think that this should be toggleable.

Comment 7

[Dan M]

  The second patch is coded so that the current popup blocking pref is a master
switch -- the user sees nothing of the popup management UI if that pref is true
(that is, if websites are allowed to open popups). With the pref false, websites
by default can still open popups, but the site-by-site blocking UI is activated.
  That's a relatively minor worry, though. If we're interested in taking this
somewhere, that's easily tweaked.

Comment 8

[Dan M]

Attachment: implementation of site-by--site popup window management

I guess I'm serious with this patch. It's much like the last one but with some
improvements and suggestions from timeless and myself.

Comment 9

[Stephen P. Morse]

This is great.  Thanks for doing it Dan.

One comment and one questions:

comment:

  Change the files named nsPopupWindowPermManager.cpp/h to 
     nsPopupWindowManager.cpp/h so as to avoid confusion with the permission
     files that are common to cookies and images (and now to popups).

suggestion:

   Wouldn't you want something similar to pref-cookies.xul for popups?
   That would give the user the ability to accept-all-popups, reject-all-popups,
      or accept pop-ups based on the permissions file.  That would address the
      pop-up blocking pref discussed in comment 7

r=morse for all the patches in extensions/cookies and its subdirectories.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 98000 [details] [diff] [review]
implementation of site-by--site popup window management

Do we really need the new opener URI in nsIXULWindow? I don't see anyone using
that in this diff, and isn't it enough that you can ask the dom window for it's
opener and get the location from it?

And while reading this diff I noticed that nsWindowCreator::GetParentURI()
doesn't set its out param (aURI) in case there's no parent or if anything else
fails.

- In navigator.js:

+function popupCheckboxClick(aCheckbox)
+{
+  try {
+    var xulwin =
window.QueryInterface(Components.interfaces.nsIInterfaceRequestor).getInterface
(Components.interfaces.nsIWebNavigation).QueryInterface(Components.interfaces.n
sIDocShellTreeItem).treeOwner.QueryInterface(Components.interfaces.nsIInterface
Requestor).getInterface(Components.interfaces.nsIXULWindow);

Whoa, dude, isn't that line just a little bit long? :-)

I'm happy to sr most of this, but lets see what other people say about this
diff before I do that.

Comment 11

[Alec Flett]

Hrm. I guess I'm not tremendously happy that the cookie module is getting stuff
added to it in this way - I mean, sure image blocking is already there of
course, and we don't have another obvious facility to do stuff on a per-site
basis, but the more stuff we dump in the cookie module, the more junk embeddors
get that they dont use.

Couldn't we do this somehow with either prefs or rdf?

Comment 12

[Dan M]

  Stephen: I've changed the file names. pref-cookies is the pref panel, right?
Yeah, my patch allows only "reject all" and "reject by site." There's room for a
third option. But in practice I personally don't run into popups at so many
sites that this has bothered me yet. But it might make sense to add a pref
panel; I'd say after this patch has had a chance to bake for a while, if it goes in.

  Johnny: Quite right, I seem to be able to get along without
nsIXULWindow::openerURI. Earlier on during development I thought I ran into a
problem with the normal way but the normal way seems to be working now. Just as
well; openerURI made me feel dirty. I've removed it. Thanks!
  nsWindowCreator::GetParentURI is a private method. Its caller has already
initialized the out parameter and understands that it may not be altered. But
I'll stick in a comment.

  Alec: But what I'm doing is so clearly a minor extension of the cookie
manager. I'd much rather use perfectly functional extant code than invent
something new. There's not a lot of new junk in the package because of this.
It's mostly just adding new strings and cases in switch statements. Alright
there's a whole new manager class, but that's just a wrapper around other extant
cookie code (and could have its implementation substituted out if someday we
wanted to).
  I'm not ready to sign up to rewrite the whole cookie subsystem, and it seems
foolish to not use it.
  By "prefs or rdf" do you mean the between-session storage mechanism? That's
such a small part of this patch. In fact there's zero persistence code in this
patch; it's just taken care of for me. Writing new persistence code would be
just the beginning of abandoning the cookie manager.

Comment 13

[Dan M]

Attachment: implementation of site-by-site popup window management

Much like the previous patch but with corrections from comment 9 and comment 10
and a couple of other serious flaws fixed. The only change made in the cookie
code that Stephen has already reviewed was the comment 9 name changes.

Comment 14

[Alec Flett]

I know, I know. I'm just lookin' out for embeddors. When we finally do rip this
stuff out of the cookie DLL you haven't made it much harder. I just wanted to
express that in case anyone else wanted to speak up and do the work :) (cuz lets
be honest, this stuff belongs in xpfe/)

Comment 15

[Dan M]

Attachment: addition to the previous patch, which had lost the notion that chrome can always do popups

Comment 16

[Dan M]

Attachment: implementation of site-by-site popup window management

This one's just like the previous pair of patches except this one addresses
an oversight. That troublesome "allow popups for this site" checkbox in the
border of popup windows is now live. It needed to be done some time. This patch
also includes some good feedback from timeless.
  C'mon you reviewers (you know who you are)! It just keeps getting longer. On
the other hand, like fine chili, it keeps improving as it sits. Don't wait for
it to start molding, is all.

Comment 17

[jag (Peter Annema)]

Comment on attachment 98253 [details] [diff] [review]
implementation of site-by-site popup window management

>Index: embedding/components/windowwatcher/src/nsWindowWatcher.cpp
>===================================================================

>+              PRInt32 parentType = nsIDocShellTreeItem::typeChrome;
>+              treeItem->GetItemType(&parentType);
>+              if (parentType != nsIDocShellTreeItem::typeContent)
>+                popupConditions = PR_FALSE;

Do we need to worry about ::typeContentWrapped?

>Index: extensions/cookie/resources/content/cookieNavigatorOverlay.xul
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/cookie/resources/content/cookieNavigatorOverlay.xul,v
>retrieving revision 1.2
>diff -u -r1.2 cookieNavigatorOverlay.xul
>--- extensions/cookie/resources/content/cookieNavigatorOverlay.xul	22 Apr 2002 23:30:34 -0000	1.2
>+++ extensions/cookie/resources/content/cookieNavigatorOverlay.xul	7 Sep 2002 04:53:44 -0000
>@@ -88,6 +102,18 @@
>       disableElement.setAttribute("disabled","true");
>       enableElement.removeAttribute("disabled");
> 
>+      blocked =
>+        permissionmanager.testForBlocking(window._content.location, WINDOWPERMISSION);

Because of the nature of JS and the DOM, a webpage could mess with this test by
redefining location. The simplest way around this is to use
|testForBlocking(getBrowser().currentURI.spec, ...)|, or use
|testForBlocking(Components.lookupMethod(content, "location").call(location),
...)|. The latter is the more generic solution for these problems, but in this
case <browser> already provides us with an easy accessor (through
nsIWebNavigation) for the location URL.

>@@ -105,6 +131,13 @@
>       } catch(e) {
>         HideImage();
>       }
>+
>+      try {
>+        if (!pref.getBoolPref("dom.disable_open_during_load"))
>+          HidePopups();
>+      } catch(e) {
>+          HidePopups();
>+      }
>     }

You can safely drop the try/catch around the getter. Since a default value is
provided (in all.js), that call won't throw.

>@@ -136,6 +170,20 @@
>           element = document.getElementById("BlockImages");
>           alert(element.getAttribute("msg"));
>           break;
>+        case "popupAllow":
>+          uri = Components.classes["@mozilla.org/network/standard-url;1"].createInstance(Components.interfaces.nsIURI);
>+          uri.spec = window._content.location;
>+          popupmanager.add(uri, true);

popupmanager.add(getBrowser().currentURI, true);

>+          element = document.getElementById("AllowPopups");
>+          alert(element.getAttribute("msg"));
>+          break;
>+        case "popupBlock":
>+          uri = Components.classes["@mozilla.org/network/standard-url;1"].createInstance(Components.interfaces.nsIURI);
>+          uri.spec = window._content.location;
>+          popupmanager.add(uri, false);

popupmanager.add(getBrowser().currentURI, false);

>+          element = document.getElementById("BlockPopups");
>+          alert(element.getAttribute("msg"));
>+          break;
>         default:
>       }
>     }  
>@@ -180,6 +228,25 @@
>         <menuitem label="&cookieDisplayImagesCmd.label;"
>                   accesskey="&cookieDisplayImagesCmd.accesskey;" 
>                   oncommand="viewImages();"/> 
>+      </menupopup>
>+    </menu>
>+    <menu label="&cookiePopupManager.label;"
>+          accesskey="&cookiePopupManager.accesskey;"
>+          id="popup"
>+          insertbefore="navBeginGlobalItems">
>+      <menupopup>
>+        <menuitem id="BlockPopups" label="&cookieBlockPopupsCmd.label;"
>+                  accesskey="&cookieBlockPopupsCmd.accesskey;" 
>+                  msg="&cookieBlockPopupsMsg.label;"
>+                  oncommand="CookieImageAction('popupBlock');"/> 
>+        <menuitem id="AllowPopups" label="&cookieAllowPopupsCmd.label;"
>+                  accesskey="&cookieAllowPopupsCmd.accesskey;" 
>+                  msg="&cookieAllowPopupsMsg.label;"
>+                  oncommand="CookieImageAction('popupAllow');"/> 
>+        <menuseparator/>
>+        <menuitem label="&cookieDisplayPopupsCmd.label;"
>+                  accesskey="&cookieDisplayPopupsCmd.accesskey;" 
>+                  oncommand="viewPopups();"/> 

Urgh... cookiePopupManager, cookieDisplayPopups, CookieImageAction... This code
screams for some refactoring. At some point in the future.

>       </menupopup>
>     </menu>
>   </menupopup>
>Index: extensions/wallet/cookieviewer/CookieViewer.js
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/wallet/cookieviewer/CookieViewer.js,v
>retrieving revision 1.68
>diff -u -r1.68 CookieViewer.js
>--- extensions/wallet/cookieviewer/CookieViewer.js	14 Aug 2002 00:05:02 -0000	1.68
>+++ extensions/wallet/cookieviewer/CookieViewer.js	7 Sep 2002 04:53:51 -0000
>@@ -463,7 +485,12 @@
> 
> function FinalizePermissionDeletions() {
>   for (var p=0; p<deletedPermissions.length; p++) {
>-    permissionmanager.remove(deletedPermissions[p].host, deletedPermissions[p].type);
>+    if (deletedPermissions[p].type == popupType) {
>+      var uri = Components.classes["@mozilla.org/network/standard-url;1"].createInstance(Components.interfaces.nsIURI);
>+      uri.spec = "http://"+deletedPermissions[p].host; // lost info. assuming...
>+      popupmanager.remove(uri);

This seems bad. You could store the prePath or the full URI on the Permission
object.

>+    } else
>+      permissionmanager.remove(deletedPermissions[p].host, deletedPermissions[p].type);
>   }
>   deletedPermissions.length = 0;
> }


>Index: xpfe/appshell/public/nsIXULWindow.idl
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/appshell/public/nsIXULWindow.idl,v
>retrieving revision 1.10
>diff -u -r1.10 nsIXULWindow.idl
>--- xpfe/appshell/public/nsIXULWindow.idl	6 Nov 2001 01:19:32 -0000	1.10
>+++ xpfe/appshell/public/nsIXULWindow.idl	7 Sep 2002 04:53:56 -0000
>@@ -28,6 +28,7 @@
> 
> interface nsIDocShell;
> interface nsIDocShellTreeItem;
>+interface nsIURI;

It looks like you forgot to remove this.

>@@ -95,6 +96,11 @@
>   const unsigned long highestZ = 9;
> 
>   readonly attribute unsigned long zlevel;
>+
>+  /**
>+   * contextFlags are from nsIWindowCreator2
>+   */
>+           attribute PRUint32  contextFlags;

I seem to recall that types native to IDL are preferred over PR types.

> NS_IMETHODIMP nsXULWindow::SetIntrinsicallySized(PRBool aIntrinsicallySized)
>Index: xpfe/appshell/src/nsXULWindow.h
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/appshell/src/nsXULWindow.h,v
>retrieving revision 1.41
>diff -u -r1.41 nsXULWindow.h
>--- xpfe/appshell/src/nsXULWindow.h	20 Aug 2002 04:34:09 -0000	1.41
>+++ xpfe/appshell/src/nsXULWindow.h	7 Sep 2002 04:54:06 -0000
>@@ -45,6 +45,7 @@
> #include "nsIXULWindow.h"
> #include "nsIPrompt.h"
> #include "nsIAuthPrompt.h"
>+#include "nsIURI.h"

It looks like you forgot to remove this.

>Index: xpfe/browser/resources/content/navigator.js
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/browser/resources/content/navigator.js,v
>retrieving revision 1.472
>diff -u -r1.472 navigator.js
>--- xpfe/browser/resources/content/navigator.js	30 Aug 2002 09:37:15 -0000	1.472
>+++ xpfe/browser/resources/content/navigator.js	7 Sep 2002 04:54:19 -0000
>@@ -67,6 +67,26 @@
> var gFocusedURL = null;
> var gFocusedDocument = null;
> 
>+const gPopupPermListener = {
>+
>+  observe: function(subject, topic, data) {
>+    if (topic != "popup perm change")
>+      return;
>+
>+  if (!window.content || !window.content.opener)
>+    return;
>+
>+    var uri = Components.classes["@mozilla.org/network/standard-url;1"].createInstance(Components.interfaces.nsIURI);
>+    uri.spec = window.content.opener.location.href;
>+    var pm = Components.classes["@mozilla.org/PopupWindowManager;1"]
>+               .getService(Components.interfaces.nsIPopupWindowManager);
>+    if (pm) {

No null check needed, if either classes[] or .getService(...) fail they'll
throw.

>+      var checkbox = document.getElementById("popup-checkbox");
>+      checkbox.checked = pm.test(uri) != Components.interfaces.nsIPopupWindowManager.eDisallow;

pm.test(getBrowser().currentURI)

>@@ -154,6 +174,22 @@
>   }
> }
> 
>+function addPopupPermListener(observer)
>+{
>+  var pm = Components.classes["@mozilla.org/PopupWindowManager;1"]
>+             .getService(Components.interfaces.nsIPopupWindowManager);
>+  if (pm)

No null check needed.

>+    pm.addObserver(observer);
>+}
>+
>+function removePopupPermListener(observer)
>+{
>+  var pm = Components.classes["@mozilla.org/PopupWindowManager;1"]
>+             .getService(Components.interfaces.nsIPopupWindowManager);
>+  if (pm)

No null check needed.

>+    pm.removeObserver(observer);
>+}
>+
> /**
> * We can avoid adding multiple load event listeners and save some time by adding
> * one listener that calls all real handlers.
>@@ -1944,4 +1985,32 @@
>       }
>     }
>   } 
>+}
>+
>+function popupCheckboxClick(aCheckbox)
>+{
>+  if (!window.content || !window.content.opener)
>+    return;
>+
>+  var uri = Components.classes["@mozilla.org/network/standard-url;1"].createInstance(Components.interfaces.nsIURI);
>+  uri.spec = window.content.opener.location.href;
>+
>+  var pm = Components.classes["@mozilla.org/PopupWindowManager;1"]
>+             .getService(Components.interfaces.nsIPopupWindowManager);
>+  if (pm)

No null check needed.

>+    if (aCheckbox.checked)
>+      pm.remove(uri);
>+    else
>+      pm.add(uri, false);
>+}



I'll take a look at the completely new files when I come back.

Comment 18

[Boris Zbarsky [:bzbarsky]]

>+      var uri =
Components.classes["@mozilla.org/network/standard-url;1"].createInstance(Components.interfaces.nsIURI);
>+      uri.spec = "http://"+deletedPermissions[p].host; // lost info. assuming...

And don't do that, in general.  Use the IOService to create a URI from a
spec....  not all uris should be nsStandardURL objects.

Comment 19

[Dan M]

Attachment: implementation of site-by-site popup window management

>+		if (parentType != nsIDocShellTreeItem::typeContent)
>+		  popupConditions = PR_FALSE;

>Do we need to worry about ::typeContentWrapped?

  No (I believe!) I don't think it can happen with a window's main content
docshell but even so, a typeContentWrapper docshell comes from the app, not the
web, so it's trusted.

>+	  permissionmanager.testForBlocking(window._content.location,
WINDOWPERMISSION);

>Because of the nature of JS and the DOM, a webpage could mess with this test
by
>redefining location. The simplest way around this is to use
>|testForBlocking(getBrowser().currentURI.spec, ...)|, or use

  Slick. I didn't even know about that lookupMethod thing.

>+	try {
>+	  if (!pref.getBoolPref("dom.disable_open_during_load"))
>+	    HidePopups();
>+	}

>You can safely drop the try/catch around the getter. Since a default value is
>provided (in all.js), that call won't throw.

  Unless your prefs got trashed. I think I'll keep my paranoia.

>+	var uri =
Components.classes["@mozilla.org/network/standard-url;1"].createInstance(Components.interfaces.nsIURI);

>+	uri.spec = "http://"+deletedPermissions[p].host; // lost info.
assuming...

> This seems bad. You could store the prePath or the full URI on the Permission

> object.

  What we have here is a clash of interfaces. My new popupmanager takes URIs;
the older permissionmanager takes strings. Both are actually talking about
URIs. I wanted to make the new interface more correct, but there's a rough fit
in places.
  It doesn't actually hurt. I can put any scheme in there. Eventually it all
goes back to the permissionmanager, which strips out only the host from
the given URI. (But initializing the URI without a scheme causes an error.)
  Best would be to rewrite the permissionmanager to take the more correct
URI. If that happens, this hack will magically disappear as a consequence.
In the meantime, it's not actually hurting anything because it's thrown away.

>+interface nsIURI;

> It looks like you forgot to remove this.

  oo. Thanks. Damn, you are reading this.

>+	     attribute PRUint32  contextFlags;

> I seem to recall that types native to IDL are preferred over PR types.

  You know I thought so too but Brendan has been telling me the exact opposite
lately, so here we are. He prefers an interface definition to be very specific
about the exact number of bits in an item.

>+    var pm = Components.classes["@mozilla.org/PopupWindowManager;1"]
>+		 .getService(Components.interfaces.nsIPopupWindowManager);
>+    if (pm) {

> No null check needed, if either classes[] or .getService(...) fail they'll
> throw.

  'k.

>+	var checkbox = document.getElementById("popup-checkbox");
>+	checkbox.checked = pm.test(uri) !=
Components.interfaces.nsIPopupWindowManager.eDisallow;

> pm.test(getBrowser().currentURI)

  In this case, not getBrowser but the lookupMethod technique on the opener
window. So should some website through some cross-advertising agreement with
another host open popups from another host, I want the checkbox in the popup to
stop the host that actually opened the popup. Got it, though.

>+function addPopupPermListener(observer)
>+  var pm = Components.classes["@mozilla.org/PopupWindowManager;1"]
>+	       .getService(Components.interfaces.nsIPopupWindowManager);
>+  if (pm)

> No null check needed.

  got it.

>+	var uri =
Components.classes["@mozilla.org/network/standard-url;1"].createInstance(Components.interfaces.nsIURI);


> And don't do that, in general.  Use the IOService to create a URI from a
> spec....  not all uris should be nsStandardURL objects.

done

I've also added xpcom shutdown listeners to the new PopupWindowManager and
the extant PermissionManager objects. This has cleared up a leak of the
PermissionManager object that I caused apparently by rearranging the point
where it was created. (Note the patch to nsObserverList.cpp! Woo hoo!
The fun never stops.)

Comment 20

[jag (Peter Annema)]

>> I seem to recall that types native to IDL are preferred over PR types.
>
>  You know I thought so too but Brendan has been telling me the exact 
> opposite lately, so here we are. He prefers an interface definition 
> to be very specific about the exact number of bits in an item.

That doesn't seem right, but I'll talk to you and brendan about it.

>> pm.test(getBrowser().currentURI)
>
>  In this case, not getBrowser but the lookupMethod technique on the 
> opener window. So should some website through some cross-advertising 
> agreement with another host open popups from another host, I want 
> the checkbox in the popup to stop the host that actually opened the 
> popup. Got it, though.

I must've overlooked the opener part, so yeah, the lookupMethod trick. jband
recently added that specifically for the reason I stated earlier.

Comment 21

[jag (Peter Annema)]

Comment on attachment 98406 [details] [diff] [review]
implementation of site-by-site popup window management

r=jag

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 98406 [details] [diff] [review]
implementation of site-by-site popup window management

Did you make sure that we do *not* throw exceptions when window.open() tries to
open an unwanted popup? If we do, that needs to be fixed. Just return null as
the new window like we do today w/o throwing an exception.

- In nsWindowWatcher.cpp:

+	 // chrome is always allowed, so clear the flag if the opener is chrome
+	 if (popupConditions) {
+	   nsCOMPtr<nsIWebNavigation> webNav(do_GetInterface(aParent));
+	   if (webNav) {
+	     nsCOMPtr<nsIDocShellTreeItem> treeItem(do_GetInterface(webNav));
+	     if (treeItem) {
+	       PRInt32 parentType = nsIDocShellTreeItem::typeChrome;
+	       treeItem->GetItemType(&parentType);
+	       if (parentType != nsIDocShellTreeItem::typeContent)
+		 popupConditions = PR_FALSE;

This is not a fool-proof way of detecting that the opener is chrome, to do that
you should call the SubjectPrincipalIsSystem() method on the security manager,
that'll do the right thing with JS running on a web page, and with JS running
in chrome, and with raw C++ calls to this method.

- In nsObserverList.cpp:

+    if ( weakRefFactory ) {
+      observerRef = getter_AddRefs(NS_STATIC_CAST(nsISupports*,
NS_GetWeakReference(weakRefFactory)));
+	    if(observerRef)
+		    removed = mObserverList->RemoveElement(observerRef);

Tabs!! Clean up this indentation. Oh, and be consistent with the space inside
parens (I know you didn't introduce the spaces in the above if check, but
please clean it up a bit while you're touching this code).

- In nsIXULWindow.idl:

   /**
+   * contextFlags are from nsIWindowCreator2
+   */
+	    attribute PRUint32	contextFlags;

More tabs?

- In nsWindowCreator::AllowWindowCreation():

+  PRUint32 rv;
+  if (NS_SUCCEEDED(pm->TestPermission(aURI, &rv)))
+    return rv;
+  return nsIPopupWindowManager::eAllow;

|rv| really sounds like a nsresult return value to me, which it's not (well, it
kinda is, but not really). You might want to rename that to something else.

- Nit of the day. In nsPopupWindowManager.cpp:

nsresult nsPopupWindowManager::Init()

Put the return type on its own line for consistency with the other methods.

- In nsPopupWindowManager::Add():

  nsCString uri;
  aURI->GetPrePath(uri);

Don't you want uri to be an nsCAutoString here to avoid a malloc? Same thing in
::Remove() too.

Other than that, sr=jst

Comment 23

[Jaime Rodriguez, Jr.]

Nominating as nsbeta1, as these changes will be needed to make the product more
usser-friendly.

Comment 24

[Jason Bassford]

What if I want to block all popups EXCEPT from those sites that belong to a
whitelist?  Currently, you're discussing a) Block all / Allow none, b) Block
none  Allow all, and c) Block site by site.  Where is d) Allow site by site? 
(As with bug 75915 for cookie whitelists - not a dupe, but could be dependent /
related.)  Does nobody use whitelists elsewhere?

(Apologies if this is off topic.)

Comment 25

[jglick]

danm, if you can attach any screenshots of your stuff, that would be great. I 
have some ideas for this feature posted here:

http://www.mozilla.org/mailnews/specs/misc/Popups.html

They go from real basic (just keep what moz currently has) to adding a new pref 
panel for popups.

Comment 26

[Darin Fisher]

so, this broke the ability for embedders to build mozilla without the cookies
module (see myotonic on the tinderbox ports page).  as a rule, extensions are
not allowed to export IDL files.

nsIPopupManagerWindow.idl needs to be moved elsewhere... perhaps into layout??

Comment 27

[Alex Bishop]

I don't want to rain on anyone's parade but isn't the correct hyphenation
'pop-up' and not 'popup'? Also checkboxes should almost always begin with
capitals so "allow popup windows from this site" should be "Allow pop-up windows
from this site".

Comment 28

[Dan M]

Jason, jglick, all: it ain't finished. Or maybe it is. The implementation in
this patch doesn't do everything one can think of, but it's a good basic set.
There's room for expansion. But I think in practice it'll turn out to be all you
want.

We've discovered from our current very simple yes/no interface that lacking some
indication that a popup has been blocked, a whitelist approach is dangerous and
I'd say undesirable. The one (blacklist) choice is simpler and I think
sufficient. It should be quickly tunable, unless you're a popup magnet. I
encourage you to give the current UI a try. How better to know what's wrong with it?

Pluses for the current implementation: popup management is grouped with and
works identically to a bunch of other very similar managers. Now we know that we
can do site-by-site popup blocking. Minuses: the blocking managers' UI could be
better and may still be too simplistic.

You can try it for yourself in tomorrow's builds. Though there seems to be
something wrong with the cute "i'm a popup" checkbox on every platform but
Windows. Sigh. Cross-platform development.

Comment 29

[Antony Mawer]

I'm sitting here pondering at the moment and I'm trying to think of an easy way
you could adapt this to a 'whitelist' type approach instead -- ie. default to
deny, and explicitly allow through only certain sites.

I only mention it because generally I run with popup blocking on, but my online
banking redirects to a new page which pops up the window in the onLoad event,
meaning it gets blocked. It's the only site I want to allow to popups from.

It would be nice as a hidden pref -- ie. set 'block all' in the UI somewhere,
and have something on prefs.js that lists the domains of sites to exclude from
popup blocking. The prefs UI has enough pointless things in there without an
advanced-user level feature like that (you have to know there's a popup being
blocked in order to whitelist it, and your average user isn't going to figure
that out themselves...)

Comment 30

[Alec Flett]

yeah, we need to find a new location for nsIPopupWindowManager.idl and the
contract ID or back this out - it busts the heck out of embedding customers. It
looks like its only in xpfe/bootstrap, so how about xpfe/components/popupmanager
or something? That at least gives us the flexibility to make a new
xpfe-component that does per-site management some time in the future.

Comment 31

[Dan M]

  Antony, Jason: I argue that you only think you want a whitelist. It seems
appealing, but then for some reason sites like, say, www.carsdirect.com don't
work. Which was my entire motivation for implementing site-by-site blocking.
(Why doesn't this link do anything? Oh. Probably Edit > Prefs > Advanced >
Scripts > Popups > OK > try again. Oh yeah.) Yesterday's UI just suppressed
carsdirect's informational windows. Today's UI says "this looks like a popup;
tell me if you want me to suppress these things." Tomorrow's UI could have
whitelist capability and give some indication that popups are being suppressed.
I don't want to give the impression that I'm even trying to preclude that. But
tomorrow's world is another big leap. In tomorrow's world, you need the latter
to really have the former. See the link in comment 25 for ideas on that.
  Tomorrow's UI is certainly more complete and rife with options. But I wonder
whether in practice all that extra complexity is particularly helpful. I'm
thinking most users will quickly tune their blacklist to their common patterns
and it'll all be good. Popups aren't as ubiquitous as cookies. But maybe I'm
wrong. Luckily, I don't have to be right. This is all fixable.

  Alec, Darin: yeah I know. Working on it.

Comment 32

[Ken Snider]

I'm going to take a minute and throw a real-world example at this argument.

*most* people I know browse with Popups off; the main reason for this is the
same as the reason the feature was suggested. There are few onLoad window.open
events that are actually "of use" - most "useful" popups trigger on mouseover /
mouseclick.

Now, Given this fact, it makes sense to spend almost all of your time with
popups off, unless you either like ads, or frequent sites that actually *do*
have important onLoad popups.

I regulary visit one such site - http://www.241pizza.com - for reasons that
still escape me clicking on "online ordering" (and yes, I know, I'm a lazy one)
opens a new page with the express purpose of launching the ordering window in an
"onLoad" event. A minor annoyance - I remember to enable popups before I order
pizza.

However, let's suppose that instead of this one site, a regularly frequent a
poorly designed site that puts information I *really* need in onLoad popups. As
such, I choose the "blacklist" option. Works perfectly because, as you have
suggested, once I've worked in my "normal usage pattern", those sites I
frequently visit that I *don't* need popups on will quickly be a non-issue.

But there's a third scenario - One where someone frequently visits a site they
*want* popups enabled, and spends a lot of time crawling the web instead of
visiting regular websites. Often Research students with poorly designed Campus
websites, as well as support and administration personnell will fall into this
category. There are intranet/customer sites that *need* popups to be working,
but it's likely they'll only visit other sites once or twice, as the result of a
Google search or somesuch.

*These* are the people to which a whitelist makes sense. Otherwise they're in a
bind: They can enable the blacklist and spend most of their time disabling
popups on sites they aren't likely to visit again, or they can disable them
altogether and either use another browser/profile to surf these sites that do
need popups, or spend a LOT of time with their pref window open. :)

Just another way to look at the situation. The importance of the above scenario
is left as an excercise to the reader, of course. :)

Comment 33

[Jason Bassford]

Some comments (a lot of email traffic last night - a good thing, but I'm now
catching up):

> lacking some indication that a popup has been blocked, a whitelist approach is 
> dangerous

Since I've always just blocked everything, this has never been a problem.  (You
could say that my current behaviour is even more dangerous than just accepting
some sites.)  When a site doesn't do something that I think it should then I
turn off the pop-up blocking to see if that's causing the problem.  I won't ever
switch to a "Block this? Yes/No" (blacklist) approach for the same reason I
won't do so where cookies are concerned.  There are simply too many different
sites I visit and the constant clicking is distracting and annoying.  Since
99.9% of the sites I go to I *DO* want blocked, that means that there's only .1%
that I want to allow pop-ups from.  Which means that a whitelist would be far
preferable to me and, I think, to most users who are in the same boat (wanting
everything blocked except a very few specific sites).

(With the inclusion of alerts/prompts now being part of pop-up windows - bug
167599, I'm now in a situation for the first time where a legitimate site that
uses alerts for notifition of new email (Horde/Imp based sites) is being blocked
and my current "deny everything" policy is, for the first time, causing me
problems with a permanent site.  I'm sure others have had similar experiences
with pop-ups in general.)

Also, I explicity *don't* want some kind of "This site is being blocked, is this
okay with you or should we whitelist it?" prompt.  (That's just as bad a
blacklist prompt on every site I haven't yet visited.)  As I mention above, if I
investigate a site that isn't working as I think it should, and discover the
remote possibility that it's because a pop-up is being prevented, then I could
manually do something to allow it.  A whitelist need not be exposed to the user,
but it should still exist in some file or prefs.js entry for those people who
want/need to edit it.

I'll stop talking about this now since I recognize the work that was done here
(excellent for those people who's browsing habits support it) and we should just
get on with the day.  (Whitelists can be treated as a separate enhancement bug.)

Additional comment: Where will the blacklist "live"?  Once you block a site, how
do you "unblock" it if you change your mind?

Comment 34

[Adam D. Moss]

Small issue: the title of the Popup Manager window is 'Image Manager'.

Comment 35

[Jason Bassford]

> Once you block a site, how do you "unblock" it if you change your mind?

Nevermind - stupid question.  RTFM.

However:  How do I disable popups altogether now?  I don't ever want to see a
popup - and I don't want to be asked every time a site tries to set one.  Is
this no longer possible?

Comment 36

[Warner Young]

Yeah, what Jason says in comment 35.  I was disturbed when using today's build
to find that, despite saying no popups, I kept getting popups that asked me if I
wanted them.  Or is this because the prefs panel hasn't been changed to add all
the new options?

Comment 37

[Brendan Eich [:brendan]]

jasonb hit the nail on the head: an incompatible change to the pref that used to
mean "no unrequested windows" where "unrequested" had a clear meaning in code is
a big regression, and should be undone ASAP.

danm, please use another pref, and respect the absolute sense of the existing
one, which lots of people including me count on.

/be

Comment 38

[Jason Bassford]

FWIW: I can see no existing pref that would do the trick.  A wholly new
preference is going to have to be created, similar to cookies, and the Script &
Plugins UI reworked to something like this:

( ) Block all popups
( ) Allow all popups
( ) Allow some popups (default to accept)

I've currently reverted to the 9/10 build, since existing behaviour is not
acceptable.

Comment 39

[jglick]

>FWIW: I can see no existing pref that would do the trick.  A wholly new
>preference is going to have to be created, similar to cookies, and the Script &
>Plugins UI reworked to something like this:

>( ) Block all popups
>( ) Allow all popups
>( ) Allow some popups (default to 
>accept)http://www.mozilla.org/mailnews/specs/misc/Popups.html

http://www.mozilla.org/mailnews/specs/misc/images/Popup3a.gif
http://www.mozilla.org/mailnews/specs/misc/images/Popup3b.gif

Comment 40

[Jason Bassford]

None of those UI examples replicate the blacklist functionality that's currently
present.  In fact, 3b ironically shows a whitelist example.

But regardless of what UI is used, the present implementation is a major
regression.  It should be backed out and only put back in once there's some kind
of UI in place to properly support it.  The ability to reject all or allow all
should only be added to, not replaced.

Comment 41

[Sander]

I have to agree with comment 40 - I specifically downloaded today's build to
take a look at this, and although in principle it's awesome to finally have this
(kudos on the work!), there should still be an overriding popup block possible. 
Also, I was wondering why I don't mind the blacklist-approach for cookies but do
for popups, and I think it's because the cookies question is a _lot_ faster and
less intrusive than the popup-checkbox. - The popup loads completely before you
can block it from then on, while with cookies you block them _before_ they're
actually set.

Finally, an error I noted (at least, I think it's an error, it could be by
design - but is inconsistent with cookie manager behaviour). The popup manager
only shows up in the Tools menu if you have the enable "unrequested windows"
unchecked. - So there's no way to get to the popup manager if you do enable popups.

Comment 42

[Jason Bassford]

According to Dan in comment 12, his patch is supposed to allow only "reject all"
and "reject by site."

I believe what's happened here is an unfortunate 180 in the logic of pref
implementation.  The current patch should be activated when "Open unrequested
windows" is CHECKED not unchecked.  Because currently we're in a situation of
"allow all" and "reject by site" - rather than what Dan originally intended.

Rather than backing this out entirely, the true/false logic on the code checking
the status of the pref needs to be reversed.  Then, when we say to block all,
they all get blocked; when we say to allow all they all get allowed *and*
there's the option to uncheck the box and block some individual sites.

Comment 43

[Dan M]

Christ! Use yesterday's build or a stable build or have some patience or
something. Try backing me out and you own this bug.

Comment 44

[jglick]

<None of those UI examples replicate the blacklist functionality that's 
>currently present.  In fact, 3b ironically shows a whitelist example.

Option 3a has the whitelist only example.

Option 3b and 3c both have the ability to block all pop-ups or allow all 
pop-ups (covers the current on/off setting in JavaScript prefs). 

They both also have a "customize" option, which would allow users to either: 

1) block all and create a whitelist, 
2) accept all and create a blacklist or 
3) an "ask me for each website" option, which would bring up the pop-ups with 
the checkbox on them like danm's current design.

Comment 45

[Brendan Eich [:brendan]]

danm's doing the right thing, so everyone give him a chance to restore the
_status quo ante_ as far as the unrequested windows pref and the meaning of its
two states.  I promise to be nice to him, too.

Discussion of new "third" states or new prefs and their UI should continue, but
maybe it's better done in a newsgroup -- just a thought.

/be

Comment 46

[Dan M]

Alright you wags. The popup window manager is unhooked, though still lurking in
the codebase. This for two reasons: it caused a Txul slowdown (expected, but I
neglected to have done damage control before the fact) and I didn't account for
the dependency that every port and embedded app has on DOM-level control of
popups. If this feature comes back it'll have to subject itself to the
current/old popup control pref, rather than replace it.

Comment 47

[Boris 'pi' Piwinger]

Please also consider changes due to bug 167559 (distinct controls for popup
windows and alerts/prompts wanted).

I guess, this bug and bug 167559 should block, I am not sure which way, though.

pi

Comment 48

[Jason Bassford]

A fix for bug 167559 would make its way into a finer-controlled preference UI,
so this bug couldn't be properly finalized until the other is resolved.

Marking dependency.

Comment 49

[[e-mail]]

Comment on attachment 98406 [details] [diff] [review]
implementation of site-by-site popup window management

>Index: xpfe/browser/resources/content/navigator.xul
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/browser/resources/content/navigator.xul,v
>retrieving revision 1.386
>diff -u -r1.386 navigator.xul
>--- xpfe/browser/resources/content/navigator.xul	11 Aug 2002 12:16:39 -0000	1.386
>+++ xpfe/browser/resources/content/navigator.xul	9 Sep 2002 06:54:23 -0000
>@@ -412,4 +412,9 @@
>     <statusbarpanel class="statusbarpanel-iconic" id="security-button"/>
>   </statusbar> 
> 
>+  <hbox id="popup-control">
>+    <separator class="thin"/>
>+    <checkbox id="popup-checkbox" label="&popup-control.label;" checked="true"
>+              oncommand="popupCheckboxClick(this)"/>
>+  </hbox>
> </window>

A checkbox on its own line? That's a lot of UI space for one feature.
Would you mind turning this into a statusbar panel à la offline?

Comment 50

[Dan M]

neil: then we'd have to show the whole status bar, which would be confusing. see
the screen shot.

Comment 51

[Dan M]

Attachment: pref panel to control popup prefs

Little known fact: (most of) the popup window manager was checked in last
weekend. This preferences panel adds the UI to activate it.

Comment 52

[Dan M]

Attachment: update implementation to use new pref structure

The last patch reorganizes the prefs controlling popup management in
anticipation of the day when we support whitelists. This patch updates the
popup manager to use that pref organization.

Comment 53

[Dan M]

Attachment: add the "i'm a popup" checkbox to the chrome

I split this out (and changed it a bit since its form in attachment 98406 [details] [diff] [review])
because it's problematic. But here it is should we decide to go with it.

Comment 54

[jag (Peter Annema)]

Comment on attachment 99583 [details] [diff] [review]
pref panel to control popup prefs

>Index: extensions/cookie/resources/content/cookiePrefsOverlay.xul
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/cookie/resources/content/cookiePrefsOverlay.xul,v
>retrieving revision 1.6
>diff -u -r1.6 cookiePrefsOverlay.xul
>--- extensions/cookie/resources/content/cookiePrefsOverlay.xul	29 Mar 2002 02:43:29 -0000	1.6
>+++ extensions/cookie/resources/content/cookiePrefsOverlay.xul	17 Sep 2002 22:49:46 -0000
>@@ -40,6 +40,12 @@
>                       label="&images.label;"/> 
>       </treerow>
>     </treeitem>
>+    <treeitem position="3">
>+      <treerow>
>+        <treecell url="chrome://cookie/content/pref-popups.xul"
>+                      label="&popups.label;"/> 

Nit: typically we try to line them up like this:

<element attr1="..." ....
	 attr4="..." ..../>

Also applies to two (or more?) places below.

>  function setButtons(all) {
>    if (all)
>      if (policyButton.value == 1)
>        customCbox.removeAttribute("disabled");
>      else
>        customCbox.setAttribute("disabled", "true");

For managability you should probably wrap the above then block in {}

sr=jag.

Comment 55

[jag (Peter Annema)]

Comment on attachment 99583 [details] [diff] [review]
pref panel to control popup prefs

>  function setButtons(all) {
>    if (all)
>      if (policyButton.value == 1)
>        customCbox.removeAttribute("disabled");
>      else
>        customCbox.setAttribute("disabled", "true");
>    if (policyButton.value == 1 && customCbox.checked)
>      manageButton.removeAttribute("disabled");
>    else
>      manageButton.setAttribute("disabled", "true");
>  }

btw, you should be able to write this as:

  if (all)
    customCheckBox.disabled = policyButton.value != 1;

  manageButton.disabled = policyButton.value != 1 || !customCheckBox.checked;

Comment 56

[jag (Peter Annema)]

Comment on attachment 99584 [details] [diff] [review]
update implementation to use new pref structure

>@@ -228,16 +232,19 @@
>                               const PRUnichar *aData)
> {
>   if (nsCRT::strcmp(aTopic, sPrefChangedTopic) == 0 &&
>-      NS_LITERAL_STRING(POLICYSTRING).Equals(aData)) {
>+        (NS_LITERAL_STRING(POLICYSTRING).Equals(aData) ||
>+         NS_LITERAL_STRING(CUSTOMSTRING).Equals(aData))) {

Nit: indentation looks a bit odd.

>@@ -262,8 +269,10 @@
> 
>   if (NS_SUCCEEDED(rv)) {
>     nsCOMPtr<nsIPrefBranchInternal> ibranch(do_QueryInterface(mPopupPrefBranch));
>-    if (ibranch)
>-      rv = ibranch->AddObserver(sPopupPrefLeaf, this, PR_FALSE);
>+    if (ibranch) {
>+      ibranch->AddObserver(sPopupPrefPolicyLeaf, this, PR_FALSE);
>+      rv = ibranch->AddObserver(sPopupPrefCustomLeaf, this, PR_FALSE);

Inconsistent use of rv. I suggest you just drop the |rv =| there.

sr=jag

Comment 57

[jag (Peter Annema)]

Comment on attachment 99591 [details] [diff] [review]
add the "i'm a popup" checkbox to the chrome

>+// opener may not have been initialized by load time (chrome windows only,
>+// and only some platforms). keep trying. (It's not a good idea to keep trying
>+// on a timeout since the opener may legitimately be null.)
>+function maybeInitPopupCheckbox()
>+{
>+  // already initialized or can't (yet) initialize?
>+  if (gOpenerOrgURI || !window.content || !window.content.opener)
>+    return;

I don't think window.content can be null. jst would know this for sure.

>Index: xpfe/browser/resources/content/navigator.xul
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/browser/resources/content/navigator.xul,v
>retrieving revision 1.388
>diff -u -r1.388 navigator.xul
>--- xpfe/browser/resources/content/navigator.xul	12 Sep 2002 01:16:50 -0000	1.388
>+++ xpfe/browser/resources/content/navigator.xul	17 Sep 2002 23:06:54 -0000
>@@ -53,6 +53,7 @@
>         titlemodifier="&mainWindow.titlemodifier;" 
>         titlemenuseparator="&mainWindow.titlemodifiermenuseparator;"
>         windowtype="navigator:browser"
>+        chromehidden="popupcontrol "
>         width="610" height="450"
>         screenX="4" screenY="4"
>         persist="screenX screenY width height sizemode"> 

I wonder how big a part this plays in slowing down the normal case (all chrome
shown, popupcontrol hidden)...

For CSS rules like:

  window[chromehidden~="toolbar"] #nav-bar-buttons

we used to get away with -- once the window element was found -- seeing that
there was no chromehidden attribute, and thus could move along to the next
rule. With this change we'll now find the attribute and will have to search for
"toolbar" in its value.
If this is indeed the case, perhaps we could inverse this, where we hide it by
default, but if "-popupcontrol" is found we show it?

Let's see what we can do to here to make this cheaper.

Comment 58

[Johnny Stenback (:jst)]

Comment on attachment 99583 [details] [diff] [review]
pref panel to control popup prefs

Nit of the day:

- In pref-popups.xul:

  <radiogroup id="popupPolicy" prefstring="privacy.popups.policy">
    <radio value="1" label="&popupAllow.label;"
	    accesskey="&popupAllow.accesskey;"
	    oncommand="selectPolicy()"/>
      <checkbox id="popupCustom" label="&popupCustom.label;"
		style="margin-left:2em"
		prefstring="privacy.popups.usecustom"
		oncommand="selectCustom(this)"/>

The above element is indented 2 spaces too far.

r=jst

Comment 59

[Johnny Stenback (:jst)]

Comment on attachment 99584 [details] [diff] [review]
update implementation to use new pref structure

r=jst

Comment 60

[Susie Wyshak]

What do you currently have as the default setting for blocking?

What about having pop up blocking disabled but when you first install have a pop
up saying "Do you hate pop ups?" and leading them to the preference panel :-) 

I just really want to make sure users know about this feature and how to control
it so we don't create more evangelism issues in case we inadvertently block
critical popups. 

Comment 61

[Dan M]

The preference panel and preference organisation patches have been checked in.
It's a go, then. That's everything, except the (optional) "i'm a popup" checkbox
which can be added at any time, or not, and ongoing arguments about whether
alerts should be controlled as well.

On to comments about comments about those last two patches:

dudes! indentation?

>Nit: typically we try to line them up like this:
>
><element attr1="..." ....
>         attr4="..." ..../>
>
but not in that file.

> <radio value="1" label="&popupAllow.label;" ...
>   <checkbox id="popupCustom" label="&popupCustom.label;" ...
>
>The above element is indented 2 spaces too far.

And here I thought I was being so clever. The checkbox isn't really part of the
radiogroup, so I wanted to call it out. And besides, the checkbox is indented
visually, in the panel itself, as well.

>For managability you should probably wrap the above then block in {}

extraneous braces are for weenies.

>btw, you should be able to write this as:
>
>  if (all)
>    customCheckBox.disabled = policyButton.value != 1;

wow! That used to not work. How cool that it does now. How long has that been
fixed? Years?

>Inconsistent use of rv. I suggest you just drop the |rv =| there.

If there's a problem with the second PrefBranch, it'll probably show up in both
attempts to AddObserver. I thought that checking both times made it a little
harder to read and was probably redundant. More me trying to be clever.

-- and now for the "i'm a popup" checkbox patch that hasn't been checked in --

>I don't think window.content can be null. jst would know this for sure.

It shouldn't ever happen, at least not in navigator.xul. But I'm still skittish
about removing the check. It's "only" JS, so a failure here would probably only
abort the function anyway. So OK... I can remove that. Skittish.

>+        chromehidden="popupcontrol "
>...
>I wonder how big a part this plays in slowing down the normal case (all chrome
>shown, popupcontrol hidden)...

This *is* how I avoid slowing down the normal case.

>we used to get away with -- once the window element was found -- seeing that
>there was no chromehidden attribute, and thus could move along to the next

I can't find that, but I do see updateToolbarStates(), which seems to be trying
to implement the chromehidden attribute for toolbars and statusbars in js. And
then it clears chromehidden without even checking for anything else! Looks like
an error.

Comment 62

[Dan M]

Susie (comment 60): By default, all popups are allowed. Which clears the way for
a popup popup :).

It's all checked in now, except for the ancillary issues I mentioned at the top
of comment 61. Now that the feature is less screenshot and hand-waving, more
visible and working, this is the point where the floor is wide open to comment
and criticism. Be gentle. I'm still nursing bruises from last time.

Comment 63

[Brendan Eich [:brendan]]

danm, you iconoclast.  Make independent entities at the same level of syntax
line up, for crying out loud.  And brace inner if-else against outer if to avoid
the notorious dangling else problem -- do you want to cause another lost space
probe or AT&T telephone switch crash?  (/me plays fast and loose with comp.risks :-)

/be

Comment 64

[Sander]

I like it - some things I'm noticing: right now I'm seeing both 
- advanced: scripts & plugins: allow scripts to open unrequested windows
and
- privacy & security: popups: allow pop-up windows
Changing the state of the one doesn't change the state of the other. (So I'm
assuming there are different underlying prefs.) How exactly are these meant to
interact? Which pref is stronger?
Also, the popup manager still doesn't appear in the Tools menu unless you're
allowing popups. This isn't consistent with the other managers.

Comment 65

[Dan M]

  Sander: Yes, there are two independent preferences. The Advanced > Scripts
preference is stronger, because it has the right of first refusal. Touching the
Popups preference (the weaker one, in the popups preference panel) will clear
the Advanced > Scripts preference. Should you then go reactivate the Advanced
pref, the Popups prefs will be disconnected and useless.
  We have to keep both preferences functional because the (older, stronger,
simpler) Advanced pref is used by every embedding app as an easy, cheap way to
implement popup blocking. Arguably it makes sense to remove the UI for setting
the Advanced pref from Mozilla, because it's redundant and confusing now.
Despite that I think I can hear howls of protest as I type, so I'm not
volunteering to do that.
  The popup manager is kind of difficult to discover as implemented, yes. But
teasing you with its presence in the Tools menu when it's been deactivated in
the preferences dialog would be confusing. I think it'd be better to ditch the
preferences panel completely, or rather move its UI to the Popup Manager dialog.
Then it would be reasonable to leave the Popup Manager always visible and active
in the Tools menu.
  Ah, but that would be a different bug. Please don't assign it to me. Wouldn't
mind being cc:ed, though.

Comment 66

[Carsten Menke]

Confirming #64 and #65, it is really confusing to have both Advanced -> Scripts
& Plugins and Privacy & Security -> PopUp Management.

I think if the options under Scripts & Plugins needs to reside than move it into
the UI of the Pop Up Manager. So People are not confused to find obvious
duplicate settings, rather then control it all from one point.

[OFFTOPIC]
does the popup appear at http://www.catchthegroove.com when you click on listen?
Anybody ?
For me not, having Scripts & Plugins -> Open Unrequested Windows unchecked.

Comment 67

[Carsten Menke]

Question: 

Wouldn't it be ok to remove the UI entry under Advanced -> Scripts & Plugins ->
Open unrequested Windows

and let Privacy & Security -> Popups -> Reject Popup windows 

do the job ?
I mean with regards to the embeddors, that "Reject Popup Windows" Calls the same
function as "Open unrequested Windows"

BTW, I checked it and it's currently not working for me.

Comment 68

[Dan M]

Attachment: rather than hide it, disable the popup management menu when appropriate

Popup management isn't supposed to be a secretable feature any longer, so
perhaps as Sander mentions (comment 64) it should just be enabled or disabled.
Still not the best UI and not quite according to guidelines on Linux and
Windows but at least this version /works/ on the #*&$% braindead Macintosh.

Comment 69

[Dan M]

Carsten, comments 66 and 67: 
Three options:
1) Put the Advanced preference in the same preference panel as Popups.
2) Remove the UI for the Advanced preference.
3) As is.

I think #1 would be be even more confusing. The popups pref panel would look
something like
------popup pref panel ------
(*) enable popup management
( ) disable popup management
[x] seriously

except less clearly worded. #2 has the drawback that the preference is still
supported and it's nice to have the visual indication of its setting. This is
especially true if somehow some other application (which shares your Mozilla
user profile; an error but it happens) gets in a tug of war over this
preference. That leaves #3; a less than optimal solution but I think the least
of available evils. On the plus side, it is an "Advanced" preference. Not for
twiddling without clues.

And yes, effectively that leaves two separate preference panels which both try
to set the value of that one preference. If the user hits both panels before
hitting OK, the results may be surprising. Sigh.

Comment 70

[Carsten Menke]

Voting for Option 1)

Comment 71

[Stephen P. Morse]

Comment on attachment 100014 [details] [diff] [review]
rather than hide it, disable the popup management menu when appropriate

r=morse

Comment 72

[Dan M]

Attachment: popup window context menu item

Thinking that attachment 99591 [details] [diff] [review], the "i'm a popup" chrome checkbox, is way
convenient but sets a bad precedent, this patch offers a replacement. It adds a
popup-blocking item to the context menu.
  There are a lot of things potentially wrong with the patch. Reviewers ahoy.

Comment 73

[Jason Bassford]

> Thinking that attachment 99591 [details] [diff] [review], the "i'm a popup" chrome checkbox, is way
> convenient but sets a bad precedent

Can you elucidate a bit on why you think it would set a bad example?

At present, I can think of a couple of things to say about the "I'm a popup"
feature - and they're both positive.  It gives a clear indication to casual
users that the window that they're seeing *is* a popup (some people might not
appreciate the distinction, thinking that the new window is simply "part of" the
site), and it also educates them on the existence of the popup manager in the
first place.

Anybody who didn't know about this new functionality would probably not
"experiment" by bringing up a context menu - they'd simply close the window and
be no more enlightened than before...

Comment 74

[rob cherny]

In the interests of usability, I think that blocking anything (images, cookies,
Pop-Ups) on a domain basis should be done THE SAME WAY browser wide.  I vote for
context menu, not a check box. Am I suddenly going to look for a checkbox in the
corner of a banner ad?  Not a great example, but I think you get my meaning.

Comment 75

[Jason Bassford]

> In the interests of usability, I think that blocking anything
> (images, cookies, Pop-Ups) on a domain basis should be done THE SAME WAY
> browser wide.

In that case we have a problem - because cookies currently DO use a checkbox (or
dialog popup if you want).  You can block/unblock from the menu too - but
there's no way (technically) of right-clicking on a "cookie" to block it and
bringing up a context menu.  There simply is no Web page element that represents
a cookie.  (Adding it to the generic Web page context menu wouldn't work either
since it wouldn't take into account specific cookies, or other cases.)

Comment 76

[rob cherny]

Of course.  Fair enough. I guess I was thinking more about the image blocking so
that was sort of a non-thinking blanket statement. Sorry.

Of course there is no page element which represents a cookie.

I know I'm coming into this late and may just be complicating things.

Actually, reading and thinking about this more, I may be about to do a 180
anyways. I'm downloading a build.  Does the checkbox only appear if the pop-up
blocking pref is enabled, or is it there on every pop-up window?

Comment 77

[Dan M]

Jason: for the record, I like the "i'm a popup" checkbox. I run with it in my
own builds. But I think it's not cut out for general release. (1) I worry that
it sets a precedent of taking up precious chrome space to solve a very specific
problem. It would be out of the question if we were talking about general
browser windows. For popups only, it's still kind of sticky, I think. (2) Both
options, checkbox and context menu, only make sense for blacklist management.
Someday we'll have support for whitelists, and it seems most users will opt for
that. Then the checkbox will be an entire row of chrome whose use has fallen out
of fashion. An element with a first-place position in the chrome should be
widely used. Mozilla's chrome is cluttered already; I don't want to be a member
of the uncelebrated group of people who've cluttered the chrome. (3) We
misidentify some windows as popups. Or rather, web programmers for some reason
code windows that open in response to legitimate user actions in ways that make
them appear to be popups. The checkbox is highly visible and in these cases,
inappropriate.

And as rob points out, /image/ blocking has already set a precedent for using a
context menu to solve this problem. The menu is less daring and a little
disappointing, but it does follow precedent.

The main thing is to provide a convenient way for blacklist users to shut down a
site. You know it when you get a popup, whether or not there's a visible
indication. I imagine a user who has taken the trouble to turn on custom popup
blocking could nearly be expected to look in the context menu for a nice
shortcut. Or not. There is the slow way.

I'm not dead set against the checkbox. It's kind of iconoclastic and it makes me
happy. But I expect that I'd regret it later, like a tattoo. If we want to
continue this, maybe we should move to, say, netscape.public.mozilla.ui, and
just present the decision in this bug. Nobody say "make it a preference." Please.

Comment 78

[timeless]

danm: the problem with the context menu is that it can already have 20 items
(not counting seps). If i pop about: into a popup i'd get a 21st, that's way too
many (a bunch of the items are just bugs, but the fact is our context menus are
too long). If people went to a whitelist system, we might not show the checkbox
in popups.

Comment 79

[Jason Bassford]

I still think that a popup window "just appearing" will not give enough
indication of what it is and/or the existence of a function that is not
specifically expressed.  (As with a checkbox.)  So *just* a "hidden" context
menu is not giving enough feedback to users as to the popup manager's existence.

So, without a checkbox, there should be some alternative visual cue informing
the user that what they are seeing is a popup - and indicating to them how they
can now block them if they so desire.

Here are some suggestions:

1. A top level menu item that appears only when what's being viewed is a popup
window.  This could be the same as moving and/or copying "Popup Manager" from
the  existing Tools menu to the right of "QA".

2. A "Block This Popup" text icon that appears on the status bar when a popup is
being displayed.

3. Prepend "Popup Window (Right-click to block) - " to the popup window's title.

I'm sure there are others.  But the point being that in the absence of a
checkbox there needs to be some UI method of communicating to the user that a)
what's being displayed is (at least most likely) a popup window, and b) how they
can block it if they wish.  None of the above 3 methods add to the existing
chrome in any detrimental fashion that I can see.

As for not using context menus - even if we don't, something like one of the
above 3 cues should still be used if checkboxes are not utilized.  Visual
feedback (with instructions if appropriate) is key.  I realise that there is no
such visual feedback with images and image blocking - but popup windows are so
much more obvious, annoying, and popup blocking is such a key feature of
Mozilla, that I think it deserves more attention.

Comment 80

[Dave]

I don't think that "educating users about the existance of popups" is in order here.
If a window appears containing commercial content that I don't wish to view, I
will check the context menu (if that's what's ultimately done) to see if it can
be disabled and, if so, disable it. If users can't tell the difference between
popups and content related to the page they are viewing, then I don't see where
the problem is. A check box somewhere in Edit->Prefs that lets users choose to
suppress popups should be easy enough for anyone to find or stumble upon if
they're looking for, or interested in, this type of functionality.

Besides, if the UI is so obvious that it generates a large amount of attention,
it may prompt advertisers to come up with a way to get around it, probably
making advertisements even more annoying than they already are. I hate intrusive
advertising as much as the next guy, but aggressively publicizing a way to
thwart the 100lb gorilla that is corporate marketing won't earn anything but
trouble.

Just my $0.02

Comment 81

[Carsten Menke]

Me voting for a "Default Deny" Policy, because popus are unlike images and
cookies just annoying.

Further more I have blocked all popup's  and there is only one page I would like
to have popups appear, can you imagine how much work it is to achieve that, with
the current blacklist policy ?

Blocking all but one page is a crazy job.

My 2 EURO

Comment 82

[Ivan Sagalaev]

What about a dialog box (alert) when first popup is displayed? The text may be
like: 'This is a popup window which often contains just advertisement. You may
disable it from appear in future'. And certainly there should be a checkbox
'Don't show this alert anymore'.

Mozilla already does this for alerting user of submitting unencrypted info. I
thinl this is a good example.

Comment 83

[timeless]

dialog boxes are extremely annoying, the only thing i can think of that's worse
than a popup is two popups which is what a dialog preceding a popup would be.

Comment 84

[jag (Peter Annema)]

Please move this discussion over to the appropriate newsgroup(s).

Comment 85

[Jason Bassford]

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

Comment 86

[Adam Lock]

Advert: Moz1.0/NS7.0 users can also use Diggler (http://diggler.mozdev.org/) to
disable/enable popups from a drop down plus some other handy features.

Comment 87

[Dan M]

  We have popup management. Closing this bug fixed!

  Mass responses:
  Pref panel wars (comment 64 - 70 except 68): I agree we have a problem, but I
don't see how any of the suggested solutions is an improvement over the status
quo. I admit I'm favoring clean(er) UI over complete disclosure.
  "i'm a popup" checkbox vs context menu wars (comment 72 - 83 except 81): I
think we're all agreed it could make sense either way. I'm exercising my right
to make it go one way. It's the context menu. Sorry about that, those who prefer
the checkbox. Let's please just run with the context menu for a while and see
how it flies. We can always add the checkbox or something else. For now I feel
I've added enough new things to the UI.
  Whitelist request (comment 24, comment 29 - 33 except 30, comment 81): Clearly
people want this. However the backend doesn't support whitelists (see bug 75915
(cookie and popup window management share the same internals)). Once general
whitelist support is implemented, teaching it to the popup window manager will
be trivial.
  Controlling popup windows separately from popup alerts (comment 47, 48): punt.
Those who want this, effectively you have what you want. The Advanced pref still
controls both, but the Manager doesn't. The Advanced pref is redundant now; if
you don't want alerts stymied, use the Manager in its harshest "no!" setting. I
promise if website author jerks ever force me to teach the Manager to control
alerts too, I'll make it configurable.

  Thanks to everyone for helping me work this thing through, and especially
thanks for the occasional positive feedback. I hope I haven't arbitrarily
ignored too many peoples' opinions and I leave the door open for enhancement
bugs. But for now, we have a feature. Happiness.

Comment 88

[Pascal Chevrel:pascalc]

Hello,

Many web sites I visit display popups I can't block because they are made up of
a flash animation, therefore I can't access the context menu. Shouldn't this be
a reason to use a checkbox instead ? Is there a way to set a pref in prefs.js so
as to have a checkbox ? Or is there a way to access Mozilla context menu when
displaying a flash animation ?

Comment 89

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Another thing that makes context menu less than optimal is that pages can
disable it.

Comment 90

[Dan M]

  Remember the context menu is only a shortcut. Its absence doesn't restrict
your capabilities, it only makes it less convenient. (right click > select)
rather than (switch windows, tools > popup manager > block). However, I keep an
up-to-date patch that I use in my builds. If someone were to open a new bug to
add the checkbox, I'd add the patch and keep an eye on the bug's vote tally. (It
seems disingenuous for me to open the bug myself.)

Comment 91

[Jason Bassford]

Done.  Opened bug 171964 as an enhancement request to add the checkbox.  CCd Dan.

Comment 92

[Adam Lock]

What about pressing the content menu key (assuming you have one)? Alternatively,
open the prefs from another window and disable popups from there.

Failing that, see my advert :)

Comment 93

[Dariusz J. Garbowski]

Regarding to comment #27:

"Pop-up" naming should be consistent. So far we have "pop-up" in Prefs panel,
but Category->Privacy & Security still calls it "popup", so is button in the
panel - "Manage Popup Permissions".

Also wrong are:

- "Popup manager" window title, it's tab title and a text on this tab.

- All menu positions in Tools

Comment 94

[Dan M]

'scuse, I'm feeling snitty today, so I have to gripe about *your* usage:

- "Popup manager" window title, it's tab title and a text on this tab.
grammar is dead                 ^^^^

But anyway quite right, popups should be given a consistent name. Done. Thanks
for reminding me.

Comment 95

[Jeff Leigh]

Popup context menu AND menu options not working in Win2k build 2002100704.
Context menu never shows up and all the menu options under Tools->Popup Managaer
are greyed out.

Comment 96

[Dariusz J. Garbowski]

danm:

Thanks :) I'm not a native English speaker and still have problems with grammar
- thanks for pointing this!

Jeff:

Have you checked "Use custom settings" on Privacy & Security->Pop-ups tab?

Comment 97

[Jeff Leigh]

"Use custom settings" is checked. 

Side note: I also can't seem to get the context menu to work on the OS X build
2002100603. Maybe I'm clicking in the wrong spot, but I've tried clicking on the
window background, pics, links...nothing about popups in the context menu. Am I
doing something wrong here?

Comment 98

[Dan M]

Been talking with Jeff. Popup management menus disabled or hidden is a version
of bug 172586. It happens only after installing (using the installer, not
untarring) on top of an older build. Trash your old build before installing a
new one and this problem should go away.

Comment 99

[Dan M]

(adding to previous comment) except on Macintosh installed builds, where the
missing popup UI is a case of bug 174466.

Comment 100

[Jeff Leigh]

Haven't been able to test on original machine (it's still broken), but popup
manager menu and context menu work fine on new machine with 10/15 build.

Comment 101

[Doron Rosenberg (IBM)]

I just noticed that the current impl. on the trunk is blocking javascript
alert()'s, which will break a ton of sites.  Is this a known issue?

Comment 102

[Sander]

doron: yes, that's known - it's even in the release ntoes for 1.2b - bug 167559
(which apparently also blocks this one)

Comment 103

[Dan M]

.

Comment 104

[Dan M]

Site-by-site popup blocking has once again been backed out of Mozilla. The
feature will no doubt return in future builds. Reassigning to feature owner.

Comment 105

[Lorenzo Colitti]

Hey, what happened? I thought this was working OK.

Of course, a whitelist would have been better, but this was a start....

Comment 106

[Dan M]

  d'oh! always with the whitelist! :)
  There were some regressions and problems (see bug 167559, bug 173016, bug
174775; maybe others) that as a company we proved unequal to the task of fixing
before 1.2 final. It's sad. I think we're too big. But expect popup blocking to
return in 1.3 and expect whitelists.

Comment 107

[Jason Bassford]

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

Comment 108

[Joseph Doer]

Wow! I'm surprised! The PopUp-Manager worked perfect since the first build it
was implemented and I liked/needed it very much!
And I can't agree to let it be the reason in changing it from black to
whitelist. It's all the way better only to block those pages which are using
"bad" popups. Whitch a whitelist the user is not able to "see" which pages uses
usefull popups. And yes, there are a lot of websites which are having such a
"service"!
I hope all of you think about that. :-)
I liked to see the PopUp-Manager like it was until some days ago in the 1.2
release. :-)

Comment 109

[Ken Snider]

I think that's mainly because the number of malicous sites that would misuse
popups far outweighs those that would use them correctly. :)

Also, in most cases, you can tell where a popup was supposed to open but didn't,
which allows you to "whitelist" the site and reload.

Comment 110

[HJ]

Ok, this patch looks clean with some exceptions:

1) this snap looks a bit to complicated:

 41 <!DOCTYPE page [
 42 <!ENTITY % prefPopupsDTD SYSTEM "chrome://cookie/locale/pref-popups.dtd" >
 43 %prefPopupsDTD;
 44 ]>

Why don't you use this line:
<!DOCTYPE page SYSTEM "chrome://cookie/locale/pref-popups.dtd">

2) Inline style code is a 'bad habit' and should not be used ;)
http://lxr.mozilla.org/seamonkey/source/extensions/cookie/resources/content/pref-popups.xul#98

3) some line indentions are wrong, check lines: 95,96,102, and 103

4) you use |selectPolicy()| and |selectCustom(this)| but |viewPopups();|
   I would say, remove the |;| or add two of them :-)

note: why isn't the bug severity set to Enhancement? It is one, right? 

Comment 111

[HJ]

CC'ing self and updating severity...

Comment 112

[Jeff Leigh]

I'm a little confused here. Didn't the site-by-site blocking and the global
blocking use the same code? So alerts would be blocked either way. How does
taking out the site-by-site blocking solve the problem? Now you can have alerts
and all popups or no alerts and no popups, versus choosing which sites can do
alerts and popups.

Comment 113

[Daniel Veditz [:dveditz]]

Alerts, confirms, and prompts are now not blocked regardless of the global
setting; there is no way to block them (there is, however, a bug on putting in
yet another pref for doing so). Has nothing to do with the per-site management
features.

Comment 114

[Daniel Veditz [:dveditz]]

Attachment: restore old UI

removing the 1.2 blocking UI was a review condition for the backend changes I
made in bug 174765 because those changes supposedly broke it, but with a
one-line change the old UI basically works with the new backend. A real
nit-picker might notice subtle behavioral differences, but to the average user
it'll appear to be the same.

We're going to change the UI in 1.3, but if you want the old one back in the
1.2 meantime here it is.

Comment 115

[Michael Lefevre]

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

Comment 116

[Dan M]

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

Comment 117

[Christian Hamacher]

CC -> self

Comment 118

[Matthias Versen [:Matti]]

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

Comment 119

[R.K.Aa.]

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

Comment 120

[R.K.Aa.]

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

Comment 121

[Matthias Versen [:Matti]]

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

Comment 122

[Jeff Leigh]

Any progress on getting the pop-up blocking added back in now that 1.2 is
finally released? I'm still using 1.2 beta just so I don't loose the functionality.

Comment 123

[Christian Franke]

Agree with Jeff (comment 122) and regret upgrading from 1.2b to 1.2:
New proplems appear (Bug 169777), old problems still there
(Bug 158285, Bug 82775).
Popup manager in 1.2b was not perfect but WORKSFORME ;-)

Comment 124

[R.K.Aa.]

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

Comment 125

[Dan M]

Attachment: restore old UI, alternate reading

1.3a looms. So here's another way to restore the old UI to the trunk. Either
version will do. But if you go with the previous "restore old UI" patch, in
pref-popups.xul you should move the domCbox.checked assignment outside the |if
(!domCbox)| clause.
  Personally I prefer this version. It doesn't use the (currently redundant)
popups pref panel at all. This eliminates the confusion between that and the
Advanced -> Scripts -> Open Unrequested Windows preference. This version also
does away with the privacy.popups preferences. I believe we plan to eliminate
these two prefs in the future, but for now they're redundant anyway. I'd like
to see them not part of the legacy prefs space if they're not going to be used.

Comment 126

[Brendan Eich [:brendan]]

Comment on attachment 108058 [details] [diff] [review]
restore old UI, alternate reading

sr=[e-mail] for 1.3a.

dveditz just had a baby daughter, I hear (congrats to mom and dad); maybe jag
can r= in his stead.

/be

Comment 127

[Daniel Veditz [:dveditz]]

Comment on attachment 108058 [details] [diff] [review]
restore old UI, alternate reading

r=dveditz

Comment 128

[Asa Dotzler [:asa]]

Comment on attachment 108058 [details] [diff] [review]
restore old UI, alternate reading

a=asa for checkin to 1.3a

Comment 129

[jag (Peter Annema)]

Comment on attachment 108058 [details] [diff] [review]
restore old UI, alternate reading

r=jag

Comment 130

[Dan M]

"restore old UI, alternate reading" was checked in yesterday evening.

Comment 131

[Gilles Durys]

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

Comment 132

[Jason Bassford]

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

Comment 133

[Daniel Veditz [:dveditz]]

This stuff is in now.

Comment 134

[[e-mail]]

Wouldn't it be good to have a tooltip explanation for the icon when the mouse
hovers over it (like for the two other icons next to it)? 

Comment 135

[Boris Zbarsky [:bzbarsky]]

Yes, it would.  Please file a bug on that... (assign to shliang, I guess.....)

Comment 136

[jglick]

Bug 194072 - No tooltip available for blocked popup icon in status bar.