195924 - popup blocking UI: use whitelisting only (similar to Phoenix)

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[sairuh (rarely reading bugmail)]

popup blocking UI: use whitelisting only (similar to Phoenix). need to change as
follows:

I. Popup Windows pref panel
---------------------------
a. get rid of the "allow popups" radiobutton and associated Exceptions button
and dialog.

b. change "suppress popups" to a checkbox. by default (ie, for new profiles),
this will be OFF (deselected).

c. keep the Exceptions button and dlg associated with "suppress popups". this
will be the whitelist.

II. Tools - Popup Manager menu items
------------------------------------
a. "Allow popups from this site". will bring up whitelist dlg, with the site
prefilled in the Add field.

b. "Suppress popups from this site". will bring up whitelist dlg, with the site
selected (for removal) in the listbox (iff the site is already listed).

c. "Manage popup permissions" will be removed.


i'll spin off separate bugs for upgrade path and help updates.

Comment 1

[shliang]

Attachment: preliminary patch

Comment 2

[Samir Gehani]

Nav triage team: nsbeta1+/adt1

Comment 3

[shliang]

Attachment: preliminary patch w/ some cleanup and info dialog

this patch combines work for this bug, bug 195924 (provide info dialog when
popup first encountered), 195928 (provide path for previous release - we've
decided to ignore old blacklists rather than clearing them out) and bugscape
22023 (should there be menuitems for this in the tools menu - right now when
blocking is off, the menuitems are always disabled as no blacklist means not
allowing blocking of one site at a time).

things seem to work with this patch but it still needs work.

Comment 4

[shliang]

Attachment: patch based on newest version of spec

Comment 5

[shliang]

Attachment: patch

Comment 6

[Gervase Markham [:gerv]]

> I. Popup Windows pref panel

I can't seem to find this; is it in Mozilla?

> a. "Allow popups from this site". will bring up whitelist dlg, with the site
> prefilled in the Add field.
> 
> b. "Suppress popups from this site". will bring up whitelist dlg, with the site
> selected (for removal) in the listbox (iff the site is already listed).

What is the rationale for this change? It makes allowing and suppressing popups,
which might be fairly common operations, a two-click instead of a one-click
operation.

Gerv

Comment 7

[shliang]

Attachment: patch

this patch is for the last test build i did which sarah has ok'ed

Comment 8

[Dan M]

Comment on attachment 117686 [details] [diff] [review]
patch

For the record, the review process has been kind of going on for a while. But I
finally had a chance to look at the whole thing, so here are some review-like
comments despite no review having been requested on this version of the patch:

Index: dom/src/base/nsGlobalWindow.cpp
===================================================================
 // static
 PRBool
 GlobalWindowImpl::CanSetProperty(const char *aPrefName)
@@ -2986,6 +3007,8 @@
   nsCOMPtr<nsIDOMChromeWindow> chrome_win(do_QueryInterface(*_retval));

   if (NS_SUCCEEDED(rv) && !chrome_win) {
+
+    FireFirstPopupEvent(mDocument);
     // A new non-chrome window was created from a call to
     // window.open() from JavaScript, make sure there's a document in
     // the new window. We do this by simply asking the new window for

Here, true enough I think in every situation you care about execution will
enter this (rv && !chrome_win) clause but what you really care about is whether
a popup was just opened, isn't it? I think it's more readable if you rework it
so:

if (NS_SUCCEEDED(rv)) {
  if (!chrome_win) {
    // worry about windows without documents
  }
  FireFirstPopupEvent()
}

2) Another point: you want to do this only after opening a popup window,
not just any window, right? Shouldn't it be

if (NS_SUCCEEDED(rv)) {
  ...
  if (CheckForAbusePoint())
    FirstFirstPopupEvent()
}

Except that even that's not quite right. That assumes CheckForAbusePoint works
as it originally worked (as its comment claims it works); that it simply
determines whether any window that might be opened would be a popup. That's not
quite true anymore: CheckForAbusePoint now determines whether the window should
be opened at all. So to make this work exactly right I think you need to change
this slightly as outlined above and also pull the call to IsPopupBlocked out of
CheckForAbusePoint, calling it instead just after CheckForAbusePoint is called
in Open.

A final point: I wouldn't hold up a review for this, but consider changing the
FirstPopupWindow event to a PopupWindow event. As this patch is written, the
privacy.popups.first_popup preference is shared between files in the dom and in
xpfe. It'd be cleaner and I think not a noticeable performance hit to just
always fire an event when a popup window is opened. Leave it entirely to the
listener, in this case the front end, to decide what to do with it; when to
ignore it. Then this privacy pref can be entirely a browser UI pref, not a
partial dom pref. Another benefit of that more general model is that it would
support some other front end which may want to always know when a popup window
is shown.

Index: extensions/cookie/resources/content/cookieNavigatorOverlay.xul
===================================================================
@@ -103,32 +102,65 @@
       }
     }

+    function viewPopupManager() {
+      window.openDialog("chrome://communicator/content/popupManager.xul", "",
+			 "chrome,resizable=yes", "", false, true);
+    }
+

You ended up not using this function

@@ -173,44 +205,34 @@
     }

     function PopupAction(action) {

PopupAction is gone now too, isn't it?

Index: xpfe/browser/resources/content/navigator.js
===================================================================
@@ -2034,6 +2038,31 @@
   FullScreen.toggle();
 }

+function onFirstPopup(aEvent) {
+  var showDialog = true;
+  var specialList = pref.getComplexValue("privacy.popups.default_whitelist",
+					 
Components.interfaces.nsIPrefLocalizedString).data;
+  if (specialList) {
+    hosts = specialList.split(",");

Is this a marketing directive, that we wait to show the first-popup dialog
until we get a popup from a site that's not on our special protected list? As a
user, I'd find this confusing.

Comment 9

[shliang]

Attachment: patch

Comment 10

[shliang]

Attachment: patch

Comment 11

[jag (Peter Annema)]

Comment on attachment 117913 [details] [diff] [review]
patch

>Index: dom/src/base/nsGlobalWindow.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/dom/src/base/nsGlobalWindow.cpp,v
>retrieving revision 1.578
>diff -u -w -r1.578 nsGlobalWindow.cpp
>--- dom/src/base/nsGlobalWindow.cpp	24 Feb 2003 20:15:54 -0000	1.578
>+++ dom/src/base/nsGlobalWindow.cpp	20 Mar 2003 22:24:33 -0000
>@@ -2837,6 +2837,23 @@
>   }
> }
> 
>+static

You don't need static here. See "The C++ Programming Language, Third Edition",
page 200 (summary: only use |static| inside classes and functions, the above
usage is a carry-over from C and meant "use internal linkage").

>+void FirePopupWindowEvent(nsIDOMDocument* aDoc)

>@@ -3005,6 +3025,10 @@
> 
>     nsCOMPtr<nsIDOMDocument> doc;
>     (*_retval)->GetDocument(getter_AddRefs(doc));
>+    }
>+    
>+    if (CheckForAbusePoint())
>+      FirePopupWindowEvent(mDocument);

You can cache the result from the previous call to CheckForAbusePoint().

>Index: extensions/cookie/resources/content/cookieNavigatorOverlay.xul
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/cookie/resources/content/cookieNavigatorOverlay.xul,v
>retrieving revision 1.12
>diff -u -w -r1.12 cookieNavigatorOverlay.xul
>--- extensions/cookie/resources/content/cookieNavigatorOverlay.xul	21 Jan 2003 07:37:30 -0000	1.12
>+++ extensions/cookie/resources/content/cookieNavigatorOverlay.xul	20 Mar 2003 22:24:33 -0000
>@@ -108,27 +107,29 @@
>       var blocked = false;
>       var policy = pref.getBoolPref("dom.disable_open_during_load");
> 
>+      var aboutManage = document.getElementById("aboutManagePopups");
>+      var bundle = document.getElementById("bundle_cookieNav");
>+      if (policy) {
>+        aboutManage.setAttribute("label", bundle.getString("managePopups"));
>+        aboutManage.setAttribute("accesskey", bundle.getString("managePopupsAccesskey"));
>+        aboutManage.setAttribute("oncommand", "window.openDialog('chrome://communicator/content/popupManager.xul', '', 'chrome,resizable=yes', '', false, true);");
>+      }
>+      else {
>+        aboutManage.setAttribute("label", bundle.getString("aboutPopups"));
>+        aboutManage.setAttribute("accesskey", bundle.getString("aboutPopupsAccesskey"));
>+        aboutManage.setAttribute("oncommand", "window.openDialog('chrome://communicator/content/aboutPopups.xul', '', 'chrome,centerscreen,resizable=yes', false);");
>+      }
>+

Instead of this, wouldn't it be simpler to have two seperate menuitems for
this, one of which will always be hidden? That way I think/hope you can drop
the stringbundle and the new .properties file.

Do any of the uri.hostPort accesses need try/catches?

To do: extensions/cookie/resources/content/pref-popups.xul and down.

Comment 12

[shliang]

Attachment: patch

Comment 13

[jag (Peter Annema)]

Comment on attachment 118260 [details] [diff] [review]
patch

>Index: extensions/cookie/resources/content/pref-popups.xul
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/cookie/resources/content/pref-popups.xul,v
>retrieving revision 1.5
>diff -u -w -r1.5 pref-popups.xul
>--- extensions/cookie/resources/content/pref-popups.xul	7 Mar 2003 02:14:56 -0000	1.5
>+++ extensions/cookie/resources/content/pref-popups.xul	24 Mar 2003 03:16:10 -0000
>@@ -159,17 +173,43 @@
>         var ret = filepicker.show();
>         if (ret == nsIFilePicker.returnOK)
>           gSoundUrlBox.value = filepicker.file.path;
>+        
>+        onSoundInput();
>+      }
>+
>+      function previewSound() {
>+        var soundUrl = gSoundUrlBox.value;
>+        var sound = Components.classes["@mozilla.org/sound;1"]
>+                              .createInstance(Components.interfaces.nsISound);
>+        if (soundUrl.indexOf("file://") == -1) {
>+          sound.playSystemSound(soundUrl);
>+        }
>+        else {
>+          var url = Components.classes["@mozilla.org/network/standard-url;1"]
>+                              .createInstance(Components.interfaces.nsIURL);

Aren't we supposed to get URIs from the IOService?


>+      function onSoundInput()
>+      {
>+        gPreviewSoundButton.disabled = gSoundUrlBox.value == "";
>       }

Everywhere else { follows the function.

>Index: xpfe/browser/resources/content/navigator.js
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/browser/resources/content/navigator.js,v
>retrieving revision 1.488
>diff -u -w -r1.488 navigator.js
>--- xpfe/browser/resources/content/navigator.js	19 Feb 2003 13:25:53 -0000	1.488
>+++ xpfe/browser/resources/content/navigator.js	24 Mar 2003 03:16:11 -0000
>@@ -475,6 +478,7 @@
>   addPrefListener(gHomepagePrefListener);
>   addPopupPermListener(gPopupPermListener);
>   addPrefListener(gPopupPrefListener);
>+  //addPrefListener(gButtonStylePrefListener);
> 
>   window.browserContentListener =
>     new nsBrowserContentListener(window, getBrowser());

Just remove that.

>@@ -1470,7 +1475,7 @@
>   window.focus();
> 
>   // Disable menus which are not appropriate
>-  var disabledItems = ['cmd_close', 'Browser:SendPage', 'Browser:EditPage', 'Browser:PrintSetup', /*'Browser:PrintPreview',*/
>+  var disabledItems = ['cmd_close', 'Browser:SendPage', 'Browser:EditPage', 'cmd_printSetup', /*'Browser:PrintPreview',*/
>                        'Browser:Print', 'canGoBack', 'canGoForward', 'Browser:Home', 'Browser:AddBookmark', 'cmd_undo',
>                        'cmd_redo', 'cmd_cut', 'cmd_copy','cmd_paste', 'cmd_delete', 'cmd_selectAll', 'menu_textZoom'];
>   for (var id in disabledItems) {

That's from another patch. Oh, as is some of the theme stuff I saw earlier.
Please make sure you check this in separately.

>@@ -2034,6 +2039,39 @@
>   FullScreen.toggle();
> }
> 
>+function onPopupWindow(aEvent) {
>+  var firstPopup = pref.getBoolPref("privacy.popups.first_popup");
>+  if (firstPopup) { 
>+    var showDialog = true;
>+    var specialList = "";
>+    try {
>+      specialList = pref.getComplexValue("privacy.popups.default_whitelist",
>+                                         Components.interfaces.nsIPrefLocalizedString).data;
>+    }
>+    catch(ex) { }
>+
>+    if (specialList) {
>+      hosts = specialList.split(",");
>+      var browser = getBrowserForDocument(aEvent.target);
>+      if (!browser)
>+        return;
>+      var currentHost = browser.currentURI.hostPort;
>+      for (var i = 0; i < hosts.length; i++) {
>+        var nextHost = hosts[i];
>+        if (nextHost == currentHost ||
>+            "."+nextHost == currentHost.substr(currentHost.length - (nextHost.length+1))) {
>+          showDialog = false;
>+          break;
>+        }       
>+      }
>+    }
>+    if (showDialog) {
>+      window.openDialog("chrome://communicator/content/aboutPopups.xul", "",
>+                        "chrome,centerscreen,resizable=yes", true);
>+      pref.setBoolPref("privacy.popups.first_popup", false);
>+    }
>+  }
>+}
> 
> function onPopupBlocked(aEvent) {
>   var playSound = pref.getBoolPref("privacy.popups.sound_enabled");


>+function getBrowserForDocument(doc) {  
>     var browsers = getBrowser().browsers;
>     for (var i = 0; i < browsers.length; i++) {
>       if (browsers[i].contentDocument == doc) {
>-        var hostPort = browsers[i].currentURI.hostPort;
>-        browsers[i].popupDomain = hostPort;
>-        if (browsers[i] == getBrowser().selectedBrowser) {
>-          var popupIcon = document.getElementById("popupIcon");
>-          popupIcon.hidden = false;
>+      return browsers[i];
>         }
>+    else {
>+      // check if popup was opened from frame within doc
>+      var frames = browsers[i].contentWindow.frames;    
>+      for (var j = 0; j < frames.length; j++) {  
>+        if (frames[j].document == doc)
>+          return browsers[i];
>       }
>     }
>   }
>+  return null;
> }

So I've talked to danm and a better way to deal with events from iframes is to
fix the dispatching code in nsGlobalWindow to find the top-most content
document and dispatch the event on that instead. Use GetTop to get the
top-level content window, then GetDocument on that to get the document, pass
that to the FireXXXEvent functions. Since this bug fix isn't really part of
this bug, feel free to move this part of the patch, or whatever fix we end up
agreeing on, to a new bug so it won't block you from checking the rest in
tonight.

>Index: xpfe/communicator/resources/content/aboutPopups.xul
>===================================================================

>+<?xml-stylesheet href="chrome://communicator/skin/" type="text/css"?>
>+<?xml-stylesheet href="chrome://navigator/skin/navigator.css" type="text/css"?>

navigator.css can be left off, right?

>+  <script type="application/x-javascript" src="chrome://global/content/strres.js"/>

I don't think you need strres.js, unless one of the other JS files there
depends on it (in that case: bad JS files, no cookie!).


Looking forward to seeing your merged patch + the above comments addresess.

Comment 14

[sairuh (rarely reading bugmail)]

bug 199216 blocks this. :(

Comment 15

[shliang]

Attachment: updated patch

Comment 16

[jag (Peter Annema)]

Comment on attachment 118504 [details] [diff] [review]
updated patch

Could you rename |parent| and |parentDoc| to |topWindow| and |topDoc|?
No need for a new patch.

Comment 17

[shliang]

resolving

Comment 18

[sairuh (rarely reading bugmail)]

vrfy'd fixed with 2003.03.27 comm trunk builds on win2k, mac 10.2.4 and linux
rh8.0. expected UI:

Popup Windows pref panel
------------------------
a. "Block unrequested popup windows" checkbox.
b. associated "Allowed Sites" button for customizing the whitelist.
c. "Play a sound" checkbox (off by default).
d. "Display an icon in the Navigator status bar" checkbox (on by default).

Tools - Popup Manager menu items
--------------------------------
a. Allow Popups From This Site
b. Manage Popups (when blocking is on) / About Popup Blocking (when blocking is off)

specific issues should be filed as new, separate bugs.

Comment 19

[Henning Schnoor]

A question, why has this been changed? I think having both white- and blacklists
makes a lot of sense, we're loosing functionality here. For me, this is a
regression. Any reason for this?

Comment 20

[Peter Lairo]

ditto question in comment #19 (mostly out of curiosity)

Comment 21

[Dan M]

Henning, Peter: I don't much like the new UI either but I think it's an
improvement over the previous one. Smart people found that one confusing, and it
seemed that almost no one was using blacklists anyway. It wouldn't have hurt to
have given a little more warning, though. Sorry about that.

You could file a bug to restore blacklists! I wouldn't count on it to go
anywhere but it might be a good place to collect a little bit of dissent.

Comment 22

[kdunder]

Eww.  Ack.  I can't believe that the blacklist is going away.  Ok, so, wouldn't
a UI to match the cookies UI be better than going to a straight whitelist? (i.e.
wouldn't it be nice if all of the UI's were the same?)

Comment 23

[Adam Egger]

Is there any way to activate the blacklist feature again (e.g. in the user.js)?
What happens to my (very big) blacklist now? Will 1.4a delete it? :-O
Henning, Peter: Did you file a new bug?

Comment 24

[Neil Pryde]

Why remove blacklisting? What is wrong with this, for me working, feature?

Comment 25

[Dan M]

As the code stands your blacklist is still active until the day you visit popup
windows prefs. You may want to go make a backup before you do that. But there's
no UI for maintaining that list. It's unsupported now.

My own experience with the blacklist, and I had built a large one, was that I
really wanted a whitelist. Give it a try, I suggest. I've learned to appreciate
it. I can't defend the new UI in its details but I do feel it's an improvement
and I've heard there are unpublished studies concluding that that's a majority
sentiment.

If you (anyone) have a great idea for an improved UI that supports both options,
I encourage you to file a separate bug with a screenshot or ASCII art or
something. Keep in mind though that this small simplification presages a general
simplification of the entire browser UI, according to
http://mozilla.org/roadmap.html.

Comment 26

[Adam D. Moss]

<qa_ignore>If we're getting opiniony -- I find that the new whitelist's
usability is so good that I weep with awe.  Flawless victory.</qa_ignore>

Comment 27

[Adam Egger]

With which switch can I re-activate the blacklist if I already was in the popup
prefs and where can I find my blacklist? 
The whitelist feature would be usable if I was able to open the blocked popups
by clicking on the blocked-popup-icon. What about this? 

Comment 28

[Heinz König]

what was wrong with the old UI?
What I don't like about the whitelist is that I might lose important pop-up
windows. Mozilla alerts that a popup was blocked, but to tell if it was a
ad-popup or a important popup I have to put the specific page onto the
whitelist, reload the page and remove it from the whitelist again if it was an
unwanted ad. This sucks! I'd rather see some ads (remeber, many free services
can only be offered because of the ad-popup!) that miss a popup with important
information. So pretty please bring back the blacklist.

Comment 29

[Adam D. Moss]

Being able to click the alert icon to have the suppressed pop-up pop-up on the
spot would seem to address most peoples' remaining complaints.  I suggest that
someone who would prefer that behaviour file a new bug specifically for it.

Comment 30

[Michal Kubecek]

As for proposal in comment #21: there is bug 201720. However it was marked
INVALID in four hours so that probably no one noticed...

Comment 31

[Tom Jac]

As for proposal in comment #21: there is bug #202394. However it was marked
INVALID so that probably no one noticed...
Sorry, but I prefer a blacklist, really! I use few sites with blocked popups.
All other popups I want to see! 
-> #201720
"I am only asking to reconsider dismissing a
very useful feature. [...] However there are people who prefer the new way.
What is wrong with letting them choose?"
Is there any way to get the blacklist for popups?

Comment 32

[Dan M]

Previous three comments: see bug 198846.

To run in blacklist mode, use Mozilla 1.3(!) Or on the trunk, if you were using
a blacklist and you've never visited the popups prefs panel since Mozilla 1.4,
you're still using a blacklist. You just can't add new sites to your blacklist
within Mozilla.

To force a blacklist in 1.4 or to add new items to your blacklist, and remember
this is an unsupported hack:

Exit Mozilla. Find your profile. On Win2k it's at <root drive>:/Documents and
Settings/<user>/Application Data/Mozilla/Profiles/<profile>/<randomized
directory name>/. On OSX it's users/<user>/Library/Mozilla/<...>. Linux,
~/.mozilla/<...>. Your crazy platform, I don't know. Edit prefs.js. Does it
contain a line that reads

user_pref("dom.disable_open_during_load", true);

? If so you've already lost your blacklist. Delete that line and save the file.
That gives you a blacklist. (Apologies for the naming of the preference; it's
historical.)

Individual site permissions are stored in a file named cookperm.txt. Hopefully
you still have your large collection of blacklist entries. To add a new domain
to your blacklist, add a new line to this file. It's one line per domain, and
each line reads

<domain>[\t0F][\t1F][\t2F]
e.g.
catholic.org\t2F

I hope you know what I mean. Angle brackets are a semantic entity, anything in
square brackets is optional and the sequence \t means a single tab character. 0
is cookies, 1 is images, 2 means popups and the F means no permission. (So a
whitelist entry would read 2T.)

Remember, when you visit the popup prefs panel your blacklist is put down. And
remember you'll lose your changes if you edit these files while Mozilla is running.

This isn't difficult, but it's a little onerous and you risk damaging your
permissions file if you aren't familiar with what you're doing.

This bug is all fixed and closed off. Bereft of life, it rests in peace. If we
hadn't nailed it to the perch, it'd be pushing up the daisies. Its metabolic
processes are now history. It's off the twig. It's kicked the bucket, shuffled
off its mortal coil, run down the curtain and joined the bleedin' choir
invisible. With apologies to Monty Python. Further discussion would perhaps be
more appropriately moved to a discussion forum such as on
http://www.mozillazine.org/forums/index.php . I couldn't find this topic under
Mozilla Features, so I guess you'd have to start a new one. However it looks
like blacklisting isn't coming back; witness the two attempts to resurrect it
quickly shot down. Will bug 198846 take care of your concerns long-term? Go vote
for it.

Comment 33

[Heinz König]

As far as I could tell from the comments the reason for removing the blacklist
is that a) the UI is confusing and b) almost no one is using the blacklist.

a) I don't know what's so confusing about the UI. Two radiobuttons and two
exception-buttons. Nothing "smart people" (comment#21)  wouldn't understand.

b) how could you tell almost no one uses the blacklist? I'd suggest reopening
bug 201720 and see how many votes / comments it gets.


comment #32 :

Will the backend-popup-blacklist code somewhen removed?
If we don't manage it to bring the blacklist UI back into mozilla will it be
possible to write an xpi addon to bring it back? Is anyone of the other
blacklist fans comfortable with writing addons? Where can I find resources on
how to write an addon?

Comment 34

[Matthias Versen [:Matti]]

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

Comment 35

[Fabrice Bacchella]

I just found that blacklist has been removed has I downloaded 1.4. Whitelist is
useless on popup, I and I think a lot of people need blacklist. Is there a
meeting point there we, all the blacklist fan, can express our point of view and
get this function back in 1.4.1 ?

Comment 36

[Michal Kubecek]

There were few attempts since blacklist removal in 1.4a (bug 201720). However,
those were marked invalid immediately without thinking about them. Thus I don't
think there is a chance to get blacklist back in 1.4 series. However, there is a
chance to get blacklist into Mozilla Browser (bug 202394).