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

For safety consider an alternative to innerHTML #1

Open
prettydiff opened this issue Apr 20, 2017 · 3 comments
Open

For safety consider an alternative to innerHTML #1

prettydiff opened this issue Apr 20, 2017 · 3 comments

Comments

@prettydiff
Copy link

This is a really cool. I looked at the code see you are referencing the innerHTML of a given node for text replacement. This is risky in that innerHTML returns all descendant code in addition to text, which means you could replace things like tag names or attribute values by accident. It would be safer if you just gathered the text. Fortunately, I have a solution for you.

This handy dandy tool allows gathering of DOM nodes by node type from any element. You can take most of the code out of this tool since you will always be looking for text node types and don't care about attribute values or other node types. Since the tool is native DOM walking it is really fast and also has no dependencies. https://github.com/prettydiff/getNodesByType

Using the mentioned solution will bloat your logic a bit as it will return an array of text nodes that you will have to iterate over instead of just a single string. You can modify the value of text node by reassigning to its textContent property and it will automagically update in the page. If you need to wrap a result in a span tag then innerHTML would be more efficient.

@mburakerman
Copy link
Owner

mburakerman commented Apr 20, 2017

When i started to do this, i was suspicious about innerHTML but wanted to publish it after all, so i can improve it. This code also help us in some cases but yes, probably not the best way.

Thank you for your consideration. I will try on it and hope i can make..!

@skurzinz
Copy link

I just came across exactly this problem mentioned by @prettydiff when using hrjs to highlight search results on stuff that gets preprocessed by XSL-T; hrjs adding spans to content that already contains another span results in invalid HTML with overlapping tags and ultimately in strange browser behaviour in trying to fix that.
Consider my +1 for fixing. Unfortunately I am not JavaScript enabled myself...

@skurzinz
Copy link

Currently fixed locally by replacing span by mark tag in hr.js -- general problem still persists.

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

No branches or pull requests

3 participants