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

[kwillems]

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

[w.olchawa]

Confirmed both in IE and FF2.

[alfonsoml]

Proposed SVN patch

[alfonsoml]

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.

[kwillems]

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

[alfonsoml]

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

[kwillems]

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

[kwillems]

patch for ticket #1865

[kwillems]

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

[alfonsoml]

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

[martinkou]

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.

[alfonsoml]

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)

[shri046]

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.

[fredck]

#2487 may be related to this ticket.

[alfonsoml]

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.

[alfonsoml]

Some simplifications

[alfonsoml]

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.