Opened 6 years ago

Closed 6 years ago

#11439 closed Bug (fixed)

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

Reported by: Lynne Kues Owned by: Olek Nowodziński
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, 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.

Change History (21)

comment:1 Changed 6 years ago by Jakub Ś

Component: GeneralCore : Tables
Status: newconfirmed
Version: 3.6.43.1

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

comment:2 Changed 6 years ago by Satya Minnekanti

Cc: satya_minnekanti@… added

comment:3 Changed 6 years ago by Teresa Monahan

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

comment:4 Changed 6 years ago by 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.

comment:5 Changed 6 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 4.3.3

Tentatively setting it to the next milestone.

comment:6 Changed 6 years ago by Jakub Ś

Owner: set to Jakub Ś
Status: confirmedassigned

comment:7 Changed 6 years ago by Jakub Ś

Status: assignedreview

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.

comment:8 Changed 6 years ago by Olek Nowodziński

Rebased branch on master.

comment:9 Changed 6 years ago by Olek Nowodziński

Summary: Setting background cell color on split cells changes cell layoutProperties get cloned in Cell Properties dialog if multiple cells are selected

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.

comment:10 Changed 6 years ago by Olek Nowodziński

Owner: changed from Jakub Ś to 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.

comment:11 Changed 6 years ago by 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.

comment:12 in reply to:  11 ; Changed 6 years ago by 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.

comment:13 in reply to:  12 Changed 6 years ago by Olek Nowodziński

Status: reviewassigned

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.

comment:14 Changed 6 years ago by Olek Nowodziński

Status: assignedreview

t/11439b (+tests)

comment:15 Changed 6 years ago by 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.

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

Status: reviewreview_failed

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.

comment:17 Changed 6 years ago by 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."

comment:18 Changed 6 years ago by 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.

comment:19 Changed 6 years ago by Olek Nowodziński

Status: review_failedreview

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.

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

Status: reviewreview_passed

comment:21 Changed 6 years ago by Olek Nowodziński

Resolution: fixed
Status: review_passedclosed

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

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