tsers-js / core

Transform-Signal-Executor framework for Reactive Streams
MIT License
145 stars 4 forks source link

Add SVG support to snabbdom #15

Open milankinen opened 8 years ago

milankinen commented 8 years ago

Continuing discussion from tsers-js/snabbdom#2

Nice observations here from @laszlokorte:

I just tried to create my first TSERS component using SVG.

https://gist.github.com/laszlokorte/2a9ae0accca2805bde3eb28c376fd660#file-tsers-component-js-L11-L13

The width and height attributes of the svg tag itself get set but the cx, cy and r attributes of the circle tag are not set.

I think the reason is this check: https://github.com/tsers-js/snabbdom/blob/master/src/index.js#L104-L107 which tries to unify snabbdoms props and attrs options.

milankinen commented 8 years ago

I've never used SVG tags with any virtual dom implementation, so I'm a total newbie with this issue. What kind of effort it'd need to implement them to @tsers/snabbdom?

And what would be the best API for them? I'd like to keep the API uniform across different DOM interpreters.

laszlokorte commented 8 years ago

To support SVG tags two things must be done:

  1. The namespace of the vnode must be set. Snabbdom does that by checking if the node is of type "svg" and then setting vnode.data.ns for the node and all it's children: https://github.com/paldepind/snabbdom/blob/master/h.js#L29-L31
  2. Attributes of svg dom elements must be set via setAttribute, not as properties. (el.setAttribute('cx', 20) instead of el.cx = 20). Snabbdoms own API requires the developer to explicitly state if a value of a node should be set as property or as attribute by passing it either as attrs or as props object. In h('circle', {attrs: {cx: 20}, props: {foo: 20}}) cx will be set via setAttributes and foo will be set as property on the dom object. I personally like that explicitness. Virtual-Dom gives this control to the developer as well: h('circle', {foo: 20, attributes: {cx: 20}}). Virtual-Dom uses props as default but provides the attributes key to opt out. tsers-snabbdom tries to handle the difference between props and attrs automatically by maintaining a list of attributes names. This fails for svg because all the svg attributes are not in the list. So the most straightforward solution would be to add all svg attributes to the list. But I do not think that this would be a good solution because there are a lot of attributes (https://developer.mozilla.org/de/docs/Web/SVG/Attribute)

Virtual-Dom provides additionally to the h helper an svghelper which checks for each property if it is an SVG attribute and then moves it from the properties object to the attributes object (https://github.com/Matt-Esch/virtual-dom/blob/master/virtual-hyperscript/svg.js#L28). In my opinion this is to much magic and I do not like using a separate svg function instead of h for creating svg nodes.

Not sure what the best solution would be.

milankinen commented 8 years ago

Ah so could we define SVG tags by using svg: prefix and use opt-in attributes field that does not do any checks to the given attributes, e.g.

h("svg:circle", {attributes: {cx: 20}})

What do you think?

laszlokorte commented 8 years ago

So the svg: prefix would be for setting the namespace? Maybe we should use svg|circle instead to be consistent with the css namespace selector syntax (https://www.w3.org/TR/css3-namespace/#css-qnames)

But I am not sure if it's nice to type svg| in front of every element name. snabbdom just detects the svg root element itself via tag name and then set's the namespace for all children recursively. But I guess this does not work if you want to be able to embed vnode observables which elements are generated later on (?)

So yeah, your suggestion would work. I am not sure if it's the most elegant one but for now it would be fine for me 👍

milankinen commented 8 years ago

I like the idea of setting namespace recursively to the children of svg node. Currently TSERS DOM implementation don't support embedded observables in vnodes but even with that, I think there is still a way to add namespace afterwards.

ghost commented 8 years ago

It works OK with ReactDOM driver. Thank you for TSERS!