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 backslash in getTargetArray method #236

Merged
merged 1 commit into from
Mar 20, 2017
Merged

Conversation

rnons
Copy link
Contributor

@rnons rnons commented Dec 8, 2016

it's a rare case, but if data-tip contains backslash, getTargetArray or more specifically querySelectorAll won't find the element

@rnons
Copy link
Contributor Author

rnons commented Dec 29, 2016

@wwayne can you take a look a this? Thanks

@huumanoid
Copy link
Contributor

Hey @rnons sorry for long response.
Thank you for this interesting case and patch for it! I think your PR looks fine.

How about to make this PR more general?

According to [https://www.w3.org/TR/html5/dom.html#the-id-attribute](HTML5 specification), the id could contain any character except space characters.

I see another case: when "id" contains double quotation mark, document.querySelectorAll("[data-for="hello"world!"]" won't find anything.
We should escape " too: document.querySelectorAll("[data-for="hello\\"world!"] (success!)

Any other cases? Please, tell me, if you find one!

@huumanoid huumanoid self-requested a review March 17, 2017 11:38
@rnons
Copy link
Contributor Author

rnons commented Mar 19, 2017

@huumanoid updated

Copy link
Contributor

@huumanoid huumanoid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good!
@wwayne I think this can be safely merged, please, take a look.

@wwayne
Copy link
Collaborator

wwayne commented Mar 20, 2017

@huumanoid Sure, will do today, thanks @rnons

@wwayne wwayne merged commit a030f50 into ReactTooltip:master Mar 20, 2017
@wwayne
Copy link
Collaborator

wwayne commented Mar 20, 2017

Merged and released as 3.2.9, it should be 3.2.8, but some error happened when I did npm publish and I was not sure if it was published successfully... So I release a new version for safe

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.

3 participants