Opened 2 years ago

Closed 2 years ago

#14667 closed Bug (fixed)

[IE] Removing background from selected text not working correctly

Reported by: Tomasz Jakut Owned by: Tomasz Jakut
Priority: Normal Milestone: CKEditor 4.5.10
Component: General Version: 4.5.11
Keywords: Internet Explorer Cc:

Description (last modified by 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).

Change History (11)

comment:1 Changed 2 years ago by Tomasz Jakut

Status: newconfirmed

comment:2 Changed 2 years ago by Tomasz Jakut

Owner: set to Tomasz Jakut
Status: confirmedassigned

comment:3 Changed 2 years ago by Tomasz Jakut

Description: modified (diff)

comment:4 Changed 2 years ago by 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.

comment:5 Changed 2 years ago by Tomasz Jakut

Status: assignedreview

comment:6 Changed 2 years ago by Marek Lewandowski

Status: reviewreview_failed

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.

comment:7 Changed 2 years ago by Tomasz Jakut

Status: review_failedreview

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.

comment:8 Changed 2 years ago by 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.

comment:9 Changed 2 years ago by 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.

comment:10 Changed 2 years ago by 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.

comment:11 Changed 2 years ago by Tomasz Jakut

Resolution: fixed
Status: reviewclosed
Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy