125887 - Window.open yes/true attributes broken by trailing spaces

Status: RESOLVED FIXED

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

Description

[Fabricio Campos Zuardi]

Steps to reproduce:

1- create a html with this window.open:
window.open ('http://www.mozilla.org','mozillawindow','width=500 , height=500 ,
resizable=yes , scrollbars=yes');
2- open this html

Expecting result:
the window open with the resize handlers and with the maximize button, just like
if you had coded without the spaces between the commas.

Actual result:
the window open but the resize handlers and the maximize button are disabled.

Comment 1

[Fabricio Campos Zuardi]

Attachment: An window.open without spaces between the attributes

This way works fine.

Comment 2

[Fabricio Campos Zuardi]

Attachment: window.open with spaces between the attributes

This way doesnt work, the resizable=yes is ignored

Comment 3

[Tim Powell]

I first thought that this was a bug with spaces in the features string of
window.open. JavaScript: The Definitive Guide says "The features argument is a
comma-separated list of features to appear in the window.... The string should
not contain any spaces or other whitespace." I don't know if this is also
documented in any DOM standard. Netscape 4.x definitely had problems with
whitespace in the string. It appears that IE5.0 and 5.5 and Mozilla do support
whitespace in the string. For example, this works:
javascript:window.open("about:blank","test","width=200 , height=200 , resizable=1") 

The problem seems to be that "resizable=yes " with a trailing space is not
working. If you change it to resizeable=1 or remove the space it works. Perhaps
the string parser needs to trim trailling spaces before it tries a comparison? 

Confirming bug because I can't find a duplicate for it.

Comment 4

[Tim Powell]

A better summary might be "Window.open yes/true attributes broken by trailing
spaces". This same problem affects the other attributes, such as toolbar=yes ,
scrollbars=yes , location=yes , etc.

In case I wasn't clear, the workaround for this is to just use 1 for the value
instead of yes or to remove the spaces.

I suspect this is really DOM0 and not JS engine, so changing component. I
confirmed this with 20020208 nightly on Win2K.

Comment 5

[Phil Schwartau]

Setting default owner and QA - 

Comment 6

[Fabricio Campos Zuardi]

Confirmed in 2002040303 win2k and changed the sumary for the one suggested in
comment #4.

Comment 7

[Håkan Waara]

Attachment: Patch

Here's a patch to ignore whitespace in the "features" argument of window.open()
and window.openDialog().

Comment 8

[Håkan Waara]

CC boris for r=.

Comment 9

[Håkan Waara]

CC caillon and sicking too. Seems like jst is busy. Maybe bz can sr= and you
guys can r=?   Thanks.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 91964 [details] [diff] [review]
Patch

Please remove the code in nsWindowWatcher::WinHasOption that skips whitespace
and add a debug-only check there that there is no whitespace in the string,
with an assertion if there is.

With that, sr=bzbarsky

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 91964 [details] [diff] [review]
Patch

Actually, use StripWhitespace() instead of StripChars()

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 91964 [details] [diff] [review]
Patch

So I guess this makes sense after talking to Boris, and I agree that we should
remove the foo in nsWindowWatcher::WinHasOption() but to me it seems wrong to
move the check out of the window watcher and into its callers.	I'd rather you
do StripWhitespace() somewhere either in nsWindowWatcher::OpenWindow() or
nsWindowWatcher::OpenWindowJS()...

Comment 13

[Håkan Waara]

Attachment: Patch v2

Better patch, addressing caillon's and bzbarsky's suggestions.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 92061 [details] [diff] [review]
Patch v2

>+#include "nsReadableUtils.h"

Don't you actually want nsString.h?  And only in debug builds?

>+#if DEBUG

#ifdef

>+    nsCString options(aOptions);

nsCAutoString

Comment 15

[Håkan Waara]

Attachment: Patch v3

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 92064 [details] [diff] [review]
Patch v3

sr=bzbarsky

Comment 17

[John Keiser (jkeiser)]

Comment on attachment 92064 [details] [diff] [review]
Patch v3

r=jkeiser

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 92064 [details] [diff] [review]
Patch v3

a=[e-mail] for trunk checkin -- seems safe and right.

/be

Comment 19

[Håkan Waara]

->moi

Comment 20

[Håkan Waara]

Fixed on trunk.

Comment 21

[Christopher Aillon (sabbatical, not receiving bugmail)]

>+    NS_ASSERTION(options.FindCharInSet(" \n\r\t") == kNotFound, 
>+                  "There should be no whitespace in this string!");

Nice indentation.  ^^^^

r=caillon with that fixed.  <yawn>