Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tables need "remove empty row" post-fixer #3280

Closed
scofalik opened this issue Aug 15, 2019 · 6 comments
Closed

Tables need "remove empty row" post-fixer #3280

scofalik opened this issue Aug 15, 2019 · 6 comments
Labels
package:table resolution:duplicate This issue is a duplicate of another issue and was merged into it. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@scofalik
Copy link
Contributor

There is at least one case when it is possible to create an empty table row, even without real-time collaboration. I suspect it is also possible through some simultaneous changes in RTC.

To reproduce it, you need a table where there is only one non-spanned cell in a row and then remove this row through removing a column. See the gif below for an example.

Aug-15-2019 17-58-20

image

A similar thing happens when merging down but it is actually handled in the command:

// Remove empty row after merging.
if ( !removedTableCellRow.childCount ) {
	removeEmptyRow( removedTableCellRow, writer );
}

However, things like this should be handled through post-fixers. A command should do what it is supposed to do and then post-fixer should clean whatever incorrect state happens. It is better to handle it in post-fixer because:

  1. Multiple commands are handled at once.
  2. RTC scenarios are handled as well.
@scofalik
Copy link
Contributor Author

Additonally, "remove unnecessary colspans/rowspans" post-fixer will be needed.

@jodator
Copy link
Contributor

jodator commented Aug 20, 2019

Hmm... I think that this might be handled by the merge cell command or other. Probably the delete row command wasn't tested. Anyway - yes we need this.

@scofalik
Copy link
Contributor Author

I am not sure if I read your post correctly but I just wrote why it cannot be handled through commands.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-table Oct 9, 2019
@mlewand mlewand added this to the nice-to-have milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:bug This issue reports a buggy (incorrect) behavior. package:table labels Oct 9, 2019
@mlewand
Copy link
Contributor

mlewand commented Mar 12, 2020

Issue in remove column described in the first post here caused #6422. I wonder whether we should really go for a post-fixer, or make sure that the commands properly handle the model. Another solution might be both.

@Reinmar
Copy link
Member

Reinmar commented Mar 12, 2020

I wonder whether we should really go for a post-fixer, or make sure that the commands properly handle the model. Another solution might be both.

I'd go with commands leaving a clean editor first and see if we have any problems. If we'll see that we cannot really control it from the command perspective, we need a post-fixer indeed. A post-fixer should be a last resort.

@jodator
Copy link
Contributor

jodator commented May 26, 2020

So ultimately we went with the cleanup after yourself in commands and utils by introducing TableUtils.removeRow() and TableUtils.removeColumn() methods.

This is required to use that always as table modifications are not trivial and simple remove row element is not enough to create a proper table state. The reason of this is that after merging some cells you might create an empty row - which will yield table model error as defined in HTML specs. The solution for this is to remove this row and update overlapping cells rowspan attribute to not create an invalid table state.

Handled, among others, by: #6502.

@jodator jodator closed this as completed May 26, 2020
@jodator jodator removed this from the nice-to-have milestone May 26, 2020
@Reinmar Reinmar added the resolution:duplicate This issue is a duplicate of another issue and was merged into it. label Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:table resolution:duplicate This issue is a duplicate of another issue and was merged into it. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

No branches or pull requests

4 participants