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. |
| "search": "", | ||
| "hash": "" | ||
| }, | ||
| "# More file URL tests by zcorpan", |
There was a problem hiding this comment.
Why do we put the author here? Should I do that too?
There was a problem hiding this comment.
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.
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.
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.
ec7d464 to
57d2c08
Compare
domenic
left a comment
There was a problem hiding this comment.
Forgot to fixup some pathnames. Let me push an extra commit...
| "href": "file:///", | ||
| "href": "file:///C:/", | ||
| "protocol": "file:", | ||
| "username": "", |
There was a problem hiding this comment.
pathname should be "/C:/" not "/"
| "input": "//d:", | ||
| "base": "file:///C:/a/b", | ||
| "href": "file://d:/", | ||
| "href": "file:///d:", |
There was a problem hiding this comment.
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 <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
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 <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
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 <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
No description provided.