alfonsoml wrote:The support for THead has been added in http://dev.fckeditor.net/ticket/2430#comment:10
Now there's a new patch to support TH in the cells dialog in http://dev.fckeditor.net/ticket/822 that needs some testing.

Couple of quick questions -
1. Applying patch 822_1 by itself and testing leads to JS error as it is unable to find the 'RenameNode' function. This is a part of the patch for 2430. Does the dependency needs to be mentioned? I am testing each patch individually so that it does not conflict with anything else.

2. After applying both patches - Creating a 3x3 table and right click on 1st cell (1st row, 1st column) and changing the cell type to 'Header' converts ONLY that cell to <th>. Is this intentional? Does the change to propagate through all cells in that row with the assumption that all columns will have a header?

<table width="200" cellspacing="1" cellpadding="1" border="1">
    <tbody>
        <tr>
            <th style="">col 1</th>
            <th style="">Col 3</th>
            <th style="">Col 4</th>
        </tr>
        <tr>
            <td>Col 5</td>
            <td>Col 6</td>
            <td>Col 7</td>
        </tr>
        <tr>
            <td>Col 8</td>
            <td>Col 9</td>
            <td>Col 10</td>
        </tr>
    </tbody>
</table>

3. FireFox adds a style="" attribute to the <th> as you can see above. IE doesn't do that.

No.
2430 has been already applied to the trunk, so you should update your local copy and it will be applied.

Sorry I missed that one. Still getting used to all the SVN stuff

Why should it?
you have selected a cell and mark it as a header or data. The rest of the cells will be whatever you want.

It was just a thought that crossed when using IE. I cannot select all cells in the first row in IE, in FireFox I can do that no problem. With the table empty initially, I cannot convert all cells to <th> at one go. I have to do them one at a time. If however, I enter the table and add text in the cells of the first row, select the entire text in all cells AND right click on the selected text and then go to cell properties to change cell type to 'Header', all cells will now become <th>. This is only specific to IE though.

Also just noticed another problem with splitting of <th> cells.
1. Add 3x3 table. Convert all cells in first row to 'Header'.

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

2. Merge first two cells

<table cellspacing="1" cellpadding="1" width="200" border="1">
    <tbody>
        <tr>
            <th colspan="2">&nbsp;c&nbsp;c</th>
            <th>&nbsp;c</th>
        </tr>
        <tr>
            <td>&nbsp;</td>
            <td>&nbsp;</td>
            <td>&nbsp;</td>
        </tr>
        <tr>
            <td>&nbsp;</td>
            <td>&nbsp;</td>
            <td>&nbsp;</td>
        </tr>
    </tbody>
</table>

3. Splitting the merged cell horizontally produces a 'Data' cell rather than a 'Header' cell.

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

Same behavior in both FireFox and IE.

Sounds good. I agree about IE selection behavior. It is one of those "nice to have" lists but may not be possible to include everything.

I ran some more tests and didn't see any issues. The patch looks good.

alfonsoml wrote:Did you filed the ticket about splitting THs?

Just did https://dev.fckeditor.net/ticket/2472.

alfonsoml wrote:I wanted to fix next #1865, but it's a little complex, so for the moment I've attached a patch to fix http://dev.fckeditor.net/ticket/2472 and it does include some other little simplifications in the code.

The patch seems ok. Tested it successfully in multiple scenarios on FireFox 2 and IE 6.

Now there are quite a few other things I noticed but they are not related to this patch. Tried searching through tickets and forums but nothing turned up.

Issue #1: Vertically splitting cell with colspan > 2 breaks table layout. (Tested below steps on demo page in FF and IE)
1. Create a 3x2 table.
2. Split cell 1 in row 1 horizontally.
3. Split cell 1 in row 2 vertically.
Result: The cell is split vertically into 2 but only the first cell has colspan="2". The second cell is missing this colspan which breaks the table layout.

Issue #2: Split non-header cell in table with <thead> throws JavaScript error. (Tested below steps on trunk samples default page in FF and IE)
1. Create a 3x2 table with first row header.
2. Split non-header cell horizontally and JS error is thrown.
3. Also splitting non-header cell vertically breaks the table layout.

I think I found another issue but I can't remember it right now. If someone can confirm the above then I can create a ticket if needed.

Shri

No problem. I have created tickets for the issues mentioned above -
https://dev.fckeditor.net/ticket/2486
https://dev.fckeditor.net/ticket/2487

Any chance of including drag-adjustable column and row borders?

alfonsoml, yes, just like that. I saw it in one version I downloaded, but can no longer find it. Users really like that capability.

alfonsoml wrote:Thanks for the testing.
I guess that I might fix them as part of #1865 because I think that it might need a great review of all the code involved.

Any progress on this one yet? I have a possible fix for https://dev.fckeditor.net/ticket/2486 still in the process of testing it.

Attached patch for https://dev.fckeditor.net/ticket/2486

The main problem is that in order to fix other bugs (that are triggered more easily like the context-menu at last row) I think that the _CreateTableMap and _InstallTableMap might need several revisions, that are more important than this issue at hand.

I can see why this is an issue now that I have been in the code for a while. Bear with me but I have a question about the create/install tablemap functions. What is the main reason driving the need for these functions in the code? Given that we already have access to the table DOM object, it seems to me that creating a map out of it is redundant.

I have been working on the fcktablehandler.js for a few days now and am trying to figure out if the create/install tablemap can be eliminated and re-write the code to work directly at the DOM level. So far I have been able to make the FCKTableHandler.HorizontalSplitCell function independent of the create/install tablemap function and it seems to be working pretty well. The advantage with this approach is that working with theads, tbodies and so on becomes easier (just my opinion). Again, pardon my ignorance if there is something missing, but is this option even worth looking into?

I would really appreciate some feedback before I go deeper into.

Thanks,
Shri

alfonsoml wrote:I guess that when Martinkou coded the new table code it seemed the best option at the moment. If you think about tables as holding only one tbody, then they are a matrix, so using a function to get the details about that matrix and handling that way all the rowspan and colspan seems indeed like a very good idea.

But when there are several of those matrix in the table, then we might need to review the code and check if it can be improved to handle properly the new situations, so if you think that you can improve the code go ahead, try to work little by little and that way we might find the code that works.

I have attached a new patch for https://dev.fckeditor.net/ticket/1865 that has some of the modifications I mentioned earlier. I do see the advantage of create tablemap as it generates a rectangular matrix which is definitely easier to work with. Going along those lines, I have modified the horizontal split function to work at the DOM level rather than use the install tablemap function. Tested it quite a bit but I need a bit of help on testing/validating that patch.

I'm sorry about not reviewing still your previous patch, but these days I'm busy and in order to test it I want to have some time to focus on it.

Not a problem. I really appreciate all the time and efforts you guys put into this. I am really hoping that we can get some of these fixes into 2.6.4 release

Thanks again,
Shri

Possible bug http://www.fckeditor.net/forums/viewtopic.php?f=6&t=11430

The post refers only to inserting table within list item. It pretty much follows for other elements as well. Tested below on demo page.

Insert a table with cursor placed inside a <p> tag and the <p> tag is split into two with the table inserted between the <p> tags.

Before inserting table:

<p>This is some <strong>sample text</strong>. You are using <a href="http://www.fckeditor.net/">FCKeditor</a>.</p>

After inserting table:

<p>This is some <strong>sample text</strong>.</p>
<table width="200" cellspacing="1" cellpadding="1" border="1">
    <tbody>
        <tr>
            <td>&nbsp;</td>
            <td>&nbsp;</td>
        </tr>
        <tr>
            <td>&nbsp;</td>
            <td>&nbsp;</td>
        </tr>
    </tbody>
</table>
<p>You are using <a href="http://www.fckeditor.net/">FCKeditor</a>.</p>

Same goes for <div> elements.

Before inserting table:

<div>This is some <strong>sample text</strong>. You are using <a href="http://www.fckeditor.net/">FCKeditor</a>.</div>

After inserting table:

<div>This is some <strong>sample text</strong>. </div>
<table width="200" cellspacing="1" cellpadding="1" border="1">
    <tbody>
        <tr>
            <td>&nbsp;</td>
            <td>&nbsp;</td>
        </tr>
        <tr>
            <td>&nbsp;</td>
            <td>&nbsp;</td>
        </tr>
    </tbody>
</table>
<div>You are using <a href="http://www.fckeditor.net/">FCKeditor</a>.</div>

Finally I could do some fixing to http://dev.fckeditor.net/ticket/1865 and some other bugs related to it.
Well, in fact, it's not a fix, but a kind of workaround.

The main problem in fcktablehandler.js is the fact that _InstallTableMap doesn't handle thead's.
Instead of building a new _InstallTableMap I just copied al the contents of thead into tbody, putting an extra attribute into those cells (so I can recognize them afterwards).
Than I make the call to _InstallTableMap.
At the end I restore thead.

As I said, it is in fact a workaround, but since the bug is 10 months old, I guess it's the best result sofar.
I just build it an hour ago, so at the moment I'm cleaning up my coding. Furthermore I planned to build in support for captions.

After that I will post the patch.

Regards,
Koen Willems

Well, considering the captions ... i've to make some adjustments to my code, since if there is one it's the firstchild of the table and not thead.

I've just submitted the patch, see http://dev.fckeditor.net/ticket/1865

There are still some questions left that need to be answered before the handling of tables can be further improved.

1. Should it be possible to merge down cells from thead to tbody?
2. If so, should the merged cell be in thead or in tbody?
3. Take a default 3 * 2 table, split one cell horizontaly and merge it back to one cell. All cells in that column have a colspan="2". Should that stay that way or should the colspan be lowered (and in this case: removed)?
4. Should the merging of a th-cel with a td-cell result in a th-cell (it does at the moment) or a td-cell?

kwillems wrote:
1. Should it be possible to merge down cells from thead to tbody?
2. If so, should the merged cell be in thead or in tbody?
4. Should the merging of a th-cel with a td-cell result in a th-cell (it does at the moment) or a td-cell?

I believe those represent different groups of cell and the merging operations should be limited to a single group.

kwillems wrote:
3. Take a default 3 * 2 table, split one cell horizontaly and merge it back to one cell. All cells in that column have a colspan="2". Should that stay that way or should the colspan be lowered (and in this case: removed)?

Having clean HTML is good. There is no sense having a column with all cells at the same colspan, and the same for rows with rowspan. So, I think it would be nice if the splitting operations would lowerdown *span attributes, finally removing them at zero.