Fix: The table cell should always have paragraph in the model. #127
Conversation
src/converters/table-post-fixer.js
Outdated
| * </tr> | ||
| * </tbody> | ||
| * </table> | ||
| * | ||
| * ## Ensuring proper table structure |
There was a problem hiding this comment.
table structure? or rather "Ensuring a proper content of table cells"?
src/converters/table-post-fixer.js
Outdated
| @@ -49,14 +50,14 @@ import TableWalker from './../tablewalker'; | |||
| * <table> | |||
| * <thead> | |||
| * <tr> | |||
| * <td rowspan="2">FOO</td> | |||
| * <td colspan="2">BAR</td> | |||
| * <td rowspan="2"><p>FOO</p></td> | |||
There was a problem hiding this comment.
It seems that my comment disappeared so one more time...
Is it correct? Those <p>s won't be rendered (there will be spans or nothing). It's a bit confusing and I'm not sure if this is a mistake or a mental shortcut.
tests/converters/table-post-fixer.js
Outdated
| @@ -181,6 +181,19 @@ describe( 'Table post-fixer', () => { | |||
| } ); | |||
| } ); | |||
|
|
|||
| describe( 'on removing tableCell contents', () => { | |||
| it( 'should fix empty table cells', () => { | |||
There was a problem hiding this comment.
It's hard to understand from this test that it checks that a paragraph should be added. I checked that it fails without the patch and the error is quite obvious then, but the code and description of this test are cryptic.
There are many opinions regarding writing so many tools for creating tests. But, leaving this aside, when we write them we should make sure that tests remain understandable.
|
Other than the issues in the code, I checked that this PR fixes the bug. |
… Rename table-post-fixer to table-layout-post-fixer.
|
@Reinmar I've updated the solution:
|
| // @param {module:engine/model/element~Element} table | ||
| // @param {module:engine/model/writer~Writer} writer | ||
| function fixTableCellContent( tableCell, writer ) { | ||
| if ( tableCell.childCount == 0 ) { |
There was a problem hiding this comment.
What if a cell already has some content, but that's a text? We can make this a followup because I don't know right now scenario which would lead to this, but it's certainly possible.
There was a problem hiding this comment.
AFAIR the $text is not allowed in the model of a table cell.
There was a problem hiding this comment.
Which does not mean it cannot end there. Schema rules are not enforced. Check out https://ckeditor5.github.io/docs/nightly/ckeditor5/latest/framework/guides/deep-dive/schema.html#implementing-additional-constraints and the following section.
Suggested merge commit message (convention)
Fix: The table cell should always have paragraph in the model. Closes ckeditor/ckeditor5#3237.
BREAKING CHANGE: The
injectTablePostFixer()function fromtable/converters/table-post-fixeris nowinjectTableLayoutPostFixer()and is moved totable/converters/table-layout-post-fixermodule.Additional information