Opened 10 years ago

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

Download all attachments as: .zip

Change History (31)

Changed 10 years ago by James

Attachment: insert table defect.jpg added

Error related to the defect

comment:1 Changed 10 years ago by Krzysztof Studnik

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

comment:2 Changed 10 years ago by Tobiasz Cudnik

Milestone: CKEditor 3.5

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

Version: 3.4.13.0

comment:4 Changed 10 years ago by Martin

Owner: set to Martin
Status: confirmedassigned

comment:5 Changed 10 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 10 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 10 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 10 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 10 years ago by Garry Yao

Milestone: CKEditor 3.5CKEditor 3.4.3

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

Attachment: 6432.patch added

comment:11 Changed 10 years ago by Martin

Status: confirmedreview

comment:12 in reply to:  10 Changed 10 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 10 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 10 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 10 years ago by Garry Yao

Attachment: 6432_2.patch added

comment:15 Changed 10 years ago by Martin

Milestone: CKEditor 3.4.3CKEditor 3.6

comment:16 Changed 10 years ago by Tobiasz Cudnik

Owner: changed from Martin to Tobiasz Cudnik
Status: review_failedassigned

I'm taking over this ticket over Mani.

Changed 10 years ago by Tobiasz Cudnik

Attachment: 6432_3.patch added

comment:17 Changed 10 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 10 years ago by Wiktor Walc

Milestone: CKEditor 3.6CKEditor 3.5.1

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

Please update the patch.

comment:20 Changed 10 years ago by Garry Yao

Status: reviewreview_failed

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

Changed 10 years ago by Garry Yao

Attachment: 6432_4.patch added

comment:21 Changed 10 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 10 years ago by Garry Yao

Attachment: 6432_5.patch added

comment:22 Changed 10 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 10 years ago by Garry Yao

Status: review_failedreview

comment:24 Changed 10 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 10 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 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy