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

Change .createElement() namespace to match UAs #213

Closed
wants to merge 1 commit into from

Conversation

ayg
Copy link
Contributor

@ayg ayg commented Apr 10, 2016

Fixes https://www.w3.org/Bugs/Public/show_bug.cgi?id=19431

See comments 12 and 20 there. Gecko tried switching to the spec's
current wording and immediately had to back out due to extension
breakage. It does not seem likely the current spec is implementable.

Fixes https://www.w3.org/Bugs/Public/show_bug.cgi?id=19431

See comments 12 and 20 there.  Gecko tried switching to the spec's
current wording and immediately had to back out due to extension
breakage.  It does not seem likely the current spec is implementable.
@annevk
Copy link
Member

annevk commented Apr 11, 2016

As far as I can tell image/svg+xml indeed gives null. That question did not seem answered in the bug.

@domenic since you touched this last, any concerns?

@domenic
Copy link
Member

domenic commented Apr 11, 2016

This is bizarre, but I guess it's what every browser does. I don't think extension-related breakage is a good reason (extensions are vendor-specific and not part of the web), but if this is what everyone implements that's a good enough reason I guess.

Here is what Blink does: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/dom/Document.cpp&q=createElement&sq=package:chromium&l=716 The setting of "document class flags" is done pretty far away in the code base, but maybe .contentType is a good enough proxy.

@annevk
Copy link
Member

annevk commented Apr 11, 2016

@foolip can you confirm for Blink?

@foolip
Copy link
Member

foolip commented Apr 12, 2016

Yes, I checked yesterday and pondered whether isXHTMLDocument() || isHTMLDocument() is exactly the same as checking at contentType. I've poked around and can't find any exceptions, so it's probably close enough, but I could have missed something.

The isXHTMLDocument() || isHTMLDocument() test is used in a few other places that are worth comparing with:

  • The document.title setter, where the spec instead just looks at the elements in the document.
  • For execCommand and related APIs, which throws an "execCommand is only supported on HTML documents" exception, but the spec instead talks about the HTMLDocument interface which is gone from the spec.
  • For createContextualFragment, in an optimization that isn't in the spec.

Having the concept of "HTML or XHTML document" might be useful.

@ayg
Copy link
Contributor Author

ayg commented Apr 12, 2016

@annevk What question didn't seem answered in the bug?

@annevk
Copy link
Member

annevk commented Apr 12, 2016

@ayg the one about image/svg+xml.

@annevk
Copy link
Member

annevk commented Apr 12, 2016

@foolip yeah, if we change some of those APIs to use these concepts in the specifications we should consider introducing it I think. Something to keep in mind.

@ayg
Copy link
Contributor Author

ayg commented Apr 12, 2016

@annevk http://w3c-test.org/dom/nodes/Document-createElement-namespace.html seems to test image/svg+xml, and comment 12 seems to indicate that browsers matched the proposed spec.

@annevk
Copy link
Member

annevk commented Apr 12, 2016

Oh, I missed that it was tested, thanks. Anyway, I was already fine with this change 😊. I'll land it later today.

@ayg
Copy link
Contributor Author

ayg commented Apr 12, 2016

It occurs to me that one peculiarity of using content type is that if you use createDocument() to create an (X)HTML document, it will have a content type of application/xml per spec, which maybe is not what we want? Is there a better way to say "HTML or XHTML document"? Chrome's createDocument() actually seems to set contentType based on the namespace, so it will behave as expected here. Perhaps it would make sense to change createDocument() to match Chrome, and then this issue goes away.

@foolip
Copy link
Member

foolip commented Apr 12, 2016

Good point @ayg, I remember now that this has come up before, but I'd forgotten it. Per spec "application/xml" is the default and it's never overridden, but Blink indeed does special case the XHTML and SVG namespaces. (So much about Document and its subinterfaces is subtly wrong per spec, so much to sort out...)

@ArkadiuszMichalski
Copy link
Contributor

Gecko goes even further and for createDocument() takes into account the passing DTD https://bugzilla.mozilla.org/show_bug.cgi?id=520969 (probably the only engine that does so).

@annevk
Copy link
Member

annevk commented Apr 13, 2016

Landed as c8ae9cb. Thanks @ayg!

@annevk annevk closed this Apr 13, 2016
@ArkadiuszMichalski
Copy link
Contributor

Any infos what Edge or WebKit set for contentType when using createDocument()? IE11 not support this IDL attribute.

console.log(document.implementation.createDocument("http://www.w3.org/1999/xhtml", "html", null).contentType);
console.log(document.implementation.createDocument("http://www.w3.org/2000/svg", "svg", null).contentType);
console.log(document.implementation.createDocument("http://www.w3.org/1998/Math/MathML", "math", null).contentType);

Firefox and Opera(Presto): application/xml application/xml application/xml
Chrome: application/xhtml+xml image/svg+xml application/xml
WebKit?
Edge?

@ayg ayg deleted the createElement-namespace branch April 13, 2016 13:00
@zcorpan
Copy link
Member

zcorpan commented Apr 13, 2016

createDocument is #217

@ArkadiuszMichalski
Copy link
Contributor

Oh, I should post there, thx to redirect.

@smaug----
Copy link
Collaborator

I don't know why the current spec would be web compatible. At least it is against what many implementations do
http://mozilla.pettay.fi/moztests/imagedoc.html

annevk pushed a commit that referenced this pull request Jun 8, 2016
Since navigation can result in documents whose content type is, e.g.,
"image/png" and those documents are still HTML documents and create
elements in the HTML namespace, the fix in #213 was not entirely
correct.

Fixes #264.

PR: #267
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants