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

Add URL parsing test for "ssh://example.com/foo/bar.git" #13516

Merged
merged 1 commit into from Oct 15, 2018

Conversation

Projects
None yet
6 participants
@foolip
Contributor

foolip commented Oct 15, 2018

Exercised in url-constructor.html and url/url-origin.html.

Points of disagreement:

Based on https://felixfbecker.github.io/whatwg-url-custom-host-repro/
for webcompat/web-bugs#19792.

@foolip

This comment has been minimized.

Contributor

foolip commented Oct 15, 2018

Browser results:

  • Chrome Canary passes url-origin.html but fails url-constructor.html because host is "". (I'll find a bug.)
  • Edge 17 passes url-constructor.html but fails url-origin.html because origin is "ssh://" (@thejohnjansen, can you find a contact?)
  • Firefox Nightly status is the same as Chrome Canary (host is "") (@annevk, who to poke?)
  • Safari Technology Preview 67 passes url-constructor.html but fails url-origin.html because origin is "ssh://example.com". (@achristensen07 FYI)
@annevk

This comment has been minimized.

Member

annevk commented Oct 15, 2018

As I noted in whatwg/url#418 to @felixfbecker I think this is already tested by equivalent tests, though I'm happy to add this specific variant. I'm also happy with other people caring about this and poking browsers.

For Firefox it's @valenting, but I think he's aware and trying to get the general infrastructure in such a shape it can eventually support a different URL parser. Not really sure what the status is though.

Add URL parsing test for "ssh://example.com/foo/bar.git"
Exercised in url-constructor.html and url/url-origin.html.

Points of disagreement:
 * `protocol` should be "ssh:" because it is simply the concatenation of
   scheme and ":": https://url.spec.whatwg.org/#dom-url-protocol. Set to
   "ssh" here: https://url.spec.whatwg.org/#scheme-state
 * `origin` should be "null" because the URL will be get an opaque
   origin in https://url.spec.whatwg.org/#concept-url-origin.
 * `host`, `hostname` and `pathname` require stepping through the parser
   to see why the result is what it is:
   https://url.spec.whatwg.org/#host-state
   https://url.spec.whatwg.org/#path-state

Based on https://felixfbecker.github.io/whatwg-url-custom-host-repro/
for webcompat/web-bugs#19792.

@foolip foolip changed the title from Add URL parsing test for "ssh://github.com/test/repo.git" to Add URL parsing test for "ssh://example.com/foo/bar.git" Oct 15, 2018

@foolip

This comment has been minimized.

Contributor

foolip commented Oct 15, 2018

@domenic as mentioned on IRC, your review on this would be much appreciated.

@domenic

Passes with jsdom/whatwg-url.

@achristensen07

This comment has been minimized.

Contributor

achristensen07 commented Oct 15, 2018

I am aware that Safari's behavior differs from the spec in the case of non-special origins. Thanks for the ping, and I'm glad to see such a wave of interest in spec compliance.

@foolip foolip merged commit 4ba3cd2 into master Oct 15, 2018

2 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@foolip foolip deleted the foolip/url-ssh branch Oct 15, 2018

@felixfbecker

This comment has been minimized.

@foolip

This comment has been minimized.

Contributor

foolip commented Nov 6, 2018

Thanks @felixfbecker!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment