window.open() should gate new tab/new popup based on toolbar visibility.

window.open() should gate new tab/new popup based on toolbar visibility.

Previously, Chrome required that toolbar, menubar, scrollbars, status,
resizable were all set to enabled to open a window as a new tab rather
than a new popup. However, this causes developer frustration if one of
window features is accidentally omitted (as it then defaults to
disabled).

Instead, just use toolbar visibility to determine whether or not
window.open() creates a new popup or a new tab, which matches Firefox.

BUG=82522
Review-Url: https://codereview.chromium.org/2773573002
Cr-Commit-Position: refs/heads/master@{#459540}
Committed: https://chromium.googlesource.com/chromium/src/+/e507bb334d3c66917eec47fbc476ed0a7d62f33f

[dcheng, 2017-03-22 23:38:59]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 23:39:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2017-03-22 23:40:17]

dcheng@chromium.org changed reviewers:
+ foolip@chromium.org

[commit-bot: I haz the power, 2017-03-23 00:55:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 00:55:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dcheng, 2017-03-23 07:22:49]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 07:23:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 08:52:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 08:52:15]

Dry run: This issue passed the CQ dry run.

[foolip, 2017-03-23 12:55:38]

A 5-digit bug, cool!

LGTM, but I wonder if scripts can tell if they're in a new tab or a separate
window? If that is possible to tell, then maybe the spec should require this
behavior and we should put tests in web-platform-tests. If it's not possible,
then good as-is.

[dcheng, 2017-03-23 18:30:41]

As it turns out, they can indirectly by checking toolbar.visible. I added a WPT
based on that... let me know what you think.

(I did consider having this test all combinations of possible window features,
but meh, scope creep, and I've mostly exhausted the time I'm willing to spend on
this bug =P)

[dcheng, 2017-03-23 18:30:47]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 18:31:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 20:10:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 20:10:29]

Dry run: This issue passed the CQ dry run.

[foolip, 2017-03-24 01:26:09]

I tweaked the test to be async tests that insert buttons to click and tried it
in Edge, Firefox and Safari. Pass in Firefox, but the toolbar=0 test fails in
Edge and Safari.

Maybe Edge and Safari want to align, not sure, but I can't find a spec that
covers this. I've filed https://github.com/whatwg/html/issues/2464 but in the
meantime I'm afraid we shouldn't put this test in web-platform -tests. Maybe
next to LayoutTests/fast/dom/Window/new-window-opener.html?

[dcheng, 2017-03-24 04:54:24]

On 2017/03/24 01:26:09, foolip_UTC9 wrote:
> I tweaked the test to be async tests that insert buttons to click and tried it
> in Edge, Firefox and Safari. Pass in Firefox, but the toolbar=0 test fails in
> Edge and Safari.
> 
> Maybe Edge and Safari want to align, not sure, but I can't find a spec that
> covers this. I've filed https://github.com/whatwg/html/issues/2464 but in the
> meantime I'm afraid we shouldn't put this test in web-platform -tests. Maybe
> next to LayoutTests/fast/dom/Window/new-window-opener.html?

Moved it out of WPT: out of curiosity, why is it important for the test to click
on buttons? Does button.click() simulate a user gesture?

[dcheng, 2017-03-24 04:54:57]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 04:55:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[foolip, 2017-03-24 05:06:15]

On 2017/03/24 04:54:24, dcheng wrote:
> On 2017/03/24 01:26:09, foolip_UTC9 wrote:
> > I tweaked the test to be async tests that insert buttons to click and tried
it
> > in Edge, Firefox and Safari. Pass in Firefox, but the toolbar=0 test fails
in
> > Edge and Safari.
> > 
> > Maybe Edge and Safari want to align, not sure, but I can't find a spec that
> > covers this. I've filed https://github.com/whatwg/html/issues/2464 but in
the
> > meantime I'm afraid we shouldn't put this test in web-platform -tests. Maybe
> > next to LayoutTests/fast/dom/Window/new-window-opener.html?
> 
> Moved it out of WPT: out of curiosity, why is it important for the test to
click
> on buttons? Does button.click() simulate a user gesture?

That was just so that popup blockers wouldn't interfere with the tests. I
actually added a button to click manually, the click() method doesn't have the
necessary powers.

[commit-bot: I haz the power, 2017-03-24 05:43:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 05:43:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dcheng, 2017-03-24 18:38:28]

OK, I added some buttons back, and changed the tests / buttons to use some
helpers. Hopefully that makes manual testing easier for now.

[dcheng, 2017-03-24 18:38:32]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2017-03-24 18:38:33]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2773573002/#ps80001
(title: "buttons")

[commit-bot: I haz the power, 2017-03-24 18:39:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 20:38:29]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490380712701410,
"parent_rev": "936e728a2bef7552fb07018d7075a5e996332bea", "commit_rev":
"e507bb334d3c66917eec47fbc476ed0a7d62f33f"}

[commit-bot: I haz the power, 2017-03-24 20:39:14]

Description was changed from

==========
window.open() should gate new tab/new popup based on toolbar visibility.

Previously, Chrome required that toolbar, menubar, scrollbars, status,
resizable were all set to enabled to open a window as a new tab rather
than a new popup. However, this causes developer frustration if one of
window features is accidentally omitted (as it then defaults to
disabled).

Instead, just use toolbar visibility to determine whether or not
window.open() creates a new popup or a new tab, which matches Firefox.

BUG=82522
==========

to

==========
window.open() should gate new tab/new popup based on toolbar visibility.

Previously, Chrome required that toolbar, menubar, scrollbars, status,
resizable were all set to enabled to open a window as a new tab rather
than a new popup. However, this causes developer frustration if one of
window features is accidentally omitted (as it then defaults to
disabled).

Instead, just use toolbar visibility to determine whether or not
window.open() creates a new popup or a new tab, which matches Firefox.

BUG=82522

Review-Url: https://codereview.chromium.org/2773573002
Cr-Commit-Position: refs/heads/master@{#459540}
Committed:
https://chromium.googlesource.com/chromium/src/+/e507bb334d3c66917eec47fbc476...
==========

[commit-bot: I haz the power, 2017-03-24 20:39:15]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/e507bb334d3c66917eec47fbc476...