Opened 6 years ago

Closed 6 years ago

#10736 closed Bug (duplicate)

confirmed merging cells does not work in inline editor

Reported by: charis Owned by: Piotrek Koszuliński
Priority: Normal Milestone:
Component: Core : Tables Version: 4.0 Beta
Keywords: Cc:

Description

Hi there

Thanks for your swift reply but......

I did as you said and cleared my cache,when I checked on CK editor using the inline editor, merging cells does not work .

with yet another freshly downloaded full build of CKeditor I found that merging cells is still not possible, with further probing

I found that merging cells does work for the fixed position editor, but not the inline editor.

What I did.....

I dowloaded a fresh full ckeditor and saved the folder to my desktop i opened 'inlineall' , and 'inlinetextarea', and tested both these pages is FF and merging cells does not work (IE does not even allow to highlight cells)

you can recreate the problem on nightly

http://nightly.ckeditor.com/13-08-18-13-05/full/samples/inlineall.html

merging cells does not work

I have tried to attached two screenshots to display the problem but I cannot figure out how to.

Pretty sure this is a legitimate bug, and so I report it to you

Have a nice day.

Keep up the good work!

Attachments (2)

CK_inline_notworking.jpg (218.4 KB) - added by charis 6 years ago.
CK_inline_notworking.2.jpg (218.4 KB) - added by charis 6 years ago.

Download all attachments as: .zip

Change History (8)

Changed 6 years ago by charis

Attachment: CK_inline_notworking.jpg added

Changed 6 years ago by charis

Attachment: CK_inline_notworking.2.jpg added

comment:1 Changed 6 years ago by Jakub Ś

Keywords: merge removed
Status: newconfirmed
Version: 4.24.0 Beta

I was using merge right and down but didn't use plain merge.
To reproduce this problem please select at least two cells, right-click and select merge. JavaScript error will pop out.

Message: cell.getDocument is not a function
Line: 403
URI: /ckeditor4-git/plugins/tabletools/plugin.js

I was able to reproduce this only in FF from CKEditor 4.0 beta.

comment:2 Changed 6 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.2.1
Owner: set to Piotrek Koszuliński
Status: confirmedreview

I had a quick look at this issue. It turns out that there are two overlapping ones.

I pushed t/10736 with a safe correction for placeCursorInCell function which in some conditions may be executed without passed cell.

However, this only prevents throwing error, but mergeCells algorithm still doesn't work. I reported #10745 with more precise description of what happens.

comment:3 Changed 6 years ago by Piotr Jasiun

Status: reviewreview_failed

It is always bad to hide errors like this:

function placeCursorInCell( cell, placeAtEnd ) {
	if ( !cell )
		return;

It could be the reason for problems when someone want to use function in the future and it doesn't work without any error. Especially when such function is part of bigger logic then one can get an error in totally different place or get no error at all.

In this case it may be not a big deal but we should avoid such construction.

It is far better to throw/cache error or just check paramereter, ex. replace:

placeCursorInCell( mergeCells( editor.getSelection(), 'right' ), true );

With:

cell = mergeCells( editor.getSelection(), 'right' );
cell && placeCursorInCell(cell, true );

comment:4 Changed 6 years ago by Piotrek Koszuliński

Status: review_failedreview

This commit does not hide errors - it's fixing logic only which additionally avoids throwing error.

From puristic point of view your solution is of course better, because running placeCursorInCell we could always expect that it fails when there's no cell. But it is exaggerated in this case, because we would need to add additional lines to every usage of placeCursorInCell( mergeXXX() ). That's why I decided to extract that logic to placeCursorInCell - to not waste space in order to satisfy purism ;).

comment:5 Changed 6 years ago by Piotr Jasiun

Status: reviewreview_failed

There are only 3 places where you have placeCursorInCell( mergeCells ( /*...*/ ) ). It is not a lot of code to check parameter in this 3 places. BTW: in two others usage of mergeCells argument is already checked.

There are also 2 other places where placeCursorInCell is called not related with this ticket:

  • placeCursorInCell( verticalSplitCell ( /*...*/ ) ) and
  • placeCursorInCell( horizontalSplitCell ( /*...*/ ) )

If you hide an error inside placeCursorInCell you will hide potential problems with verticalSplitCell and horizontalSplitCell so we will never know if there something wrong with these function.

comment:6 Changed 6 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.2.1
Resolution: duplicate
Status: review_failedclosed

After a bit of discussion I decided not to go with such fix which does not fix anything (without it editor does not crash either) and it does not fix the algorithm for which I reported #10745. So I'm closing this ticket in favour of #10745 which is more precise.

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