-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
More tests for file URL Window drive letter quirk #4382
Conversation
Notifying @Sebmaster, @domenic, @frewsxcv, @mikewest, @rubys, @sideshowbarker, @smola, @tomalec, @xiaojunwu, and @zcorpan. (Learn how reviewing works.) |
FirefoxTesting revision e389fa7 All results/url/a-element-xhtml.xhtml
/url/a-element-origin.html
/url/a-element-origin-xhtml.xhtml
/url/a-element.html
|
ChromeTesting revision e389fa7 All results/url/a-element-xhtml.xhtml
/url/a-element-origin.html
/url/a-element-origin-xhtml.xhtml
/url/a-element.html
|
There are a few issues with these:
Pretty sure the second and third one are correct failures, not quite sure about the first one. |
Not sure exactly what you mean by "correct failures" @Sebmaster, but I agree with jsdom/whatwg-url that the first result should be |
"correct failures" as in: I'm pretty sure what whatwg-url does is correct here, mainly because of the triple slash thing (also because Chrome agrees with that resolution too). |
I guess I was confused by reading the spec. But would be nice with confirmation from @annevk that the expected results are actually per spec. |
@@ -4587,5 +4587,76 @@ | |||
"pathname": "/", | |||
"search": "", | |||
"hash": "" | |||
}, | |||
"# More file URL tests by zcorpan", |
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.
Why do we put the author here? Should I do that too?
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 just wanted to separate these tests from the ones above to direct any blame at me, but of course git blame
works too. Can remove it though.
{ | ||
"input": "//d:", | ||
"base": "file:///C:/a/b", | ||
"href": "file:///d:", |
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.
This should end in a slash, no?
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.
https://url.spec.whatwg.org/#file-state "/"
https://url.spec.whatwg.org/#file-slash-state "/"
https://url.spec.whatwg.org/#file-host-state "d:" appended to buffer, then EOF. Buffer is a Windows drive letter.
https://url.spec.whatwg.org/#path-state step 1.4.2 appends buffer to path, without adding a trailing slash.
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.
That seems correct, but then pathname wouldn't end in a "/" either. It's also not entirely clear to me that is the desired outcome.
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.
ec7d464
to
57d2c08
Compare
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.
Forgot to fixup some pathnames. Let me push an extra commit...
@@ -4652,7 +4652,7 @@ | |||
{ | |||
"input": "/", | |||
"base": "file:///C:/a/b", | |||
"href": "file:///", | |||
"href": "file:///C:/", | |||
"protocol": "file:", | |||
"username": "", |
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.
pathname should be "/C:/" not "/"
@@ -4666,7 +4666,7 @@ | |||
{ | |||
"input": "//d:", | |||
"base": "file:///C:/a/b", | |||
"href": "file://d:/", | |||
"href": "file:///d:", |
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.
pathname should be "/d:" not "/d:/"
@annevk all looks good to me now. Can we merge this, and deal with whatwg/url#193 as a follow-up? |
I think that's reasonable. Will let @zcorpan merge in case there was anything else. |
Added at the following PR: + web-platform-tests/wpt#4382 + web-platform-tests/wpt#4700
Added at the following PR: * web-platform-tests/wpt#4382 * web-platform-tests/wpt#4700 PR-URL: #11123 Fixes: #10978 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
Added at the following PR: * web-platform-tests/wpt#4382 * web-platform-tests/wpt#4700 PR-URL: nodejs#11123 Fixes: nodejs#10978 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
Added at the following PR: * web-platform-tests/wpt#4382 * web-platform-tests/wpt#4700 PR-URL: #11123 Fixes: #10978 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
No description provided.