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

Encoding: ISO-2022-JP encoder "SO/SI ESC" test #26158

Merged
merged 2 commits into from
Oct 21, 2020

Conversation

JKingweb
Copy link
Contributor

The test dates from 2014; expected behaviour changed in 2016 with whatwg/encoding@f9540e5.

The test dates from 2014; expected behaviour changed in 2016
@annevk
Copy link
Member

annevk commented Oct 19, 2020

Thank you for contributing this! It seems you may have uncovered several issues by writing this test, which I hope to sort through in whatwg/url#557 before taking action here.

@hsivonen
Copy link
Member

Gecko bug

@JKingweb
Copy link
Contributor Author

Thank you for contributing this! It seems you may have uncovered several issues by writing this test, which I hope to sort through in whatwg/url#557 before taking action here.

Always happy to cause trouble. ;)

I'd just finished writing an ISO-2022-JP encoder and was using Firefox to validate my results (since the tests are not very portable). Imagine my surprise when I discovered Firefox was wrong despite passing every test. I didn't expect to open a whole can of worms, though.

@annevk
Copy link
Member

annevk commented Oct 20, 2020

@JKingweb would you mind duplicating the test you modified and add a non-ASCII code point to the input and the expectations? That will show some more variety in browsers. (I'm also happy to push such a test if that's okay with you.)

I'll (also) work on the changes to the URL Standard that are needed.

@annevk
Copy link
Member

annevk commented Oct 21, 2020

@hsivonen I went ahead and pushed a bunch more tests. If these look okay to you I suggest we go ahead and land these and then I'll work on the specification patches next.

annevk added a commit to whatwg/encoding that referenced this pull request Oct 21, 2020
This avoids the need for "prepend" in the "process" algorithm as is needed to fix #235. 

Additionally, this commit adds two clarifying asserts to the "process" algorithm documenting what error modes can be in effect when.

Related tests: web-platform-tests/wpt#26158.
Copy link
Member

@hsivonen hsivonen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I verified that Gecko passes these with the pending Gecko patch.

@annevk annevk merged commit 1821fb5 into web-platform-tests:master Oct 21, 2020
annevk added a commit to whatwg/url that referenced this pull request Oct 21, 2020
Since the ISO-2022-JP encoder is stateful, percent-encoding needs to hold onto an instance of the encoder and manually perform error handling. This also requires the input to be the full string rather than individual code points as otherwise the callers of percent-encoding would need to be aware of this too. (As UTF-8 encoding cannot fail this problem does not affect those endpoints.)

Depends on this Encoding PR: whatwg/encoding#238.

Tests: web-platform-tests/wpt#26158.

Fixes #557.
annevk added a commit to whatwg/url that referenced this pull request Oct 28, 2020
Since the ISO-2022-JP encoder is stateful, percent-encoding needs to hold onto an instance of the encoder and manually perform error handling. This also requires the input to be the full string rather than individual code points as otherwise the callers of percent-encoding would need to be aware of this too. (As UTF-8 encoding cannot fail this problem does not affect those endpoints.)

Builds on this Encoding PR: whatwg/encoding#238.

Tests: web-platform-tests/wpt#26158.

Fixes #557.
watilde added a commit to watilde/node that referenced this pull request Oct 28, 2020
annevk added a commit to whatwg/url that referenced this pull request Oct 28, 2020
Since the ISO-2022-JP encoder is stateful, percent-encoding needs to hold onto an instance of the encoder and manually perform error handling. This also requires the input to be the full string rather than individual code points as otherwise the callers of percent-encoding would need to be aware of this too. (As UTF-8 encoding cannot fail this problem does not affect those endpoints.)

Builds on this Encoding PR: whatwg/encoding#238.

Tests: web-platform-tests/wpt#26158.

Fixes #557.
watilde added a commit to nodejs/node that referenced this pull request Oct 29, 2020
Refs: web-platform-tests/wpt#25988
Refs: web-platform-tests/wpt#26158

PR-URL: #35794
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
annevk added a commit to whatwg/url that referenced this pull request Nov 2, 2020
Since the ISO-2022-JP encoder is stateful, percent-encoding needs to hold onto an instance of the encoder and manually perform error handling. This also requires the input to be the full string rather than individual code points as otherwise the callers of percent-encoding would need to be aware of this too. (As UTF-8 encoding cannot fail this problem does not affect those endpoints.)

Builds on this Encoding PR: whatwg/encoding#238.

Tests: web-platform-tests/wpt#26158 and web-platform-tests/wpt#26317.

Fixes #557.
annevk added a commit to whatwg/url that referenced this pull request Nov 2, 2020
Since the ISO-2022-JP encoder is stateful, percent-encoding needs to hold onto an instance of the encoder and manually perform error handling. This also requires the input to be the full string rather than individual code points as otherwise the callers of percent-encoding would need to be aware of this too. (As UTF-8 encoding cannot fail this problem does not affect those endpoints.)

Builds on this Encoding PR: whatwg/encoding#238.

Tests: web-platform-tests/wpt#26158 and web-platform-tests/wpt#26317.

Fixes #557.
targos pushed a commit to nodejs/node that referenced this pull request Nov 3, 2020
Refs: web-platform-tests/wpt#25988
Refs: web-platform-tests/wpt#26158

PR-URL: #35794
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit to nodejs/node that referenced this pull request Dec 8, 2020
Refs: web-platform-tests/wpt#25988
Refs: web-platform-tests/wpt#26158

PR-URL: #35794
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit to nodejs/node that referenced this pull request Dec 10, 2020
Refs: web-platform-tests/wpt#25988
Refs: web-platform-tests/wpt#26158

PR-URL: #35794
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit to nodejs/node that referenced this pull request Dec 15, 2020
Refs: web-platform-tests/wpt#25988
Refs: web-platform-tests/wpt#26158

PR-URL: #35794
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants