Opened 14 years ago

Closed 14 years ago

#4574 closed New Feature (fixed)

V3: Table merging tools

Reported by: Frederico Caldeira Knabben 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 (5)

4574.patch (14.9 KB) - added by Garry Yao 14 years ago.
4574_2.patch (14.5 KB) - added by Garry Yao 14 years ago.
4574_3.patch (17.7 KB) - added by Garry Yao 14 years ago.
4574_4.patch (19.8 KB) - added by Garry Yao 14 years ago.
4574_5.patch (23.0 KB) - added by Garry Yao 14 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 14 years ago by shri

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 14 years ago by Frederico Caldeira Knabben

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 14 years ago by Garry Yao

Owner: set to Garry Yao
Status: newassigned
Version: SVN (CKEditor)

Changed 14 years ago by Garry Yao

Attachment: 4574.patch added

comment:4 Changed 14 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 14 years ago by Garry Yao

Attachment: 4574_2.patch added

comment:6 Changed 14 years ago by Garry Yao

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

comment:7 Changed 14 years ago by pomu0325

Cc: pomu@… added

comment:8 Changed 14 years ago by shri

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 14 years ago by Garry Yao

Attachment: 4574_3.patch added

comment:9 Changed 14 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 14 years ago by shri

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 14 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 14 years ago by Garry Yao

Attachment: 4574_4.patch added

comment:12 Changed 14 years ago by Garry Yao

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

comment:13 Changed 14 years ago by shri

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 14 years ago by Frederico Caldeira Knabben

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 14 years ago by Garry Yao

Attachment: 4574_5.patch added

comment:15 Changed 14 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 14 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

Please commit it into the 3.1.x branch.

comment:17 Changed 14 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [4609] at 3.1.x branch.

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy