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

Fix entities in SVG download #1130

Merged
merged 7 commits into from
Jun 1, 2017
Merged

Fix entities in SVG download #1130

merged 7 commits into from
Jun 1, 2017

Conversation

jimallman
Copy link
Member

@jimallman jimallman commented May 24, 2017

We've had some problems with unsupported HTML entities (typically  ) in our SVG output. See this related Gitter chat for details.

This change serializes the main SVG node to Unicode, which removes all entities entirely, then relies on Base64 to encode all characters in the data URI. This is working and tested on devtree, see for example this tree with unlabeled internal nodes. This should download and print with no problems. Also note that I've added some diacritical marks and challenging punctuation in the topmost tip's label Mycôsphærëlla "zoomer" >!;. The new solution seems to handle these characters just fine.

jimallman added 5 commits May 24, 2017 15:01
Otherwise any tree with such nodes will fail when exporting/downloading
as SVG (which doesn't recognize most mnemonic HTML entities).
This should be a safer path for SVG in data URIs.
This reverts commit 9d16902.
This change was moot, since `#160` would "render" as `nbsp` in the HTML
DOM. Instead, we'll serialize the node to a Unicode string and depend on
Base64 to handle all encoding.
@jimallman
Copy link
Member Author

Should we need to explore heavier client-side libraries for decoding/encoding HTML entities to XML-compatible numeric entities, one promising option is https://www.npmjs.com/package/html-entities

@josephwb
Copy link
Member

Doesn't seem to work for me. I tried this tree which gives the same error as before. Clicking on the utton in Safari brings up this new page:
tree svg

@jimallman
Copy link
Member Author

jimallman commented May 30, 2017

@josephwb These are working for me on devtree. Please try again after clearing your browser's cache. (This can be difficult in Safari. What version are you running?)

UPDATE: Hmm, I'm having trouble in Safari here too. Checking now for old script vs. bad logic...

@josephwb
Copy link
Member

Ah, okay, I too can not get it to work on Safari. Works fine in Opera if that helps.

@jimallman
Copy link
Member Author

Thanks, it looks like XMLSerializer works differently in Safari, preserving HTML entities like   and > (in tip label Mycôsphærëlla "zoomer" >!; above). I'll look at some more thorough fixes and update this thread when there's something to try.

@jar398
Copy link
Member

jar398 commented May 30, 2017

Sounds like a bug in Safari XMLSerializer . The only mention of it I found was pbakaus/domvas#11 which is rather muted. I wonder whether there is a good way to report it.

@jimallman
Copy link
Member Author

jimallman commented May 30, 2017

Sounds like a bug in Safari XMLSerializer

Not so much a bug as a subtle but intended behavior, apparently. The doctype of the chosen node's parent document (in this case, the surrounding web page) is used to determine how entities are treated, as described in this WebKit test. In short, if the parent doctype is XHTML, serialization will use HTML entities that will fail in "vanilla" XML documents.

Since document.doctype is a read-only property, the solution is to clone the SVG node and wrap it in a new, temporary XML document. Since DTDs are no longer recommended for SVG docs(!), we rely on document.implentation.createDocument() to create a "vanilla" XML doc if no doctype is provided.

This works for me on devtree, including in Safari, for both my preferred tree and the one @josephwb is attempting to use. I'll keep testing in other browsers, but please let me know if you see more failures on devtree!

UPDATE: Both trees linked above work for me in Firefox, Chrome, and Safari.

@jimallman jimallman self-assigned this May 30, 2017
@jar398 jar398 merged commit d6497ca into master Jun 1, 2017
@jar398 jar398 deleted the fix-entities-in-svg-download branch June 1, 2017 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants