Ticket #4574 (closed New Feature: fixed)

Opened 5 years ago

Last modified 4 years ago

V3: Table merging tools

Reported by: fredck Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.1
Component: General Version: SVN (CKEditor) - OLD
Keywords: Confirmed Review+ Cc: pomu@…

Description

CKEditor misses the merging tools for table cells that are present on V2.

Attachments

4574.patch (14.9 KB) - added by garry.yao 4 years ago.
4574_2.patch (14.5 KB) - added by garry.yao 4 years ago.
4574_3.patch (17.7 KB) - added by garry.yao 4 years ago.
4574_4.patch (19.8 KB) - added by garry.yao 4 years ago.
4574_5.patch (23.0 KB) - added by garry.yao 4 years ago.

Change History

comment:1 Changed 5 years ago by shri046

I searched through the open tickets but couldn't find one for "Splitting table cells". Should that be included with this ticket or should I open a new one?

Having the split/merge functions is one of our important editing requirements and also one of the reasons why we use CKEditor. I do realize that there was a rather lengthy discussion on split/merge functions for tables as it can get very complicated. I really hope these functions will be available soon.

I have already started working on the "merge" function and made some progress. I can upload a patch if you would like to take a look at it.

comment:2 Changed 5 years ago by fredck

Let's consider the split as part of this ticket also. If you have a patch for it, just go attaching it. We may or may not use it ;)

comment:3 Changed 4 years ago by garry.yao

  • Owner set to garry.yao
  • Status changed from new to assigned
  • Version set to SVN (CKEditor)

Changed 4 years ago by garry.yao

comment:4 Changed 4 years ago by garry.yao

  • Keywords Review? added

The following issues are out the scope of this ticket:

  1. Multiple cells selection broken with context menu open, which was on #4041;
  2. Unable to disable menu items on submenu due to a context menu bug.

Changed 4 years ago by garry.yao

comment:6 Changed 4 years ago by garry.yao

Updated TCs in considering of more rational rowSpan/colSpan values, attaching a new patch accordingly.

comment:7 Changed 4 years ago by pomu0325

  • Cc pomu@… added

comment:8 Changed 4 years ago by shri046

I am testing the second patch and have run into a few issues. All tests were done in the editor mode without switching to source mode.

Below issues were found in tests with Firefox 2

  1. When cells are merged there is an extra <br/> tag between the data that is merged from both cells. I think this stems from the fact that a "bogus <br/>" is added to the cells initially.
  1. When deleting the extra <br/> tags after merging cells, sometimes the 'Undo' history is wiped out. The 'Undo' will restore the linebreak however does not restore any of the merged cells. I haven't been able to come up with a good test case where this happens all the time and it has been rather sporadic so far.
  1. Merging th and td cells vertically breaks the table layout. Although I am not really sure if you should be allowed to merge th and td cells.
  1. Extra <p>&nbsp;</p> still get added to the end of the table when using some of the merge/split functions. No reliable test case for this either yet.


Below issues were found in tests with IE 6 (also used patch provided in http://dev.fckeditor.net/ticket/4041)

  1. Unable to merge cells vertically. IE cell selection always goes from left to right and not vertically as in Firefox so it is not possible to select two cells so that they can be merged vertically. I think this maybe an IE limitation and maybe the reason why we had separate 'Merge Right' and 'Merge Down' functions in FCKEditor.
  1. Trying to merge empty cells is not intuitive. When trying to select empty cells in IE the selection is just not reliable and clear. In the sense that the selection is not visible as compared to Firefox which is not really user friendly.

Changed 4 years ago by garry.yao

comment:9 Changed 4 years ago by garry.yao

Thanks for the test effort from shri046, I'm coming with a new patch with all dependent tickets' patch included.

@shri Fixings to FF issues 1,2,3 are targeted in the new patch. For the rests:

  1. FF4 - It's not related to this ticket;
  2. IE1 - That's true, my original intention is to open a new ticket for it;
  3. IE2 - Works for me, what kind of empty content in your test?

comment:10 Changed 4 years ago by shri046

Replying to garry.yao:

  1. IE2 - Works for me, what kind of empty content in your test?

I just inserted a default table in the editor. In IE all table cells by default have &nbsp; set as the content. Here's an example

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

Part of the problem is that when I make a selection table cell borders are not highlighted (maybe IE limitation) as in Firefox. If I right click at that point the selection is not highlighted anymore (light gray color on selection as displayed in Firefox) which is a bit confusing for regular users.

comment:11 Changed 4 years ago by garry.yao

Well, that's what we have on IE, the selection is obscure, but so long as it doesn't prevent you from selecting multiple cells.

Changed 4 years ago by garry.yao

comment:12 Changed 4 years ago by garry.yao

Adding merge right/down functionality, corresponding TCs added at :
http://ckeditor.t/tt/4574/1.html.

comment:13 Changed 4 years ago by shri046

Tested latest patch and here are the results.

IE - Merge down on a cell on the first attempt throws an error. Later attempts work.

Will the merge functions be displayed depending on the browser environment? Right now in IE I see the "Merge Cells" option which is disabled on single cell operations but there is no need for it. In Firefox only "Merge Cells" option should be visible.

comment:14 Changed 4 years ago by fredck

  • Keywords Review- added; Review? removed

I was not able to reproduce the IE error mentioned above by shri046.


With FF3.5:

  1. Create a default table.
  2. Right click into the first cell.
  3. Merge right.

A "zero height" cell is created, with the caret blinking on it.


With FF3.5:

  1. Create a default table.
  2. Right click into the first cell.
  3. Split vertical.
  4. Type text.

An empty line is appended before the typed text.

Changed 4 years ago by garry.yao

comment:15 Changed 4 years ago by garry.yao

  • Keywords Review? added; Review- removed

New patch targeting the above mentioned issues.

@shri046

Will the merge functions be displayed depending on the browser environment?

No, all reasonable operations are enabled currently.

Right now in IE I see the "Merge Cells" option which is disabled on single cell operations but there is no need for it.

Are you sure? Merge operations should only be available on multiple cells.

comment:16 Changed 4 years ago by fredck

  • Keywords Review+ added; Review? removed

Please commit it into the 3.1.x branch.

comment:17 Changed 4 years ago by garry.yao

  • Status changed from assigned to closed
  • Resolution set to fixed

Fixed with [4609] at 3.1.x branch.

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