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

Expect InvalidStateError instead of InvalidAccessError in document.open #13411

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@dguenther

dguenther commented Oct 7, 2018

While working on servo/servo#21882, I noticed that the spec for document.open(url, name, features) states that it should throw an InvalidStateError, but tests expect an InvalidAccessError. I changed the expectation to align with the spec, but that may not be correct -- here's some additional information:

I believe the error was changed in this PR: whatwg/html#2672

Chrome, Safari, Firefox throw InvalidAccessError: https://wpt.fyi/results/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/document.open-02.html

InvalidAccessError is deprecated: https://heycam.github.io/webidl/#invalidaccesserror

@wpt-pr-bot wpt-pr-bot added the html label Oct 7, 2018

@foolip

This comment has been minimized.

Show comment
Hide comment
@foolip

foolip Oct 7, 2018

Contributor

You've found the right definition in the spec, and per that the spec is wrong. However, given that 3 implementations already throw InvalidAccessError I think it'd be unfortunate to try to change it. Better to change the spec back to use InvalidAccessError I think.

Contributor

foolip commented Oct 7, 2018

You've found the right definition in the spec, and per that the spec is wrong. However, given that 3 implementations already throw InvalidAccessError I think it'd be unfortunate to try to change it. Better to change the spec back to use InvalidAccessError I think.

foolip added a commit to whatwg/html that referenced this pull request Oct 7, 2018

Change an exception name in document.open() to "InvalidAccessError"
This was changed in #2672 without
being explicitly discussed. This was discovered because the tests
still match the old behavior:
web-platform-tests/wpt#13411
@foolip

This comment has been minimized.

Show comment
Hide comment
@foolip

foolip Oct 7, 2018

Contributor

Thanks for reporting this, @dguenther! I've sent whatwg/html#4066 to change the spec back, and have a question for you there.

Contributor

foolip commented Oct 7, 2018

Thanks for reporting this, @dguenther! I've sent whatwg/html#4066 to change the spec back, and have a question for you there.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Oct 7, 2018

Member

I'm surprised we didn't catch this during the course of @TimothyGu's work on this test suite, and I'm surprised this doesn't affect more tests. Would love to hear his thoughts.

Member

domenic commented Oct 7, 2018

I'm surprised we didn't catch this during the course of @TimothyGu's work on this test suite, and I'm surprised this doesn't affect more tests. Would love to hear his thoughts.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Oct 8, 2018

Member

I realize now why @TimothyGu and I didn't catch this is because we weren't concerned about the (url, name, features) overload.

Member

domenic commented Oct 8, 2018

I realize now why @TimothyGu and I didn't catch this is because we weren't concerned about the (url, name, features) overload.

domenic added a commit to whatwg/html that referenced this pull request Oct 8, 2018

Change an exception name in document.open() to "InvalidAccessError"
This was changed in #2672 without
being explicitly discussed. This was discovered because the tests
still match the old behavior:
web-platform-tests/wpt#13411.
@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Oct 8, 2018

Member

Closing since it was the spec that was wrong, not the tests. Merged whatwg/html#4066 so now they match :).

Member

domenic commented Oct 8, 2018

Closing since it was the spec that was wrong, not the tests. Merged whatwg/html#4066 so now they match :).

@domenic domenic closed this Oct 8, 2018

@dguenther dguenther deleted the dguenther:update-document-open-error branch Oct 8, 2018

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