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

When the namespace is the empty string, it should be made null #327

Closed
marrus-sh opened this issue Sep 17, 2021 · 5 comments
Closed

When the namespace is the empty string, it should be made null #327

marrus-sh opened this issue Sep 17, 2021 · 5 comments
Labels
breaking change Some thing that requires a version bump due to breaking changes bug Something isn't working good first issue Good for newcomers help-wanted External contributions welcome spec:DOM Living Standard https://dom.spec.whatwg.org/ testing
Milestone

Comments

@marrus-sh
Copy link

In XML, a null namespace and a namespace of "" are equivalent.

const implementation = new DOMImplementation;
const document = implementation.createDocument("http://www.w3.org/1999/xhtml", "html", null);
const emptyStringDiv = document.createElementNS("", "div");
const nullDiv = document.createElementNS(null, "div");
emptyStringDiv.namespaceURI == nullDiv.namespaceURI; // should be `true`

Although DOM Level 2 doesn’t explicitly talk about namespaces which are an empty string (as far as I can tell), replacing "" with null is the first step of validating and extracting a namespace and qualifiedName in the current DOM standard: https://dom.spec.whatwg.org/#validate-and-extract. This algorithm is used by all the *NS() methods, such as createElementNS().

@karfau karfau added spec:DOM Living Standard https://dom.spec.whatwg.org/ help-wanted External contributions welcome good first issue Good for newcomers labels Sep 17, 2021
@karfau karfau added this to the before 1.0.0 milestone Sep 17, 2021
@karfau
Copy link
Member

karfau commented Sep 17, 2021

Thx for reporting this.
I'm not sure of the impact/severity of this, would you consider this a bug? Should implementing this be considered a breaking change?
Potentially related: We recently made a change to not serialize falsy namespaces (#244).
I didn't check if Dom Level 3 already talks about this.
So the prio i gave it is really just from a gut feeling and can be discussed.

The change seems to be simple to do in the code, but the test coverage for the DOM API is quite low, so adding this should be done in a TDD way to make sure all different cases are covered for all those methods.

@marrus-sh
Copy link
Author

This is easy enough to work around (just do !element.namespaceURI instead of element.namespaceURI == null), but I would anticipate that people would expect the latter to work in all cases. So I would consider it a bug.

I ran into this when writing tests (trying to test whether the namespace of an element was/wasn’t set); I think that’s probably the most common case where someone might check to see if a namespace is null.

Technically this would probably be a breaking change, but I’m not sure why anybody would expect element.namespaceURI == "" to work or rely on that in their code?

@karfau karfau added bug Something isn't working testing breaking change Some thing that requires a version bump due to breaking changes labels Sep 17, 2021
@karfau karfau modified the milestone: before 1.0.0 Dec 21, 2021
@karfau
Copy link
Member

karfau commented Jun 11, 2023

@marrus-sh This might already be resolved since https://github.com/xmldom/xmldom/releases/tag/0.9.0-beta.7 (which is currently available as the next version on npm).

Do you have time to verify that I didn't miss any case?

@marrus-sh
Copy link
Author

@karfau Sorry for the late response. This issue appears to be fixed. The following give the expected behaviour (null):

// assume `document` is an already‐created document
document.createElementNS("", "div").namespaceURI; //= null
new DOMParser().parseFromString(`<foo xmlns=""/>`, "application/xml").documentElement.namespaceURI; //= null

However, I noticed that you can manually set namespaceURI to "". namespaceURI is supposed to be read‐only, so this should not be allowed.

const element = document.createElement("div");
element.namespaceURI = ""; // should not actually change the value

This problem isn’t exclusive to namespaceURI (it also impacts things like tagName) and so should be considered a separate issue.

(Typically, setting a read‐only property fails silently in non‐strict mode and throws in strict mode.)

@karfau
Copy link
Member

karfau commented Jun 29, 2023

Yes, most of the implementation is using plain properties even for fields that contain the same data.

Fixing that everywhere would be a huge effert and will most likely only happen when we switch to some way of transpiling/transforming the code.

Thank you for taking the time to check it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Some thing that requires a version bump due to breaking changes bug Something isn't working good first issue Good for newcomers help-wanted External contributions welcome spec:DOM Living Standard https://dom.spec.whatwg.org/ testing
Projects
Status: Done
Development

No branches or pull requests

2 participants