Closed Bug 1009529 Opened 6 years ago Closed 6 years ago

Window.open() is missing scrollbars in v27+

Categories

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

27 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
firefox29 --- wontfix
firefox30 --- verified
firefox31 --- verified
firefox32 --- verified
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: daniel.schoonmaker, Assigned: Gijs)

References

()

Details

(Keywords: regression, site-compat, testcase, Whiteboard: p=1 s=it-32c-31a-30b.2 [qa!])

Attachments

(2 files)

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)
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?
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM: Core & HTML
Ever confirmed: true
Flags: needinfo?(Ms2ger)
OS: Mac OS X → All
Product: Firefox → Core
Hardware: x86 → All
<Ms2ger> 	301 peterv
Flags: needinfo?(Ms2ger) → needinfo?(peterv)
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.
(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?
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(peterv) → needinfo?(bzbarsky)
If we can detect the features the new window was opened with from mochitest-plain, then sure.
Flags: needinfo?(bzbarsky)
This seems to work (with gratitude to jgraham for suggesting this on the spec bug...)
Attachment #8421869 - Flags: review?(bzbarsky)
Comment on attachment 8421869 [details] [diff] [review]
treat null features as '' features in window.open calls,

r=me
Attachment #8421869 - Flags: review?(bzbarsky) → review+
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/882aa0d65975
Whiteboard: [fixed-in-inbound]
(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.
https://hg.mozilla.org/mozilla-central/rev/882aa0d65975
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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?
Attachment #8421869 - Flags: approval-mozilla-beta?
Attachment #8421869 - Flags: approval-mozilla-aurora?
Flags: needinfo?(bzbarsky)
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.
Attachment #8421869 - Flags: approval-mozilla-beta?
Attachment #8421869 - Flags: approval-mozilla-beta+
Attachment #8421869 - Flags: approval-mozilla-aurora?
Attachment #8421869 - Flags: approval-mozilla-aurora+
(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!
Flags: needinfo?(bzbarsky)
WebIDL changes don't affect binary compat, so don't count as IDL changes, indeed.
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!
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(daniel.schoonmaker)
Flags: in-testsuite?
(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.
Flags: needinfo?(daniel.schoonmaker)
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.
Verified that this fails pre-patch and succeeds post-patch. Neil, thanks for the hint, and Liz, thanks for chasing me on this one. :-)
Attachment #8423796 - Flags: review?(bzbarsky)
Marco, can you add this to the backlog as well, I guess? :-)
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mmucci)
Whiteboard: p=1 s=it-32c-31a-30b.2
Added to Iteration 32.2
Flags: needinfo?(mmucci)
Whiteboard: p=1 s=it-32c-31a-30b.2 → p=1 s=it-32c-31a-30b.2 [qa!]
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.
Attachment #8423796 - Flags: review?(bzbarsky) → review+
(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
Flags: in-testsuite? → in-testsuite+
I can also confirm that this is working for me now on Nightly 32.0a1 (2014-05-16).

Thanks Gijs for the fix!
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
(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
(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
Depends on: 1011874
(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
You need to log in before you can comment on or make changes to this bug.