Skip to content
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 XHR.prototype.open parameter toString test #9696

Merged
merged 1 commit into from Mar 1, 2018

Conversation

@michaelficarra
Copy link
Member

michaelficarra commented Feb 27, 2018

This codifies the WHATWG algorithm. IE/Edge are known to fail this test, while other major browsers pass.

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Feb 27, 2018

Build PASSED

Started: 2018-02-28 22:10:51
Finished: 2018-02-28 22:19:14

View more information about this build on:

@michaelficarra

This comment has been minimized.

Copy link
Member Author

michaelficarra commented Feb 28, 2018

Turns out old Safari also toStrings url before method.

let expected = [
'method',
'url',
// NOTE: 'async' intentionally missing

This comment has been minimized.

Copy link
@annevk

annevk Feb 28, 2018

Member

It'd be more helpful to comment on why it's missing. It seems that per https://tc39.github.io/ecma262/#sec-toboolean which is used for boolean conversion it'd end up being true because you passed an object. The note below therefore seems wrong too...

This comment has been minimized.

Copy link
@michaelficarra

michaelficarra Feb 28, 2018

Author Member

I think it's missing not because it is ToBoolean'd and ToBoolean doesn't invoke valueOf, but because of the open algorithm, particularly step 5

If the async argument is omitted, set async to true, and set username and password to null. Note: Unfortunately legacy content prevents treating the async argument being undefined identical from it being omitted.

and step 12

Set the synchronous flag, if async is false, and unset the synchronous flag otherwise.

I could be wrong though, in which case I will change the below note.

This comment has been minimized.

Copy link
@annevk

annevk Feb 28, 2018

Member

Before you get to that algorithm, the algorithm @tobie pointed out applies due to it using IDL. And that will convert the arguments to strings and booleans.

This comment has been minimized.

Copy link
@michaelficarra

michaelficarra Feb 28, 2018

Author Member

@annevk Thanks. Updated.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Feb 28, 2018

Overall this seems fine to test, assuming IDL indeed defines this order for the conversion.

@annevk annevk requested a review from tobie Feb 28, 2018

@tobie

tobie approved these changes Feb 28, 2018

Copy link
Contributor

tobie left a comment

Overall this seems fine to test, assuming IDL indeed defines this order for the conversion.

It does in step 11.5 of the overload resolution algorithm.

@michaelficarra michaelficarra force-pushed the michaelficarra:xhr-open-tostring branch from c77b787 to a2bdd68 Feb 28, 2018

@annevk annevk merged commit 6d99020 into web-platform-tests:master Mar 1, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@annevk

This comment has been minimized.

Copy link
Member

annevk commented Mar 1, 2018

Thanks @michaelficarra. Will you file bugs on Edge and Safari?

@michaelficarra michaelficarra deleted the michaelficarra:xhr-open-tostring branch Mar 1, 2018

@michaelficarra

This comment has been minimized.

Copy link
Member Author

michaelficarra commented Mar 1, 2018

@annevk I believe latest Edge and latest Safari actually pass this test. All IEs, all but latest Edge, and all but latest Safari fail. In different ways, of course.

Zirro added a commit to Zirro/jsdom that referenced this pull request Mar 12, 2018

domenic added a commit to Zirro/jsdom that referenced this pull request Apr 1, 2018

domenic added a commit to jsdom/jsdom that referenced this pull request Apr 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.