guardian / scribe

DEPRECATED: A rich text editor framework for the web platform
http://guardian.github.io/scribe/
Apache License 2.0
3.51k stars 245 forks source link

Logic bug in node.removeChromeArtifacts() #464

Closed mindplay-dk closed 8 years ago

mindplay-dk commented 8 years ago

Possible logic bug here:

      if (!node.getAttribute('style')) {
        node.removeAttribute('style');
      }

I'm not sure what this was intended to do, but it appears to say, "if the node doesn't have the attribute, remove the attribute" - I'm not sure what the intent was, but I don't think this code does anything?

rrees commented 8 years ago

@regiskuckaertz do you remember what this code was trying to achieve?

regiskuckaertz commented 8 years ago

@rrees Javascript and type coercion... the empty string is false in JS: this code was meant to eliminate instances of <elem style="">. At least that's what I think.

rrees commented 8 years ago

@regiskuckaertz ah, would it be clearer to have the comparison with the empty string here to avoid people deleting it as a no-op?

regiskuckaertz commented 8 years ago

@rrees yes definitely, that's a code smell. I can raise a PR if you want?

rrees commented 8 years ago

@regiskuckaertz that'd be smashing