Fix Windows drive letter handling in the file slash state by rmisev · Pull Request #343 · whatwg/url

rmisev · 2017-09-11T19:23:05Z

Introduces term: "a string starts with Windows drive letter". It simplifies the file state and solves the remaining problem (remaining variable ambiguity #308).
Fixes Drive letters get duplicated when resolving Windows file: URL with base #303.

Test Windows drive letter quirk in the file slash state: https://url.spec.whatwg.org/#file-slash-state URL Standard: whatwg/url#343.

annevk

Just editorial feedback. I haven't reviewed the new behavior in detail. Did you look at various implementations?

annevk · 2017-09-12T12:06:23Z

+ <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 (#).


its length (xref Infra) is two*

annevk · 2017-09-12T12:07:51Z

+
+<ul>
+ <li><p>it contains at least two code points and the first two code points are
+ <a>Windows drive letter</a>.


its length is greater than 1*

I'd give the second requirement its own bullet point since we already AND the whole list

Also, no bullet at the end here since the sentence isn't finished

a Windows drive letter*

annevk · 2017-09-12T12:08:15Z


+<p>A string <dfn>starts with Windows drive letter</dfn> if all of the following are true:
+
+<ul>


Probably better to use <ul class=brief> here without <p>s to make it more of a single paragraph.

annevk · 2017-09-12T12:09:30Z

+ <table>
+  <tr>
+   <th>String
+   <th>Result


Maybe "Starts with a Windows drive letter" instead of "Result" to make it more clear?

annevk · 2017-09-12T12:09:45Z

 <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:


a Windows drive letter*

Also, to allow alternative spelling later on use lt="start with a Windows drive letter|starts with a Windows drive letter" here.

annevk · 2017-09-12T12:11:03Z

-          </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


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.

annevk · 2017-09-12T12:13:44Z

@@ -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


annevk · 2017-09-12T12:13:45Z

       <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


s/ and/, and/
s/in the/in/

rmisev · 2017-09-13T07:21:06Z

Just editorial feedback. I haven't reviewed the new behavior in detail. Did you look at various implementations?

I tested this new behavior on patched https://github.com/jsdom/whatwg-url and it passes all the tests.
I can prepare PR.

Do we need to say the substring from pointer onward? We also haven't really defined substring. Not sure if that's problematic.

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]));
}

domenic · 2017-09-13T07:59:05Z

My guess is @annevk was especially curious about browser implementations, although of course keeping jsdom/whatwg-url up to date is always appreciated.

rmisev · 2017-09-13T09:41:00Z

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:

Subtest	Results	Messages
`Parsing: </c:/foo/bar> against <file:///c:/baz/qux>`	PASS
`Parsing: </c\|/foo/bar> against <file:///c:/baz/qux>`	PASS
`Parsing: <file:\c:\foo\bar> against <file:///c:/baz/qux>`	PASS
`Parsing: </c:/foo/bar> against <file://host/path>`	PASS

Chrome Unstable (google-chrome-unstable_current_amd64.deb)

Subtest	Results	Messages
`Parsing: </c:/foo/bar> against <file:///c:/baz/qux>`	PASS
`Parsing: </c\|/foo/bar> against <file:///c:/baz/qux>`	FAIL	`assert_equals: href expected "file:///c:/foo/bar" but got "file:///c%7C/foo/bar"`
`Parsing: <file:\c:\foo\bar> against <file:///c:/baz/qux>`	PASS
`Parsing: </c:/foo/bar> against <file://host/path>`	FAIL	`assert_equals: href expected "file:///c:/foo/bar" but got "file://host/c:/foo/bar"`

Microsoft Edge 14.14393:

Subtest	Results	Messages
`Parsing: </c:/foo/bar> against <file:///c:/baz/qux>`	FAIL	`assert_equals: href expected "file:///c:/foo/bar" but got "file:///c:/c:/foo/bar"`
`Parsing: </c\|/foo/bar> against <file:///c:/baz/qux>`	FAIL	`assert_equals: href expected "file:///c:/foo/bar" but got "file:///c:/c%7C/foo/bar"`
`Parsing: <file:\c:\foo\bar> against <file:///c:/baz/qux>`	FAIL	`assert_equals: href expected "file:///c:/foo/bar" but got "file:///c:/c:/foo/bar"`
`Parsing: </c:/foo/bar> against <file://host/path>`	FAIL	`assert_equals: href expected "file:///c:/foo/bar" but got "file://host/c:/foo/bar"`

annevk · 2017-09-13T12:13:24Z

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.

annevk · 2017-09-13T12:13:52Z

I filed whatwg/infra#152 as a follow-up for the substring usage. No changes needed here.

annevk

Anyone else want to review this? I'll probably merge this Monday (please ping me if forgot) if there are no further concerns.

Follows whatwg/url#343. Tests web-platform-tests/wpt#7326.

domenic · 2017-09-13T21:51:28Z

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.

GPHemsley

Some initial thoughts.

GPHemsley · 2017-09-16T03:00:23Z

+   <td>❌
+ </table>
+</div>
+


I'm not a huge fan of using symbols to represent the values of the boolean.

Which one is preferred: yes/no or true/false?

cc @annevk

GPHemsley · 2017-09-16T03:07:48Z

+<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 (\),


This should say "its" instead of "the" for consistency.

(Though I feel like this whole phrasing is clunky somehow...)

GPHemsley · 2017-09-16T03:14:37Z

-          <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


"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?

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.

GPHemsley · 2017-09-16T03:14:40Z

-        "<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:


Same as above.

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.

annevk · 2017-09-18T09:54:13Z

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>

Follows 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: 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.

Because the URL standard fix whatwg/url#343 is merged. Tests: web-platform-tests/wpt#7326

rmisev added 3 commits September 11, 2017 21:36

Add term "string starts with Windows drive letter"

e145270

Use term "starts with Windows drive letter" in the file state

7652071

Fix Windows drive letter handling in the file slash state

1ee0d39

Fixes whatwg#303

rmisev mentioned this pull request Sep 11, 2017

URL: Windows drive letter quirk in the file slash state web-platform-tests/wpt#7326

Merged

rmisev added the topic: parser label Sep 12, 2017

annevk reviewed Sep 12, 2017

View reviewed changes

feedback

74706a2

annevk approved these changes Sep 13, 2017

View reviewed changes

rmisev added a commit to rmisev/whatwg-url that referenced this pull request Sep 13, 2017

Spec update: Windows drive letter quirk in the file slash state

bb32f62

Follows whatwg/url#343. Tests web-platform-tests/wpt#7326.

rmisev mentioned this pull request Sep 13, 2017

Spec update: Windows drive letter quirk in the file slash state jsdom/whatwg-url#103

Merged

GPHemsley reviewed Sep 16, 2017

View reviewed changes

s/the /its /

32aa25e

annevk pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 18, 2017

URL: Windows drive letter quirk in the file slash state

5d149f0

Test Windows drive letter quirk in the file slash state: https://url.spec.whatwg.org/#file-slash-state URL Standard: whatwg/url#343.

annevk merged commit 2eef975 into whatwg:master Sep 18, 2017

rmisev deleted the file-slash-state-fix branch September 18, 2017 11:25

rmisev mentioned this pull request Sep 20, 2017

Inconsistency between Chrome/Node.js in url parsing spec #345

Closed

bcoe mentioned this pull request Sep 20, 2017

url: parsing should not serialize windows drive letter twice nodejs/node#15490

Closed

3 tasks

bcoe pushed a commit to bcoe/node-1 that referenced this pull request Sep 21, 2017

url: fix windows drive letter handling

8c0a56a

Address issue with Windows drive letter handling that was causing es-module test suite to fail, see: whatwg/url#343

domenic pushed a commit to jsdom/whatwg-url that referenced this pull request Sep 25, 2017

Spec update: Windows drive letter quirk in the file slash state

b53b6e8

Follows whatwg/url#343.

GPHemsley mentioned this pull request Oct 29, 2017

"the substring from pointer in input" #349

Closed

GPHemsley mentioned this pull request Mar 31, 2018

"Let pointer be a pointer to first code point in..." #373

Closed

rmisev added a commit to upa-url/upa that referenced this pull request May 24, 2020

Remove macro WHATWG_URL_SPEC_ISSUE_303

8fe0173

Because the URL standard fix whatwg/url#343 is merged. Tests: web-platform-tests/wpt#7326


		<p>A string <dfn>starts with Windows drive letter</dfn> if all of the following are true:

		<ul>

Conversation

rmisev commented Sep 11, 2017 • edited by pr-preview Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

annevk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmisev commented Sep 13, 2017

Uh oh!

domenic commented Sep 13, 2017

Uh oh!

rmisev commented Sep 13, 2017

Uh oh!

annevk commented Sep 13, 2017

Uh oh!

annevk commented Sep 13, 2017

Uh oh!

annevk left a comment

Choose a reason for hiding this comment

Uh oh!

domenic commented Sep 13, 2017

Uh oh!

GPHemsley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

annevk commented Sep 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

rmisev commented Sep 11, 2017 •

edited by pr-preview Bot

Loading