Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standardize the condition for opening a popup by window.open, and BarProp values #6530

Merged
merged 7 commits into from
Oct 29, 2021

Conversation

arai-a
Copy link
Contributor

@arai-a arai-a commented Mar 25, 2021

For fixing #5872 and #4431

The proposed behavior of the condition for opening popup by window.open (open a popup only when width is specified) is based on current Safari's behavior, that's changed from the current Chrome's behavior, so I expect not so much breakage by standardizing this.

The proposed behavior of BarProp values is based on current Chrome's behavior, so I expect not so much breakage from this as well.


/browsers.html ( diff )
/infrastructure.html ( diff )
/window-object.html ( diff )

@birtles
Copy link
Contributor

birtles commented Mar 26, 2021

@annevk I've just signed up Birchill as a participant in WHATWG and Arai-san is a member of the birchill GitHub org but I think I need an editor to manually verify the submission and Birchill's participation in order for the Participation check to pass?

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me. Only nits. Hopefully @domenic can help out to push this over the finish line, but otherwise happy to help when I'm back as I mentioned.

@birtles thanks for proactively organizing that! I verified the entity.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed some edits; please take a look and make sure they look good to you.

Editorially this LGTM. However I worry that your implementer commitments line is not fully accurate; you seem to link to various comments and commentary on the general project, but not support for the exact proposal you've suggested here.

The best way to figure that out is to probably finish the test suite to make sure it covers all the normative changes (I'll check on that now), and see if all browsers pass them. If so, then we can take that as support. Otherwise, we'll want to get more explicit signoff on the spec text you've written here.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@domenic
Copy link
Member

domenic commented Mar 29, 2021

Hmm, web-platform-tests/wpt#28243 doesn't seem to match this PR at all? This PR only consults width (and its alias innerwidth), but the WPTs imply that many more features cause is popup to become true (as measured by the BarProp interfaces).

@domenic domenic added the do not merge yet Pull request must not be merged per rationale in comment label Mar 29, 2021
@domenic
Copy link
Member

domenic commented Mar 29, 2021

Oh, sorry, I read the tests backward. OK, so it's just a question of whether browsers match them... let's go check the results.

@domenic
Copy link
Member

domenic commented Mar 29, 2021

OK, so no browsers currently match the tests:

  • Chrome seems to always set the BarProp props to false
  • Firefox seems to do the exact opposite of this PR
  • Safari seems to always set the BarProp props to true.

So there's definitely some work needed here to gather multi-implementer interest by continuing the discussions in #5872.

@arai-a
Copy link
Contributor Author

arai-a commented Mar 29, 2021

Thank you!
the edits look good to me.

And yes, the proposed behavior doesn't match any implementation (it's a mix of Chrome + Safari), and the testcase is supposed to fail on all of them, in the current behavior.

@domenic domenic added the needs implementer interest Moving the issue forward requires implementers to express interest label Mar 29, 2021
@annevk
Copy link
Member

annevk commented Apr 20, 2021

I thought we would match https://arai-a.github.io/window-open-features/#IsPopup-Safari which suggests Safari does return false if width is passed? Could that be because in automation it doesn't create actual popups? And the same for Chrome I suppose, comparing the results with your earlier analysis. Having said that, if we can get away with always returning true (or false for that matter) for these properties I think that would be acceptable too.

@arai-a
Copy link
Contributor Author

arai-a commented Apr 20, 2021

I thought we would match https://arai-a.github.io/window-open-features/#IsPopup-Safari which suggests Safari does return false if width is passed?

the condition to use popup in this PR matches to the current Safari's behavior.
it opens popup whenever width feature is explicitly specified, regardless of other features.

Other browsers uses different condition and needs change.
Chrome needs to check width, and drop other features,
Firefox is already checking width, but needs to drop other features.

Could that be because in automation it doesn't create actual popups?

I don't get your point.
What automation is it about?

is it about web-platform-tests/wpt#28243 ?
I've confirmed this fails with Firefox Nightly.

And the same for Chrome I suppose, comparing the results with your earlier analysis.

Chrome doesn't use width as condition, as far as I know.

Having said that, if we can get away with always returning true (or false for that matter) for these properties I think that would be acceptable too.

here too I don't get your point.
can you describe more?

@annevk
Copy link
Member

annevk commented Apr 20, 2021

Per @domenic's link above Safari fails the tests for width and innerWidth (returning true there too) in automation. I tried the tests locally in both Safari and Safari Technology Preview and there I get the same results (after trying a few times to ensure Safari knows it's fine to open that many windows), suggesting Safari always returns true.

@arai-a
Copy link
Contributor Author

arai-a commented Apr 20, 2021

The testcase checks the relation between width feature and BarProp value, instead of checking whether it opens popup or not, given "popup or not" isn't normative.

Then, Safari fails because their BarProp value is different than the PR
#4431 (comment)

This PR uses Chrome's behavior, that returns same value for all BarProps,
but Safari always return true for menubar, scrollbars, and statusbar.

Safari's locationbar, personalbar, and toolbar follows this PR.

@annevk
Copy link
Member

annevk commented Apr 20, 2021

Aah thanks, that's what I was missing.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this still looks great to me. Found a couple nits.

One thing I was wondering about is whether we want type as an enum or popup as a boolean feature. A popup boolean seemed slightly better to me as a) it's also a window and b) it matches the precedent of other recent additions such as noopener.

Would be good if @mfreed7 could have a look at this too.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
@michaelwasserman
Copy link

michaelwasserman commented Oct 11, 2021

An enum would be more extensible and could potentially parallel the Web Applications Manifest's display modes: (fullscreen, standalone, minimal-ui, browser), or perhaps even the newer display-override member, which lets applications specify a fallback chain of modes, in case some are not supported.

@arai-a
Copy link
Contributor Author

arai-a commented Oct 12, 2021

To align with display modes,and avoid "window" word, possible options would be "browser" and "minimal-ui", but I'm not sure saying "browser" there makes much sense.
perhaps "standard" and "minimal-ui" ?

@domenic
Copy link
Member

domenic commented Oct 12, 2021

I don't think an enum makes much sense here. window.open() is not related to PWA window modes. I agree with @annevk that a boolean seems better.

@arai-a
Copy link
Contributor Author

arai-a commented Oct 12, 2021

Okay. changed to use popup boolean feature, and use it when it's present.

Copy link
Contributor

@mfreed7 mfreed7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good! Thanks for also updating the proposal doc - that's very helpful.

These changes seem like they are much more likely to be web compatible. I wonder if it would be worthwhile trying to actively deprecate usage of the "old" feature strings, e.g. by emitting console warnings when they're used? That'll likely just end up being console spam, so maybe not.

source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@annevk annevk requested a review from domenic October 27, 2021 12:38
@annevk annevk removed needs implementer interest Moving the issue forward requires implementers to express interest do not merge yet Pull request must not be merged per rationale in comment labels Oct 27, 2021
@annevk annevk requested a review from mfreed7 October 29, 2021 12:17
@mfreed7
Copy link
Contributor

mfreed7 commented Oct 29, 2021

Apparently Chromium reflects the specified UI-reated feature to BarProp.visible value, regardless of the actual visibility in the UI. (I've used only width feature there to test, that makes all visible properties false, but that didn't mean they're always false)

I did understand this difference when we discussed this change at the last triage meeting. So this isn't a surprise to me, and I'm ok with this change. I think the likelihood of compat issues is much less for this change than it was for the change to the "triggers" for a popup.

I already have code in Chromium to do this, behind the flag I was using for the prior change. So I can easily enable just this behavior. Sounds like now's the time to do that. Or shortly after the tests and this PR land.

Copy link
Contributor

@mfreed7 mfreed7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

domenic pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 29, 2021
@domenic domenic merged commit ab08b9b into whatwg:main Oct 29, 2021
@domenic
Copy link
Member

domenic commented Oct 29, 2021

Thanks for all the effort here @arai-a! It's great to get interop on this.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 2, 2021
…by window.open, and BarProp values for each case, a=testonly

Automatic update from web-platform-tests
HTML: test BarProp and window.open() feature interactions

For whatwg/html#5872, whatwg/html#4431, and whatwg/html#6530.
--

wpt-commits: 29faceaa68f5cb31798cc17fe6868eaf5fa5fb7f
wpt-pr: 28243
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Nov 3, 2021
…by window.open, and BarProp values for each case, a=testonly

Automatic update from web-platform-tests
HTML: test BarProp and window.open() feature interactions

For whatwg/html#5872, whatwg/html#4431, and whatwg/html#6530.
--

wpt-commits: 29faceaa68f5cb31798cc17fe6868eaf5fa5fb7f
wpt-pr: 28243
arai-a added a commit to arai-a/content that referenced this pull request Nov 7, 2021
…Prop values

for whatwg/html#6530

 * Added popup feature
 * Added description about requesting popup
 * Changed examples not to use the backward-compat-only UI parts features
 * Updated BarProp.visible to reflect popup request
 * Moved outerWidth/outerHeight to obsolete features page
arai-a added a commit to arai-a/browser-compat-data that referenced this pull request Nov 7, 2021
arai-a added a commit to arai-a/browser-compat-data that referenced this pull request Nov 7, 2021
arai-a added a commit to arai-a/browser-compat-data that referenced this pull request Nov 7, 2021
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 10, 2021
See [1] and [2] for more context, but this CL implements new behavior
for how window.open() interprets the windowPreferences argument when
deciding whether to open the window as a new tab or as a "popup",
which is a separate window with minimal UI (toolbars, onmibox,
etc.), and also what to return from the BarProp visible properties,
e.g. window.toolbar.visible.

The existing "trigger" behavior for popups will be retained by this
CL, namely that a popup will be opened instead of a tab if:
 1. the windowFeatures parameter is *not* empty, and
 2. one of the following conditions is true:
  * both `location` and `toolbar` are false or missing
  * `menubar` is false or missing
  * `resizable is false or missing
  * `scrollbar` is false or missing
  * `status` is false or missing

With this CL, an additional windowFeature called 'popup' is added,
so that if 'popup' is present and truthy.

Additionally, all BarProp properties (locationbar,menubar,
personalbar,scrollbars,statusbar, and toolbar) will always return
the same values, either false if a popup was opened, or true if
a tab/window was opened.

The spec for this behavior is part of the HTML spec:
https://html.spec.whatwg.org/multipage/window-object.html#popup-window-is-requested

The intent to ship is here:
https://groups.google.com/a/chromium.org/g/blink-dev/c/q6ybnmxxvpE

[1] whatwg/html#5872
[2] whatwg/html#6530

Fixed: 1192701
Change-Id: I50e745b1000d460c49085edd57d13f420b875ff3
arai-a added a commit to arai-a/browser-compat-data that referenced this pull request Nov 16, 2021
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 19, 2021
See [1] and [2] for more context, but this CL implements new behavior
for how window.open() interprets the windowPreferences argument when
deciding whether to open the window as a new tab or as a "popup",
which is a separate window with minimal UI (toolbars, onmibox,
etc.), and also what to return from the BarProp visible properties,
e.g. window.toolbar.visible.

The existing "trigger" behavior for popups will be retained by this
CL, namely that a popup will be opened instead of a tab if:
 1. the windowFeatures parameter is *not* empty, and
 2. one of the following conditions is true:
  * both `location` and `toolbar` are false or missing
  * `menubar` is false or missing
  * `resizable is false or missing
  * `scrollbar` is false or missing
  * `status` is false or missing

With this CL, an additional windowFeature called 'popup' is added,
so that if 'popup' is present and truthy.

Additionally, all BarProp properties (locationbar,menubar,
personalbar,scrollbars,statusbar, and toolbar) will always return
the same values, either false if a popup was opened, or true if
a tab/window was opened.

The spec for this behavior is part of the HTML spec:
https://html.spec.whatwg.org/multipage/window-object.html#popup-window-is-requested

The intent to ship is here:
https://groups.google.com/a/chromium.org/g/blink-dev/c/q6ybnmxxvpE

[1] whatwg/html#5872
[2] whatwg/html#6530

Fixed: 1192701
Change-Id: I50e745b1000d460c49085edd57d13f420b875ff3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2950386
Reviewed-by: Joey Arhar <[email protected]>
Commit-Queue: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#943716}
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this pull request Nov 20, 2021
See [1] and [2] for more context, but this CL implements new behavior
for how window.open() interprets the windowPreferences argument when
deciding whether to open the window as a new tab or as a "popup",
which is a separate window with minimal UI (toolbars, onmibox,
etc.), and also what to return from the BarProp visible properties,
e.g. window.toolbar.visible.

The existing "trigger" behavior for popups will be retained by this
CL, namely that a popup will be opened instead of a tab if:
 1. the windowFeatures parameter is *not* empty, and
 2. one of the following conditions is true:
  * both `location` and `toolbar` are false or missing
  * `menubar` is false or missing
  * `resizable is false or missing
  * `scrollbar` is false or missing
  * `status` is false or missing

With this CL, an additional windowFeature called 'popup' is added,
so that if 'popup' is present and truthy.

Additionally, all BarProp properties (locationbar,menubar,
personalbar,scrollbars,statusbar, and toolbar) will always return
the same values, either false if a popup was opened, or true if
a tab/window was opened.

The spec for this behavior is part of the HTML spec:
https://html.spec.whatwg.org/multipage/window-object.html#popup-window-is-requested

The intent to ship is here:
https://groups.google.com/a/chromium.org/g/blink-dev/c/q6ybnmxxvpE

[1] whatwg/html#5872
[2] whatwg/html#6530

Fixed: 1192701
Change-Id: I50e745b1000d460c49085edd57d13f420b875ff3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2950386
Reviewed-by: Joey Arhar <[email protected]>
Commit-Queue: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#943716}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 23, 2021
See [1] and [2] for more context, but this CL implements new behavior
for how window.open() interprets the windowPreferences argument when
deciding whether to open the window as a new tab or as a "popup",
which is a separate window with minimal UI (toolbars, onmibox,
etc.), and also what to return from the BarProp visible properties,
e.g. window.toolbar.visible.

The existing "trigger" behavior for popups will be retained by this
CL, namely that a popup will be opened instead of a tab if:
 1. the windowFeatures parameter is *not* empty, and
 2. one of the following conditions is true:
  * both `location` and `toolbar` are false or missing
  * `menubar` is false or missing
  * `resizable is false or missing
  * `scrollbar` is false or missing
  * `status` is false or missing

With this CL, an additional windowFeature called 'popup' is added,
so that if 'popup' is present and truthy.

Additionally, all BarProp properties (locationbar,menubar,
personalbar,scrollbars,statusbar, and toolbar) will always return
the same values, either false if a popup was opened, or true if
a tab/window was opened.

The spec for this behavior is part of the HTML spec:
https://html.spec.whatwg.org/multipage/window-object.html#popup-window-is-requested

The intent to ship is here:
https://groups.google.com/a/chromium.org/g/blink-dev/c/q6ybnmxxvpE

[1] whatwg/html#5872
[2] whatwg/html#6530

Fixed: 1192701
Change-Id: I50e745b1000d460c49085edd57d13f420b875ff3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2950386
Reviewed-by: Joey Arhar <[email protected]>
Commit-Queue: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#943716}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 30, 2021
…ndowPreferences and popups, a=testonly

Automatic update from web-platform-tests
Change behavior of window.open w.r.t. windowPreferences and popups

See [1] and [2] for more context, but this CL implements new behavior
for how window.open() interprets the windowPreferences argument when
deciding whether to open the window as a new tab or as a "popup",
which is a separate window with minimal UI (toolbars, onmibox,
etc.), and also what to return from the BarProp visible properties,
e.g. window.toolbar.visible.

The existing "trigger" behavior for popups will be retained by this
CL, namely that a popup will be opened instead of a tab if:
 1. the windowFeatures parameter is *not* empty, and
 2. one of the following conditions is true:
  * both `location` and `toolbar` are false or missing
  * `menubar` is false or missing
  * `resizable is false or missing
  * `scrollbar` is false or missing
  * `status` is false or missing

With this CL, an additional windowFeature called 'popup' is added,
so that if 'popup' is present and truthy.

Additionally, all BarProp properties (locationbar,menubar,
personalbar,scrollbars,statusbar, and toolbar) will always return
the same values, either false if a popup was opened, or true if
a tab/window was opened.

The spec for this behavior is part of the HTML spec:
https://html.spec.whatwg.org/multipage/window-object.html#popup-window-is-requested

The intent to ship is here:
https://groups.google.com/a/chromium.org/g/blink-dev/c/q6ybnmxxvpE

[1] whatwg/html#5872
[2] whatwg/html#6530

Fixed: 1192701
Change-Id: I50e745b1000d460c49085edd57d13f420b875ff3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2950386
Reviewed-by: Joey Arhar <[email protected]>
Commit-Queue: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#943716}

--

wpt-commits: 3660b77d3cec8138401c12e055bee44d62e9b060
wpt-pr: 29334
arai-a added a commit to arai-a/browser-compat-data that referenced this pull request Nov 30, 2021
arai-a added a commit to arai-a/browser-compat-data that referenced this pull request Nov 30, 2021
arai-a added a commit to arai-a/browser-compat-data that referenced this pull request Nov 30, 2021
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Nov 30, 2021
…ndowPreferences and popups, a=testonly

Automatic update from web-platform-tests
Change behavior of window.open w.r.t. windowPreferences and popups

See [1] and [2] for more context, but this CL implements new behavior
for how window.open() interprets the windowPreferences argument when
deciding whether to open the window as a new tab or as a "popup",
which is a separate window with minimal UI (toolbars, onmibox,
etc.), and also what to return from the BarProp visible properties,
e.g. window.toolbar.visible.

The existing "trigger" behavior for popups will be retained by this
CL, namely that a popup will be opened instead of a tab if:
 1. the windowFeatures parameter is *not* empty, and
 2. one of the following conditions is true:
  * both `location` and `toolbar` are false or missing
  * `menubar` is false or missing
  * `resizable is false or missing
  * `scrollbar` is false or missing
  * `status` is false or missing

With this CL, an additional windowFeature called 'popup' is added,
so that if 'popup' is present and truthy.

Additionally, all BarProp properties (locationbar,menubar,
personalbar,scrollbars,statusbar, and toolbar) will always return
the same values, either false if a popup was opened, or true if
a tab/window was opened.

The spec for this behavior is part of the HTML spec:
https://html.spec.whatwg.org/multipage/window-object.html#popup-window-is-requested

The intent to ship is here:
https://groups.google.com/a/chromium.org/g/blink-dev/c/q6ybnmxxvpE

[1] whatwg/html#5872
[2] whatwg/html#6530

Fixed: 1192701
Change-Id: I50e745b1000d460c49085edd57d13f420b875ff3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2950386
Reviewed-by: Joey Arhar <[email protected]>
Commit-Queue: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#943716}

--

wpt-commits: 3660b77d3cec8138401c12e055bee44d62e9b060
wpt-pr: 29334
ddbeck pushed a commit to mdn/browser-compat-data that referenced this pull request Nov 30, 2021
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
See [1] and [2] for more context, but this CL implements new behavior
for how window.open() interprets the windowPreferences argument when
deciding whether to open the window as a new tab or as a "popup",
which is a separate window with minimal UI (toolbars, onmibox,
etc.), and also what to return from the BarProp visible properties,
e.g. window.toolbar.visible.

The existing "trigger" behavior for popups will be retained by this
CL, namely that a popup will be opened instead of a tab if:
 1. the windowFeatures parameter is *not* empty, and
 2. one of the following conditions is true:
  * both `location` and `toolbar` are false or missing
  * `menubar` is false or missing
  * `resizable is false or missing
  * `scrollbar` is false or missing
  * `status` is false or missing

With this CL, an additional windowFeature called 'popup' is added,
so that if 'popup' is present and truthy.

Additionally, all BarProp properties (locationbar,menubar,
personalbar,scrollbars,statusbar, and toolbar) will always return
the same values, either false if a popup was opened, or true if
a tab/window was opened.

The spec for this behavior is part of the HTML spec:
https://html.spec.whatwg.org/multipage/window-object.html#popup-window-is-requested

The intent to ship is here:
https://groups.google.com/a/chromium.org/g/blink-dev/c/q6ybnmxxvpE

[1] whatwg/html#5872
[2] whatwg/html#6530

Fixed: 1192701
Change-Id: I50e745b1000d460c49085edd57d13f420b875ff3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2950386
Reviewed-by: Joey Arhar <[email protected]>
Commit-Queue: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#943716}
NOKEYCHECK=True
GitOrigin-RevId: cdad240f282dca85fb85fa3ecc809d7f34862996
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants