-
Notifications
You must be signed in to change notification settings - Fork 137
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
Fix Windows drive letter handling in the file slash state #343
Conversation
Test Windows drive letter quirk in the file slash state: https://url.spec.whatwg.org/#file-slash-state URL Standard: whatwg/url#343.
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.
Just editorial feedback. I haven't reviewed the new behavior in detail. Did you look at various implementations?
url.bs
Outdated
<a>Windows drive letter</a>. | ||
|
||
<li><p>it contains two code points or the third code point is U+002F (/), U+005C (\), U+003F (?), | ||
or U+0023 (#). |
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.
its length (xref Infra) is two*
url.bs
Outdated
|
||
<ul> | ||
<li><p>it contains at least two code points and the first two code points are | ||
<a>Windows drive letter</a>. |
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.
its length is greater than 1*
I'd give the second requirement its own bullet point since we already AND the whole list
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.
Also, no bullet at the end here since the sentence isn't finished
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.
a Windows drive letter*
url.bs
Outdated
@@ -1068,6 +1068,33 @@ code point is U+003A (:). | |||
<p class="note">As per the <a href=#url-writing>URL writing</a> section, only a | |||
<a>normalized Windows drive letter</a> is conforming. | |||
|
|||
<p>A string <dfn>starts with Windows drive letter</dfn> if all of the following are true: | |||
|
|||
<ul> |
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.
Probably better to use <ul class=brief>
here without <p>
s to make it more of a single paragraph.
url.bs
Outdated
<table> | ||
<tr> | ||
<th>String | ||
<th>Result |
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.
Maybe "Starts with a Windows drive letter" instead of "Result" to make it more clear?
url.bs
Outdated
@@ -1068,6 +1068,33 @@ code point is U+003A (:). | |||
<p class="note">As per the <a href=#url-writing>URL writing</a> section, only a | |||
<a>normalized Windows drive letter</a> is conforming. | |||
|
|||
<p>A string <dfn>starts with Windows drive letter</dfn> if all of the following are true: |
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.
a Windows drive letter*
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.
Also, to allow alternative spelling later on use lt="start with a Windows drive letter|starts with a Windows drive letter"
here.
url.bs
Outdated
</ul> | ||
|
||
<p>then set <var>url</var>'s <a for=url>host</a> to <var>base</var>'s <a for=url>host</a>, | ||
<p>If the substring from <var>pointer</var> in the <var>input</var> does not |
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.
No need for "the" in front of input I think. Do we need to say the substring from pointer onward? We also haven't really defined substring. Not sure if that's problematic.
url.bs
Outdated
@@ -1878,7 +1897,8 @@ string <var>input</var>, optionally with a <a>base URL</a> <var>base</var>, opti | |||
<ol> | |||
<li> | |||
<p>If <var>base</var> is non-null and <var>base</var>'s <a for=url>scheme</a> is |
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.
s/ and/,/
url.bs
Outdated
@@ -1878,7 +1897,8 @@ string <var>input</var>, optionally with a <a>base URL</a> <var>base</var>, opti | |||
<ol> | |||
<li> | |||
<p>If <var>base</var> is non-null and <var>base</var>'s <a for=url>scheme</a> is | |||
"<code>file</code>", then: | |||
"<code>file</code>" and the substring from <var>pointer</var> in the <var>input</var> does |
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.
s/ and/, and/
s/in the/in/
I tested this new behavior on patched https://github.com/jsdom/whatwg-url and it passes all the tests.
Yes. A substring isn't defined in this spec., but it's already used to define remaining. Yet another approach is not to use a substring here, for example I created such function for testing: const fileOtherwiseCodePoints = new Set([p("/"), p("\\"), p("?"), p("#")]);
function startsWithWindowsDriveLetter(input, pointer) {
const length = input.length - pointer;
return length >= 2 &&
isWindowsDriveLetterCodePoints(input[pointer], input[pointer + 1]) &&
(length === 2 || fileOtherwiseCodePoints.has(input[pointer + 2]));
} |
My guess is @annevk was especially curious about browser implementations, although of course keeping jsdom/whatwg-url up to date is always appreciated. |
I tested by hand (https://quuz.org/url/liveview2.html) the Chrome 63.0.3213.3 for Windows - it passes all four tests and Safari 10.1.2 - also passes the tests. Other browser's results from https://travis-ci.org/w3c/web-platform-tests/builds/274332313 INFO: /home/travis/build/w3c/web-platform-tests/tools/ci/check_stability Firefox Nightly:
Chrome Unstable (google-chrome-unstable_current_amd64.deb)
Microsoft Edge 14.14393:
|
Okay, I guess it's fine, but it seems a little weird to decide against Edge for behavior Microsoft created at some point. Reminds me of XMLHttpRequest. |
I filed whatwg/infra#152 as a follow-up for the substring usage. No changes needed here. |
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.
Anyone else want to review this? I'll probably merge this Monday (please ping me if forgot) if there are no further concerns.
Since @rmisev has done the tests and jsdom/whatwg-url work, my usual reviewing function is done, so I'm happy to have this without my review. |
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.
Some initial thoughts.
<td>❌ | ||
</table> | ||
</div> | ||
|
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'm not a huge fan of using symbols to represent the values of the boolean.
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.
Which one is preferred: yes
/no
or true
/false
?
cc @annevk
url.bs
Outdated
<ul class=brief> | ||
<li>its <a for=string>length</a> is greater than or equal to 2 | ||
<li>its first two code points are a <a>Windows drive letter</a> | ||
<li>its <a for=string>length</a> is 2 or the third code point is U+002F (/), U+005C (\), |
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 say "its" instead of "the" for consistency.
(Though I feel like this whole phrasing is clunky somehow...)
<var>url</var>'s <a for=url>path</a> to a copy of <var>base</var>'s <a for=url>path</a>, | ||
and then <a>shorten</a> <var>url</var>'s <a for=url>path</a>. | ||
<p>If the substring from <var>pointer</var> in <var>input</var> does not | ||
<a>start with a Windows drive letter</a>, then set <var>url</var>'s <a for=url>host</a> to |
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.
"The substring from pointer in input" seems incomplete to me. (I assume this is intended to imply that the substring runs until the end of the string?)
What do we gain by removing reference to remaining?
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.
Yes, the substring runs until the end of the string like in the JavaScript when indexEnd is omitted.
@annevk opened the issue whatwg/infra#152 to define substring usage.
"<code>file</code>", then: | ||
<p>If <var>base</var> is non-null, <var>base</var>'s <a for=url>scheme</a> is | ||
"<code>file</code>", and the substring from <var>pointer</var> in <var>input</var> does not | ||
<a>start with a Windows drive letter</a>, then: |
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.
Same as above.
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.
What do we gain by removing reference to remaining?
The simpler check, that there are at least two code points:
- without remaining:
substr.length >= 2
- with remaining:
c != EOF && remaining.length >= 1
Test Windows drive letter quirk in the file slash state: https://url.spec.whatwg.org/#file-slash-state URL Standard: whatwg/url#343.
I missed the new comments by @GPHemsley, sorry. I think they're addressed to my satisfaction though. I think using emojis for true/false is reasonable in examples. We do that here and there. |
Address issue with Windows drive letter handling that was causing es-module test suite to fail, see: whatwg/url#343
Address issue with Windows drive letter handling that was causing es-module test suite to fail. PR-URL: #15490 Ref: whatwg/url#343 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Address issue with Windows drive letter handling that was causing es-module test suite to fail. PR-URL: #15490 Ref: whatwg/url#343 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Address issue with Windows drive letter handling that was causing es-module test suite to fail. PR-URL: nodejs/node#15490 Ref: whatwg/url#343 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Test Windows drive letter quirk in the file slash state: https://url.spec.whatwg.org/#file-slash-state URL Standard: whatwg/url#343.
Test Windows drive letter quirk in the file slash state: https://url.spec.whatwg.org/#file-slash-state URL Standard: whatwg/url#343.
Because the URL standard fix whatwg/url#343 is merged. Tests: web-platform-tests/wpt#7326
Preview | Diff