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.

Nav triage team: nsbeta1+/adt1

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.

> 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

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

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

bug 199216 blocks this. :(

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.

resolving

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.

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?

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

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.

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

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?

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

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.

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

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?

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.

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.

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

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?

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.

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?

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

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 ?

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