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

Living standard: Add document.implementation.createHTMLDocument() #924

Closed
wants to merge 7 commits into from
Closed

Living standard: Add document.implementation.createHTMLDocument() #924

wants to merge 7 commits into from

Conversation

fhemberger
Copy link
Contributor

This adds support for document.implementation.createHTMLDocument()to the living standard.
It took some time to get my head around the jsdom code, hopefully I did it right. ;)

@domenic
Copy link
Member

domenic commented Oct 27, 2014

Hi! This is awesome work.

I'd really like to get the web-platform-tests running directly. Is there a reason why you think that's not possible?

@fhemberger
Copy link
Contributor Author

It only differs from the original tests in one point: In the living standard, localName is also returned for nodes without namespace (i.e. lowercase version of nodeName value), which goes against the DOM Level 1(?) specs. I wasn't sure how to implement this only being available in the living standard version, so I left it out for now as it's only a minor issue.

@domenic
Copy link
Member

domenic commented Oct 27, 2014

Well, anything in DOM level 1 is just wrong, so let's definitely fix that, even if jsdom has tests to the contrary.

@fhemberger
Copy link
Contributor Author

Here you go. localName is now returned for type ELEMENT_NODE (doesn't make much sense to me for the other types). Tests are passing.

@domenic
Copy link
Member

domenic commented Oct 27, 2014

Awesome! Can we run the web-platform-tests directly now? Per https://github.com/tmpvar/jsdom/blob/master/Contributing.md#writing-or-importing-tests ?

@fhemberger
Copy link
Contributor Author

Unfortunately not yet:

  • Creating a doctype with
    document.doctype = this.createDocumentType("html"); should (according to the w3c test) result in publicId and systemId to be empty strings (returns "undefined" as string instead).
  • Metadata is still missing, I didn't know where to get it.

As this is my first PR to this project (with a complex codebase), I tried to be cautious not to break anything. If you can point me to the right direction, I'll see to get the w3c test passing as well in the next days.

@Sebmaster
Copy link
Member

Fixing createDocumentType should be as simple as replacing level2/core.js by

doctype._publicId = publicId ? String(publicId) : "";
doctype._systemId = systemId ? String(systemId) : "";

@fhemberger
Copy link
Contributor Author

Ok, added the w3c test, one test is still failing, regarding character escaping on href attributes when using createElement.

@domenic
Copy link
Member

domenic commented Oct 27, 2014

OK sweet, we'll try to fix that for you and merge this asap :)

@fhemberger
Copy link
Contributor Author

Cool, thanks!



// https://dom.spec.whatwg.org/#concept-element-local-name
defineGetter(core.Node.prototype, "localName", function() {
Copy link
Member

Choose a reason for hiding this comment

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

Element, not Node

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@domenic
Copy link
Member

domenic commented Oct 29, 2014

Thanks so much for your awesome work. Merged as 5fc594d and d84dab8, and I'll clean up a few more things and do a release shortly.

Thanks for bearing with us in an area of the code that's messier than usual due to the historical levels stuff.

@domenic domenic closed this Oct 29, 2014
@fhemberger
Copy link
Contributor Author

You're welcome! Thanks for all the work with jsdom and your great help with the PR. Not many maintainers take the time to review contributions in such detail.

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

Successfully merging this pull request may close these issues.

3 participants