132105 - xlink with actuate="onLoad" and show="new" will open a new window with the "Open unrequested windows" checkbox cleared

Status: VERIFIED FIXED

(Core :: XML, defect, P2)

Description

[Gary Coady]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.9+)
Gecko/20020319
BuildID:    2002031903

The "Open Unrequested Windows" setting in "Scripts & Windows" is supposed to
restrict new windows from being opened when a page is visited. But the
actuate="onLoad" feature of xlink doesn't appear to check the capability to open
new windows before opening them.

Reproducible: Always
Steps to Reproduce:
1. Turn off "Open unrequested windows" in the "Scripts & Windows" options.
2. Go to the reference URL.


Actual Results:  An extra new window will open up, which should be stopped by
checking the capability to open new unrequested windows.

Expected Results:  Mozilla should not open a new window in response to an onLoad
event.

Comment 1

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

It isn't strictly a script that is opening the window, but maybe checking that
pref would make sense.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yeah... The pref should apply to all methods of opening a window onload.

Comment 3

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

Hmm, is the issue a) opening links in new windows or b) automatically following
links? I have a patch that makes XLinks check "browser.block.target_new_window"
pref before opening in new window. Is that sufficient? I'll attach the patch.

Comment 4

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

Attachment: check pref before opening links in new windows

Comment 5

[Gary Coady]

My concern is mainly that websites could use the xlink feature to open popup and
popunder ad windows, despite having dom.disable_open_during_load set to true.

What would be closer to what I'd imagine to be the correct behaviour is to block
if either browser.block.target_new_window OR dom.disable_open_during_load is set
to PR_TRUE.

For the first pref, not blocking will cause the first page to be replaced by the
second, probably not the desired behaviour - since the second page is
automatically loaded and might usually be expected to be the adjunct, not
primary content?

And for the second pref, the code in question in your patch is concerned with
when xlink:actuate="onLoad", so it should not open any windows during onLoad.

So 
inline PRBool BlockNewWindow()
{
  PRBool blockNewWindow = PR_FALSE;
  PRBool blockNewWindowOnLoad = PR_FALSE;
  nsCOMPtr<nsIPref> prefs(do_GetService(kPrefServiceCID));
  if (prefs) {
    prefs->GetBoolPref("browser.block.target_new_window", &blockNewWindow);
    prefs->GetBoolPref("dom.disable_open_during_load", &blockNewWindowOnLoad);
  }
  return (blockNewWindow == PR_TRUE || blockNewWindowOnLoad == PR_TRUE);
}

I don't have a CVS tree and I haven't written C in ages, so that may be wrong,
but you get the idea :-)

Comment 6

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

I checked what happens if "browser.block.target_new_window" is true and you
click on regular HTML a link: we replace the content. Therefore I think we
should just replace.

Also, please compare automatically loading XLinks that replace the content to
HTML refresh mechanism with 0 timeout. You can do that with HTTP headers or meta
elements:

<meta HTTP-EQUIV="refresh" CONTENT="0;URL=<link here>">

The preference "dom.disable_open_during_load" does not affect that, so I don't
think automatic XLinks that replace content should be affected with that.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 85843 [details] [diff] [review]
check pref before opening links in new windows

sr=jst

Comment 8

[harishd]

Comment on attachment 85843 [details] [diff] [review]
check pref before opening links in new windows

r=harishd

Comment 9

[Gary Coady]

Far be it from me to stop a patch going in which improves the situation (i.e.
please check it in), but I will just say that semantically, I feel that the pref
being used usually blocks new windows from opening during interactive webpage
use, while the other pref usually blocks new windows from opening at load time.

Obviously, this is not the real meaning of either - I personally feel that both
prefs should be amalgamated as a new pref with values of ('never', 'onload',
'always') for when windows should be opened, and all methods where a window
could be opened should refer to that, rather than having two separate prefs for
different methods.

But then this should be a different bug, or else discussed on n.p.m.general I guess.

Comment 10

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

Attachment: better fix

Ok, I think this should satisfy everyone. If "dom.disable_open_during_load" is
true, we don't open links in new windows when loading the document (but they
replace the current content instead, see reasons for this in earlier comments).
If "browser.block.target_new_window" is true, we *never* open links in new
window; instead they replace the content (again, see discussion before for
reasons).

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 86246 [details] [diff] [review]
better fix

sr=jst

Comment 12

[harishd]

Comment on attachment 86246 [details] [diff] [review]
better fix

r=harishd

Comment 13

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

Checked in.

Comment 14

[Rakesh Mishra]

verified fixed on mozilla trunk 2002-12-19-08-trunk on win2K with the test case
at the URL http://www.snee.com/xml/xlink/sxlinkdemo.xml
marking verified