Skip to content

setInnerHTML for XHTML documents #2161

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.

Sign up for GitHub

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

Closed
audreyt wants to merge 6 commits into from
Closed

setInnerHTML for XHTML documents #2161

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1b86c51
setInnerHTML for XHTML documents
audreyt Sep 8, 2014
8e20b8d
Move the test inside canUseDOM
audreyt Sep 8, 2014
9f33b6b
Typo
audreyt Sep 8, 2014
1274f2f
Update the tweak to work on Safari
audreyt Sep 8, 2014
b901438
Uniformly use .body
audreyt Sep 8, 2014
cadc94c
Remove <body>...</body> around innerHTML
audreyt Sep 8, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/browser/ui/dom/setInnerHTML.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,21 @@ var setInnerHTML = function(node, html) {
};

if (ExecutionEnvironment.canUseDOM) {
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');
Copy link
Contributor

@alexeyraspopov alexeyraspopov Sep 8, 2014

Choose a reason for hiding this comment

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

HTML format is not supported by Safari browsers (both, desktop and mobile). We need to do something like this

Copy link
Author

@audreyt audreyt Sep 8, 2014

Choose a reason for hiding this comment

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

Added support to Safari as per your suggestion. Now it works in iBooks.app too! ❤️

node.innerHTML = new XMLSerializer().serializeToString(dom.body).replace(/^<body[^>]*>/, '').replace(/<\/body>$/, '');
Copy link
Member

@zpao zpao Sep 9, 2014

Choose a reason for hiding this comment

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

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).

Copy link
Collaborator

@sophiebits sophiebits Sep 9, 2014

Choose a reason for hiding this comment

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

This looks rather slow. Perhaps we can generate XHTML-compatible markup from the start instead.

Copy link
Contributor

@syranide syranide Sep 10, 2014

Choose a reason for hiding this comment

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

@spicyj I imagine it should be quite simple, but perhaps SVG could be an obstacle?

Copy link
Contributor

@alexeyraspopov alexeyraspopov Sep 10, 2014

Choose a reason for hiding this comment

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

Maybe we can just read innerHTML from body?

node.innerHTML = dom.body.innerHTML

Copy link
Author

@audreyt audreyt Sep 10, 2014

Choose a reason for hiding this comment

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

@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.

};
}
else if (document.xmlVersion) {
// Safari: Same thing, different API
setInnerHTML = function(node, html) {
var dom = document.implementation.createHTMLDocument('');
dom.body.innerHTML = html;
node.innerHTML = new XMLSerializer().serializeToString(dom.body).replace(/^<body[^>]*>/, '').replace(/<\/body>$/, '');
};
}
// IE8: When updating a just created node with innerHTML only leading
// whitespace is removed. When updating an existing node with innerHTML
// whitespace in root TextNodes is also collapsed.
Expand Down