HTML: Add tests for optional window.open position, size features
#5390
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 @deniak, @dontcallmedom, @gsnedders, @jdm, @jgraham, @zcorpan, and @zqzhang. (Learn how reviewing works.) |
Firefox (nightly channel)Testing web-platform-tests at revision 4c1570c All results17 tests ran/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-negative-innerwidth-innerheight.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-negative-width-height.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-height.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-innerheight.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-innerwidth.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-screenx.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-screeny.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-innerheight-innerwidth.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-top-left.html| Subtest | Results | Messages | |
|
*This report has been truncated because the total content is 91534 characters in length, which is in excess of GitHub.com's limit for comments (65536 characters). Chrome (unstable channel)Testing web-platform-tests at revision 4c1570c Chrome (unstable channel)Testing web-platform-tests at revision 4c1570c Unstable results
Unstable results
All results17 tests ran/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-negative-innerwidth-innerheight.html| `" height = 402" should set height of opened window` | **FAIL: 6/10, PASS: 4/10** | `assert_equals: expected 402 but got 363` | | `/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-width-height.html` | `"height==402" should set height of opened window` | **FAIL: 6/10, PASS: 4/10** | `assert_equals: expected 402 but got 363` | | `/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-width-height.html` | `"\nheight= 402" should set height of opened window` | **FAIL: 8/10, PASS: 2/10** | `assert_equals: expected 402 but got 363` | | `/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-width-height.html` | `",height=402,," should set height of opened window` | **FAIL: 8/10, PASS: 2/10** | `assert_equals: expected 402 but got 363` |
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-negative-screenx-screeny.html
All results17 tests ran/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-negative-top-left.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-negative-screenx-screeny.html| Subtest | Results | Messages | /html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-negative-width-height.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-negative-top-left.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-height.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-negative-width-height.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-height.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-innerheight.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-innerheight.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-innerwidth.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-innerwidth.html| Subtest | Results | Messages |
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-screenx.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-screeny.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-screenx.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-screeny.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-top.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-width.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-top.html
/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-width.html| Subtest | Results | Messages | /html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-innerheight-innerwidth.html
|
zcorpan
reviewed
Apr 6, 2017
| assert_equals(data.left, 152); | ||
| })); | ||
| var win = window.open(prefixedMessage.url(windowURL), '', features); | ||
| }, `"${features}" (${idx + 1} of ${arr.length}) should set left position of opened window`); |
There was a problem hiding this comment.
Remove index/length from the test name. Removing or adding tests shouldn't cause other tests to be renamed.
| <script> | ||
| var windowURL = 'resources/message-opener.html'; | ||
|
|
||
| [ 'width=142', |
There was a problem hiding this comment.
Need bigger value for width and height as browsers have a minimum size for windows. Maybe 400 something is ok.
There was a problem hiding this comment.
FWIW, that value seems to be 100 in at least FireFox's case. But sure, yes, wider is fine!
|
You may also need to specify dimentions in all tests to avoid getting it opened as a tab... |
|
I made a commit to address some of my comments but did not have permission to push to your branch. For now pushed the branch to this repo, see 590ee4b |
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
|
@zcorpan: When I read the CSSOM View's coverage of |
|
@zcorpan Thanks for feedback! I have your commit now and I'm going to add some tests for invalid values (via rules for parsing integers ) and different combinations. |
|
I think specifying one of width or height works, it's just that the unspecified one might be too big which in turn overrides positioning (the spec allows adjusting values to fit available space). |
|
These tests are now available on w3c-test.org |
|
@zcorpan I've pushed a commit that adds some tests for invalid values of Anyway, before I go further: it seems evident that no browser implements parsing these values per the "rules for parsing integers". Am I misunderstanding the spec ? Is the spec wrong? Or just browsers not following the spec :) ? BTW, my source here is step 11 under step 5 (parsing the tokens) in the CSSOM View spec. While the HTML spec now, as of your recent changes, defines the tokenization itself, it doesn't—so far as I know—define the parsing of the tokens. Right? An aside: during the work on these tests, I got bogged down for some time by flaky behavior in my stable Chrome (57.0.2987.133) wherein sometimes opened windows inexplicably set their |
|
Don't use /TR/ version, use https://drafts.csswg.org/cssom-view/#the-features-argument-to-the-open()-method |
Maybe cssom-view is wrong. What do browsers do?
HTML doesn't define parsing of the values, right. It just passes |
zcorpan
reviewed
Apr 7, 2017
| [ 'top=/104', | ||
| 'top=_104', | ||
| 'top=L104', | ||
| 'top=0x68' |
There was a problem hiding this comment.
0x68 should probably be in a different bucket since it doesn't produce an error.
| 'top=105^4', | ||
| 'top=105*3', | ||
| 'top=105/5', | ||
| 'top=105 ' |
|
No need to worry about bugs in stable releases of browsers, here we care about dev/nightly. (Although as a policy we avoid depending on features that are not yet available in stable releases of browsers.) |
lyzadanger
force-pushed
the
window-open-optional-features
branch
from
211bf6e
to
650b414
Compare
|
@zcorpan I've fleshed out considerably and also cleaned up history. For the current batch of tests:
Of these, the Safari disparity might be worth checking out. Not sure. Note tests 015 and 016 : observed browser behavior vis-a-vis invalid |
Interesting! So for |
zcorpan
added a commit
to w3c/csswg-drafts
that referenced
this pull request
Apr 10, 2017
|
So negative values should instead be equivalent to baseline Would be good to test that |
Yep! You can see that in the negative value tests I use those values for setting up the baseline window.
Will work on this one. |
zcorpan
approved these changes
Apr 10, 2017
zcorpan
reviewed
Apr 10, 2017
| var featureString = `${featuresPrefix}left=0,${feature}`; | ||
| prefixedMessage.onMessage(t.step_func_done((data, e) => { | ||
| e.source.close(); | ||
| assert_equals(data.top, baselineDimensions.top, `"${feature} is negative and should be ignored"`); |
| var featureString = `${featuresPrefix}top=0,${feature}`; | ||
| prefixedMessage.onMessage(t.step_func_done((data, e) => { | ||
| e.source.close(); | ||
| assert_equals(data.left, baselineDimensions.left, `"${feature} is negative and should be ignored"`); |
| var featureString = `${featuresPrefix}left=0,${feature}`; | ||
| prefixedMessage.onMessage(t.step_func_done((data, e) => { | ||
| e.source.close(); | ||
| assert_equals(data.top, baselineDimensions.top, `"${feature} is negative and should be ignored"`); |
| var featureString = `${featuresPrefix}top=0,${feature}`; | ||
| prefixedMessage.onMessage(t.step_func_done((data, e) => { | ||
| e.source.close(); | ||
| assert_equals(data.left, baselineDimensions.left, `"${feature} is negative and should be ignored"`); |
| var featureString = `${featuresPrefix}height=405,${feature}`; | ||
| prefixedMessage.onMessage(t.step_func_done((data, e) => { | ||
| e.source.close(); | ||
| assert_equals(data.width, baselineDimensions.width, `"${feature} is negative and should be ignored"`); |
| var featureString = `${featuresPrefix}width=404,${feature}`; | ||
| prefixedMessage.onMessage(t.step_func_done((data, e) => { | ||
| e.source.close(); | ||
| assert_equals(data.height, baselineDimensions.height, `"${feature} is negative and should be ignored"`); |
lyzadanger
force-pushed
the
window-open-optional-features
branch
from
e34188d
to
3b84b3c
Compare
|
OK: Let's hope this gets the final test-message details right (and I cleaned up the branch)! |
|
If you check the checkbox in the sidebar to allow edits from maintainers then I can push to your branch. :-) The messages are good now 👍 A thing that irks me now (sorry!) is the filenames. I understand that you follow the documentation for css tests, but the numbering naming convention generally sucks since it's impossible to tell which of the 16 files is the one that tests non-integer values for Suggested fix: b84de1a The instability in Chrome sucks, and the bug you pointed to does not appear to be fixed... I can reproduce |
|
|
|
@zcorpan I've given you permissions to commit your changes here! Wanna take it from here? |
|
Sure! Thanks |
|
Also filed an issue for chromium |
zcorpan
added a commit
that referenced
this pull request
Apr 13, 2017
Followup to #5390 #5390 (review) > Would also be good to test interaction of width and innerwidth > features (which one is used if both are present, if they are > repeated...)
|
@bobholt, the formatting in #5390 (comment) seems to have gone wrong. Note how one of the test names is supposedly |
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
zcorpan
added a commit
that referenced
this pull request
Jun 21, 2017
Followup to #5390 #5390 (review) > Would also be good to test interaction of width and innerwidth > features (which one is used if both are present, if they are > repeated...)
zcorpan
added a commit
that referenced
this pull request
Aug 25, 2017
Followup to #5390 #5390 (review) > Would also be good to test interaction of width and innerwidth > features (which one is used if both are present, if they are > repeated...)
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
First, I'll say that I'm not entirely confident that these tests accomplish useful things, so I will not be miserable if the consensus is to not land this.
These tests are for the
left,top,width,height(and legacy equivalentsscreenx,screeny,innerheight,innerwidth) window features used inwindow.open, as per HTML 7.3.1 and related to new changes in spec around tokenization of features. All of these tests are optional, as browsers are not required to pay attention to any of those position or size features. Results are strongly affected by test-runner environments, etc. And obviously size/position for opened windows isn't relevant or has different constraints on many platforms (e.g. mobile).To annotate this optional nature, I dropped a
metatag in each per documentation about CSS tests. I recognize that this is not a flag typically used in JSharness tests.To support these tests, I whipped up a quick little tool to make it easier to have multiple tests in a test file that depend on
window.postMessageto get info from opened windows. It's modeled after what I did withLocalStorage. Hope that wasn't too presumptuous!This change is