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

Trust svg #2097

Merged
merged 2 commits into from
Mar 6, 2018
Merged

Trust svg #2097

merged 2 commits into from
Mar 6, 2018

Conversation

pygy
Copy link
Member

@pygy pygy commented Mar 2, 2018

Add support for SVG in m.trust() strings, and some DOMParser support in the mocks.

Description

This leverages the DOMParser API (IE 10+) to enable users to use m.trust() to create raw SVG fragments.

How Has This Been Tested?

I've added corresponding tests for render and the mocks.

The mocks test also pass in Firefox, Chrome, Safari IE11 and Edge (thanks to @Yatekii for the last two).

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation change

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated docs/change-log.md

@pygy
Copy link
Member Author

pygy commented Mar 2, 2018

I'll also add a SVG example in the docs

Copy link
Member

@barneycarroll barneycarroll left a comment

Choose a reason for hiding this comment

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

Neat work, fantastic turnaround!

@pygy
Copy link
Member Author

pygy commented Mar 2, 2018

I'd like to rely on the same API for HTML, tough we'd probably have to adapt the possibleParent logic.

MathML can probably be done the same way, but I haven't tried.

@tivac
Copy link
Contributor

tivac commented Mar 2, 2018

This lgtm, but I know little enough about the underlying pieces that I'm not gonna approve the review. @isiahmeadows is probably a better choice.

@pygy
Copy link
Member Author

pygy commented Mar 2, 2018

@tivac I don't know much either.

I knew there was a client-side markup parser, and I learnt how to use it yesterday.

There may be corner cases that are not handled properly, similar to the possibleParent table for HTML. I don't actually know what would break if we always used a div as a container for setting the innerHTML...

As mentioned in Gitter, I've tried to abuse synchronous XHR with data:URI to parse SVG in IE9, but it is rejected (using XMLHttpRequest, XDomainRequest and ActiveXObject("MSXML2.XMLHTTP")) but the trick works in browsers that don't need it since they supprt DOMParser.

Copy link
Member

@dead-claudia dead-claudia left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -64,12 +71,16 @@ module.exports = function($window) {
insertNode(parent, vnode.dom, nextSibling)
return vnode.dom
}
function createHTML(parent, vnode, nextSibling) {
var possibleParents = {caption: "table", thead: "table", tbody: "table", tfoot: "table", tr: "tbody", th: "tr", td: "tr", colgroup: "table", col: "colgroup"}
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate pulling this out. (I almost did over half a dozen times, and just didn't.)

@pygy
Copy link
Member Author

pygy commented Mar 3, 2018

Actually, please don't merge this yet, it can be done without the DOMParser API after all:

temp.innerHTML = "<svg>" + value + "</svg>"; temp = temp.firstChild

I'll update this tomorrow evening.

@pygy pygy merged commit ad46a21 into MithrilJS:next Mar 6, 2018
@pygy
Copy link
Member Author

pygy commented Mar 6, 2018

Here we go, less code, and IE9 compat (for the 1 out of 500 people who still use it worldwide 🙄 )

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.

4 participants