Closed Bug 107949 Opened 23 years ago Closed 22 years ago

Add pref for ignoring window feature options on window.open()

Categories

(Core :: DOM: Core & HTML, enhancement)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.0.1

People

(Reporter: hsivonen, Assigned: caillon)

References

Details

(Whiteboard: [adt2 rtm], custrtm+)

Attachments

(1 file, 4 obsolete files)

patch 1.1
14.84 KB, patch
danm.moz
: review+
jag+mozilla
: superreview+
Details | Diff | Splinter Review 
window.open() lets Web authors deprive users of basic navigation functionality.
I find it annoying. I would like to have a pref for disregarding author-supplied
window feature options.
jst, mstoltz,

The patch adds a pref called dom.disable_options_on_open for ignoring
window.open() options. Is adding the pref OK? Is the name of the pref OK?
Target Milestone: --- → mozilla0.9.6
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Johnny, do we want this option? I'm not sure. What's it for?
I don't really feel strongly either way, if we do tho, we should name the pref
something like dom.window.open.disable_options, to make it clearer what it
means. I don't see us ever creating a UI for such a pref tho...
What is the purpose of a pref that has no UI?  Making a browser for the masses
sometimes requires saying no...
>What's it for?

It is for avoiding annoyance when a site tries to take the toolbars away or when
the a site tries to take window resizability away.

> if we do tho, we should name the pref
> something like dom.window.open.disable_options

OK. The name I picked was imitated from dom.disable_open_during_load.

> What is the purpose of a pref that has no UI?

Users who have read the documentation can customize their browsing experience.
We already have a number of these including dom.disable_open_during_load and
browser.target_new_blocked.

BTW, Opera has this sort of thing enabled by default.
Attachment #56137 - Attachment is obsolete: true
Blake, we have several important prefs with no UI. It's become an accepted way
to add power-user features easily. However, I'm not so sure about this one.
Power user features still generally have UI, but in some sort of advanced area.
We have to ask ourselves how many people would possibly use the pref if it's
known by word of mouth within the project.  Usually people wanting such hidden
prefs don't even think it should be in the documentation, at least this one has
*some* chance of being used by more than 3 people (even though I'm not sure I
see the need for it).
I posted to n.p.m.ui with the subject "Pref for ignoring window.open() options"
asking whether anyone else wants this.
Keywords: patch
Futuring until we can decide whether a pref for this is acceptable.
Target Milestone: mozilla0.9.7 → Future
Could we please just make a call here?  Yes or no?  jst, I'd say this is your
call, since distributors other than Netscape could choose to create UI for this
pref.  (And this pref would, imo, fit in perfectly in the Scripts & Windows
panel).
Sure, why not, it won't cost us much. But I won't have time to do it so someone
will have to contribute a patch...
Blocks: 123569
Comment on attachment 56249 [details] [diff] [review]
Patch with dom.window.open.disable_options as the pref name

The orphan patch crew like the idea, but we have to cache the pref and use an
observer: hitting the pref service on every window open is going to sting in a
very tender place.

I think bz might give this some love.
Attachment #56249 - Flags: needs-work+
Comment on attachment 68698 [details] [diff] [review]
Patch along the same lines using a static for the prefservice

Don't get the pref service and call it every time: get the pref on first
creation, and install a listener.  That way we don't take any perf hit but a
branch on window.open (Txul is sacred!) and you'll always have the right data.
Attachment #68698 - Flags: needs-work+
*** Bug 143886 has been marked as a duplicate of this bug. ***
See also:
bug 141863 Pref to prevent hiding status bar
bug 101509 Pref (?) to disable resizable=no
Related: bug 86284
Blocks: 86195
Let me summarize some things I found.

For this bug here I don't understand if it should disable all those options
right away or one by one. Bug 86284 is about the latter. My opinion is that a
web page should not mess up the UI and thereby destroy usability. So I consider
fixing this bug important. If some of you are hesitant about adding it in the
preferences menu right away, let us first implement it for prefs.js and see what
people will do with it.

Summary of bug 86284:
window.open has flags which allow the site to ask the browser to open a new
window without
- main menu
- icon bar
- urlbar
- personal toolbar
- status bar
- scroll bars
- allowing maximize, minimize or resize
Two ways to implement: 1) Don't allow those (individually as options) or 2)
always have a way to restore standard behavior.
Further, placing windows (partially) outside the screen or opening bigger than
the screen (now bug 118717).


Related bugs:
bug 64560 Turn window.open into normal link
bug 84754 Malicious javascript can be used to hide a window or pop up ads, etc.
bug 86195 
bug 86284 Never let sites disable main menu / icon bar / status bar etc.
bug 101509 Pref (?) to disable resizable=no
bug 107949 (this bug;-) Add pref for ignoring window feature options on
window.open()
bug 118717 Never let sites position windows outside the screen
bug 141863 Pref to prevent hiding status bar

Depending on how this bug should be solved, we might dupe bug 86284, bug 101509,
and bug 141863 onto this one. I'll do the first now.


Other JavaScript annoyances:
bug 70135 Should have a pref to disable sending middle/right click events to
javascript
bug 86193 There is no way to always open the context menu upon right-click
(oncontextmenu?)
(those two might be dupes)


So there seems to be a lot of activities to handel JavaScript annoyances. As we
have seen with this bug and bug 86284, many activities don't know about the
others. Is there a bug like "[Tracking] Coping with JavaScript annoyances"? If
not I will be glad to set something up. There are people with a much deeper
insight (Hello, Boris!) who might be able to do it much better than I could. Or
is bug 86195 already the bug I am looking for?

pi
*** Bug 86284 has been marked as a duplicate of this bug. ***
As bug 86284 is now duped to this one, maybe the summary is no longer suitable? 
However I've moved my vote ;)
"[Tracking] Coping with JavaScript annoyances" seems like a good idea, but 
isn't bug 86195 kinda that?
Blocks: 101509
*** Bug 145903 has been marked as a duplicate of this bug. ***
Konqueror handles this by putting "display menu" in the context-menu for a popup
Web page.  Once the menu is displayed, it can be used to turn on the other toolbars.

This is a nice solution because mostly, you want to see what the Web designer
wanted you to; it's just if you suddenly find yourself needing the functionality
of the menu or toolbar that you want to override it.

Sounds like a nice idea indeed. Have not the slightliest idea about coding so 
I'm not the one to request such things, but we'd need to fix Bug 86193 first 
for this feature to be usuable.
Maybe instead have a menu item "remove JS annoyances" which gives back full 
control over the window, enables toolbars etc..
But as I've said, there has to be a way to always access the contextmenu or 
something similar.
Nice thing might also be what Internet Explorer has, these dialog boxes asking 
if you want to permit a page to open a window, close a window (would also 
improve security regardings things like bug 103452 )
Doh guess this is going kinda OT, will have a look for other bugs regarding 
this.
caillon has volunteered to code this fix; we're going to add an "allow scripts
to hide status bar" option to Scripts&Windows.
Assignee: hsivonen → caillon
If this is wanted for RTM, can we get this nsbeta1+'d?  Actually, I'm thinking a
separate bug specifically for the statusbar feature would be ideal....
Status: NEW → ASSIGNED
Keywords: nsbeta1
Whiteboard: custrtm
Target Milestone: Future → mozilla1.0.1
Attached patch Patch v1.0 (obsolete) — — Splinter Review 
This patch fixes the whole bug actually.  It allows ignoring all feature
options on window.open() via prefs.js, but there is only a UI exposed for the
statusbar in this patch.
Attachment #68698 - Attachment is obsolete: true
Comment on attachment 85937 [details] [diff] [review]
Patch v1.0

>   } else { //not first time it was loaded, get default values from data 
> 
>@@ -116,6 +124,7 @@
>     document.getElementById("allowImageSrcChange").checked = data["allowImageSrcChange"].checked;
>     document.getElementById("allowDocumentCookieSet").checked = data["allowDocumentCookieSet"].checked;
>     document.getElementById("allowDocumentCookieGet").checked = data["allowDocumentCookieGet"].checked;
>+    document.getElementById("allowHideStatusBar").checked = data["allowHideStatusBar"].checked;
>     document.getElementById("javascriptAllowNavigator").checked = data["javascriptAllowNavigator"].checked;

Are these lines really necessary? The panel still works fine for me after
removing the entire else section.
caillon:  
there is a bug for the statusbar, bug 141863, mentioned in an
earlier comment.  

sgifford et al:
I categorically disagree with comment #24.  I don't want to have to
right click, choose "restore menu", then Pull down View->Show/Hide
three or four times just to get things back to the way they were
supposed to be in the first place.  I don't want to jump through
hoops to restore things to normalcy, every time a website decides
I shouldn't be permitted access to the UI.  

I want to see what the web designer intended ONLY *within the 
canvas area* and NOWHERE else.  Nowhere else EVER, under any
circumstances, for any reason.  

<rant>
Outside the canvas area, I don't want the web page to have any 
say in what else I see.  I don't want the web page to close other
windows I have open, resize my window, take away my toolbars, 
mess with the status area, put icons on my desktop, change the 
size of my gnome panel, taskbar, dock, or whatever, alter my 
wallpaper, add items to or remove them from my history list,
alter my browser start page settings or other prefs, change my 
system colours, fool with things that belong to the window manager 
(such as minimize buttons), remove my always-on-top clock, hide the
windows system tray, pop up systemmodal dialog boxes, change my 
colour depth to 8-bit for "best viewing", take away the address
bar, crank my resolution up to 2048x1536 so I need a telescope to 
look at it, take away the scrollbars, animate cute dancing bunnies 
running around my screen, clinging to the edges of windows, change 
the throbber, take away the menubar, or do anything else outside 
its designated canvas area.   (XPI installation is a different
matter, but that's acting on direct instructions from the user.)

Inside that canvas area, the webpage can paint what it wants 
(or if not, this bug is not about those limits).  

Outside the canvas area is categorically none of the website's 
business.  EVER.  Never should have been in the first place.  
(Note:  some of the behaviors I mention above have AFAIK not 
to date been implemented, but I'm convinced that's because 
the people who decided to include the others didn't think of 
them.)

It never ceases to amaze me that anyone could possibly WANT
websites to be able to do stuff like that.  People do; they
come out of the woodwork to defend this behavior any time
it comes up on usenet.  Otherwise, I'd say this wouldn't even
need to be a pref, those features of Javascript should just
be unemplemented.  But I know better:  it does need to be
a pref (proving, IMO, that a significant number of people 
are insane).  But while there are many people who want the
website to be able to dictate all these things, there are
also a significant number of us who do NOT.  So, a pref is
in order.  As usual, I care not what is the default.

Summary:  I don't want to have to restore the UI; it should
just always be there, and it's none of any website's business.
Keep the website within its designated area, please.  So
Comment 24 would not be an adequate resolution for this bug.
</rant>


Just for the record, I stand by my statement in comment 24.  I don't have any
objection to a preference for turning these sorts of things off for people who
do want that, but that's not something I would use.  I'll probably file an
enhancement bug for the functionality requested if this is resolved with a
preference.

I've worked with Web sites which were so horribly misdesigned that the Back
button doesn't work, and I've worked with Help windows that are meant to be
small, and a big toolbar/menu/quickbar at the top makes it too big.  I
personally don't mind a Web designer making a practical or stylistic decision to
hide those things, as long as they're not completely unavailable.  Just as there
are no menus, buttons, personal toolbars, or status bars on the "Save As" dialog
in Mozilla, on some small Web pages it's appropriate to hide those things.

I don't want to have a big discussion about this here; if we have a fix for the
original bug, let's take it, and we'll have the discussion in a newsgroup or in
another bug.

Re comment 29.  Try loading the pref panel, changing some settings, clicking 
over to a different pref panel and then clicking back...
Re commment 32: Already tried that before posting comment 29. It works. I do
remember that the lines were needed once, though. But they aren't anymore.
Methinks this may be a side effect of this checkin:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=pref-scripts.xul&root=/cvsroot&subdir=mozilla/xpfe/components/prefwindow/resources/content&command=DIFF_FRAMESET&rev1=1.5&rev2=1.6
Indeed, you can remove that else() branch.

My main issue with this is that for example on netscape.com, if you uncheck the
checkbox (ie disallow to disable the statusbar), the resulting popup's statusbar
is missing the security icon and online/offline indicator.  

I doubt this pref will help much if this bug isn't fixed first.
  (speaking as a reviewer of the patch:)
  Ech. I guess so. My ambilavence about this bug/feature is swayed by the
embedding component library's unfortunate new dependence on the prefs library.
Hmm. Not enough I guess to try to stop the bug. Nevertheless I think I'm going
to be a review jerk now.
  readability issue: I'd like to lobby for a change to the name of the new
method CanDisableFeature. Its purpose and its name don't coincide in my head. I
had to read it closely and check its usage before I understood. How about
FeatureIsForceEnabled or some such?
  functionality issue: I think I can simplify your feature set a bit. This
bug/feature is about disallowing unscrupulous web authors from opening lame
windows on you. Not really fitting into this category and in my opinion
pointless anyway are (centerscreen, dependent, dialog, popup). centerscreen
affects window positioning. With the centerscreen pref on, no window can be
opened anywhere but centered. I'm thinking that's not what you want. Similarly
with dependent (in gtk parlance, transient). A pref making it impossible to open
a window that isn't dependent? dialog and popup affect mostly the type of OS
border the window comes with. Popup windows look like popup windows. These also
seem like features that no one would want on all the time.
  performance problem: WinHasOption is called about twenty times for each window
open. You're hammering on expensive-looking pref and security calls each time.
I'm concerned about harm to new-window time. You could cache the securityManager
but I think you should also find some way not to call SubjectPrincipalIsSystem
each time. I'll ask Mitch how expensive that is. But the main problem is the
refetch of the pref manager and reconstruction of the
dom.disable_window_open_feature branch each time. I'm gonna have to see numbers
proving this doesn't hurt.
  quibble: The changes to nsWindowWatcher.cpp at lines 1120 and 1197; those are
unnecessary, aren't they? I'd rather leave it the way it is, with the fetch and
use of the security manager nicely juxtaposed. (I'm guessing this is an artifact
of a previous version of the patch that you didn't go with. But you may have to
go back to it if you address my other issues, above.)
Whiteboard: custrtm → custrtm-
Attached patch patch 1.1 — — Splinter Review 
Use an inline macro instead...
Attachment #85937 - Attachment is obsolete: true
Comment on attachment 86150 [details] [diff] [review]
patch 1.1

'k. r=danm. watch the window open times after checkin, please.
Attachment #86150 - Flags: review+
Comment on attachment 86150 [details] [diff] [review]
patch 1.1

sr=jag
Attachment #86150 - Flags: superreview+
We really want this for RTM. This will change UI, so I think custrtm- is
incorrect, removing.
Keywords: nsbeta1 → adt1.0.1, mozilla1.0.1, nsbeta1+
Whiteboard: custrtm- → [ADT2 RTM]
Keywords: adt1.0.1, mozilla1.0.1, nsbeta1+ → nsbeta1
Whiteboard: [ADT2 RTM]
If you want this for rtm, Heikki could you nsbeta1+ the bug and add the impact. thx
Attachment 86150 [details] [diff] has been checked in on the trunk only.  I will be making a
scaled down version of this patch under the auspices of bug 141863 which will
track the statusbar only portion of this patch.  (Note: the trunk patch still
only has a UI for statusbar.  Other patches will deal with the UI for those)
The adt is interested in taking this but this needs approval from mcarlson for
an l10n freeze exception.
Keywords: nsbeta1 → adt1.0.1, nsbeta1+
Whiteboard: [adt2 rtm]
New prefs will be added to prevent web authors from creating misleading UI in
the product. Adding to watch list.
Blocks: 121324
Whiteboard: [adt2 rtm] → [adt2 rtm], custrtm+
l10n approved IF you can get this into the branch by tomorrow. thanks
adding adt1.0.1+ for adt approval.  Please get drivers approval before checking
into the branch.
Keywords: adt1.0.1 → adt1.0.1+
removing adt1.0.1+ since the patch we're taking is 141863.
Keywords: adt1.0.1+ → adt1.0.1
Marking fixed since this has been fixed on the trunk.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
I meant to remove the nomination as well.
Keywords: adt1.0.1
Is there a documentation (I mean without understanding the patch;-)?

pi
Pi, these are the new prefs:

pref("dom.disable_window_open_feature.titlebar",    false);
pref("dom.disable_window_open_feature.close",       false);
pref("dom.disable_window_open_feature.toolbar",     false);
pref("dom.disable_window_open_feature.location",    false);
pref("dom.disable_window_open_feature.directories", false);
pref("dom.disable_window_open_feature.personalbar", false);
pref("dom.disable_window_open_feature.menubar",     false);
pref("dom.disable_window_open_feature.scrollbars",  false);
pref("dom.disable_window_open_feature.resizable",   false);
pref("dom.disable_window_open_feature.minimizable", false);
pref("dom.disable_window_open_feature.status",      false);

Set any of them to true to prevent scripts from messing with that feature.
I don't understand titlebar, i.e., I have never seen that this is suppressed.

I don't know what close and directories should be.

Maybe it would be nice to be able to recover all the bars (menu, location,
personal) from the titlebar. IIRC this worked in some earlier version (or
Netscape 4?).

pi
> I don't understand titlebar, i.e., I have 
> never seen that this is suppressed.

Some window managers do not allow it to be
surpressed.  Though FWIW I have never seen
it surpressed either; perhaps the script
to suppress it is not well-known or does 
not work in other browsers?
adt1.0.1- per the ADT. let's get it in the next release.
Keywords: adt1.0.1 → adt1.0.1-
See also bug 161903, Add pref for ignoring window size options on window.open().
user_pref("dom.disable_window_open_feature.chrome", true);
will NOT disable the window feature chrome=yes in the open() call. 

I wish we could REOPEN this bug until chrome=yes can be disabled, countered by
the above user_pref instruction. chrome is a VERY powerful window feature option
as chrome=yes will remove
- menubar
- toolbar and location bar (the whole Navigation Toolbar)
- personalbar (directories)
- statusbar (even if the user has uncheck Allow scripts to: Hide the status bar)
- tab bar if its display is requested
- Site Navigation Bar if its display is requested

XP Pro SP1, build 2003010604

*** Bug 241571 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: