Properties get cloned in Cell Properties dialog if multiple cells are selected

[Lynne Kues]

1. Create a table, 2 rows, 3 columns.
2. Split cell vertically first row, first column.
3. Split cell vertically first row, last column.
4. Enter cell data for first row "a", "a1" for first split cell, "b" for second cell, "c", "c1" for last split cell.
5. Drag select the first row so that you select all the values you entered above.
6. Set Cell Properties - set background to red.

Notice that the cell layout changes.

Apparent on 3.6 and current 4.x. Browsers IE and Chrome.

[Jakub Ś]

Problem can be reproduced from CKEditor 3.1 (When splitting cells was made available) in all browsers.

[Teresa Monahan]

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

[Frederico Caldeira Knabben]

It's not the background color thing that is bringing this issue. It's a more generic problem with the dialog. For instance, if you change other things in the dialog (e.g. "Word Wrap"), the same problem gonna happen.

[Frederico Caldeira Knabben]

Tentatively setting it to the next milestone.

[Jakub Ś]

This is first push ​git:fdfa322 to find out if approach taken is correct.

Source of the problem
This is happening because cell properties dialog is filled with information of first cell only.

Proposed solution
It seems that completely different approach is needed here.

1. Setup - all selected cells should be passed into setup method. The setup method should check if values are different and if so it should not enter anything into input filed.
   I think this is most sane approach. It is possible to select multiple cells with different values but we have only one input filed.
   Proposed solution is - if passed values are same, display the value, if not, leave this field empty.
2. Commit - in order to avoid attaching properties of first cell to all selected cells we need to check if value in input field was changed or not. That way either old cell value will be used or new from input field.

TODO

1. Tests
2. Probably some code style

Possible TODO

1. There might be always someone who will say that not displaying values is confusing (like error behaviour was better). Perhaps we could display some small warning icon (with tooltip) next to field saying something like "Multiple values were used for selected cells. If you want to leave them, don't change this value. If you want to change all selected cells, insert value."

This change seems to also fix #11255 and #2568.

[Olek Nowodziński]

Rebased branch on master.

[Olek Nowodziński]

A more general case:

1. Set editor HTML

   <table border="1" cellpadding="1" cellspacing="1">
   	<tbody>
   		<tr>
   			<td>a[a</td>
   			<td rowspan="2">bb</td>
   		</tr>
   		<tr>
   			<td>a]a1</td>
   		</tr>
   	</tbody>
   </table>
   

2. Right click, select Cell->Cell Properties.
3. Click OK in the dialog (don't change anything).

Expected: The same HTML as in 1.

Actual:

<table border="1" cellpadding="1" cellspacing="1">
	<tbody>
		<tr>
			<td>aa</td>
			<td>bb</td>
		</tr>
		<tr>
			<td>aa1</td>
		</tr>
	</tbody>
</table>

What happened: The dialog loaded rowspan value of the first selected cell ("aa"), which is 0 (no attribute). While closing, the value is applied to all cells within the selection. This is not only the (row|col) span problem though, while it concerns all the properties in the dialog.

[Olek Nowodziński]

I pushed a fix to ​t/11439b, which appears to be much simpler than the one from comment:7. Tests included in corresponding branch.

[Jakub Ś]

@a.nowodzinski this solution looks super smart, is nicer and is definetely faster than mine (In 50x50 (2500 cells) table with same values mine method took 190 milliseconds longer than original code).

One remark - I'm not sure about concept of showing first cell values. Imagine below scenario

<table border="1" cellpadding="1" cellspacing="1" style="width:500px">
	<tbody>
		<tr>
			<td style="width:50px">&nbsp;</td>
			<td>&nbsp;</td>
		</tr>
		<tr>
			<td>&nbsp;</td>
			<td>&nbsp;</td>
		</tr>
	</tbody>
</table>

User has above code in editor. He selects all cells and selects cell properties dialog in which he sees 50px set for width field.
User than thinks - this is what I wanted to do apply 50px to all selected cells.
When he presses OK - nothing happens.
To change this value he would have to first change value to e.g. 51 and than again to 50.

That is why I was thinking that "if values are different don't show anytinh in input filed" is the best approach here.

[Frederico Caldeira Knabben]

Replying to j.swiderski:

That is why I was thinking that "if values are different don't show anytinh in input filed" is the best approach here.

Yes, it seems to be a good way for it.

[Olek Nowodziński]

Replying to fredck:

Replying to j.swiderski:

That is why I was thinking that "if values are different don't show anytinh in input filed" is the best approach here.

Yes, it seems to be a good way for it.

Alright. I'll work it out.

[Olek Nowodziński]

​t/11439b (+tests)

[Jakub Ś]

This new commit looks ok to me. I have tested it manually for 30 minutes with different scenarios and didn’t get any problems with it.

[Piotrek Koszuliński]

I stumbled upon one tricky case. What if one cell has different different width's unit, but the same "number". E.g. 50% and 50px. Currently, on ​branch:t/11439b in such case the "number" is loaded in the dialog, when unit is left empty. I think that it's incorrect, because 50px has totally different meaning than 50% so loading "50" doesn't make sense. There's also a bug, because if I set unit and close dialog one cell will still has 50% and the second 50px.

In my opinion the most reasonable behaviour would be:

1. Load both values empty (number and unit).
2. If user sets number but leaves unit empty we set width to <number>px on all cells. We default to pixels because it's more likely that user meant pixels than percents. If we were wrong, user can open the dialog again and alter the unit.
3. If user sets both values (number and unit) we set width to <number><unit>.
4. If user sets unit, but leaves number empty, we do nothing.

[Jakub Ś]

There's also a bug, because if I set unit and close dialog one cell will still has 50% and the second 50px.

This needs checking. I see this happening in every proposed fix.

incorrect, because 50px has totally different meaning than 50% so loading "50" doesn't make sense.

In my opinion the most reasonable behaviour would be:

What about KISS? Width field is used only to display numeric value so if they are the same for all fields it IMO makes sense to show it.
I think that real problem here is that unit value doesn't get updated and this part should be fixed only.

@Reinmar I think that point 2 in proposed solution introduced new bug: "If we were wrong, user can open the dialog again and alter the unit."

[Piotrek Koszuliński]

What about KISS? Width field is used only to display numeric value so if they are the same for all fields it IMO makes sense to show it.

That's according to developer. But I bet that user won't see any similarity between 50% and 50px. And I cannot imagine a case when someone wants to change both values to e.g... 30% and 30px ;). It's a completely useless behaviour.

@Reinmar I think that point 2 in proposed solution introduced new bug: "If we were wrong, user can open the dialog again and alter the unit."

This is not a bug - this is "action-feedback-action-..." approach. You cannot explain user every nuance about feature's behaviour. But that's not necessary, because based on immediate feedback (thanks god it's a WYSIWYG editor) he can correct his mistakes.

Anyway, if it's much more complicated to have this working the way I proposed, let's just fix the bug I mentioned. All this is one big edge case, so there's no need to have perfect solution.

[Olek Nowodziński]

I created #11584 so all the concerns expressed by Reinmar and j.swiderski in comment:16 (and later) will be resolved there. The proposed solution is not straightforward and there's not much time left to deal with it in 4.3.3.

[Olek Nowodziński]

​git:4339b82 landed in master (33ec9fd tests).