-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
URL: ToASCII #5976
URL: ToASCII #5976
Conversation
@rmisev would you be interested in reviewing this? I included tests you created for whatwg/url#309. I'll also go ahead and file issues against all browsers so we get ToASCII handling consistent, even when the input is just ASCII. I suppose I'll also file an issue against Node.js. |
(This is basically a much simplified version of #4504, to get the high-level issues between implementations ironed out first.) |
*This report has been truncated because the total content is 252267 characters in length, which is in excess of GitHub.com's limit for comments (65536 characters). Firefox (nightly)Testing web-platform-tests at revision b5d96af All results1 test ran/url/toascii.window.html
|
*This report has been truncated because the total content is 177786 characters in length, which is in excess of GitHub.com's limit for comments (65536 characters). Sauce (safari)Testing web-platform-tests at revision b5d96af All results1 test ran/url/toascii.window.html
| |
*This report has been truncated because the total content is 182604 characters in length, which is in excess of GitHub.com's limit for comments (65536 characters). Chrome (unstable)Testing web-platform-tests at revision b5d96af All results1 test ran/url/toascii.window.html
|
*This report has been truncated because the total content is 259355 characters in length, which is in excess of GitHub.com's limit for comments (65536 characters). Sauce (MicrosoftEdge)Testing web-platform-tests at revision b5d96af All results1 test ran/url/toascii.window.html
|
I see some uncovered cases. The following tests can be added:
I checked only tests (no test code) and they LGTM. |
Maybe we should also test domain longer than 255 with labels shorter than 63. |
Yes, makes sense. |
Tests for whatwg/url#53 and friends.
3a97ad9
to
192384e
Compare
@rmisev could you review once more? It seems Chrome would rather have this merged and then evaluate. |
Tests: web-platform-tests/wpt#5976. Fixes #53 and fixes #267 by no longer breaking on on hyphens in the 3rd and 4th position of a domain label. This is known to break YouTube: r3---sn-2gb7ln7k.googlevideo.com. This is done by setting the proposed CheckHyphens flag to false. Fixes #110 by clarifying that BIDI and CONTEXTJ checks are to be done by setting the proposed CheckBidi and CheckJoiners flags to true. Follow-up #313 is filed to remove the proposed bits once Unicode is updated.
I think the tests are OK, but I suggest adding two more tests to highlight the fast path problem:
{
"input": "xn--1ug.example",
"output": null
},
{
"input": "xn--a-yoc",
"output": null
}, |
Good call! (And thanks for figuring out the Punycode.) |
url/toascii.window.js
Outdated
if (typeof hostTest === "string") { | ||
continue // skip comments | ||
} | ||
if(hostTest.output === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer that this kind of normalization be done ahead of time, so that there is less logic in the tests. I will push a commit to fix.
We have another one in Node.js: {
"comment": "Forbidden character",
"input": "\uFFFD.com",
"output": null
}, |
... and the same encoded: {
"comment": "U+FFFD character encoded in Punycode",
"input": "xn--zn7c.com",
"output": null
}, |
Tests: web-platform-tests/wpt#5976. Fixes #53 and fixes #267 by no longer breaking on hyphens in the 3rd and 4th position of a domain label. This is known to break YouTube: r3---sn-2gb7ln7k.googlevideo.com. This is fixed by setting the proposed CheckHyphens flag to false. Fixes #110 by clarifying that BIDI and CONTEXTJ checks are to be done by setting the proposed CheckBidi and CheckJoiners flags to true. Follow-up #313 is filed to remove the proposed bits once Unicode is updated. #317 also tracks a potential cleanup.
Remove custom tests for invalid IDNA domains in url-idna.js in favor of the more comprehensive official set. Refs: whatwg/url#309 Refs: web-platform-tests/wpt#5976
Remove custom tests for invalid IDNA domains in url-idna.js in favor of the more comprehensive official set. PR-URL: nodejs#13362 Refs: whatwg/url#309 Refs: web-platform-tests/wpt#5976 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
Remove custom tests for invalid IDNA domains in url-idna.js in favor of the more comprehensive official set. PR-URL: #13362 Refs: whatwg/url#309 Refs: web-platform-tests/wpt#5976 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
Remove custom tests for invalid IDNA domains in url-idna.js in favor of the more comprehensive official set. PR-URL: nodejs#13362 Refs: whatwg/url#309 Refs: web-platform-tests/wpt#5976 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
Remove custom tests for invalid IDNA domains in url-idna.js in favor of the more comprehensive official set. Backport-PR-URL: #17365 PR-URL: #13362 Refs: whatwg/url#309 Refs: web-platform-tests/wpt#5976 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
Remove custom tests for invalid IDNA domains in url-idna.js in favor of the more comprehensive official set. Backport-PR-URL: #17365 PR-URL: #13362 Refs: whatwg/url#309 Refs: web-platform-tests/wpt#5976 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
Remove custom tests for invalid IDNA domains in url-idna.js in favor of the more comprehensive official set. Backport-PR-URL: #17365 PR-URL: #13362 Refs: whatwg/url#309 Refs: web-platform-tests/wpt#5976 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
Remove custom tests for invalid IDNA domains in url-idna.js in favor of the more comprehensive official set. Backport-PR-URL: #17365 PR-URL: #13362 Refs: whatwg/url#309 Refs: web-platform-tests/wpt#5976 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
Tests for whatwg/url#53 and friends.