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.
I would file the splitting of th as a new ticket, as it isn't a problem created by the current patch, that problem also exist right now if you create a th in source view. That way it's easier to manage everything if we go little by little.
The selection of several cells might be different in IE and Firefox, but the fact is that the way to work with those browsers are different, so we might want to fix as much as possible, but we can't do everything.
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.
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.
I've created a patch for http://dev.fckeditor.net/ticket/1982 it should affect only IE7, and besides the horizontal dimension, it also fixes the problem that a submenu can't be higher than the main menu.
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.
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.
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'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.
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.
splitting the <p> is ok, tables shouldn't be nested within <p>. I agree that splitting the div isn't nice, but the problem is that theoricaly, inside the div you should add a container like a <p> (so that <p> is splited, not the div). As I said, I agree that the div shouldn't be splitted.
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.
That's great, please add in the ticket the proposed patch so it can be reviewed and tested by more people.
But I don't understand the comment about captions. The current table features already include captions. Or do you mean that it doesn't work with your current code?
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.
Re: Improving the tables
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.
Re: Improving the tables
Re: Improving the tables
Re: Improving the tables
Re: Improving the tables
The selection of several cells might be different in IE and Firefox, but the fact is that the way to work with those browsers are different, so we might want to fix as much as possible, but we can't do everything.
Re: Improving the tables
I ran some more tests and didn't see any issues. The patch looks good.
Re: Improving the tables
Re: Improving the tables
Re: Improving the tables
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.
Re: Improving the tables
Re: Improving the tables
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.
Re: Improving the tables
No problem. I have created tickets for the issues mentioned above -
https://dev.fckeditor.net/ticket/2486
https://dev.fckeditor.net/ticket/2487
Re: Improving the tables
Re: Improving the tables
Re: Improving the tables
I've created a patch for http://dev.fckeditor.net/ticket/1982 it should affect only IE7, and besides the horizontal dimension, it also fixes the problem that a submenu can't be higher than the main menu.
Re: Improving the tables
Re: Improving the tables
That's the dragresizetable plugin.
Re: Improving the tables
Re: Improving the tables
Attached patch for https://dev.fckeditor.net/ticket/2486
Re: Improving the tables
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
Re: Improving the tables
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'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.
Re: Improving the tables
Re: Improving the tables
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:
Re: Improving the tables
Re: Improving the tables
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
Re: Improving the tables
But I don't understand the comment about captions. The current table features already include captions. Or do you mean that it doesn't work with your current code?
Re: Improving the tables
Re: Improving the tables
I've just submitted the patch, see http://dev.fckeditor.net/ticket/1865
Re: Improving the tables
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?
Re: Improving the tables
I believe those represent different groups of cell and the merging operations should be limited to a single group.
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.
Frederico Knabben
CKEditor Project Lead and CKSource Owner
--
Follow us on: Twitter | Facebook | Google+ | LinkedIn