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

Update simple-html-tokenizer to v0.5.8 #960

Merged

Conversation

josemarluedke
Copy link
Contributor

Upgrade simple-html-tokenizer to get changes from tildeio/simple-html-tokenizer#69.

Unfortunately, this breaks a test.
Screen Shot 2019-07-22 at 4 27 47 PM

https://github.com/glimmerjs/glimmer-vm/blob/master/packages/@glimmer/syntax/test/parser-node-test.ts#L53-L70

Should the changes in that PR affect SVG title? Happy to update the test if it makes sense.

Interestingly, MDN docs say that SVG title can be "Any elements or character data".
https://developer.mozilla.org/en-US/docs/Web/SVG/Element/title#Usage_notes

@CvX
Copy link
Contributor

CvX commented Jul 22, 2019

I'm looking into it. 🙂 (On my fix-rehydration branch there's also a second failing test)

You'd probably want to also bump the version in packages/@glimmer/integration-tests and packages/@glimmer/syntax. 🙂

@CvX
Copy link
Contributor

CvX commented Jul 22, 2019

Should the changes in that PR affect SVG title? Happy to update the test if it makes sense.

Interestingly, MDN docs say that SVG title can be "Any elements or character data".
https://developer.mozilla.org/en-US/docs/Web/SVG/Element/title#Usage_notes

Yeah, the test should be updated. A quick test in a browser shows that html tags and entities don't get parsed in svg title.

@josemarluedke
Copy link
Contributor Author

@CvX Thanks for the responses. I have updated again and tests should pass now.

@rwjblue Should I cherry-pick this into release-0-38-alpha branch?

@rwjblue rwjblue merged commit dde1270 into glimmerjs:master Jul 25, 2019
@rwjblue rwjblue added the bug label Jul 25, 2019
@rwjblue
Copy link
Member

rwjblue commented Jul 25, 2019

Yes please!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants