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

createElementNS() QName/Name confusion #671

Closed
cscott opened this issue Jul 27, 2018 · 16 comments · Fixed by #945
Closed

createElementNS() QName/Name confusion #671

cscott opened this issue Jul 27, 2018 · 16 comments · Fixed by #945
Labels
clarification Standard could be clearer

Comments

@cscott
Copy link

cscott commented Jul 27, 2018

The spec is not clear about the valid arguments to Document#createElementNS(). It states:

To validate a qualifiedName, throw an "InvalidCharacterError" DOMException if qualifiedName does not match the Name or QName production.

But this doesn't explicitly clarify what happens when the qualifiedName matches Name but not QName, for instance the string foo:bar:bat. Since QName is defined in term of Name anyway, I believe this would be more clearly stated as just ...if qualifiedName does not match the QName production (eliminating the reference to Name).

However, this would change the errors thrown in some corner cases. According to the test suite, calling document.createElementNS(null, ":foo") ought to throw a NAMESPACE_ERR (line 24), but :foo doesn't match QName (prefix must be at least one character in the production), so it ought to throw an INVALID_CHARACTER_ERROR. (This assumes that prefix will be set to the empty string on line 5 which will trigger the exception on line 6 because the empty string is "non-null".)

On the other hand, document.createElementNS("http://example.com", ":foo") ought to throw a NAMESPACE_ERR (test case ine 56) yet there is no spec language to justify this: if it failed the "validate qualifiedName" test it ought to throw a INVALID_CHARACTER_ERROR, and steps 5 and 6 state:

  1. If qualifiedName contains a ":" (U+003E), then split the string on it and set prefix to the part before and localName to the part after.
  2. If prefix is non-null and namespace is null, then throw a "NamespaceError" DOMException.

At this point the namespace is non-null and the prefix is the empty string. There is no language triggering a NamespaceError.

See also web-platform-tests/wpt#12202 which covers some other issues with the test suite for createElementNS(), so fixing the test suite might be reasonable.

For best compliance with the existing test suite, the "validate a qualifiedName" test could be written as:

To validate a qualifiedName, throw an "InvalidCharacterError" DOMException if qualifiedName does not match the Name production. Otherwise, throw a "NamespaceError" DOMException if qualifiedName does not match the QName production.

That would be my recommended fix.

@cscott
Copy link
Author

cscott commented Jul 27, 2018

It appears that #319 and cscott/web-platform-tests@85470b4 recently (Mar 16 2017) changed some some of the NamespaceErrors back to InvalidCharacterError. In light of this, I think my first recommended fix is best:

To validate a qualifiedName, throw an "InvalidCharacterError" DOMException if qualifiedName does not match the QName production.

That is, remove the reference to or Name.

@Codifier
Copy link

@cscott You state:

On the other hand, document.createElementNS("http://example.com", ":foo") ought to throw a NAMESPACE_ERR (test case ine 56) yet there is no spec language to justify this: if it failed the "validate qualifiedName" test it ought to throw a INVALID_CHARACTER_ERROR, and steps 5 and 6 state: [...]

I believe this example is an ambiguous case that could also be taken to mean: it matched the Name production (where colons are allowed as NameStartChar), but non-null namespaceURI was passed, hence the NAMESPACE_ERR.

The specs for Document#createElementNS() are currently pretty lenient as to what combinations of arguments are allowed, particularly where allowing the creation of non-namespaced elements is concerned. It's a bit counter-intuitive to me, when the method name (...NS()) seems to suggests it is specifically aimed at creating namespaced element.

So, I agree the specs could use some further clarification, or perhaps reevaluation as what should be allowed and what not.

@Codifier
Copy link

Although "non-namespaced" of course implies the default namespace. But that basically translates to calling Document#createElement().

@Codifier
Copy link

Perhaps the documentation could be updated to include something like:

If the namespaceURI argument evaluates to null (empty string or null), the qualifiedName argument must match the Name production, otherwise it must match the QName production.

cscott added a commit to fgnass/domino that referenced this issue 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 issue 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
Copy link
Author

cscott commented Jul 28, 2018

Well, as long as the "validate the qualified name" step only allows a QName, then we don't have to worry about empty strings being the prefix, since QName requires at least one character before the colon.

@annevk
Copy link
Member

annevk commented Jul 31, 2018

If Name is indeed truly a subset of QName it does seem 61f40b0 should be further simplified to remove the mention of Name. Justification and PRs to that effect welcome.

@annevk annevk added good first issue Ideal for someone new to a WHATWG standard or software project clarification Standard could be clearer labels Jul 31, 2018
@Codifier
Copy link

But Name is not a subset of QName.

QName allows for only one colon which must be preceded by a NCName:

QName          ::= PrefixedName | UnprefixedName
PrefixedName   ::= Prefix ':' LocalPart
UnprefixedName ::= LocalPart
Prefix         ::= NCName
LocalPart      ::= NCName

Name, however, allows for multiple colons, even at the start of the token:

NameStartChar ::=   	":" | [A-Z] /* etc. */
NameChar      ::=   	NameStartChar | "-" /* etc. */
Name          ::=   	NameStartChar (NameChar)*

As an aside:
In my previous comments my interpretation of how lenient createElementNS() actually is, was incorrect. I interpreted it as though a null namespace allowed for a Name production element name, and I even thought I was reassured by testing it in Firefox, but it seems I must have confused a createElement() test with a createElementNS() test, in that I thought the latter actually allowed createElementNS( null, '::p' ). However, it doesn't, as it shouldn't anyway, now that I've inspected the specs more carefully. In other words createElement( '::p' ) does not produce the same result as createElementNS( null, '::p' ).

This is the outcome of some tests in Firefox 61:

console.log( document.createElement( 'p' ) ) /* <p> */
console.log( document.createElement( ':p' ) ) /* <:p> */
console.log( document.createElement( '::p' ) ) /* <::p> */
console.log( document.createElementNS( 'my-namespace', 'mn:p' ) ) /* <mn:p>*/
console.log( document.createElementNS( null, 'p' ) ) /* <p> */
console.log( document.createElementNS( null, '::p' ) ) /* throws InvalidCharacterError */
console.log( document.createElementNS( null, ':p' ) ) /* throws InvalidCharacterError */
console.log( document.createElementNS( null, 'mn:p' ) ) /* throws NamespaceError */

So, to conclude:
Name is not a subset of QName and the specs for createElementNS() should not allow Name, given its current state. Unless, of course, the goal is to actually let it be more lenient, in that it would, for instance, accept createElementNS( null, '::p' ) as valid.

@annevk
Copy link
Member

annevk commented Jul 31, 2018

What I meant is that if you check against QName is there potential for additional failures to be detected if you check against Name afterwards?

@Codifier
Copy link

Ah right, of course. In that case: no, I don't believe there is.

cscott added a commit to cscott/dom that referenced this issue Aug 8, 2018
This addresses two cases overlooked by 61f40b0:

* The QName production is a strict subset of the Name production.  To
  avoid confusion, drop "or Name" from the definition of "validate
  a qualifiedName".

* The non-normative "for web developers" text for `createElementNS`
  was not updated by the original commit.

Fixes: whatwg#671
@cscott
Copy link
Author

cscott commented Aug 8, 2018

PR submitted as #675.

@David263
Copy link

Issue #671 is marked with the "good first issue" label. I disagree. I am a git beginner and cannot understand why the pull request #675 was not granted. This seems a small fix in the DOM Standard, and hardly important since XML Namespaces will be deprecated soon if they are not already deprecated. I will try to change the labels, if I have permission.

@pshaughn
Copy link

pshaughn commented Feb 5, 2020

My reading here is that we should be throwing InvalidCharacterError on a non-QName Name, and the issue remains open only because of a question about rights assignment on the spec change PR. Is that correct?

@cscott
Copy link
Author

cscott commented Feb 5, 2020

Yes, that is my understanding.

@David263
Copy link

Just my personal opinion, but I see many projects like this on GitHub: they have been discussed and a change is ready to be released, but it just doesn't happen, in this case for a year, but frequently for many years. In some cases it is because the single original developer has abandoned the project. I'm not sure of the entire range of reasons.

As a retired software engineer, I would like to participate in Open Software development, but am completely turned off to the idea because, with the exception of currently popular and very active projects, changes proposed for projects don't actually get made.

In addition, I've taken several courses in git and GitHub and tried to make changes using pull requests (both locally on cloned repositories and on GitHub), but these have failed with error messages I cannot understand. I find the git process to be arcane, meaning too difficult to use.

Open Software is a great idea: letting skilled people contribute their time for free to improve shared products. But the main tool supporting Open Software seems impossible for an experienced retired software engineer to learn. There is a disconnect here somewhere. There isn't even an appropriate forum (that is, meta-concerned about Open Software and its tools) to post this complaint on.

annevk added a commit that referenced this issue Jan 26, 2021
Once you have thrown for all QName cases, you can no longer throw for Name cases. Tested as part of #423.

Also cleanup createElementNS() domintro.

Fixes #671 and closes #675.
@annevk
Copy link
Member

annevk commented Jan 26, 2021

I created #945 to address this instead. I hope that's acceptable @cscott. (And let me know if you don't want to be acknowledged.)

@David263
Copy link

I have no idea whether that is appropriate in this case. I do hope it moves this stalled issue forward, of course, and I thank annevk. In any case, I stand by my comments of Feb 24, 2020: there is no good place to complain about the broken GitHub model for open software development.

annevk added a commit that referenced this issue Jan 27, 2021
Once you have thrown for all QName cases, you can no longer throw for Name cases. Tested as part of #423.

Also cleanup createElementNS()'s domintro.

Fixes #671 and closes #675.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Standard could be clearer
5 participants