[IE] Removing background from selected text not working correctly

[Tomasz Jakut]

Steps to reproduce

1. Open ​http://ckeditor.dev/samples.
2. In source mode, set editor's content to:

   <p><span style="background-color: rgb(255,255,0);">Text with background</span></p>
   

3. In WYSIWYG mode, select "with".
4. Open console and paste into it:

   var style = new CKEDITOR.style( {
       element: 'span',
       styles: {
           'background-color': '#ff0'
       },
       type: CKEDITOR.STYLE_INLINE
   } );
   
   style.remove( CKEDITOR.instances.editor );
   

Note that "with" must be still selected inside the editor.

Expected result

Background is removed from the selected text.

Actual result

Background is removed from the entire paragraph.

Other details (browser, OS, CKEditor version, installed plugins)

Only in Internet Explorer 9+.

It can be reproduced only on master, ( ​git:0528e5aaeb104ffd283aeb4ff5efa2fcdb0616e4).

[Tomasz Jakut]

It's a super weird bug… It seems that it only affects span[style*="background-color"] (and every other style, even simple background is not affected) after treating it with element.breakParent, element.removeStyle and element.remove (so the logic behind the CKEDITOR.style.remove). If one of these operations is not performed, the bug is not present.

So my proposal of fix just switches off removing background-color style in IE. Very hacky & dirty solution, but I couldn't come up with anything better. Pushed fix to ​branch:t/14667.

[Marek Lewandowski]

Unfortunately this fix is not acceptable, we need to fix the source of the problem. We need to come with a better, more clean solution. Your findings are correct, as it looks that execContentsAction function in range.js file. Then when elements returned by this function are used, it will strip the background-color from other nodes.

[Tomasz Jakut]

After some more research, I've found that the issue is connected with element.breakParent, especially with DocumentFragment used there.

After moving the whole content of the DocumentFragment into temporary element and overwriting content's background-color with the same value, issue is not present. Although such fix is event more hacky than the previous one…

Pushed "fix" to ​branch:t/14667c.

[Tomasz Jakut]

The new fix (new implementation of element.breakParent), on ​branch:t/14667c, works as follows:

1. Split the element and move the splitted part into DocumentFragment.
2. If we're on IE, get every element from DocumentFragment and overwrite it's background-color with the same value. Otherwise the background color would be lost after reinserting element back into the DOM.
3. On all browsers, append the contents of DocumentFragment into temporary element (div).
4. Insert temporary element into correct place in DOM.
5. Remove the element preserving the children.

The current fix for IE is connection of overwriting the background-color and moving elements from DocumentFragment to the temporary element. If one of these actions is not performed, the background color would be lost.

[Marek Lewandowski]

Actually the new workaround is way more viable than the former one. So far it's looking very promising, I'm still testing it but it's looking good.

[Marek Lewandowski]

Branch ​t/14667c looks nice so R+ from me. I did some adjustments, so @t.jakut please take a look on changes and review them. Once you're fine with the changes add a changelog entry and merge back to master branch.

Here are some notes: The reason why docFrag.getFirst() works in case o nested spans is that breakParent() gets called for whole hierarchy up to the parent.

I've added some changes:

• Strategy that involves adding div element and all the workarounds is used only for browsers that need it IEs.
• Removed custom command for style removing in manual test, and reused remove format plugin instead.

[Tomasz Jakut]

Merged to master with ​git:b997321303c4e8f4ca14e4553e95d1bc87e63358.