Opened 9 years ago

Closed 8 years ago

#2389 closed Bug (worksforme)

Merge Cells removes attributes from tr in table

Reported by: Matthew Leffler Owned by:
Priority: Normal Milestone:
Component: General Version: FCKeditor 2.5 Beta
Keywords: Confirmed DataLoss Cc:

Description

When two cells are merged the class attribute on the tr(s) in the table is removed.

Steps to reproduce:

Click on the source button and remove the existing code and paste in the following

<table>
    <tbody>
        <tr class="sample_class">
            <td>Test cell</td>
            <td>Test cell 2</td>
        </tr>
    </tbody>
</table>

Click the source button again to exit source view and click in the two cells. Right-click and select merge cells (or merge right). Click source again and the class attribute is removed from the tr.

This is reproducible in either the demo on FCKeditor.net or the nightly build. It reproduces in FF3, FF2, IE7, and IE6 and doesn't seem to be dependent on platform used.

Attachments (3)

2389.patch (3.1 KB) - added by shri 9 years ago.
2389.2.patch (2.9 KB) - added by shri 9 years ago.
Revised patch
2389_3.patch (685 bytes) - added by Koen Willems 9 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 9 years ago by Alfonso Martínez de Lizarrondo

Component: Core : StylesGeneral
Keywords: Confirmed added
Version: FCKeditor 2.6.2FCKeditor 2.5 Beta

The problem is the FCKTableHandler._InstallTableMap function that does removes all the rows in the table and then recreates them again, so any attribute in any row affected by the table operations (merge and split cells) will be lost.

It's not related to the Styles component, but to the tables handling, the code was introduced with [690], part of 2.5beta

comment:2 Changed 9 years ago by Alfonso Martínez de Lizarrondo

Keywords: DataLoss added
Summary: Merge Cells removes class from tr in tableMerge Cells removes attributes from tr in table

comment:3 in reply to:  2 Changed 9 years ago by shri

Replying to alfonsoml:

Adding a patch for this defect. Tested it on FireFox 2.0 and IE 6 and seems to have fixed the issue.

Changed 9 years ago by shri

Attachment: 2389.patch added

Changed 9 years ago by shri

Attachment: 2389.2.patch added

Revised patch

comment:4 Changed 9 years ago by Alfonso Martínez de Lizarrondo

Having a function named CopyAttributesToArray that returns an object looks really strange. The comments should reflect the functionality in a generic way (don't use "on the row element", or that "could probably be used in other scenarios")

To me it looks strange that the functions added to fckdomtools are asymetric. One takes care of doing all the loop, but the second one only applies one attribute at a time.

The logic about rowCount is flawed. You can test that merging some cells in this table changes the classes:

<table cellspacing="1" cellpadding="1" border="1" width="200">
    <tbody>
        <tr class="odd">
            <td>&nbsp;</td>
            <td>&nbsp;</td>
        </tr>
        <tr>
            <td>&nbsp;</td>
            <td>&nbsp;</td>
        </tr>
        <tr class="odd">
            <td>&nbsp;</td>
            <td>&nbsp;</td>
        </tr>
    </tbody>
</table>

Changed 9 years ago by Koen Willems

Attachment: 2389_3.patch added

comment:5 Changed 9 years ago by Koen Willems

I added a litle patch to preserve classes on rows and so on.

Regards, Koen Willems

comment:6 Changed 9 years ago by Koen Willems

Oops ... please wait reviewing this patch. I stumbled upon some problems with another patch i'm making.

comment:7 Changed 9 years ago by Koen Willems

Ok, go ahead ... the patch is working!

comment:8 Changed 8 years ago by Matthew Leffler

Resolution: worksforme
Status: newclosed

Closing as this WFM on the CKEditor trunk.

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