-
Notifications
You must be signed in to change notification settings - Fork 65
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
spec: share now resolves url relative to the document base URL. #35
Conversation
@sammc can you look at this? Note there's a nifty new "Preview" and "Diff" link in the PR which you can use to see the delta on the rendered HTML (but the example code is a bit borked in the Diff view). |
index.html
Outdated
@@ -118,6 +123,17 @@ | |||
"!WEBIDL#securityerror"><code>SecurityError</code></a>, and abort | |||
these steps. | |||
</li> | |||
<li>Let <var>url</var> be the result of running the <a data-cite= |
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 if the URL isn't valid?
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's a good point. I thought the URL parser was resilient to invalid URLs but it seems it has two levels of failure, "validation error" (a warning which is generally ignored) and "failure" (a parsing error).
One such fatal error is an invalid port number:
navigator.share({url: 'http://localhost:65536'});
In Chrome, this currently fails silently, replacing the URL with the empty string and completing the successful share. Do you think this should cause navigator.share to fail with a new error? (I do.)
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.
And if so, I think it should be a TypeError, which is apparently the JavaScript equivalent of Python's ValueError and TypeError combined, not a SyntaxError which is reserved for JavaScript parse errors.
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.
FYI: I've filed a Chrome bug crbug.com/734486 for updating Chrome's behaviour.
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.
TypeError sounds good. It matches fetch.
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.
Done, PTAL.
This is consistent with the implementation in Chrome 60. Closes w3c#34.
This isn't the current behaviour in Chrome, but this is intended to be fixed in Chrome 61: https://crbug.com/734486.
a9f2eac
to
69f29d6
Compare
This is consistent with the current implementation in Chrome.
Closes #34.
Preview | Diff