guardian / scribe

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

insertHTML fix: don't purge user-generated SPANs #339

Closed christopherliu closed 9 years ago

christopherliu commented 9 years ago

A while back, this was written in insert-html.js: "FIXME: what if the user actually wants to use SPANs? This could cause conflicts." Turns out that user was me. This updated patch is more targeted, removing only SPANs that are truly superfluous. In addition, it also covers more than one SPAN, where the original code would only cover the first child SPAN.

I've only tested this fix on my own code.

rrees commented 9 years ago

At first glance this looks good, second opinion @hmgibson23 ? I'd be happy to see if it passes our test suite and if so merge it.

hmgibson23 commented 9 years ago

Makes sense to me.

hmgibson23 commented 9 years ago

Tests work on chrome - can't run firefox locally because of the Webdriver bug. Can anyone else? @cutandpastey @robinedman @rrees

theefer commented 9 years ago

Just to clarify, this won't stop us from stripping SPANs in our HTML (eg when pasted in, etc)?

christopherliu commented 9 years ago

Hey @theefer , it should still remove SPANs, but double-check my logic to make sure it's going to match what you need.

Original version: Remove the first SPAN child at any level PR version: Remove all SPANs where the only attribute is style and the only CSS selector in style is line-height.

I've only had line-height selectors in my test pastes, but that might not be universally true. The other simple option is to get rid of any SPAN where the only attribute is style. The worry would be if styled text was pasted into the editor pane and it was pasted as a SPAN, e.g. . Since I only saw line-height, I kept the criteria for deletion narrow.

rrees commented 9 years ago

@christopherliu I think we'll need some more tests around what we think the behaviour should be, if you can expand the PR to include the behaviour you've observed and the new behaviour that would help us a lot in progressing the change

christopherliu commented 9 years ago

@rrees Sounds good. I'll write it out here. I could probably do something on the test side as well but I'd need a little direction since I haven't looked at that code.

Original version: (Input -> Output)

<p>1<b style="line-height: 2;">2</b><span style="line-height: 2;">3</span></p>
->
<p>1<b>2</b>3</p>

<p>1<b style="line-height: 2;">2</b><span style="line-height: 2;">3</span><span style="line-height: 2;">4</span></p>
->
<p>1<b>2</b>3<span style="line-height: 2;">4</span></p>

<p>1<b style="line-height: 2;">2</b><span style="line-height: 2;">3</span><b style="line-height: 2;">4</b></p>
->
<p>1<b>2</b>3<b style="line-height: 2;">4</b></p>

<p><span style="color:green;">1</span></p>
->
<p>1</p>

New version: (Input -> Output)

<p>1<b style="line-height: 2;">2</b><span style="line-height: 2;">3</span></p>
->
<p>1<b>2</b>3</p>

<p>1<b style="line-height: 2;">2</b><span style="line-height: 2;">3</span><span style="line-height: 2;">4</span></p>
->
<p>1<b>2</b>34</p>

<p>1<b style="line-height: 2;">2</b><span style="line-height: 2;">3</span><b style="line-height: 2;">4</b></p>
->
<p>1<b>2</b>3<b>4</b></p>

<p><span style="color:green;">1</span></p>
->
<p><span style="color:green;">1</span></p>

This is at least the idealized version of what I had in mind, which may perhaps make it all the better to test against.

christopherliu commented 9 years ago

385