Opened 5 years ago

Closed 5 years ago

#11438 closed Bug (fixed)

Splitting table cells causes table content to move.

Reported by: Lynne Kues Owned by: Jakub Ś
Priority: Normal Milestone: CKEditor 4.3.3
Component: Core : Tables Version: 3.1
Keywords: IBM Cc: satya_minnekanti@…

Description

  1. Create a table 2 rows, by 3 columns.
  2. Specify first row data as "a", "b", "c".
  3. Specify second row data as "1", "2", "3".
  4. Split vertically first cell on first row. Enter "a1" as data for new cell.
  5. Split vertically last cell on first row.

Notice that the data you entered in step 4 moves.

See this behavior in 3.6, but also current 4.x.

Change History (16)

comment:1 Changed 5 years ago by Satya Minnekanti

Cc: satya_minnekanti@… added

comment:2 Changed 5 years ago by Teresa Monahan

This is a customer reported issue. Can you please prioritize it for the 4.3.3 release?

comment:3 Changed 5 years ago by Frederico Caldeira Knabben

Status: newconfirmed

Tentatively setting it to the next milestone.

comment:4 Changed 5 years ago by Jakub Ś

Component: GeneralCore : Tables
Version: 3.6.43.1

Just like #11439 this issue can be reproduced from CKEditor 3.1 in all browsers.

comment:5 Changed 5 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 4.3.3

comment:6 Changed 5 years ago by Jakub Ś

Owner: set to Jakub Ś
Status: confirmedassigned

comment:7 Changed 5 years ago by Jakub Ś

Status: assignedreview

I have created t/11438.

Seems that simple change in append method has done the trick but please test this.

comment:8 Changed 5 years ago by Piotrek Koszuliński

Status: reviewreview_failed

The patch seems to work, although few things are missing or have to be fixed:

  1. Code style issue - missing space.
  2. false doesn't have to be passed - it's a default value of append()'s second arg.
  3. Commit message shows some problem with encoding (git:2992549).
  4. Commit message does not explain how the issue has been fixed by this commit. It tells only that it was fixed, what's not enough.
  5. There's no test for this change - it should be added to tabletools/tabletools.html.

comment:9 Changed 5 years ago by Jakub Ś

Status: review_failedreview

Corrected: https://github.com/cksource/ckeditor-dev/tree/t/11438

  1. Fixed
  2. Fixed
  3. I believe I have fixed that by changing some options in Git Gui but I will have to wait to check this untill next commit with non latin character (https://github.com/msysgit/msysgit/wiki/Git-for-Windows-Unicode-Support).
  4. Method append(true) uses insertBefore so it doesn’t work in this particular case. If you were inserting cell 1.3 and then 1.1 error would not happen.
    In case of append() method, native appendChild(node) is used which seems to handle these things by default as it inserts TD into correct position.
  5. Tests created under branch t/11438 under commit git:bb29ba6

comment:10 Changed 5 years ago by Piotrek Koszuliński

DUP closed #9075. The same patch was proposed there.

comment:11 Changed 5 years ago by Olek Nowodziński

Status: reviewreview_failed

I rebased and force-pushed both branches. Also renamed the commit in dev branch.

Still:

  1. Duplicated test in tabletools.html L161-173:
    			// (#6111)
    			'test merge one cell': function()
    			{
    				this.doTest( 'merge-cell-right', 'cellMergeRight' );
    				this.doTest( 'merge-cell-down', 'cellMergeDown' );
    			},
    
    			// (#6111)
    			'test merge one cell': function()
    			{
    				this.doTest( 'merge-cell-right', 'cellMergeRight' );
    				this.doTest( 'merge-cell-down', 'cellMergeDown' );
    			},
    
  2. The file should get 'use strict'; to prevent such errors.
  3. White space characters (tabs) in L149 and L155.

comment:12 Changed 5 years ago by Jakub Ś

Status: review_failedreview
  1. Fixed.
  2. Added
  3. Fixed

I have pushed new commit git:663abc6 for branch t/11438.

Last edited 5 years ago by Jakub Ś (previous) (diff)

comment:13 Changed 5 years ago by Olek Nowodziński

Status: reviewreview_failed

White space characters (tabs) in lines 13, 152, 158, 163 and 222 (tabletools.html). I advise you to configure your text editor to either remove or display trailing white-spaces.

comment:14 Changed 5 years ago by Jakub Ś

Status: review_failedreview

Pushed git:5e76c93.

If there are more white spaces I will have to change editor.

comment:15 Changed 5 years ago by Olek Nowodziński

Status: reviewreview_passed

comment:16 Changed 5 years ago by Jakub Ś

Resolution: fixed
Status: review_passedclosed

Fixed on master with commit git:c878409 on dev and 8ec7c03 commit on test.

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