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

Escape colon in tag names #21

Merged
merged 1 commit into from Feb 15, 2017
Merged

Escape colon in tag names #21

merged 1 commit into from Feb 15, 2017

Conversation

vwochnik
Copy link
Contributor

This fixes an issue where the tag name includes a :. In this case, an exception is thrown. This solution escapes the colon so the querySelector* methods work again.

I have tried writing a test, but it doesn't work, because the behaviour of jsdom is not browser-compilant. However, it works in the browsers I have tested it in.

@AvraamMavridis
Copy link
Collaborator

AvraamMavridis commented Feb 15, 2017

@vwochnik Can you please add a test for this case?

@vwochnik
Copy link
Contributor Author

No, not possible. For this, we'd need to move the tests to use PhantomJS. The jsdom engine behaves differently from browsers in this specific case.

@vwochnik
Copy link
Contributor Author

Do you want me to port this to use PhantomJS?

@AvraamMavridis
Copy link
Collaborator

@vwochnik It would be nice, or maybe mock the jsdom implementation.

@vwochnik
Copy link
Contributor Author

I would have to change the querySelector* methods there, that's too much effort. OK, I will change the build system to use Karma/Jasmine/PhantomJS then. But you agree to merge the change then? Should I also upgrade the build pipeline?

@AvraamMavridis
Copy link
Collaborator

@vwochnik Thats too much effort too, maybe can be a separate PR/issue. I will merge this one, and I will push to npm. Give me some hours because I am on the way. And thx for the PR :)

@vwochnik
Copy link
Contributor Author

OK. If my boss agrees I would love to upgrade your build system for a test to be written for this case. Thanks for merging anyways! I'm awaiting the new version then.

@AvraamMavridis AvraamMavridis merged commit 5dcadd7 into ericclemmons:master Feb 15, 2017
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.

None yet

2 participants