Dan Schoonmaker Reporter
	Description • 10 years ago

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131202182626

Steps to reproduce:

My webapp has javascript to open a link in a new tab:
   window.open("http://cnn.com", "_blank", null);


Actual results:

After v27, this opened links in a new window with the vertical scrollbar missing. We added the parameter "scrollbars=yes", to resolve that piece, but it still opens in a new window.


Expected results:

Before v27, this opened links correctly in a new tab with scrollbars enabled (if necessary)

	:Gijs (he/him) Assignee
	Comment 1 • 10 years ago

Using empty string instead of null seems to work fine. I expect this is because we implicitly convert null to a string, and if that ends up meaning "null", that would explain the weird behaviour. I'd argue that the correct conversion for null is empty string (and that is certainly what chrome does). The spec says:

The third argument, features, has no defined effect and is mentioned for historical reasons only. User agents may interpret this argument as instructions to set the size and position of the browsing context, but are encouraged to instead ignore the argument entirely.

( http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers.html#dom-open )

So I guess we could per-spec specialcase "null" and make it === "" for the purposes of implementing this. Maybe?

	:Gijs (he/him) Assignee
	Comment 2 • 10 years ago

<Ms2ger> 	301 peterv

	Kohei Yoshino
	Comment 3 • 10 years ago

https://twitter.com/FxSiteCompat/status/466250824683757569

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 10 years ago

For compat, I'd say just make this nullable so that null is treated like "".

That said, we should consider just dropping the ability for untrusted pages to do scrollbars=no.  WebKit and Blink don't allow it, for example.

	:Gijs (he/him) Assignee
	Comment 5 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #4)
> For compat, I'd say just make this nullable so that null is treated like "".
> 
> That said, we should consider just dropping the ability for untrusted pages
> to do scrollbars=no.  WebKit and Blink don't allow it, for example.

Filed bug 1009661.

Stealing, because this looks doable at first glance... I presume this would need a test; is mochitest-plain the right thing here?

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 10 years ago

If we can detect the features the new window was opened with from mochitest-plain, then sure.

	:Gijs (he/him) Assignee
	Comment 7 • 10 years ago

This seems to work (with gratitude to jgraham for suggesting this on the spec bug...)

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 10 years ago

Comment on attachment 8421869 [details] [diff] [review]
treat null features as '' features in window.open calls,

r=me

	:Gijs (he/him) Assignee
	Comment 9 • 10 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/882aa0d65975

	neil@parkwaycc.co.uk
	Comment 10 • 10 years ago

(In reply to Boris Zbarsky from comment #6)
> If we can detect the features the new window was opened with from
> mochitest-plain, then sure.

From mochitest-plain, you could look at the BarProp objects on the window.

From mochitest-chrome, you could poke at the XUL window's chrome flags directly.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 11 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/882aa0d65975

	:Gijs (he/him) Assignee
	Comment 12 • 10 years ago

Comment on attachment 8421869 [details] [diff] [review]
treat null features as '' features in window.open calls,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 918345
User impact if declined: window.open(x, "_blank", null) behaves oddly
Testing completed (on m-c, etc.): on m-c, local
Risk to taking this patch (and alternatives if risky): reasonably low - webidl only change on how to treat null
String or IDL/UUID changes made by this patch: webidl change. I don't /think/ that counts as an IDL change? Boris?

	Sylvestre Ledru [:Sylvestre]
	Comment 13 • 10 years ago

Comment on attachment 8421869 [details] [diff] [review]
treat null features as '' features in window.open calls,

I asked to RyanVM recently and webIDL does not count as IDL in the uplift context.

	:Gijs (he/him) Assignee
	Comment 14 • 10 years ago

(In reply to Sylvestre Ledru [:sylvestre] from comment #13)
> Comment on attachment 8421869 [details] [diff] [review]
> treat null features as '' features in window.open calls,
> 
> I asked to RyanVM recently and webIDL does not count as IDL in the uplift
> context.

Thanks!

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 10 years ago

WebIDL changes don't affect binary compat, so don't count as IDL changes, indeed.

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/64302eea04d9
https://hg.mozilla.org/releases/mozilla-beta/rev/41a1653a0199

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 10 years ago

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/41a1653a0199

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 18 • 10 years ago

Gijs, are you writing tests for this? 

Dan, would you mind testing this manually from your webapp or is it something where you could attach a simple test case? THanks!

	:Gijs (he/him) Assignee
	Comment 19 • 10 years ago

(In reply to Liz Henry :lizzard from comment #18)
> Gijs, are you writing tests for this? 

I can look into it based on comment #10. I'm not sure yet if it'll be possible to distinguish the failing case successfully here. Leaving the needinfo request for that purpose.

> Dan, would you mind testing this manually from your webapp or is it
> something where you could attach a simple test case? THanks!

There's a simple test case linked from the URL field of the bug that you can use for manual verification.

	Cornel Ionce [:noni] [Hubs QA]
	Comment 20 • 10 years ago

Verified as fixed on Mac OS X 10.8, Ubuntu 12.04 and Windows 7 64bit using:
1. Latest Nightly, build ID: 20140515030202
2. Latest Aurora, build ID: 20140515004001
3. Fx 30 beta 5, build ID: 20140515140857.

	:Gijs (he/him) Assignee
	Comment 21 • 10 years ago

Verified that this fails pre-patch and succeeds post-patch. Neil, thanks for the hint, and Liz, thanks for chasing me on this one. :-)

	:Gijs (he/him) Assignee
	Comment 22 • 10 years ago

Marco, can you add this to the backlog as well, I guess? :-)

	Marco Mucci [:MarcoM]
	Comment 23 • 10 years ago

Added to Iteration 32.2

	Boris Zbarsky [:bzbarsky]
	Comment 24 • 10 years ago

Comment on attachment 8423796 [details] [diff] [review]
add test for window.open with null and with empty string having the same barprops,

You can probably do this.removeEventListener, right?

r=me either way.

	:Gijs (he/him) Assignee
	Comment 25 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #24)
> Comment on attachment 8423796 [details] [diff] [review]
> add test for window.open with null and with empty string having the same
> barprops,
> 
> You can probably do this.removeEventListener, right?

Good catch.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6cca7678a853

	Dan Schoonmaker Reporter
	Comment 26 • 10 years ago

I can also confirm that this is working for me now on Nightly 32.0a1 (2014-05-16).

Thanks Gijs for the fix!

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 10 years ago

This patch had the misfortune of landing in the middle of a large bustage pileup on inbound that led to me having to revert to a last-good cset in lieu of waiting 3-4 hours for green instead. Unfortunately, that means this was backed out along with the others. If this was confirmed green on Try, it can re-land at your convenience.

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb2a6f7785a2

	:Gijs (he/him) Assignee
	Comment 28 • 10 years ago

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #27)
> This patch had the misfortune of landing in the middle of a large bustage
> pileup on inbound that led to me having to revert to a last-good cset in
> lieu of waiting 3-4 hours for green instead. Unfortunately, that means this
> was backed out along with the others. If this was confirmed green on Try, it
> can re-land at your convenience.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/eb2a6f7785a2

Looking at TBPL, I'm almost tempted to use that as a try run, seeing as it was green and this should really only be affecting mochitest-plain and all of the b2g tests went orange because of the push before mine... but either way. I can land this tomorrow once there's results from:

remote:   https://tbpl.mozilla.org/?tree=Try&rev=52d87222f73b

	:Gijs (he/him) Assignee
	Comment 29 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #28)
> remote:   https://tbpl.mozilla.org/?tree=Try&rev=52d87222f73b

Fully green, so landed as remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/61d0065e67a6

	:Gijs (he/him) Assignee
	Comment 30 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #28)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #27)
> > This patch had the misfortune of landing in the middle of a large bustage
> > pileup on inbound that led to me having to revert to a last-good cset in
> > lieu of waiting 3-4 hours for green instead. Unfortunately, that means this
> > was backed out along with the others. If this was confirmed green on Try, it
> > can re-land at your convenience.
> > 
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/eb2a6f7785a2
> 
> Looking at TBPL, I'm almost tempted to use that as a try run, seeing as it
> was green and this should really only be affecting mochitest-plain and all
> of the b2g tests went orange because of the push before mine... but either
> way. I can land this tomorrow once there's results from:
> 
> remote:   https://tbpl.mozilla.org/?tree=Try&rev=52d87222f73b

Clearly this jinxed it. I didn't test b2g-desktop, which promptly started failing consistently. I've disabled the test there for now (as this isn't awfully relevant and really, the webidl code is the same cross-platform anyway) and filed bug 1011874 to track re-enabling it.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/68947f7d04ec

	Phil Ringnalda (:philor)
	Comment 31 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/61d0065e67a6
https://hg.mozilla.org/mozilla-central/rev/68947f7d04ec