setInnerHTML for XHTML documents #2161
Conversation
When running under XML mode, normalize the incoming HTML in setInnerHTML.
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! |
|
Hi! When adapting ReactJS for a EPUB3 project ( https://chinesecubes.github.io/react-odp/ ), this was the only change needed for it to work in XHTML mode. Thanks for this awesome library that makes seamless pre-rendering a breeze! 🌈 Update: Signed up for CLA. In addition to CLA, I'd like to offer my contributions to the React project under CC0 1.0 Universal — To the extent possible under law, I waive all copyright and related or neighboring rights to my current and future pull requests to this project. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
| if (document.contentType === "application/xhtml+xml") { | ||
| // XML mode: Turn HTML into valid XML before setting innerHTML | ||
| setInnerHTML = function(node, html) { | ||
| var dom = new DOMParser().parseFromString(html, 'text/html'); |
There was a problem hiding this comment.
HTML format is not supported by Safari browsers (both, desktop and mobile). We need to do something like this
There was a problem hiding this comment.
Added support to Safari as per your suggestion. Now it works in iBooks.app too! ❤️
| // XML mode: Turn HTML into valid XML before setting innerHTML | ||
| setInnerHTML = function(node, html) { | ||
| var dom = new DOMParser().parseFromString(html, 'text/html'); | ||
| node.innerHTML = new XMLSerializer().serializeToString(dom.body).replace(/^<body[^>]*>/, '').replace(/<\/body>$/, ''); |
There was a problem hiding this comment.
At a glance the whole thing seems pretty reasonable, but I haven't looked closely.
The first things that jumps out at me here is that we should pull these regexs out to constants to dedupe (and to only create them once).
There was a problem hiding this comment.
This looks rather slow. Perhaps we can generate XHTML-compatible markup from the start instead.
There was a problem hiding this comment.
@spicyj I imagine it should be quite simple, but perhaps SVG could be an obstacle?
There was a problem hiding this comment.
Maybe we can just read innerHTML from body?
node.innerHTML = dom.body.innerHTML
There was a problem hiding this comment.
@alexeyraspopov Well, this evaluates to <br> not <br /> and hence still fails to validate as XML:
new DOMParser().parseFromString("<br>", "text/html").body.innerHTML;Unless there is a way to cast a HTMLDocument into XMLDocument, serialization still seems required.
|
Related #2273 |
|
If we support this, then we should generate XML compatible markup directly. There are certain performance expectations that goes with this change. |
|
@sebmarkbage so what's the actionable feedback here? Generating XML compatible markup would likely fall under a whole different PR and shouldn't be done here. Should we close this and work on that separately? Do we really want to generate XML compatible markup all the time? HTML5 allows the use of self closing tags but HTML4 doesn't (at least if you want to actually validate, I assume browser don't care). Which means some real person will care. |
|
I'm not sure how important it is to validate beyond the parsing rules in HTML5. The HTML5 parsing rules should be enough. We could always generate XML compatible markup, if that's viable. I guess the other PR would need to explore that. Otherwise it could be two different branches. Closing this out so that we can get another PR that generates XML compatible strings. |
When running under XML mode, normalize the incoming HTML in setInnerHTML.