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

"FYI" Review of new window.open() behavior #691

Closed
1 task done
mfreed7 opened this issue Nov 19, 2021 · 5 comments
Closed
1 task done

"FYI" Review of new window.open() behavior #691

mfreed7 opened this issue Nov 19, 2021 · 5 comments

Comments

@mfreed7
Copy link

mfreed7 commented Nov 19, 2021

Braw mornin' TAG!

I'm requesting a TAG review of the newly-specified window.open() behavior.

Per my Chromium Intent to Ship, it was requested that I file an FYI TAG review - hence this request.

Here is a summary of the changes, copied from the chromestatus entry for this change:


The window.open() API is currently confusing to use, in terms of trying to get it to open a popup vs. a tab/window. This change simplifies the usage by adding a single boolean argument called 'popup' that works as you'd expect: popup=1 gets you a popup, and popup=0 gets a tab/window:

const popup = window.open('_blank','','popup=1');
const tab = window.open('_blank','','popup=0');

Also there were previously confusingly-behaved getters for the BarProp visible properties (e.g. window.statusbar.visible) which didn't correctly represent what was actually visible. Now, those all return true if you got a new window/tab, and false if you got a popup.

This is an interop-driven change, to align Chromium with the newly-landed spec for window.open. It does not change existing behavior around whether a popup or tab/window is opened, to avoid compat issues.


Further details:

  • I have reviewed the TAG's Web Platform Design Principles
  • Relevant time constraints or deadlines: none
  • The group where the work on this specification is currently being done: WHATWG
  • The group where standardization of this work is intended to be done: WHATWG
  • Major unresolved issues with or opposition to this specification: None
  • This work is being funded by: Google/Mozilla

We'd prefer the TAG provide feedback as (please delete all but the desired option):

💬 leave review feedback as a comment in this issue and @-notify @mfreed7

@LeaVerou
Copy link
Member

LeaVerou commented Dec 9, 2021

@plinss and I took a look at this during our virtual f2f today.

We think that a boolean argument (popup=0 or popup=1) is not extensible, if in the future authors want to select
between more than these two kinds of window UI, whereas a keyword-based argument (e.g. type=popup)
would not have this problem.

As a side point, this sentence in the explainer was unclear:

The window.open() API is currently confusing to use, in terms of trying to get it to open a popup vs. a tab/window.

until I read this section on MDN:

Most modern browsers (listed below) don't allow web content to control the visibility of UI parts separately.
[snip]
UI-related items of windowFeatures are used as a condition to whether opening a popup or a new tab, or a new window, and UI parts visibility of each of them is fixed.

Furthermore, we understand that this method has a long history, but if it were designed today we would strongly recommend using a dictionary instead of a serialized string that has to be parsed and re-serialized, see: w3ctag/design-principles#213. It may be worth considering allowing users to pass a dictionary in place of this string for future enchancements, similarly to what was done for addEventListener().

We generally encourage people to seek feedback before features have already shipped in browsers.
Given that this has already shipped in both Blink and Gecko, it seems too late to have any of our feedback taken on board. Therefore we're closing this issue, feel free to ping us if there's anything further to discuss.

@mfreed7
Copy link
Author

mfreed7 commented Dec 14, 2021

Thanks for the review! I agree that it would have been better if this was brought to the TAG earlier. (I am merely the humble Chromium implementer - this change was primarily driven by others.) I do tend to agree with you that type=popup leaves more space for changes later. Though... this was discussed. It was proposed here on the issue, and then there was a discussion on the spec PR that resulted in the spec editors both agreeing that a boolean was better. I think the primary rationale was that a boolean was more parallel to the other recent additions to windowFeatures, such as noopener, noreferrer, etc.

I will post a heads-up back to that thread, pointing here, in case people want to take some action!

I think everyone likely agrees with you that a dictionary would have been better than the serialized string for windowFeatures, but that ship sailed a very long time ago.

Thanks again for the review!

@plinss
Copy link
Member

plinss commented Dec 14, 2021

@mfreed7 We know that the serialized string has a long history, our point about the dictionary is that it's not actually too late to change that. The method could take either a string or a dictionary for that argument, like how addEventListener was extended to take a dictionary or the legacy boolean. New features, such as this one, could possibly be restricted to the dictionary to help incentivize people to modernize their code (especially anything that would require extending the micro-syntax). Too late for this feature, but food for thought for the next one.

@mfreed7
Copy link
Author

mfreed7 commented Dec 14, 2021

Ahh very good point! Sorry I missed your point the first time. I brought up the point about type=popup on the issue thread, so I’ll also toss this one over there too. This was a recent change, so it’s possible there’s still room to change. But if not, yeah, next time. Thanks again!

@domenic
Copy link
Member

domenic commented Dec 14, 2021

I proposed such a redesign of window.open() in WICG/attribution-reporting-api#130 (comment) (for a different feature) but @annevk wasn't very interested, and instead suggested continuing to use the features string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants