Deleting cells in nested table removes outer table cell.

[David J. Hamilton]

STEPS TO REPRODUCE

0. Use either Chrome or Firefox. Bug is likely reproducible in IE as well, but I haven't confirmed this.
1. Navigate to a 3.6.2 cke sample page. Presently, ​http://ckeditor.com/demo exhibits this error.
2. Clear any existing content in the editor.
3. Create a table with 3 rows and 2 columns (OUTER).
4. In the top left cell, create a nested table with 3 rows and 2 columns (INNER).
5. In the INNER table, select the top two cells.
6. Right click to get the context menu and select cell -> delete cells.

EXPECTED RESULT

Only the top two cells of the INNER table are deleted.

ACTUAL RESULT

The top left cell of the OUTER table is removed, thus removing the entire INNER table.

MORE INFO

Please see related ticket #6289. This seems to be a regression from sometime after 3.4.2. Note that #6289 was reported as an IE-only issue, but this ticket affects FF and Chrome (and likely IE as well).

DIAGNOSIS

The two bits of relevant code are in the following files:

​http://dev.ckeditor.com/browser/CKEditor/trunk/_source/plugins/tabletools/plugin.js ​http://dev.ckeditor.com/browser/CKEditor/trunk/_source/core/dom/walker.js

In tabletools, the function getSelectedCells is of interest, especially lines 63-68.

54	while ( ( node = walker.next() ) )
55	{
56	  // If may be possible for us to have a range like this:
57	  // <td>^1</td><td>^2</td>
58	  // The 2nd td shouldn't be included.
59	  //
60	  // So we have to take care to include a td we've entered only when we've
61	  // walked into its children.
62	
63	  var parent = node.getAscendant( 'td' ) || node.getAscendant( 'th' );
64	  if ( parent && !parent.getCustomData( 'selected_cell' ) )
65	  {
66	    CKEDITOR.dom.element.setMarker( database, parent, 'selected_cell', true );
67	    retval.push( parent );
68	  }
69	}

This loop examines each node from the range start to the range end, according to walker.js. I am not entirely clear on the semantics of walker.js so I cannot say for sure whether it is correct, but from tracing it seems to be doing something reasonable.

When one selects two cells, the relevant portion of the DOM looks something like this:

TABLE (outer)
  TR
    TD (call this cell OA)
      BR
      TABLE (inner)
        TR
          TD (call this cell IA)
            BR  (call this node IABR)
          TD (call this cell IB)
            BR  (call this node IBBR)
        TR
          TD (call this cell IC)
            BR  (call this node ICBR)
          TD
            BR
        TR
          TD
            BR
          TD
            BR
    TD
       BR
   TR
     TD
       BR
     TD
       BR
   TR
     TD
       BR
     TD
       BR

When we select the top two cells of the INNER table our range goes from IABR to IBBR. It is also very easy to accidentally select the first cell of the next row, resulting in a range that goes from IABR to ICBR. The walker considers these nodes, but also the table cells IA, IC and their parent TR.

Note that line 63 fails to pass true (i.e. includeself) to getAscendant, and so erroneously includes cell OA in retval.

PROGNOSIS/PATCH

Please find attached a patch for this problem. This patch does two things.

1. Passes includeself for the two calls to getAscendant.
2. Ignores a number of table related nodes (TABLE TR TBODY TFOOT THEAD). If the walker were to consider a TBODY element in a nested table, I think the same behaviour would happen without this check.

This patch was made against cke 3.6.2.

OTHER

If anything is unclear about the issue or patch, please do not hesitate to email me at either dhamilton at verticalresponse.com or cke at hjdivad.com. Thank you in advance for your time.

[Jakub Ś]

Reproducible from CKEditor 3.6.1 rev [6918]

To reproduce either insert table at the end of content or clear contents and insert table.

NOTE:IE9 has problems with inserting nested tables in first cell: #8769

[David J. Hamilton]

@j.swiderski, why did you drop the haspatch keyword? Do you have some concerns with the patch that I could address?

[Jakub Ś]

By mistake. Sorry.

[Jakub Ś]

This one should be easier to apply

[David J. Hamilton]

No worries; just wanted to fixup the patch if it was needed. Thanks for looking at the issue. ^_

[Jakub Ś]

I had some problems with running tha patch for the first time but after refreshing the cache patch worked as expected.

[David J. Hamilton]

@j.swiderski what's preventing this patch from making it upstream? Is there something I can do to help it along?

[Jakub Ś]

@hjdivad the fix looks correct and it looks like the same problem can be reproduced on CKEDitor 4.0.

To get your proposed fix tested and applied (if it is correct) please fork CKEditor 4 code from ​https://github.com/ckeditor/ckeditor-dev, prepare your fix and make a "pull request" (here is the example list of pull request). In comment please specify link to bug for which this fix is.
This is much faster way to get proposed fix implemented/rejected (if they are invalid). At least in V4.
In CKEditor 3.x we go through proposed SVN patches every couple of weeks (sometimes moths) and decide whether to apply them or not. This of course takes more time then pull requests on github but this is what versioning system offered and we had to adjust to it.

[David J. Hamilton]

Ticket moved to ​https://github.com/ckeditor/ckeditor-dev/pull/12

[Olek Nowodziński]

• Ported and fixed ​https://github.com/ckeditor/ckeditor-dev/pull/12
• Pushed solution to ​t/8675@cksource.
• Created appropriate tests in t/8675@ckeditor-tests-v4.

[Piotrek Koszuliński]

Tabletools plugin needs refac, because at least code of this method may be optimized, but this requires new ticket.

[Olek Nowodziński]

Replying to Reinmar:

Tabletools plugin needs refac, because at least code of this method may be optimized, but this requires new ticket.

Extracted #9813 with this idea.

[Olek Nowodziński]

Rebased branches.

[Olek Nowodziński]

• Masterized dev ​git:fc2bf605
• Masterized tests 4cbb19e