confirmed merging cells does not work in inline editor

[charis]

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!

[Jakub Ś]

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.

[Piotrek Koszuliński]

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.

[Piotr Jasiun]

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 );

[Piotrek Koszuliński]

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 ;).

[Piotr Jasiun]

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.

[Piotrek Koszuliński]

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.