Closed Bug 198846 Opened 22 years ago Closed 21 years ago

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

Categories

(SeaMonkey :: UI Design, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: Bugzilla2-Briareos, Assigned: mvl)

References

Details

Attachments

(5 files, 2 obsolete files)

User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3) Gecko/20030312 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3) Gecko/20030312 Currently, when you click on the "!" icon in the status bar, you are given the option (via the Popup Manager) to add a whole site to the "Allowed Popups"-whitelist. The problem here is that for most sites, you don't really know whether the suppressed popup was part of the site's navigation or just some annoying ad popup, or (even worse) one of those funny popups that spawn even more popups as fast as they can, thus making the browser unusable. Probably the easiest way to find out what's really going on would be to have the "!" icon open the first suppressed popup instead of the Popup Manager, as you can easily add the site to the whitelist via the context menu in the popup, and just opening a single popup would avoid the nastyness of such "popup bombs; the "!" icon should probably stay in the status bar as long as there are more suppressed popups that could be shown. Reproducible: Always Steps to Reproduce: 1. Visit a site spawning a popup. 2. Notice the "!" icon in the status bar. 3. Scratch your head what to do now. Actual Results: You don't know what kind of popup was suppressed. Expected Results: You should be able to look at a single (or as so many times as you click on the "!" icon, if there's more) popup and base your choice of adding the site to the whitelist or not on that.
OS: Windows 2000 → All
Hardware: PC → All
I haven't found this bug elsewhere so I'm going to go ahead and confirm it. Personally, I think that this should be implemented via a context menu for the popup icon on the status bar. You could right-click and "Allow this popup" - which would display the current popup being blocked, but still not whitelist it. (Future occurances of the specific popup, as well as all others on the site, would keep being blocked.) Of course, "Add to whitelist" should also be part of the context menu. (Another option could be "Show popup details", showing, among other things, the link that's being called. That way you could get information about the popup prior to actually viewing it.) I don't agree, however, that the current left-click behaviour should change.
Status: UNCONFIRMED → NEW
Ever confirmed: true
*** Bug 201631 has been marked as a duplicate of this bug. ***
See bug 199705 comment 1 for a way to get at the URL of the blocked popup window from the front-end event handler.
See also bug 176564, same bug for Firebird.
*** Bug 212201 has been marked as a duplicate of this bug. ***
*** Bug 228755 has been marked as a duplicate of this bug. ***
taking
Assignee: blake → mvl
Attached patch patch v1 (obsolete) — Splinter Review
This patch does two things: add the options string to the popup blocked event, and adds a contect menu to the statusbar icon which allows to open the blocked popups.
Attachment #139814 - Flags: superreview?(jst)
Attachment #139814 - Flags: review?(danm-moz)
Comment on attachment 139814 [details] [diff] [review] patch v1 I like it but for these things: (1) >+function StatusbarPopupShowing(event) { >+ var parent = document.getElementById("showPopupsMenu"); >+ var children = parent.childNodes; >+ for (var i = 0; i < children.length; i++) { >+ parent.removeChild(children[i]); >+ } this has gotta be more efficient as while (parent.firstChild) parent.removeChild(parent.firstChild) (2) >+ <popupset> >+ <popup id="popupBlockerMenu"> >+ <menu label="&popupIconMenu.showpopupstext;"> >+ <menupopup id="showPopupsMenu" oncommand="StatusbarPopupCommand(event.target);" >+ onpopupshowing="return StatusbarPopupShowing(event)" /> >+ </menu> >+ </popup> >+ </popupset> I'd prefer the contextual menu to simply contain a list of popups as first-level menu items. We can always reconsider if someday we want to add more menu items. I'm attaching a png of the double stuf contextual menu and cc:ing the UI owners for a second opinion. Also please keep line lengths under 80 characters. That makes it prettier for those of us who still actually prefer snappy '70s-era character-based editors to toilsome mouse-driven ones. That's it! Pretty minor gripes, really. It's almost there, I think.
Attachment #139945 - Attachment description: double stuf contextual (see comment 9) → double stuf contextual menu (see comment 9)
The reason for the second level menu was that a list of just some urls doesn't tell you what it will do. So if you want a single level menu, there must be some header or anything. The main menu entry was a nice and easy header :)
I see your point. I'd still find it easier to navigate without the extra indirection. Here's hoping for a ruling from one of the UI guys. Another thought, and it's just a thought: instead of Show Blocked Popups -> http://wp.netscape.com/ex/shak/home_popups/popup_03.html?X http://wp.netscape.com/ex/shak/home_popups/gotcha.html perhaps Open #1 wp.netscape.com/.../popup_03.html Open #2 wp.netscape.com/.../gotcha.html
Context menus should show the user a small number of commands that directly relate to the item that spawned the menu for quick access. Submenus significantly slow down the access time and hide items, so they should be avoided (not to mention that they can be rather unpredictably placed - imagine what where the menu would appear if you moved your Mozilla window so that its right edge was near the left of the screen). Also, the items for opening blocked popups should really be duplicated in the Tools > Popup Manager submenu, where they're more discoverable.
If the third parameter of the open() method is known as the "window features" shouldn't the parameter on nsIDOMPopupBlockedEvent and terminology used throughout this patch be known as "features" rather than "options"? As a developer familiar with Mozilla code, I read this patch and was curious initially as to what "options" were... there was no documentation in the interface and the term is a non-standard one as I have said. Oh and the UI is awful, but it's Seamonkey. Surprise surprise.
Comment on attachment 139814 [details] [diff] [review] patch v1 Ben is one of those UI owners we've been wanting to hear from, so there you have it. Not much in the guidance direction, but at least there's disapproval ;) I don't know what Ben has planned for Firebird so from my perspective, this patch is quite workable and I think r=able with comments 9 and 13 addressed. In particular, as Alex mentioned, it's widely considered good practice that context menus not have submenus, and that they be a secondary means of getting to the functionality they represent. So for UI happiness, lose the submenu from the context menu, and, hooboy, find a way to mention the capability in the main menu where it's part of the "now what?" feature set. I suppose I'll suggest, with every expectation of being smacked down, a third-level Tools -> Popup Manager submenu.
Attachment #139814 - Flags: review?(danm-moz) → review-
I don't mind renaming Options to Features, but i got the name from http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.cpp#3122 which comes from http://lxr.mozilla.org/seamonkey/source/dom/public/idl/base/nsIDOMWindowInternal.idl#154 So if other mozilla codes names it Options, shouldn't i use it to, for consistency?
Right, I forgot about that. You borrowed from an ancient parameter name. Most window.open references generally refer to open's third parameter as "features," not "options." And it's generally called "features" throughout the Mozilla codebase. I agree with Ben that you should at least name your nsIDOMPopupBlockedEvent new member variable and parameter "features." If you want, you could optionally also change its name in nsGlobalWindow.* and nsIDOMWindowInternal. That's not a hint to do it; I'm ambivalent on that one.
"throughout the Mozilla codebase": see nsWindowWatcher (which itself has an exception, the internal method WinHasOption).
Attached patch patch v2 (obsolete) — Splinter Review
Updated patch. Renamed Options to Features. Made the contect menu single level, with "Show http://domain/path" duplicated the menu into tools->popup managers, but not as a third level, but below a separator. Screenshots coming. (when you need a real site with popups, you cant find one...)
Attachment #139814 - Attachment is obsolete: true
Attached image screenshot of the menus
The screenshot (obviously modified to show two menus in one screenshot, but you get the idea)
Attachment #139814 - Flags: superreview?(jst)
Attachment #140433 - Flags: review?(danm-moz)
> I suppose I'll suggest, with every expectation of being > smacked down, a third-level Tools -> Popup Manager submenu. Did I mention that more than one level of submenus is bad too?
Comment on attachment 140433 [details] [diff] [review] patch v2 Thanks for the smackage, Alex. I like it. The patch. Two small things: >+function createShowPopupsMenu(parent) { >+ var children = parent.childNodes; You're not using |children| any longer; you can ditch that line. Also please add a comment here saying something about how extensions/cookie/resources/content/cookieNavigatorOverlay.xul ... > <menuitem id="ManagePopups" label="&cookieManagePopups.label;" > accesskey="&cookieManagePopups.accesskey;" > oncommand="OpenManagePopups('');" > hidden="true"/> >+ <menuseparator id="popupMenuSeparator" /> <!-- items are added programmatically; see StatusbarPopupShowing --> in case some poor wretch tries to add more items between you and your separator. It's no bigs but I figure the mixture of hard and soft items deserves a note.
Attachment #140433 - Flags: review?(danm-moz) → review+
Attachment #140433 - Flags: superreview?(jst)
Comment on attachment 140433 [details] [diff] [review] patch v2 I like this second patch. Picking a nit: don't put spaces between " and > in your XUL, i.e. |"...">| and |"..."/>| instead of |"..." >| and |"..." />|, unless the file's style is like that. I must say that "Show http://blabla.com/jksadfh232%25jfaa" in the popup manager submenu doesn't exactly make it clear to me what it's going to show and why it's under that submenu. Perhaps "Show popup http://blabla.com/jksadfh232%25jfaa" or "Show blocked http://blabla.com/jksadfh232%25jfaa"? Then again, that may be a bit too word-y, and maybe most URIs will be recognizable as popup URIs, so perhaps "Show http://blabla.com/jksadfh232%25jfaa" will be okay. >Index: content/events/src/nsDOMEvent.cpp >=================================================================== >RCS file: /cvsroot/mozilla/content/events/src/nsDOMEvent.cpp,v >retrieving revision 1.164 >diff -u -6 -p -d -r1.164 nsDOMEvent.cpp >--- content/events/src/nsDOMEvent.cpp 1 Feb 2004 10:09:00 -0000 1.164 >+++ content/events/src/nsDOMEvent.cpp 2 Feb 2004 21:12:40 -0000 >@@ -1389,12 +1391,23 @@ NS_IMETHODIMP nsDOMEvent::GetPopupWindow > nsPopupBlockedEvent* event = NS_STATIC_CAST(nsPopupBlockedEvent*, mEvent); > *aPopupWindowURI = event->mPopupWindowURI; > NS_IF_ADDREF(*aPopupWindowURI); > return NS_OK; > } > *aPopupWindowURI = 0; >+ return NS_OK; // Don't throw an exception >+} >+ >+/* readonly attribute DOMString popupFeatures; */ >+NS_IMETHODIMP nsDOMEvent::GetPopupWindowFeatures(nsAString &aPopupWindowFeatures) >+{ >+ if (mEvent->eventStructType == NS_POPUPBLOCKED_EVENT) { >+ nsPopupBlockedEvent* event = NS_STATIC_CAST(nsPopupBlockedEvent*, mEvent); >+ aPopupWindowFeatures = event->mPopupWindowFeatures; >+ return NS_OK; >+ } > return NS_OK; // Don't throw an exception > } You only really need one return NS_OK here, remove the one inside the block. Alternatively add a line to clear |aPopupWindowFeatures| before the last return if you want to do like the other code does. >Index: xpfe/browser/resources/content/navigator.js >=================================================================== >RCS file: /cvsroot/mozilla/xpfe/browser/resources/content/navigator.js,v >retrieving revision 1.523 >diff -u -6 -p -d -r1.523 navigator.js >--- xpfe/browser/resources/content/navigator.js 22 Jan 2004 01:47:58 -0000 1.523 >+++ xpfe/browser/resources/content/navigator.js 2 Feb 2004 21:12:41 -0000 >@@ -2172,12 +2178,55 @@ function StatusbarViewPopupManager() { > } > catch(ex) { } > > // open whitelist with site prefilled to unblock > window.openDialog("chrome://communicator/content/popupManager.xul", "", > "chrome,resizable=yes", hostPort); >+} >+ >+function StatusbarPopupShowing(event) { It's not statusbar specific anymore. Perhaps a more generic name? >+// var parent = document.getElementById("popupBlockerMenu"); Remove the above line. >+ var parent = event.target; >+ var browser = getBrowser().selectedBrowser; >+ var separator = document.getElementById("popupMenuSeparator"); Initially I thought the separator couldn't be null here, but it looks like part of the code (XUL, JS) lives in navigator which is always built, and part of the code lives in cookie, which is optionally built. So when I disable the cookie extension I don't get image blocking and popup blocking either? Ugh. Maybe I'm just missing something. I hope. Anyway, if anyone's ever totally bored I'd like to see the cookie dependent code moved out of navigator into the cookie extension. >+function createShowPopupsMenu(parent) { >+ var children = parent.childNodes; You can remove this, like danm said. >+ while (parent.lastChild && parent.lastChild.getAttribute("uri")) >+ parent.removeChild(parent.lastChild); Nice, I was going to suggest lastChild instead of firstChild. Use |hasAttribute| instead of |getAttribute|. >+ var browser = getBrowser().selectedBrowser; >+ >+ for (i = 0; i < browser.popupUrls.length; i++) { Add |var| before |i = 0|. >+ var menuitem = document.createElement("menuitem"); >+ menuitem.setAttribute("label", gNavigatorBundle.getString('popupMenuShow') + >+ " " + browser.popupUrls[i].spec); Big i18n no-no. Use "Show %S" instead in the .properties file and replace getString with formatString. >+ menuitem.setAttribute("uri", browser.popupUrls[i].spec); >+ menuitem.setAttribute("features", browser.popupFeatures[i]); >+ parent.appendChild(menuitem); >+ } >+ >+ return true; >+} >+ >+function StatusbarPopupCommand(target) { >+ var uri = target.getAttribute("uri"); >+ if (uri) { >+ var features = target.getAttribute("features"); >+ window.open(uri, "", features); Just inline the getAttribute here.
Attachment #140433 - Flags: superreview?(jst) → superreview-
Attached patch patch v3Splinter Review
Patch updated to jst's comments. > Anyway, if anyone's ever totally bored I'd like to see the cookie dependent > code moved out of navigator into the cookie extension. I'd rather see it move to its own directory somewhere, or just be intergral part of navigator. But it being part of cookies makes no sense at all. Popup blocker code that is there should move out of it. Too bad it depends on nsIPermissionManager which still lives in cookies..... Anyway, this is not the bug to discuss that :)
Attachment #140434 - Attachment is obsolete: true
Attachment #140695 - Flags: superreview?(jst)
Attachment #140433 - Attachment is obsolete: true
Comment on attachment 140434 [details] screenshot of the menus This image is still valid. I checked the wrong checkbox, and obsoleted this instead of the patch. I suck :(
Attachment #140434 - Attachment is obsolete: false
Comment on attachment 140695 [details] [diff] [review] patch v3 That was jag, not jst :-)
Attachment #140695 - Flags: superreview?(jst) → superreview?(jag-mozbugs)
Attachment #140695 - Flags: superreview?(jag-mozbugs) → superreview?(jag)
Comment on attachment 140695 [details] [diff] [review] patch v3 sr=jag
Attachment #140695 - Flags: superreview?(jag)
Attachment #140695 - Flags: superreview+
Attachment #140695 - Flags: review?(danm-moz)
Attachment #140695 - Flags: review?(danm-moz) → review+
Checked in. Let the popups come back!
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Good to see another feature copied from MultiZilla. Now we can remove our icon and fix some of your errors :-)
Reopening. This bug caused bug 235457.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
*** Bug 238277 has been marked as a duplicate of this bug. ***
(In reply to comment #29) > Good to see another feature copied from MultiZilla. Now we can remove our icon > and fix some of your errors :-) Damnit Neil, I'm pissed. This bug could have been fixed before mozilla 1.7b was released. Crap, just inform the mozilla developers next time, like I asked you to do so, they don't care about MultiZilla, however, security is a serious issue for all of us, with or without MultiZila installed!
*** Bug 240376 has been marked as a duplicate of this bug. ***
Changing summary to make this bug easier to find. pi
Summary: Add a way to manually show a single blocked pop-up window → Add a way to manually show a single blocked pop-up window (unblock using right click)
Depends on: 235457
*** Bug 219663 has been marked as a duplicate of this bug. ***
Could this here have caused the regression in bug 210664. pi
higly unlikely, because this was backed out.
Checked back in again.
Status: REOPENED → RESOLVED
Closed: 21 years ago21 years ago
Resolution: --- → FIXED
This doesn't work for something like: <body onload="document.foo.submit()"> <form name="foo" target="_blank" method="post" action="whatever"> <!-- form elements here --> </form> </body> right? Is there a bug filed on making that work?
just built firefox and dont see any of this functionality at all, right clicking the blocked icon does nothing, double clicking brings up the same old window.
bz: ther is no bug filed that i know of. But you are right, it will fail. And i don't see an easy way of fixing it :( Ewok: that's because this is a seamonkey bug, and a seamonkey patch. For firefox, see bug 176564
mvl, I don't see an obvious easy way of fixing it either, to be truthful... File a bug, cc me and danm. We really need to fix the way new windows are opened for target="_blank" anyway. If we do it right (i.e. if we replace the current "window.open and then load" setup with a "open window and load in one call" setup) we should be able to fix the issue I raised.
I'd like to repeat here what I said on irc... I had problems finding this functionality, and I even knew that it existed. I was looking for it in the "add exception" dialog, it didn't occur to me to right-click the button (I guess that's because statusbar buttons don't usually have contextmenus. yes I know that the offline button has a contextmenu as well). can we add it to the dialog as well, somehow?
And I thought this was one of those stealth features that nobody wanted... Agreed, UI mavens generally consider a context menu to be a shortcut, not the main UI. But what's wrong with the Tools -> Popup Manager submenu?
I feel lost. The functionality is gone, so the bug is no longer fixed. Is there a new bug to add the desired behavior? pi
Actually, there is. Comment 39 says this is checked in again. So, new builds will have it.
I see, but it did not make it into 1.7, right? If so, could this be added to 1.7.1? pi
I've noticed that a pop-up window that is generated with the JavaScript document.write() method doesn't work. Instead it just shows the original document. See the attached testcase for an example. I think my code is valid, and the file works as expected if you allow pop-up from the site. Perhaps this has something to do with what Boris was saying in comment #43?
(In reply to comment #49) > I've noticed that a pop-up window that is generated with the JavaScript > document.write() method doesn't work. Instead it just shows the original > document. I just came across this on http://quote.bloomberg.com, as well. Have you filed/is there a bug for this? I have searched but found none.
Gabe, I've filed bug 259623 regarding the JavaScript bug.
Depends on: 259623
No longer depends on: 259623
Product: Core → Mozilla Application Suite
Component: XP Apps: GUI Features → UI Design
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: