Opened 6 years ago

Closed 5 years ago

#6432 closed Bug (fixed)

Error occurring when trying to insert a table over a highlighted list

Reported by: james c Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.5.1
Component: UI : Dialogs Version: 3.0
Keywords: IBM Cc: damo, joek, satya

Description

Steps to reproduce the defect:

  1. Open the Ajax sample.
  2. Create a numbered/bulleted list containing 5 items.
  3. Press CTRL + A to highlight all of the text. (note: if using FF highlight the text using the mouse)
  4. Click on the Insert Table icon in the toolbar.
  5. Press OK in the Table Properties dialog.

Expected: The table dialog should close and a table should appear in the editor.

Actual: The table dialog does not close and the text in the list disappears leaving 2 empty bullets/numbers. Also, an error occurs. (details of the error are in the screenshot)

Attachments (6)

insert table defect.jpg (68.3 KB) - added by james c 6 years ago.
Error related to the defect
6432.patch (2.2 KB) - added by mani 6 years ago.
6432_2.patch (978 bytes) - added by garry.yao 6 years ago.
6432_3.patch (1.4 KB) - added by tobiasz.cudnik 6 years ago.
6432_4.patch (2.2 KB) - added by garry.yao 5 years ago.
6432_5.patch (2.1 KB) - added by garry.yao 5 years ago.

Download all attachments as: .zip

Change History (31)

Changed 6 years ago by james c

Error related to the defect

comment:1 Changed 6 years ago by krst

  • Component changed from General to UI : Dialogs
  • Keywords Firefox added
  • Status changed from new to confirmed

comment:2 Changed 6 years ago by tobiasz.cudnik

  • Milestone set to CKEditor 3.5

comment:3 Changed 6 years ago by Saare

  • Version changed from 3.4.1 to 3.0

comment:4 Changed 6 years ago by mani

  • Owner set to mani
  • Status changed from confirmed to assigned

comment:5 Changed 6 years ago by mani

  • Status changed from assigned to pending

How is Your expectation about this case ? Similar situation with paragraph works that inserting table remove selected items.

comment:6 Changed 6 years ago by james c

I would expect the following to occur:

  1. The table dialog would close.
  2. The list will be completely removed.
  3. The table will be displayed in the editor overriding the list.

comment:7 Changed 6 years ago by mani

What if we select for example 2 items of 5 items length list, we should completly remove list or only slected items to replace them by table ?

comment:8 follow-up: Changed 6 years ago by tobiasz.cudnik

  • Status changed from pending to confirmed

Let's not complicate the case, the new table should remove the list like every other selected element, even if it's partially selected. Although for nested lists, it should remove only a deeper one.

comment:9 Changed 6 years ago by garry.yao

  • Milestone changed from CKEditor 3.5 to CKEditor 3.4.3

comment:10 in reply to: ↑ 8 ; follow-up: Changed 6 years ago by garry.yao

Replying to tobiasz.cudnik:

Let's not complicate the case, the new table should remove the list like every other selected element...

For me it should break the list into two instead.

Changed 6 years ago by mani

comment:11 Changed 6 years ago by mani

  • Status changed from confirmed to review

comment:12 in reply to: ↑ 10 Changed 6 years ago by garry.yao

  • Status changed from review to review_failed

When only get partially selected, extract content should not delete the entire list but break it into two parts. Replying to garry.yao:

Replying to tobiasz.cudnik:

Let's not complicate the case, the new table should remove the list like every other selected element...

For me it should break the list into two instead.

comment:13 Changed 6 years ago by mani

  • Keywords Discussion added

I think we should sum up how this functionality should work:

Let`s assume we have such a situations:

I situation

<ul>
 <li>A</li>
 <li>[B]</li> 
 <li>C</li>
<ul>

so after insert table I expect:

<ul>
 <li>A</li>
 <li> <table></table> </li> 
 <li>C</li>
<ul>

II situation:

<ul>
 <li>A</li>
 <li>B^</li> 
 <li>C</li>
<ul>

After insert table I expect

<ul>
 <li>A</li>
 <li>B<table></table></li> 
 <li>C</li>
<ul>

III situation

<ul>
 <li>[A</li>
 <li>B</li> 
 <li>C]</li>
<ul>

After insert table I expect

<table></table>

IV situation

<ul>
 <li>A</li>
 <li>
  <ul>
   <li>B</li> 
   <li>[C</li> 
  </ul>
 </li>
 <li>D]</li>
<ul>

You think we should replace all list or create one of list as under:

<ul>
 <li>A</li>
 <li>
   <ul>
     <li>B</li>
   </ul>
 </li>
 <li><table></table></li>
<ul>

OR

<ul>
 <li>A</li>
 <li>
   <ul>
     <li>B</li>
     <li><table></table></li>
   </ul>
 </li>
<ul>

comment:14 Changed 6 years ago by garry.yao

Nice to see those cases are clear listed here, I don't know too, it maybe browser dependent? But the best way to check the expected results is to have the table insertion work just like pasting a table with the same selection.

Changed 6 years ago by garry.yao

comment:15 Changed 6 years ago by mani

  • Milestone changed from CKEditor 3.4.3 to CKEditor 3.6

comment:16 Changed 6 years ago by tobiasz.cudnik

  • Owner changed from mani to tobiasz.cudnik
  • Status changed from review_failed to assigned

I'm taking over this ticket over Mani.

Changed 6 years ago by tobiasz.cudnik

comment:17 Changed 6 years ago by tobiasz.cudnik

  • Status changed from assigned to review

This is yet another approach, which does simple unification on the selection across a list elements. It has more similar results in various browsers than usage of insertHtml function from the second patch.

comment:18 Changed 6 years ago by wwalc

  • Milestone changed from CKEditor 3.6 to CKEditor 3.5.1

comment:19 Changed 6 years ago by Saare

Please update the patch.

comment:20 Changed 5 years ago by garry.yao

  • Status changed from review to review_failed

6432_3.patch doesn't achieve the result requested by comment:6.

Changed 5 years ago by garry.yao

comment:21 Changed 5 years ago by garry.yao

  • Owner changed from tobiasz.cudnik to garry.yao
  • Status changed from review_failed to review

Note that the list selection in IE has a bug that it's always impossible to select a full list (proved by clicking on path bar), propose a simple fix to range::deleteContents that handles this situation in a better way, as there's no fix could be delivered on the browser side.

Changed 5 years ago by garry.yao

comment:22 Changed 5 years ago by Saare

  • Status changed from review to review_failed

I didn't test the patch yet, but the patch targets IE only, what about other browsers?

comment:23 Changed 5 years ago by garry.yao

  • Status changed from review_failed to review

comment:24 Changed 5 years ago by Saare

  • Status changed from review to review_passed

The patch fixes the initial TC, please open another ticket for the rest of the TCs.

comment:25 Changed 5 years ago by garry.yao

  • Keywords Firefox Discussion removed
  • Resolution set to fixed
  • Status changed from review_passed to closed

Fixed with [6334].

#6986 is created for selection with partial list.

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