Opened 6 years ago

Closed 6 years ago

#11258 closed Bug (fixed)

Empty spans not being removed

Reported by: Marek Lewandowski Owned by: Piotr Jasiun
Priority: Normal Milestone: CKEditor 4.3.1
Component: General Version: 4.3.1
Keywords: Cc:

Description (last modified by Marek Lewandowski)

occures at current master, was not present in 4.3 release

Extra, empty span is left in certian situations

  1. open any sample with CKEditor (i.e. samples/replacebyclass.html)
  2. set source to following html
    <h3>Type the title here</h3>
    
    <p>Type the text here</p>
    
  3. go back to wysiwyg mode
  4. open search dialog
  5. type the text in find textfield
  6. click find
  7. close dialog

--- note already you have extra span left ---

You may now move cursor elsewhere and go back to the text substring, and span will still be there.

Expected result:
Produced source code:

<h3>Type the title here</h3>

<p>Type the text here</p>

Current result:
Code has extra span:

<h3>Type the title here</h3>

<p>Type <span>the text</span> here</p>

additional info:

  1. you can insert multiple ammount of spans, by clicking find multiple times, none of them will be cleaned
  2. i've reproduced it witch chrome and firefox @ Win8

Change History (8)

comment:1 Changed 6 years ago by Piotrek Koszuliński

Description: modified (diff)

comment:2 Changed 6 years ago by Marek Lewandowski

Description: modified (diff)

comment:3 Changed 6 years ago by Piotrek Koszuliński

Status: newconfirmed

comment:4 Changed 6 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: confirmedreview

Changes in t/11258 and corresponding test branch.

comment:5 Changed 6 years ago by Piotr Jasiun

What I do not like in this fix is the fact that I put changes in removeFromInsideElement. This method is called only from applyStyle but its name does not suggest this. I've added some description to removeFromInsideElement but I don't feel good about this.

comment:6 Changed 6 years ago by Piotrek Koszuliński

Yes, it's not plainly clear, but the comment made it understandable. I'm ok with such solution. Because it makes sense and also, because there's no other way except reorganising big part of that code :).

comment:7 Changed 6 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.3.1
Status: reviewreview_passed

I moved tests to style/applyremove. Those tests in style/editor are meant to test more than style system itself - they check if selection is ok and some other aspects of integration (like editor configuration).

I also pushed one commit to dev.

comment:8 Changed 6 years ago by Piotr Jasiun

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