Implement DOMParser interface #137
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This implements the DOMParser interface. As of now, only the "text/html" type is supported, the unsupported types will throw.
penguinput
force-pushed
the
add/domparser
branch
from
c585f01 to
737b617
Compare
penguinput
changed the title
cscott
requested changes
Feb 14, 2019
| @@ -4,6 +4,7 @@ var EventTarget = require('./EventTarget'); | |||
| var Location = require('./Location'); | |||
| var sloppy = require('./sloppy'); | |||
| var utils = require('./utils'); | |||
| var DOMParserWrapper = require("./DOMParser"); | |||
There was a problem hiding this comment.
Why is this DOMParserWrapper and not just DOMParser? None of the other classes have ...Wrapper suffixes.
| @@ -3036,6 +3035,9 @@ | |||
| "dom/nodes/Element-siblingElement-null-xhtml.xhtml": [ | |||
| "Uncaught: Unexpected token <" | |||
| ], | |||
| "dom/nodes/Element-tagName.html": [ | |||
| "tagName should be updated when changing ownerDocument" | |||
| ], | |||
There was a problem hiding this comment.
This is a new test failure. Why?
| "Location value", | ||
| "DOMParser parses HTML tag soup with no problems", | ||
| "DOMParser throws on an invalid enum value" | ||
| "baseURI value" |
There was a problem hiding this comment.
Nice that we pass these tests now!
| @@ -12,6 +13,7 @@ function Window(document) { | |||
| this.document._scripting_enabled = true; | |||
| this.document.defaultView = this; | |||
| this.location = new Location(this, this.document._address || 'about:blank'); | |||
| this.DOMParser = DOMParserWrapper(this); | |||
There was a problem hiding this comment.
Yeah, this isn't the right way to export it. You should instead add this to lib/impl.js, which then gets merged into Window in via utils.expose in the last line of this file.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This implementation seems to work correctly when the content type is set
to
text/html, however the XML types fall through the HTMLParser sincethere's no XMLSerializer interface.
The tests for the
text/htmltype are passing, except thebaseURIone, since its getter is not yet implemented in thelib/Node.jsfile.If, in the future, the XMLSerializer interface is implemented I think it should be easy to add the missing logic.
Lastly: I'm not sure if the DOMParser interface should be exposed the way I did.
It needs to access the active document address and I didn't find any other way to make it work.
Maybe, by logic, it should go into the
lib/impl.jsfile?Edit:
At the end I decided to definitely throw on the not-yet-implemented XML types to avoid misleading behavior with the HTMLParser fallback (case sensitivity to name one).
Still, I'm not really sure I exposed the DOMParser interface the proper way.