HTML: Add tests for tokenization of noopener window.open feature #5306
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Notifying @jdm, @jgraham, @zcorpan, and @zqzhang. (Learn how reviewing works.) |
zcorpan
reviewed
Apr 3, 2017
| var featureVariants = [ | ||
| ' noopener', | ||
| 'noopener = ', | ||
| ' no opener', |
There was a problem hiding this comment.
This test is wrong. Per the proposed spec as it stands right now this is equivalent to no=opener, but that's wrong also (https://github.com/whatwg/html/pull/2476/files#r109389922). I think this should result in name no opener and empty value (so unknown feature name). Let me try to fix the spec...
There was a problem hiding this comment.
I must've misread the steps about ignoring whitespace in the tokenization: is it the case that whitespace inside/within of a name is not ignored? Only "trimmed" from start/end, as it were? I saw some comments in the HTML spec thread about white space and specific handling of characters. That's helpful information! Keep it coming and I'll get the right tests in place.
zcorpan
reviewed
Apr 3, 2017
| test (t => { | ||
| var featureVariants = [ | ||
| 'noopener=false', | ||
| ',noopener=0, ', |
There was a problem hiding this comment.
I was thinking a bit about this case the other day. Maybe we should specify noopener to use the same logic as other "boolean" features like toolbar. They're set to true if the value is empty string or "yes" or parses to a non-zero integer (not sure about the range, I think short in chromium), otherwise false.
There was a problem hiding this comment.
@mikewest any opinion about this one? I think it is somewhat surprising that the noopener feature is enabled in toolbar=0,status=0,noopener=0...
|
The general approach looks good. We can merge the tests and the changes to the standard at the same time when they're ready. |
|
These tests are now available on w3c-test.org |
|
@zcorpan Adjusted the bad whitespace-ish test. I'd like to keep this open and watch the specifics of the spec change PR for a bit so I can add more specific tests as language changes... |
|
@zcorpan Updated significantly with details from updated spec PR. Locally Chrome is passing every single test, which is remarkable. Still open:
|
|
Opened a Mozilla bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1353466 |
|
*This report has been truncated because the total content is 69155 characters in length, which is in excess of GitHub.com's limit for comments (65536 characters). Firefox (nightly channel)Testing web-platform-tests at revision ef14bd8 Firefox (nightly channel)Testing web-platform-tests at revision ef14bd8 All results10 tests ran/IndexedDB/idbdatabase-transaction-exception-order.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-001.html| Subtest | Results | y>10 tests ran /IndexedDB/idbdatabase-transaction-exception-order.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-001.html
/html/webappapis/timers/type-long-setinterval.html
/html/webappapis/timers/type-long-settimeout.htmlshould read characters until first window feature separator as value` | FAIL | `assert_equals: "noopener=noopener" should set "noopener" expected null but got object "[object Window]"` | | `"noopener" should be based on name (key), not value` | FAIL | `assert_equals: "noopener=false" should activate feature "noopener" expected null but got object "[object Window]"` | | `invalid feature names should not tokenize as "noopener"` | PASS | |/html/webappapis/timers/type-long-setinterval.html
/html/webappapis/timers/type-long-settimeout.html
/streams/piping/pipe-through.dedicatedworker.html
/streams/piping/pipe-through.dedicatedworker.html
/streams/piping/pipe-through.html
/streams/piping/pipe-through.html
/streams/piping/pipe-through.serviceworker.https.html
/streams/piping/pipe-through.serviceworker.https.html
/streams/piping/pipe-through.sharedworker.html| Subtest | Results | Messages | /streams/piping/pipe-through.sharedworker.html
/webrtc/rtcpeerconnection/canTrickleIceCandidates.html| Subtest | Results | Messa-| /webrtc/rtcpeerconnection/canTrickleIceCandidates.html
/webvr/idlharness.html
/webvr/idlharness.html
|
Chrome (unstable channel)Testing web-platform-tests at revision ef14bd8 All results10 tests ran/IndexedDB/idbdatabase-transaction-exception-order.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-001.html
/html/webappapis/timers/type-long-setinterval.html
/html/webappapis/timers/type-long-settimeout.html
/streams/piping/pipe-through.dedicatedworker.html
/streams/piping/pipe-through.html
/streams/piping/pipe-through.serviceworker.https.html
/streams/piping/pipe-through.sharedworker.html
/webrtc/rtcpeerconnection/canTrickleIceCandidates.html
/webvr/idlharness.html
|
|
I think something like |
|
@zcorpan: Improved tests around breaking on separators, like the |
zcorpan
reviewed
Apr 4, 2017
| @@ -79,6 +81,7 @@ | |||
| 'noopener = \t ,', | |||
| 'noopener\n=\r noopener,', // => ('noopener', 'noopener') | |||
| 'noopener=,yes', // => ('noopener'), ('yes') | |||
| 'noopener= foo=,' // => ('noopener', ''), ('foo', '') | |||
|
This is looking pretty good! Again @w3c-bots is confused about the commit range. -_- It would be good with tests for U+0000 and U+000C as well. After that I think this and the spec change are ready to go. We can deal with |
|
@zcorpan You want I should do some rebasing to make this less chattery, history-wise? |
|
Sure, a squash and rebase from latest master might also bring sanity to the stability checker. |
lyzadanger
force-pushed
the
window-open-feature-noopener
branch
from
16c744b
to
6e1b23b
Compare
zcorpan
approved these changes
Apr 5, 2017
zcorpan
requested changes
Apr 5, 2017
| '\n=noopener=', | ||
| '\tnoopener', | ||
| '\r,,,=noopener', | ||
| '\u0000noopener', |
There was a problem hiding this comment.
Wait, this one is wrong. U+0000 is not a separator; ('\0noopener', '') is not a supported feature.
zcorpan
approved these changes
Apr 5, 2017
zcorpan
added a commit
to w3c/csswg-drafts
that referenced
this pull request
Apr 6, 2017
See whatwg/html#2476 Also change the behavior for left=foo to act as if left=0 to align with the behavior of at least WebKit and Chromium. Tests: web-platform-tests/wpt#5306 web-platform-tests/wpt#5390
zcorpan
added a commit
to whatwg/html
that referenced
this pull request
Apr 6, 2017
This was specified in CSSOM View but the "noopener" feature did not use the same tokenizer as the legacy features. Fixes #2474. Also specify the aliases screenx, screeny, innerwidth, innerheight for left, top, width, and height, respectively. Part of #2464. Closes w3c/csswg-drafts#1128. The tokenizer specified here closely follows Edge. Chromium and WebKit are also very similar to Edge. Difference from Edge: U+0000 does not end the string. Difference from Chromium: U+0000 is not a separator. Difference from WebKit/Chromium/Edge: U+000C is a separator. For the input `width toolbar=450, height=450`, Edge tokenizes like `width, toolbar=450, height=450` while WebKit/Chromium like `width=450, height=450`. The Edge behavior seems better. Tests: web-platform-tests/wpt#5306 web-platform-tests/wpt#5390
cdumez
reviewed
Apr 26, 2017
| 'noopener,=', | ||
| 'noopener foo', // => ('noopener', ''), ('foo', '') | ||
| 'foo noopener=1', // => ('foo', ''), ('noopener', '1') | ||
| 'foo=\u000Cnoopener' // => ('foo', ''), ('noopener', '') |
There was a problem hiding this comment.
I am unclear why this should activate noopener feature. I personally would have expected the result to be ('foo', 'noopener'). I would think the loop at 7.1 (https://html.spec.whatwg.org/#concept-window-open-features-tokenize) would skip past both the '=' and the '\u000C' since both are feature separators. Could someone please clarify?
There was a problem hiding this comment.
This is a bug in the test. Oops! I can fix.
http://software.hixie.ch/utilities/js/live-dom-viewer/saved/5061
zcorpan
added a commit
that referenced
this pull request
Apr 27, 2017
Found by cdumez in #5306 (comment)
This was referenced Apr 27, 2017
annevk
pushed a commit
that referenced
this pull request
Apr 27, 2017
Found by cdumez in #5306 (comment).
inikulin
pushed a commit
to HTMLParseErrorWG/html
that referenced
this pull request
May 9, 2017
This was specified in CSSOM View but the "noopener" feature did not use the same tokenizer as the legacy features. Fixes whatwg#2474. Also specify the aliases screenx, screeny, innerwidth, innerheight for left, top, width, and height, respectively. Part of whatwg#2464. Closes w3c/csswg-drafts#1128. The tokenizer specified here closely follows Edge. Chromium and WebKit are also very similar to Edge. Difference from Edge: U+0000 does not end the string. Difference from Chromium: U+0000 is not a separator. Difference from WebKit/Chromium/Edge: U+000C is a separator. For the input `width toolbar=450, height=450`, Edge tokenizes like `width, toolbar=450, height=450` while WebKit/Chromium like `width=450, height=450`. The Edge behavior seems better. Tests: web-platform-tests/wpt#5306 web-platform-tests/wpt#5390
inikulin
pushed a commit
to HTMLParseErrorWG/html
that referenced
this pull request
May 9, 2017
This was specified in CSSOM View but the "noopener" feature did not use the same tokenizer as the legacy features. Fixes whatwg#2474. Also specify the aliases screenx, screeny, innerwidth, innerheight for left, top, width, and height, respectively. Part of whatwg#2464. Closes w3c/csswg-drafts#1128. The tokenizer specified here closely follows Edge. Chromium and WebKit are also very similar to Edge. Difference from Edge: U+0000 does not end the string. Difference from Chromium: U+0000 is not a separator. Difference from WebKit/Chromium/Edge: U+000C is a separator. For the input `width toolbar=450, height=450`, Edge tokenizes like `width, toolbar=450, height=450` while WebKit/Chromium like `width=450, height=450`. The Edge behavior seems better. Tests: web-platform-tests/wpt#5306 web-platform-tests/wpt#5390
alice
pushed a commit
to alice/html
that referenced
this pull request
Jan 8, 2019
This was specified in CSSOM View but the "noopener" feature did not use the same tokenizer as the legacy features. Fixes whatwg#2474. Also specify the aliases screenx, screeny, innerwidth, innerheight for left, top, width, and height, respectively. Part of whatwg#2464. Closes w3c/csswg-drafts#1128. The tokenizer specified here closely follows Edge. Chromium and WebKit are also very similar to Edge. Difference from Edge: U+0000 does not end the string. Difference from Chromium: U+0000 is not a separator. Difference from WebKit/Chromium/Edge: U+000C is a separator. For the input `width toolbar=450, height=450`, Edge tokenizes like `width, toolbar=450, height=450` while WebKit/Chromium like `width=450, height=450`. The Edge behavior seems better. Tests: web-platform-tests/wpt#5306 web-platform-tests/wpt#5390
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Attn @zcorpan on this one. Apologies in advance that the summary here is long-ish.
There's still a bunch of work to be done in this area (testing
window.openfeatures overall) but I wanted to open a smallish PR here as there are more wrinkles than usual. This series of tests chucks differently-formatted variants of thenoopenerfeature atwindow.open.First, note that this PR is for spec stuff that hasn't landed as of time of writing, but is looking like it is getting there: whatwg/html#2476 ... bit of a chicken and egg situation: I don't know whether tests need to land first here or whether that spec change needs to be merged first.
While Firefox has implemented the
noopenersupport (disowningopenerand returningnullas expected in those cases), it has some not-to-spec behavior aroundwindow.close. To wit: a newly-created browser context with noopenercannot close itself.window.closein those situations will log a warning ("Scripts may not close windows that were not opened by script.") and theclosewill be unsuccessful. This behavior is alluded to in this bug thread but does not appear to have a bug opened specifically against it. Current spec language suggests thewindowin this case should be closable:The current FF behavior means that for any time a
windowis opened and successfully disowns itsopener, that window gets left behind at the end of the tests because it cannot be closed by any method I know of (automatically). Now, FF is currently failing to tokenize most variants ofnoopenerso there aren't many windows left behind (just one for now), but, ick.Which leads to another question: does this open too many windows for one test file? Are there performance/timeout concerns?
And also: are my assumptions for feature names that should be tokenized to
noopenerultimately accurate? I tried to step through the tokenization steps and think of various valid and invalid variants.Anyway, like I said, I wanted to get this out there before continuing on to other pieces of this in case I'm way off base.
This change is