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.

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

Jump to bottom

SVG elements with children #41

Closed
SkaterDad opened this issue Feb 4, 2017 · 5 comments · May be fixed by #52
Closed

SVG elements with children #41

SkaterDad opened this issue Feb 4, 2017 · 5 comments · May be fixed by #52

Comments

@SkaterDad
Copy link

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?
@emilbayes emilbayes mentioned this issue Jun 13, 2017
Open
@goto-bus-stop goto-bus-stop mentioned this issue Aug 12, 2019
Open
@mreinstein
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

mreinstein commented Feb 6, 2021 •
edited
Loading

@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 mreinstein mentioned this issue Feb 11, 2021
Merged
@mreinstein
Copy link
Contributor

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

@mreinstein
Copy link
Contributor

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.

@mreinstein mreinstein mentioned this issue Jan 1, 2022
Closed
mreinstein added a commit that referenced this issue Oct 30, 2023
@mreinstein
Merge pull request #81 from mreinstein/master
16690b2 
handle optional closing tags for self-closing tags. fixes #41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Close/self-closing foreign elements emilbayes/hyperx
2 participants
@mreinstein @SkaterDad