111752 - document.open(a, b, c); should act like window.open(a, b, c) when all 3 arguments are specified.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Max]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.6) Gecko/20011120
BuildID:    2001112009

In the page http://www.bravenet.com/reviews/coolscripts/ , clicking on the
listed hyperlinks should bring up a pop-up window. It doesn't

Reproducible: Always
Steps to Reproduce:
1. Click on one of the listed links, for example 'ColorPicker Script'


Actual Results:  It stays in the same window, keeps the background and just show
it's loading in the top-right corner.

Expected Results:  It should just open up a pop-up window.

All the hyperlinks are using javscript

Comment 1

[Boris Zbarsky [:bzbarsky]]

Evangelism.  Page is using "open()" instead of "window.open()".  The problem is
that "open()" on it's own is "document.open()" and hence we just open the
current document for writing (wiping out all the content) instead of opening a
new window.  The page should use "window.open()"

In some other browsers "open()" is treated as "window.open()" in spite of the
fact that "document.open()" is also supported. This leads to very inconsistent
behavior in scripts that are assuming correctly implemented Javascript namespaces...

Comment 2

[Bob Clary [:bc] (inactive)]

bz, why is open considered to be document.open and not window.open? window is
the global object in js in a browser and I would have thought that an
unqualified open would resolve against the global object not document. --bc

Comment 3

[Boris Zbarsky [:bzbarsky]]

An unqualified variable name of any kind resolves against document first, then
window, if I recall correctly....

Comment 4

[Bob Clary [:bc] (inactive)]

Attachment: test of global object resolution of variables in js

This example shows that a regular variable is resolved against the window as
global object rather than document.

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK, that's weird.  jst, what's up here?

Comment 6

[Johnny Stenback (:jst)]

Event handlers run with the node they're execeuted on as the scope, thus in this
case we'll end up resolving |open| in the link's scope. The link doesn't have a
|open| property, so we go to the link's parent and its parent until we find a
JSObject with a |open| property on it, the first one we find is |document.open|
so we call that. They need to qualify the name |open| when called from an event
handler. IE is broken, that's why this code works in IE.

Comment 7

[Bob Clary [:bc] (inactive)]

This is actually a 4x compatible dom 0 bug since both IE and Navigator 4, treat
a javascript:open as a document.open with 0-2 arguments and as window.open if it
has 3+ arguments.

Comment 8

[Bob Clary [:bc] (inactive)]

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

Comment 9

[Bob Clary [:bc] (inactive)]

Attachment: test case with open with 3 arguments

Comment 10

[Johnny Stenback (:jst)]

Oh man, I can't believe this quirky behavior, this is really really weird :-)

Patch coming up to hack up this quirky DOM0 behavior, document.open() works
exactly like window.open() does when called with 3 or more arguments.

Comment 11

[Johnny Stenback (:jst)]

Attachment: Make document.open(uri, name, features) work like window.open(...)

Comment 12

[Brendan Eich [:brendan]]

Chouck, what does the 4.x code do? 
http://lxr.mozilla.org/classic/source/lib/libmocha/lm_doc.c#1617 turns
document.open (open in an event handler) calls into window.open if argc > 2 ||
argv[0] == [the "" value].

/be

Comment 13

[Johnny Stenback (:jst)]

We should push this into the embedding branch befroe we ship something real off
of that branch since this has shown up in a number of bug reports already, and I
bet there will be many many more.

Comment 14

[Brendan Eich [:brendan]]

As I pointed out to jst, and mentioned in passing in my last comment here, this
quirk was added because <input type=button onclick='open("http://foo.com", "",
"resizable=no,height=400,width=500")'> should call window.open, not
document.open as the scope chain would have it.

/be

Comment 15

[Johnny Stenback (:jst)]

Attachment: Keep this JS specific hack in JS specific code.

Comment 16

[Johnny Stenback (:jst)]

Attachment: Fixes a bug pointed out by Fabian, the callee's window must be used when calling window.open(), not the callers window.

Comment 17

[Fabian Guisset]

Comment on attachment 61570 [details] [diff] [review]
Fixes a bug pointed out by Fabian, the callee's window must be used when calling window.open(), not the callers window.

r=fabian

Comment 18

[Judson Valeski]

Johnny, what other bug reports has this shown up in? Just trying to get an
understanding of the gravity of this.

Comment 19

[Johnny Stenback (:jst)]

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

Comment 20

[Johnny Stenback (:jst)]

Brendan, would you sr the latest patch in this bug?

Comment 21

[Judson Valeski]

cc'ing brendan, as jst requested sr from him.

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 61570 [details] [diff] [review]
Fixes a bug pointed out by Fabian, the callee's window must be used when calling window.open(), not the callers window.

sr=[e-mail], good catch Fabian!

/be

Comment 23

[Johnny Stenback (:jst)]

Fix checked in on the trunk.

Judson, I don't have the numbers of the bugs where this has shown up in handy,
but I've seen a number of them go by, some of them were probably incorrectly
marked INVALID, some were given to evangelism (at least bug 113147, probably
others too).

The reson I think we should take this on the embedding branch is that this bug
breaks functionality that dates back to some of the first versions of Netscape
and IE, it breaks commonly done things that has always worked in other browsers,
and it's something that any web developer can do, and is likely to do, on
websites. If we don't fix this, it's very likely that this bug will break new
pages that are not yet written, as well as old pages that we don't know about
yet. We have a fix, it's now on the trunk, lets let it bake on the trunk for a
few days, just to make sure, and then land it on the trunk. This is a small
prise to pay for having yet more (non-broken) sites work in embedding clients.

Comment 24

[Michael Dunn]

Will plus for 094 once fix is checked into trunk and tested.

Comment 25

[Johnny Stenback (:jst)]

Michael, this fix has already been checked in on the trunk, the fix was checked
in on the trunk on 12/20 so I can't say how much testing it has gotten, but it's
been on the trunk since then. Do we want more testing, or should this be plussed
already?

Comment 26

[Schelstraete Bart]

FYI
works with the latest build 
-> http://bugzilla.mozilla.org/show_bug.cgi?id=113568  also works now.

Comment 27

[Marek Z. Jeziorek]

adding + to edt0.9.4

Comment 28

[Johnny Stenback (:jst)]

Fix checked in on the 0.9.4 branch.

Comment 29

[Boris Zbarsky [:bzbarsky]]

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