Opened 7 years ago

Closed 7 years ago

#6432 closed Bug (fixed)

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

Reported by: James Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.5.1
Component: UI : Dialogs Version: 3.0
Keywords: IBM Cc: Damian, joek, Satya Minnekanti

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 7 years ago.
Error related to the defect
6432.patch (2.2 KB) - added by Martin 7 years ago.
6432_2.patch (978 bytes) - added by Garry Yao 7 years ago.
6432_3.patch (1.4 KB) - added by Tobiasz Cudnik 7 years ago.
6432_4.patch (2.2 KB) - added by Garry Yao 7 years ago.
6432_5.patch (2.1 KB) - added by Garry Yao 7 years ago.

Download all attachments as: .zip

Change History (31)

Changed 7 years ago by James

Attachment: insert table defect.jpg added

Error related to the defect

comment:1 Changed 7 years ago by Krzysztof Studnik

Component: GeneralUI : Dialogs
Keywords: Firefox added
Status: newconfirmed

comment:2 Changed 7 years ago by Tobiasz Cudnik

Milestone: CKEditor 3.5

comment:3 Changed 7 years ago by Sa'ar Zac Elias

Version: 3.4.13.0

comment:4 Changed 7 years ago by Martin

Owner: set to Martin
Status: confirmedassigned

comment:5 Changed 7 years ago by Martin

Status: assignedpending

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

comment:6 Changed 7 years ago by James

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 7 years ago by Martin

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 Changed 7 years ago by Tobiasz Cudnik

Status: pendingconfirmed

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 7 years ago by Garry Yao

Milestone: CKEditor 3.5CKEditor 3.4.3

comment:10 in reply to:  8 ; Changed 7 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 7 years ago by Martin

Attachment: 6432.patch added

comment:11 Changed 7 years ago by Martin

Status: confirmedreview

comment:12 in reply to:  10 Changed 7 years ago by Garry Yao

Status: reviewreview_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 7 years ago by Martin

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 7 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 7 years ago by Garry Yao

Attachment: 6432_2.patch added

comment:15 Changed 7 years ago by Martin

Milestone: CKEditor 3.4.3CKEditor 3.6

comment:16 Changed 7 years ago by Tobiasz Cudnik

Owner: changed from Martin to Tobiasz Cudnik
Status: review_failedassigned

I'm taking over this ticket over Mani.

Changed 7 years ago by Tobiasz Cudnik

Attachment: 6432_3.patch added

comment:17 Changed 7 years ago by Tobiasz Cudnik

Status: assignedreview

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 7 years ago by Wiktor Walc

Milestone: CKEditor 3.6CKEditor 3.5.1

comment:19 Changed 7 years ago by Sa'ar Zac Elias

Please update the patch.

comment:20 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

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

Changed 7 years ago by Garry Yao

Attachment: 6432_4.patch added

comment:21 Changed 7 years ago by Garry Yao

Owner: changed from Tobiasz Cudnik to Garry Yao
Status: review_failedreview

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 7 years ago by Garry Yao

Attachment: 6432_5.patch added

comment:22 Changed 7 years ago by Sa'ar Zac Elias

Status: reviewreview_failed

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

comment:23 Changed 7 years ago by Garry Yao

Status: review_failedreview

comment:24 Changed 7 years ago by Sa'ar Zac Elias

Status: reviewreview_passed

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

comment:25 Changed 7 years ago by Garry Yao

Keywords: Firefox Discussion removed
Resolution: fixed
Status: review_passedclosed

Fixed with [6334].

#6986 is created for selection with partial list.

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