SVG elements with children

[SkaterDad]

Issue

hyperx is not allowing many SVG elements to have animation-related tags embedded in them.

Both of these examples reproduce the issue:

const rectNode = html`<svg><rect>
        <animate attributeType="CSS" attributeName="opacity" 
                from="1" to="0" dur="5s" repeatCount="indefinite" />
        </rect></svg>`

//Markup by Aurer on Codepen: http://codepen.io/aurer/pen/jEGbA
const spinnerNode = html`
        <div class="loader loader--style1" title="0">
        <svg version="1.1" id="loader-1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
        width="40px" height="40px" viewBox="0 0 40 40" enable-background="new 0 0 40 40" xml:space="preserve">
            <path opacity="0.2" fill="#000" d="M20.201,5.169c-8.254,0-14.946,6.692-14.946,14.946c0,8.255,6.692,14.946,14.946,14.946
                s14.946-6.691,14.946-14.946C35.146,11.861,28.455,5.169,20.201,5.169z M20.201,31.749c-6.425,0-11.634-5.208-11.634-11.634
                c0-6.425,5.209-11.634,11.634-11.634c6.425,0,11.633,5.209,11.633,11.634C31.834,26.541,26.626,31.749,20.201,31.749z"/>
            <path fill="#000" d="M26.013,10.047l1.654-2.866c-2.198-1.272-4.743-2.012-7.466-2.012h0v3.312h0
                C22.32,8.481,24.301,9.057,26.013,10.047z">
                <animateTransform attributeType="xml"
                attributeName="transform"
                    type="rotate"
                    from="0 20 20"
                    to="360 20 20"
                    dur="0.5s"
                    repeatCount="indefinite"/>
            </path>
        </svg>
        </div>`

In the 1st example, Hyperx is treating the and tags as sibling elements.
In the 2nd example, Hyperx is treating the 2nd and tags as siblings.

It seems to be a fairly common technique for animating SVGs, based on Codepen examples and articles like this: https://css-tricks.com/guide-svg-animations-smil/

Is this something that could be allowed?

[mreinstein]

I've started running this through the debugger and there is definitely a problem in the parse() function.

I've been able to reduce the minimum markup to make this fail:

const input = `<svg>
      <use></use>
    </svg>`

I'm still investigating but it seems to have something to do with using a tag that can be self closing but has a closing tag provided.

[mreinstein]

further reduced the minimum case; this fails too:

const input = `<svg><use></use>  </svg>`

I think this is not a problem with parse(), it seems to be in the handling of the generated tokens, because the generated tokens seems right (I was mistaken in my last comment because I misunderstood the token types.)

[mreinstein]

@SkaterDad I've found the exact bug!

When we iterate over tokens and an encounter a tag that is in a list of ones we know can be self closing (rect, use, path, etc.) we immediately assume that it is closed, and that an explicit close tag will never appear in the token list.

Then later, if we encounter that unexpected closing tag (</use>, </rect>, etc.) we close again. So basically these elements get put one level too high in the stack. here it is illustrated.

input string:

<svg><use></use>☺</svg>

expected tree structure:

  svg
      use
      text (☺ character)

actual tree structure:

  svg
    use
  text (☺ character)

That's why these html strings are throwing the multiple root elements must be wrapped in an enclosing tag error; due to this bug we end up with a tree that has more than one root node, and hyperx is designed to allow only one root node.

I think I can develop a fix for this, where we explicitly check to see if a self-closing tag does indeed close itself, rather than assume that based on the name of the tag. Whatever strategy we pursue to fix this would require a major version bump because it would definitely change the behavior of the parser. @goto-bus-stop what are your thoughts on this? Is this something you'd be keen to accept a PR for?

[mreinstein]

@goto-bus-stop got a fix for this! I'd love some feedback.

[mreinstein]

I'd really like to get this fix merged, if this is something we're open to.

hyperx underpins some really high value frameworks, so I'm keen to see some of these foundational problems get resolved.

I'm also happy to help with a bit of maintainership if that would be useful.