Contextmenu in last row of a table with thead doesn't work

[Koen Willems]

I've just updated from version 2.4.3 to version 2.5.1.

I noticed that in a table with a thead en th-cells the contextmenu doesn't work when right-clicking in de last (bottom) row.

Example:

<table title="Tabel" cellspacing="0" cellpadding="0" summary="Tabel">
    <thead>
        <tr>
            <th scope="col">nummer</th>
            <th scope="col">Onderwerp</th>
        </tr>
    </thead>
    <tbody>
        <tr>
            <td>1</td>
            <td>abc</td>
        </tr>
        <tr>
            <td>2</td>
            <td>def</td>
        </tr>
    </tbody>
</table>

Without the thead there seems to be no problem.

regards, Koen Willems

[Wojciech Olchawa]

Confirmed both in IE and FF2.

[Alfonso Martínez de Lizarrondo]

Proposed SVN patch

[Alfonso Martínez de Lizarrondo]

The call to FCKTableHandler._CreateTableMap did use the tbody, so I've simplified the code by just passing the cell and then always navigate up to the table element in that function.

[Koen Willems]

Using the proposed 1865-patch I noticed splitting and mercing cells is messing up the output (I used the table given in my opening post). Regards, Koen Willems

[Alfonso Martínez de Lizarrondo]

Reviewing the code properly might take some time, so targetting to 2.7

[Koen Willems]

I sincerely hope that will be soon :-) But anayway: thanx very much for trying sofar.

[Koen Willems]

patch for ticket #1865

[Koen Willems]

Hello Alfonso,
I've been working on fcketablehandler.js myself (hope you don't mind).
I've attached a new patch-file.
Some of my coding looks awfull, but as far as I can see, things work fine now.

Regards,
Koen Willems

[Alfonso Martínez de Lizarrondo]

The suggested patch does generate errors if the dragresizetable plugin is loaded.

[Martin Kou]

With the thead feature added in #2430 I think this ticket would need to be fixed in 2.6.4 as well. I can help with fixing the dragresizetable plugin if needed.

[Alfonso Martínez de Lizarrondo]

Fixing the dragresizetable plugin is easy, just three lines and it works fine, but the proposed patch still has several problems. As I pointed out in #2048 the ability to split cells that aren't spanning any dimensions leads to extra code and it's hard to get all the cases right.

Let's remember that tables can contain lots of things as direct childs: one caption, several colgroups, one thead, one tfoot and several tbody. This must be kept in mind when trying to create the patch or testing if it works properly (at least it shouldn't break anything that did work previously)

[shri]

Attached a new patch that addresses a couple of issues. There is quite a bit of code change so this will need some stress testing. So far I have tested with IE6, FF2 and 3 creating complex tables with thead and tfoot and seems to be working fine.

The FCKTableHandler.HorizontalSplitCell function has a major change, in that I have eliminated the use of the FCKTableHandler._InstallTableMap function working directly at the DOM level. Apart from that this patch includes a possible fix for at least 2 more tickets #2486 and #2487. I am planning to update these tickets individually but just mentioned them here for reference as the code change seems to be related in these cases.

[Frederico Caldeira Knabben]

#2487 may be related to this ticket.

[Alfonso Martínez de Lizarrondo]

Regarding the 1865_1.patch:

In FCKTableHandler.HorizontalSplitCell it tries to get a reference to the table starting from the cell and reading .offsetParent but ​MDC states that the element returned should be the first positioned ancestor. The fact that it returns the table might not be crossbrowser compatible so it should be better to move up using the DOM: FCKTools.GetElementAscensor( cell, 'table' )

The modifications in FCKTableHandler._GetCellIndexSpan are wrong.

It might be better to provide a patch that does a single thing as it will be easier to understand and review.

[Alfonso Martínez de Lizarrondo]

Some simplifications

[Alfonso Martínez de Lizarrondo]

This patch does some simplifications, calling directly the parentNode of a cell to get the row, reusing the new existing FCKTools functions, removing the unused FCKTableHandler._GetColumnCells method and the like.

All the functionality should be preserved, these are just some details to avoid keeping them until a proper patch is created. The review+ would mean only that this patch can be applied, but the bug will remain open.

[Frederico Caldeira Knabben]

@alfonsoml, please remove the Review+ keyword after committing this patch, so the ticket remains pending. Thanks!

[Alfonso Martínez de Lizarrondo]

Changes applied with [2780]

If anyone has any proposal about other fixes for this ticket then step ahead :-)

[Koen Willems]

Attached is a new patch for fcktablehandler.js

The main problem in fcktablehandler.js is the fact that _InstallTableMap() doesn't handle thead's the proper way. Instead of building a new _InstallTableMap() I copy al the contents of thead into the first tbody before making the call to _InstallTableMap(). Afterwards the thead is reinstalled.

There was an issue I stumpled upon. After merging down cells empty rows appear, whereas the cells in the previous non_empty row have a wrong rowspan. I made a function to handle this (the removal of those empty rows in Gecko is a bit dirty; perhaps someone can improve that part).

Well anyway, things are working better than ever before.

Regards, Koen Willems

[Alfonso Martínez de Lizarrondo]

The change in _CreateTableMap will make the DragResizeTable plugin fail.
The getElementsByTagNames gets all the contents of a node, including sub-tables, so that will generate errors.
Unless strictly necessary it's better to focus just on one bug, trying to fix several bugs at once makes a more complex patch that it's harder to review

As the pattern

	FCKTableHandler.saveThead( refCell );
	this._InstallTableMap( tableMap, refCell.parentNode.parentNode ) ; 
	FCKTableHandler.restoreThead ( FCKTools.GetElementAscensor( refCell, 'TABLE' ) ) ;  

is used several times It might be more logical to put that as a function.

[Koen Willems]

Thanx for reviewing Alfonso!

About the DragResizeTable plugin: well, that's not the biggest problem at the moment. In _CreateTableMap

var table = refCell.parentNode.parentNode.parentNode ;

should be changed to

'var table = (refCell.nodeName == 'TABLE' ? refCell : refCell.parentNode.parentNode.parentNode ) ;

The pattern you mentioned is used on purpose in this stage, because I did't want to change _InstallTableMap at this moment. But in fact, it has nog significant effect on the working of the code.

The function getElementsByTagNames() does indeed grab all td- and th-cells, even those in nested tables. At this point I think it's better first to define what the tablehandler should be capable of at all. You know my opinion on nested tables: 'better not use them at all, because of a decrease of accessibility. On the other hand: it's possible to exclude cell of nested tables in getElementsByTagNames().

Regards, Koen

[Koen Willems]

I renewed the patch. It's working now with the DragResizeTable plugin.

I did some testing on getElementsByTagNames(). It is possible to exclude cells from nested tables. But fot the time beeing I think it's better to focus on single tables and leave issues concerning nested tables for later.

Regards,
Koen

[Alfonso Martínez de Lizarrondo]

Whenever you attach a new patch, don't overwrite the previous one, because it will be harder to understand the comments without the original code.

If you want to keep that getElementsByTagNames function then it should be placed in the proper module, and justify why you need it (with the risk involved that will need later adjustments) instead of getting just the cells with a simple loop. And also is it really needed to sort the output?

The fact that you don't like nested tables doesn't matter. The rest of the world uses them so the code must take them into account.

Creating a new function/adjusting the _InstallTableMap function is a must. All the calls to _InstallTableMap now are wrapped with these new functions, so there's no reason to keep them separate. Also, nowhere else the SaveThead and RestoreThead are used so that's another reason to "hide" them in a single call.

[Koen Willems]

Well, since fckeditor doesn't support to create nested tables (- after clicking in a cell one gets the table properties dialog -) and because there is no ticket to build such a feature I don't think it should have priority in this stage, resolving this particular ticket (#1865).

About getElementsByTagNames itself: I think it is possible to leave it out at and (for instance) save the row numbers when saving thead to the first tbody. Do you think that's a better approach?

I'll implement the new functions into _InstallTableMap. :-)

But, back to the patch itself, the idea of 'saving' thead to the first tbody, is that ok with you?

Regards,
Koen Wilems

[Alfonso Martínez de Lizarrondo]

Replying to kwillems:

Well, since fckeditor doesn't support to create nested tables (- after clicking in a cell one gets the table properties dialog -) and because there is no ticket to build such a feature I don't think it should have priority in this stage, resolving this particular ticket (#1865).

Uh?
Nested tables work without any problem, you can bet that if support for them was removed the forums will get full of complaints quickly. Put your cursor in a cell and then insert a new table, it just works.

But, back to the patch itself, the idea of 'saving' thead to the first tbody, is that ok with you?

When you readjust the code you will see that probably it will be extra work, you are marking each cell, then changing its location, recreate the rows and after that change again its location. I guess that it should be possible to do it without any change of location.

Besides, there's another bug that it's interesting to remember: the fact that any attribute set on rows are lost, I bet that it can be fixed at the same time by getting an array of rows and their parents.

[Koen Willems]

Wow! I really did not know that it is possible to create nested tables. I looked at my code ... it appeared long time ago I made some changes which (in fact unintended) disabled the ability to insert a table into a cell ....

Well, since I intend to get a decent tablehandler before the end of this year I made several steps back. I guess it's better to solve all problems concerning tables just step by step :-)

Attached is a new patch, just for further development purposes. I made some changes to _InstallTableMap so it will support tablesections like thead. For Geckolike user agents I made use of innerHTML. It could be done with more DOM-like functions, but I think this way it works the fastest (and with less coding).

As far as I could test the patch it's preserving al further capabilities of the current tablehandler. It supports the dragresizetable-plugin and more important ... it supports nested tables. I stopped testing nested tabled onto a third level, but I guess it could hold thousands of them!

Just to keep it possible to keep theads when splitting down a cell, I preserved the changes I made before to VerticalSplitCell. It's just to keep decent functionality at the moment. In a later stage VerticalSplitCell should make a call to _InstallTableMap (I already did some testing, but in the adepting of the tablemap I stumbled upon some empty array elements).

Next steps in the further development of the tablehandler would be:

1. Make it impossible to merge down cells from one tablesection to another (as it has been suggested in ​http://www.fckeditor.net/forums/viewtopic.php?f=11&t=10726
2. Adjust the colspans when splitting and merging horizontaly (it's a matter of removing identical columns from the tableMap)
3. Adjust rowspans via the tablemap and _InstallTableMap after vertically splitting and merging cells
4. Resolve the other tickets concerning tables.

Well anyway, take a look at this one.

And Alfonso ... when we get it solved before the end of the year I'll buy you a beer, ok?

[Koen Willems]

I did some improvement on VerticalSplitCell() to preserve tablesections after splitting down cells. The code is now much more efficient.

[Frederico Caldeira Knabben]

Fixed with [2906]. Thanks Koen and Alfonso for your support on this.