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

noopener feature in window.open inconsistent with other boolean features #2600

Closed
zcorpan opened this Issue Apr 27, 2017 · 8 comments

Comments

4 participants
@zcorpan
Member

zcorpan commented Apr 27, 2017

Related:
#2476
#2464

I originally found this while reviewing the tests for tokenizing features: web-platform-tests/wpt#5306 (comment)

So there are features like toolbar which are boolean-like, but browsers look at the value to determine if it's true or false. For noopener the spec says to ignore the value. Should we make noopener consistent with the legacy boolean features?

The behavior for legacy booleans is basically:

  1. If value_string is the empty string or ASCII-case-insensitively "yes", set value to 1.
  2. Otherwise, parse value_string as an integer and set value to the result. Failure is 0.
  3. A non-zero value means true.

So for example, currently window.open(url, "", "noopener=0,toolbar=0") sets noopener to true but toolbar to false.

@lyzadanger @mikewest @cdumez @zetafunction

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Apr 27, 2017

Member

I suspect this was an oversight because we hadn't defined the actual parser. I think it would be good to make these the same as less parser code is a plus.

Member

annevk commented Apr 27, 2017

I suspect this was an oversight because we hadn't defined the actual parser. I think it would be good to make these the same as less parser code is a plus.

@zcorpan

This comment has been minimized.

Show comment
Hide comment
@zcorpan

zcorpan Apr 27, 2017

Member

It wouldn't really result in less parser code though. In Chromium this would be basically changing this line from setting to true to instead setting to value. So I think the main benefit is consistency, and allowing developers to do things like:

const features = `noopener=${noopener_enabled},toolbar=${toolbar_enabled}`;

instead of something more convoluted like:

const noopener = noopener_enabled ? "noopener," : "";
const features = `${noopener}toolbar=${toolbar_enabled}`;
Member

zcorpan commented Apr 27, 2017

It wouldn't really result in less parser code though. In Chromium this would be basically changing this line from setting to true to instead setting to value. So I think the main benefit is consistency, and allowing developers to do things like:

const features = `noopener=${noopener_enabled},toolbar=${toolbar_enabled}`;

instead of something more convoluted like:

const noopener = noopener_enabled ? "noopener," : "";
const features = `${noopener}toolbar=${toolbar_enabled}`;
@lyzadanger

This comment has been minimized.

Show comment
Hide comment
@lyzadanger

lyzadanger Apr 27, 2017

So there are features like toolbar which are boolean-like, but browsers look at the value to determine if it's true or false. For noopener the spec says to ignore the value. Should we make noopener consistent with the legacy boolean features?

I will say that when, initially getting familiar with the features-parsing vis-a-vis noopener, as an implementor, I expected it to behave like other legacy boolean features, so I'd generally vote for consistency here. Not sure about implementation impact, though...my opinion here is not strong, but it seems intuitive that noopener would act like other booleans.

lyzadanger commented Apr 27, 2017

So there are features like toolbar which are boolean-like, but browsers look at the value to determine if it's true or false. For noopener the spec says to ignore the value. Should we make noopener consistent with the legacy boolean features?

I will say that when, initially getting familiar with the features-parsing vis-a-vis noopener, as an implementor, I expected it to behave like other legacy boolean features, so I'd generally vote for consistency here. Not sure about implementation impact, though...my opinion here is not strong, but it seems intuitive that noopener would act like other booleans.

@cdumez

This comment has been minimized.

Show comment
Hide comment
@cdumez

cdumez Apr 27, 2017

Contributor

+1 for treating noopener similarly to other features. I was about to align WebKit's noopener support with the spec but I think it is even better if noopener=0 does not active 'noopener' feature.

Contributor

cdumez commented Apr 27, 2017

+1 for treating noopener similarly to other features. I was about to align WebKit's noopener support with the spec but I think it is even better if noopener=0 does not active 'noopener' feature.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Apr 28, 2017

Member

Note the last comment in https://bugs.chromium.org/p/chromium/issues/detail?id=630770 which also expected that to happen. (Stumbled upon that while trying to find something else.)

Member

annevk commented Apr 28, 2017

Note the last comment in https://bugs.chromium.org/p/chromium/issues/detail?id=630770 which also expected that to happen. (Stumbled upon that while trying to find something else.)

hubot pushed a commit to WebKit/webkit that referenced this issue Apr 28, 2017

cdumez@apple.com
Tweak window.open features argument tokenizer to match HTML standard …
…and Edge

https://bugs.webkit.org/show_bug.cgi?id=170548

Reviewed by Geoffrey Garen.

LayoutTests/imported/w3c:

* web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-noopener-expected.txt:
Rebaseline test now that more checks are passing. The remaining failures are because the test currently expects "noopener=0" / "noopener=false" to activate
the 'noopener' feature. The test matches the specification which currently says that if the 'noopener' key is present, then the 'noopener' feature should be
activated, no matter its value. However, I am intentionally not making this change yet because:
- This behavior would be inconsistent with other Window features
- There is upstream discussion on this (whatwg/html#2600) and the current feedback is that the specification should likely
  change to treat 'noopener' more consistently with other features.
I will follow-up once the specification / test settles.

* web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-noopener.html:
Re-sync test from upstream after web-platform-tests/wpt#5715.

Source/WebCore:

Update window.open() features argument tokenizer to match HTML standard:
- https://html.spec.whatwg.org/#concept-window-open-features-tokenize

Also update window.open() to return null instead of the window when
the 'noopener' feature is activated, as per:
- https://html.spec.whatwg.org/#dom-open (Step 10)

No new tests, rebaselined existing test.

* page/DOMWindow.cpp:
(WebCore::DOMWindow::createWindow):
Update window.open() to return null instead of the window when
the 'noopener' feature is activated, as per:
- https://html.spec.whatwg.org/#dom-open (Step 10)

* page/WindowFeatures.cpp:
(WebCore::isSeparator):
Treat all ASCII spaces as feature separators, as per:
- https://html.spec.whatwg.org/#feature-separator
This has the effect of adding U+000C (FormFeed) as a separator.

(WebCore::processFeaturesString):
Align tokenizing code with the specification:
- https://html.spec.whatwg.org/#concept-window-open-features-tokenize
In particular, the following changes were made:
- After the key, skip to first '=', but don't skip past a ',' or a non-separator.
  The "or a non-separator" part is new in the spec (step 3.6.1) and is now implemented.
- After looking for the '=', only treat what follows as a value if the current character
  is a separator. This is as per step 7 in the spec.
These changes now cause us to parse 'foo noopener=1' as ('foo', ''), ('noopener', '1').

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@215945 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@cdumez

This comment has been minimized.

Show comment
Hide comment
@cdumez

cdumez Oct 8, 2018

Contributor

Any update on this? WebKit recently started treating noopener differently than other booleans (https://trac.webkit.org/changeset/236802) in order to pass WPT tests. If the spec / WPT tests gets updated, I am happy to revert this change though. As I mentioned early, I'd prefer if all booleans were treated the same way.

Contributor

cdumez commented Oct 8, 2018

Any update on this? WebKit recently started treating noopener differently than other booleans (https://trac.webkit.org/changeset/236802) in order to pass WPT tests. If the spec / WPT tests gets updated, I am happy to revert this change though. As I mentioned early, I'd prefer if all booleans were treated the same way.

@zcorpan

This comment has been minimized.

Show comment
Hide comment
@zcorpan

zcorpan Oct 8, 2018

Member

I think we should do this. @cdumez would you be interested in making a PR for the spec?

Member

zcorpan commented Oct 8, 2018

I think we should do this. @cdumez would you be interested in making a PR for the spec?

@cdumez

This comment has been minimized.

Show comment
Hide comment
@cdumez

cdumez Oct 10, 2018

Contributor

I will give it a try.

Contributor

cdumez commented Oct 10, 2018

I will give it a try.

cdumez pushed a commit to cdumez/html that referenced this issue Oct 10, 2018

Chris Dumez
Specify parsing of 'noopener' feature in window.open
Specify parsing of 'noopener' feature in window.open so that noopener is only set to true
if the value is either:
- The empty string.
- The string "yes".
- The value 1 when parsed as an integer.

Fixes whatwg#2600.

cdumez pushed a commit to cdumez/html that referenced this issue Oct 10, 2018

Chris Dumez
Specify parsing of 'noopener' feature in window.open
Specify parsing of 'noopener' feature in window.open so that noopener is only set to true
if the value is either:
- The empty string.
- The string "yes".
- The value is not 0 or an error when parsed as an integer.

Fixes whatwg#2600.

zcorpan added a commit that referenced this issue Oct 11, 2018

Specify parsing of 'noopener' feature in window.open
Specify parsing of 'noopener' feature in window.open so that noopener is only set to true
if the value is either:
- The empty string.
- The string "yes".
- The value is not 0 or an error when parsed as an integer.

Fixes #2600.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment