search

OiWorld / MindTheWord

An extension for Google Chrome that helps people learn new languages while they browse the web.
43 stars 47 forks source link

fix empty replacements #153

Closed ankit-m closed 8 years ago

ankit-m commented 8 years ago

If an HTML tag (with nodetype TEXT_NODE) was passed to the function, it would replace it with a blank string. This led to data loss and UI breaks. This PR fixes the issue by adding a check for empty newText variable.

ceilican commented 8 years ago

This fix looks a bit like a hack to me. Shouldn't we check the input passed to the function, instead of checking the output it generates? Alternatively, shouldn't we ensure that "replaceAll" is not called with wrong inputs? In other words, shouldn't we fix the callers of "replaceAll" instead of hacking "replaceAll"?

ankit-m commented 8 years ago

@Ceilican, I tried doing that. But the issue with detecting such nodes in the caller is that all <b> and <i> tags are returned as text nodes. I could not find any other way to detect these.

ankit-m commented 8 years ago

The other option was to find all these tags using regex. For we will have to make an exhaustive list of all such tags and compare, which made it complex and rather unreliable. Hence I chose to modify replaceAll.

ceilican commented 8 years ago

Could you add this explanation as a comment in the code?