|
|
Descriptionwindow.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
Patch Set 1 #Patch Set 2 : Update tests #Patch Set 3 : wpt test #Patch Set 4 : Move out of WPT #Patch Set 5 : buttons #
Messages
Total messages: 30 (21 generated)
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dcheng@chromium.org changed reviewers: + foolip@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
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)
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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?
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?
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
OK, I added some buttons back, and changed the tests / buttons to use some helpers. Hopefully that makes manual testing easier for now.
The CQ bit was checked by dcheng@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490380712701410, "parent_rev": "936e728a2bef7552fb07018d7075a5e996332bea", "commit_rev": "e507bb334d3c66917eec47fbc476ed0a7d62f33f"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/e507bb334d3c66917eec47fbc476... |