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

Fix test case which relied on outdated Name/QName production #12202

Merged
merged 1 commit into from
Aug 10, 2018

Conversation

cscott
Copy link
Contributor

@cscott cscott commented Jul 26, 2018

The latest version of the DOM spec references the Name and QName productions from XML (https://dom.spec.whatwg.org/#validate). In errata 9 from XML 1.0 these productions were changed to be much more inclusive ( https://www.w3.org/XML/xml-V10-4e-errata#E09 ) which means these old test cases are overly restrictive.
Changed to mark \u0BC6 as valid in a qname, and added a new test case using \u037E for the invalid case.

@cscott
Copy link
Contributor Author

cscott commented Jul 27, 2018

I updated my patch with a few more problematic 0BC6 cases. See also whatwg/dom#671 for some brief discussion of the exception type used (InvalidCharacterError vs NamespaceError).

The latest version of the DOM spec references the `Name` and `QName`
productions from XML (https://dom.spec.whatwg.org/#validate).  In
errata 9 from XML 1.0 these productions were changed to be much more
inclusive ( https://www.w3.org/XML/xml-V10-4e-errata#E09 ) which means
these old test cases are overly restrictive.

Changed to mark `\u0BC6` as valid in a qname, and added new test cases
using \u0300 (valid only as non-initial character) and \u037E (never valid).
cscott added a commit to fgnass/domino that referenced this pull request Jul 27, 2018
…lementNS()`

The spec (and its test suite) is itself a bit confused; see
web-platform-tests/wpt#12202 and
whatwg/dom#671 for more details.

Used proper exception type (InvalidCharacterError in more cases, after
whatwg/dom#319) and ensured that we did the
proper WebIDL type coercion thing if given null/undefined as arguments.
cscott added a commit to fgnass/domino that referenced this pull request Jul 27, 2018
…lementNS()`

The spec (and its test suite) is itself a bit confused; see
web-platform-tests/wpt#12202 and
whatwg/dom#671 for more details.

Used proper exception type (InvalidCharacterError in more cases, after
whatwg/dom#319) and ensured that we did the
proper WebIDL type coercion thing if given null/undefined as arguments.
@annevk
Copy link
Member

annevk commented Jul 31, 2018

Thanks, I'm guessing some browsers fail these tests given they implement XML 1.0 4th edition generally?

@gsnedders
Copy link
Member

AFAIK, every browser deliberately implements XML 1.0 4th ed (and not the 5th ed including this errata), so DOM should just cite 4th ed and be clear that you cannot substitute that citation for a later edition?

@cscott
Copy link
Contributor Author

cscott commented Aug 8, 2018

My link is to XML 4th edition, not 5th edition, and the errata linked in the bug description is E09 to the 4th edition (as is clear from the URL).

It is confusing because the 5th edition includes all the errata to the 4th edition. But a note saying "4th edition of XML" is not clarifying; you'd have to say "4th edition of XML including errata E01 through E08 but no later errata" or something like that.

I did verify that Firefox is still throwing an InvalidCharacterError for document.createElement('\u0BC6foo') and the like. I don't know whether this is "deliberate" or not.

@annevk
Copy link
Member

annevk commented Aug 9, 2018

@gsnedders that's no longer true, Chrome and WebKit have 5th at least for their parser.

@cscott
Copy link
Contributor Author

cscott commented Aug 9, 2018

@gsnedders in theory Firefox's behavior was harmonized by https://bugzilla.mozilla.org/show_bug.cgi?id=1358104 but it appears they made it match the exception type but didn't update their xml character class. It appears that https://bugzilla.mozilla.org/show_bug.cgi?id=501837 is their open bug for upgrading to the latest version of the expat library, which would fix the issue.

@annevk
Copy link
Member

annevk commented Aug 10, 2018

So I checked and U+037E and U+0300 are allowed per https://www.w3.org/TR/xml/#NT-NameChar. I think we want to require the 5th edition at this point. That's also what the DOM Standard references.

@cscott
Copy link
Contributor Author

cscott commented Aug 10, 2018

0300 is allowed as NameChar but not as NameStartChar. 037E is never allowed, as the text immediately above your link points out.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Sorry, the a: threw me off.

@annevk annevk merged commit 848ceff into web-platform-tests:master Aug 10, 2018
@cdumez
Copy link
Contributor

cdumez commented Sep 12, 2019

Shouldn't this have updated html/dom/elements/global-attributes/dataset-set.html as well?

-PASS Setting element.dataset['foo豈'] should throw an INVALID_CHARACTER_ERR'
+FAIL Setting element.dataset['foo豈'] should throw an INVALID_CHARACTER_ERR' assert_throws: function "function () { testSet('foo\uF900', 'dummy') }" did not throw

@cdumez
Copy link
Contributor

cdumez commented Sep 12, 2019

Proposed updated in #19031.

annevk pushed a commit that referenced this pull request Sep 13, 2019
This test failed to get updated as the same time as #12202.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants