From 5744877e2b16846d5e180de133c2f1af9bfd0ae9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 25 Sep 2018 16:35:35 +0200 Subject: [PATCH 1/3] Fix: The table cell should always have paragraph in the model. --- src/converters/table-post-fixer.js | 123 ++++++++++++++++----------- tests/converters/table-post-fixer.js | 13 +++ 2 files changed, 87 insertions(+), 49 deletions(-) diff --git a/src/converters/table-post-fixer.js b/src/converters/table-post-fixer.js index 16023a9f..bcb13e19 100644 --- a/src/converters/table-post-fixer.js +++ b/src/converters/table-post-fixer.js @@ -21,6 +21,7 @@ import TableWalker from './../tablewalker'; * * * All table rows have the same size. * * None of a table cells that extend vertically beyond their section (either header or body). + * * A table cell has always at least one element as child. * * If the table structure is not correct, the post-fixer will automatically correct it in two steps: * @@ -35,12 +36,12 @@ import TableWalker from './../tablewalker'; * * * - * FOO - * BAR + * FOO + * BAR * * - * BAZ - * XYZ + * BAZ + * XYZ * *
* @@ -49,14 +50,14 @@ import TableWalker from './../tablewalker'; * * * - * - * + * + * * * * * - * + * * * *
FOOBAR

FOO

BAR

BAZ - * XYZ + *

BAZ

XYZ

@@ -84,14 +85,14 @@ import TableWalker from './../tablewalker'; * * * - * - * + * + * * * * * - * + * * * *
FOOBAR

FOO

BAR

BAZ - * XYZ + *

BAZ

XYZ

@@ -109,12 +110,12 @@ import TableWalker from './../tablewalker'; * * * - * - * + * + * * * - * + * * * *
1112

11

12

21 - * 22 + *

21

22

@@ -130,18 +131,18 @@ import TableWalker from './../tablewalker'; * * * - * - * - * + * + * + * * * - * - * - * + * + * + * * * - * - * + * + * * * *
1112(empty, inserted by A)

11

12

(empty, inserted by A)

2122(empty, inserted by A)

21

22

(empty, inserted by A)

(empty, inserted by B)(empty, inserted by B)

(empty, inserted by B)

(empty, inserted by B)

@@ -151,19 +152,19 @@ import TableWalker from './../tablewalker'; * * * - * - * - * + * + * + * * * - * + * + * + * * * - * - * - * + * + * + * * * *
1112(empty, inserted by A)

11

12

(empty, inserted by A)

21 - * 22 - * (empty, inserted by A)

21

22

(empty, inserted by A)

(empty, inserted by B)(empty, inserted by B)(empty, inserted by a post-fixer)

(empty, inserted by B)

(empty, inserted by B)

(empty, inserted by a post-fixer)

@@ -175,17 +176,17 @@ import TableWalker from './../tablewalker'; * * * - * - * + * + * * * - * - * + * + * * * - * - * - * + * + * + * * * *
1112

11

12

2122

21

22

(empty, inserted by B)(empty, inserted by B)(empty, inserted by a post-fixer)

(empty, inserted by B)

(empty, inserted by B)

(empty, inserted by a post-fixer)

@@ -195,23 +196,41 @@ import TableWalker from './../tablewalker'; * * * - * - * - * + * + * * * - * + * + * * * - * + * + * * * *
1112(empty, inserted by a post-fixer after undo) + *

11

12

(empty, inserted by a post-fixer after undo)

21 - * 22 - * (empty, inserted by a post-fixer after undo) + *

21

22

(empty, inserted by a post-fixer after undo)

(empty, inserted by B) - * (empty, inserted by B) - * (empty, inserted by a post-fixer) + *

(empty, inserted by B)

(empty, inserted by B)

(empty, inserted by a post-fixer)

* + * ## Ensuring proper table structure + * + * A table cells must contains at least one block as a child. The empty table cell will have empty `` as a child. + * + * + * + * + * + *
+ * + * Will be fixed to: + * + * + * + * + * + *
+ * * @param {module:engine/model/model~Model} model */ export default function injectTablePostFixer( model ) { @@ -233,6 +252,12 @@ function tablePostFixer( writer, model ) { for ( const entry of changes ) { let table; + // Enforce paragraph in tableCell even after other feature remove its contents. + if ( entry.type == 'remove' && entry.position.parent.is( 'tableCell' ) && entry.position.parent.childCount == 0 ) { + writer.insertElement( 'paragraph', entry.position.parent ); + wasFixed = true; + } + // Fix table on table insert. if ( entry.name == 'table' && entry.type == 'insert' ) { table = entry.position.nodeAfter; diff --git a/tests/converters/table-post-fixer.js b/tests/converters/table-post-fixer.js index 9626a482..1d5775eb 100644 --- a/tests/converters/table-post-fixer.js +++ b/tests/converters/table-post-fixer.js @@ -181,6 +181,19 @@ describe( 'Table post-fixer', () => { } ); } ); + describe( 'on removing tableCell contents', () => { + it( 'should fix empty table cells', () => { + setModelData( model, modelTable( [ [ '11[]' ] ] ) ); + + model.change( writer => { + // Remove paragraph from table cell. + writer.remove( Range.createIn( root.getNodeByPath( [ 0, 0, 0 ] ) ) ); + } ); + + expect( formatTable( getModelData( model, { withoutSelection: true } ) ) ).to.equal( formattedModelTable( [ [ '' ] ] ) ); + } ); + } ); + describe( 'on collaboration', () => { it( 'should add missing cells to columns (remove column vs insert row)', () => { _testExternal( From 9bad1217be4af92cd4d17aaae0f7f7ec25bb18af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Mon, 1 Oct 2018 12:36:27 +0200 Subject: [PATCH 2/3] Docs: Remove `

` from examples in table cell post-fixer. --- src/converters/table-post-fixer.js | 90 +++++++++++++++--------------- 1 file changed, 45 insertions(+), 45 deletions(-) diff --git a/src/converters/table-post-fixer.js b/src/converters/table-post-fixer.js index bcb13e19..1a346a95 100644 --- a/src/converters/table-post-fixer.js +++ b/src/converters/table-post-fixer.js @@ -50,14 +50,14 @@ import TableWalker from './../tablewalker'; * * * - * - * + * + * * * * * - * - * + * + * * * *

FOO

BAR

FOOBAR

BAZ

XYZ

BAZXYZ
@@ -85,14 +85,14 @@ import TableWalker from './../tablewalker'; * * * - * - * + * + * * * * * - * - * + * + * * * *

FOO

BAR

FOOBAR

BAZ

XYZ

BAZXYZ
@@ -110,12 +110,12 @@ import TableWalker from './../tablewalker'; * * * - * - * + * + * * * - * - * + * + * * * *

11

12

1112

21

22

2122
@@ -131,18 +131,18 @@ import TableWalker from './../tablewalker'; * * * - * - * - * + * + * + * * * - * - * - * + * + * + * * * - * - * + * + * * * *

11

12

(empty, inserted by A)

1112(empty, inserted by A)

21

22

(empty, inserted by A)

2122(empty, inserted by A)

(empty, inserted by B)

(empty, inserted by B)

(empty, inserted by B)(empty, inserted by B)
@@ -152,19 +152,19 @@ import TableWalker from './../tablewalker'; * * * - * - * - * + * + * + * * * - * - * - * + * + * + * * * - * - * - * + * + * + * * * *

11

12

(empty, inserted by A)

1112(empty, inserted by A)

21

22

(empty, inserted by A)

2122(empty, inserted by A)

(empty, inserted by B)

(empty, inserted by B)

(empty, inserted by a post-fixer)

(empty, inserted by B)(empty, inserted by B)(empty, inserted by a post-fixer)
@@ -176,17 +176,17 @@ import TableWalker from './../tablewalker'; * * * - * - * + * + * * * - * - * + * + * * * - * - * - * + * + * + * * * *

11

12

1112

21

22

2122

(empty, inserted by B)

(empty, inserted by B)

(empty, inserted by a post-fixer)

(empty, inserted by B)(empty, inserted by B)(empty, inserted by a post-fixer)
@@ -196,19 +196,19 @@ import TableWalker from './../tablewalker'; * * * - * - * - * + * + * + * * * - * - * - * + * + * + * * * - * - * - * + * + * + * * * *

11

12

(empty, inserted by a post-fixer after undo)

1112(empty, inserted by a post-fixer after undo)

21

22

(empty, inserted by a post-fixer after undo)

2122(empty, inserted by a post-fixer after undo)

(empty, inserted by B)

(empty, inserted by B)

(empty, inserted by a post-fixer)

(empty, inserted by B)(empty, inserted by B)(empty, inserted by a post-fixer)
From 996136f24bbb7e49424eaf487b5006511c03f845 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Mon, 1 Oct 2018 14:17:09 +0200 Subject: [PATCH 3/3] Changed: Extract table-cell-content-post-fixer from table-post-fixer. Rename table-post-fixer to table-layout-post-fixer. --- .../table-cell-content-post-fixer.js | 112 ++++++++++++++++ ...st-fixer.js => table-layout-post-fixer.js} | 40 +----- src/tableediting.js | 6 +- .../table-cell-content-post-fixer.js | 126 ++++++++++++++++++ ...st-fixer.js => table-layout-post-fixer.js} | 15 +-- 5 files changed, 250 insertions(+), 49 deletions(-) create mode 100644 src/converters/table-cell-content-post-fixer.js rename src/converters/{table-post-fixer.js => table-layout-post-fixer.js} (91%) create mode 100644 tests/converters/table-cell-content-post-fixer.js rename tests/converters/{table-post-fixer.js => table-layout-post-fixer.js} (96%) diff --git a/src/converters/table-cell-content-post-fixer.js b/src/converters/table-cell-content-post-fixer.js new file mode 100644 index 00000000..3ee86b46 --- /dev/null +++ b/src/converters/table-cell-content-post-fixer.js @@ -0,0 +1,112 @@ +/** + * @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md. + */ + +/** + * @module table/converters/table-cell-content-post-fixer + */ + +/** + * Injects a table cell post-fixer into the model. + * + * The role of the table post-fixer is to ensure that the table cells have the correct content + * after a {@link module:engine/model/model~Model#change `change()`} block was executed. + * + * A table cells must contains at least one block as a child. The empty table cell will have empty `` as a child. + * + * + * + * + * + *
+ * + * Will be fixed to: + * + * + * + * + * + *
+ * + * @param {module:engine/model/model~Model} model + */ +export default function injectTableCellContentPostFixer( model ) { + model.document.registerPostFixer( writer => tableCellContentsPostFixer( writer, model ) ); +} + +// The table cell contents post-fixer. +// +// @param {module:engine/model/writer~Writer} writer +// @param {module:engine/model/model~Model} model +function tableCellContentsPostFixer( writer, model ) { + const changes = model.document.differ.getChanges(); + + let wasFixed = false; + + for ( const entry of changes ) { + // Enforce paragraph in tableCell even after other feature remove its contents. + if ( entry.type == 'remove' && entry.position.parent.is( 'tableCell' ) ) { + wasFixed = fixTableCellContent( entry.position.parent, writer ) || wasFixed; + } + + // Analyze table cells on insertion. + if ( entry.type == 'insert' ) { + if ( entry.name == 'table' ) { + wasFixed = fixTable( entry.position.nodeAfter, writer ) || wasFixed; + } + + if ( entry.name == 'tableRow' ) { + wasFixed = fixTableRow( entry.position.nodeAfter, writer ) || wasFixed; + } + + if ( entry.name == 'tableCell' ) { + wasFixed = fixTableCellContent( entry.position.nodeAfter, writer ) || wasFixed; + } + } + } + + return wasFixed; +} + +// Fixes all table cells in a table. +// +// @param {module:engine/model/element~Element} table +// @param {module:engine/model/writer~Writer} writer +function fixTable( table, writer ) { + let wasFixed = false; + + for ( const row of table.getChildren() ) { + wasFixed = fixTableRow( row, writer ) || wasFixed; + } + + return wasFixed; +} + +// Fixes all table cells in a table row. +// +// @param {module:engine/model/element~Element} tableRow +// @param {module:engine/model/writer~Writer} writer +function fixTableRow( tableRow, writer ) { + let wasFixed = false; + + for ( const tableCell of tableRow.getChildren() ) { + wasFixed = fixTableCellContent( tableCell, writer ) || wasFixed; + } + + return wasFixed; +} + +// Fixes all table cell content by adding paragraph to a table cell without any child. +// +// @param {module:engine/model/element~Element} table +// @param {module:engine/model/writer~Writer} writer +function fixTableCellContent( tableCell, writer ) { + if ( tableCell.childCount == 0 ) { + writer.insertElement( 'paragraph', tableCell ); + + return true; + } + + return false; +} diff --git a/src/converters/table-post-fixer.js b/src/converters/table-layout-post-fixer.js similarity index 91% rename from src/converters/table-post-fixer.js rename to src/converters/table-layout-post-fixer.js index 1a346a95..347ea67b 100644 --- a/src/converters/table-post-fixer.js +++ b/src/converters/table-layout-post-fixer.js @@ -4,7 +4,7 @@ */ /** - * @module table/converters/table-post-fixer + * @module table/converters/table-layout-post-fixer */ import Position from '@ckeditor/ckeditor5-engine/src/model/position'; @@ -12,9 +12,9 @@ import { createEmptyTableCell, findAncestor, updateNumericAttribute } from './.. import TableWalker from './../tablewalker'; /** - * Injects a table post-fixer into the model. + * Injects a table layout post-fixer into the model. * - * The role of the table post-fixer is to ensure that the table rows have the correct structure + * The role of the table layout post-fixer is to ensure that the table rows have the correct structure * after a {@link module:engine/model/model~Model#change `change()`} block was executed. * * The correct structure means that: @@ -212,36 +212,17 @@ import TableWalker from './../tablewalker'; * * * - * - * ## Ensuring proper table structure - * - * A table cells must contains at least one block as a child. The empty table cell will have empty `` as a child. - * - * - * - * - * - *
- * - * Will be fixed to: - * - * - * - * - * - *
- * * @param {module:engine/model/model~Model} model */ -export default function injectTablePostFixer( model ) { - model.document.registerPostFixer( writer => tablePostFixer( writer, model ) ); +export default function injectTableLayoutPostFixer( model ) { + model.document.registerPostFixer( writer => tableLayoutPostFixer( writer, model ) ); } -// The table post-fixer. +// The table layout post-fixer. // // @param {module:engine/model/writer~Writer} writer // @param {module:engine/model/model~Model} model -function tablePostFixer( writer, model ) { +function tableLayoutPostFixer( writer, model ) { const changes = model.document.differ.getChanges(); let wasFixed = false; @@ -252,13 +233,6 @@ function tablePostFixer( writer, model ) { for ( const entry of changes ) { let table; - // Enforce paragraph in tableCell even after other feature remove its contents. - if ( entry.type == 'remove' && entry.position.parent.is( 'tableCell' ) && entry.position.parent.childCount == 0 ) { - writer.insertElement( 'paragraph', entry.position.parent ); - wasFixed = true; - } - - // Fix table on table insert. if ( entry.name == 'table' && entry.type == 'insert' ) { table = entry.position.nodeAfter; } diff --git a/src/tableediting.js b/src/tableediting.js index df0329e1..c2fbc6ad 100644 --- a/src/tableediting.js +++ b/src/tableediting.js @@ -33,7 +33,8 @@ import SetHeaderColumnCommand from './commands/setheadercolumncommand'; import { findAncestor } from './commands/utils'; import TableUtils from '../src/tableutils'; -import injectTablePostFixer from './converters/table-post-fixer'; +import injectTableLayoutPostFixer from './converters/table-layout-post-fixer'; +import injectTableCellContentsPostFixer from './converters/table-cell-content-post-fixer'; import injectTableCellPostFixer from './converters/tablecell-post-fixer'; import '../theme/tableediting.css'; @@ -145,7 +146,8 @@ export default class TableEditing extends Plugin { editor.commands.add( 'setTableColumnHeader', new SetHeaderColumnCommand( editor ) ); editor.commands.add( 'setTableRowHeader', new SetHeaderRowCommand( editor ) ); - injectTablePostFixer( model ); + injectTableLayoutPostFixer( model ); + injectTableCellContentsPostFixer( model ); // Handle tab key navigation. this.editor.keystrokes.set( 'Tab', ( ...args ) => this._handleTabOnSelectedTable( ...args ), { priority: 'low' } ); diff --git a/tests/converters/table-cell-content-post-fixer.js b/tests/converters/table-cell-content-post-fixer.js new file mode 100644 index 00000000..cb2dab25 --- /dev/null +++ b/tests/converters/table-cell-content-post-fixer.js @@ -0,0 +1,126 @@ +/** + * @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md. + */ + +import Range from '@ckeditor/ckeditor5-engine/src/model/range'; +import Position from '@ckeditor/ckeditor5-engine/src/model/position'; +import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; +import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor'; +import { getData as getModelData, setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; + +import TableEditing from '../../src/tableediting'; +import { formatTable } from './../_utils/utils'; +import UndoEditing from '@ckeditor/ckeditor5-undo/src/undoediting'; + +describe( 'Table cell content post-fixer', () => { + let editor, model, root; + + beforeEach( () => { + return VirtualTestEditor + .create( { + plugins: [ TableEditing, Paragraph, UndoEditing ] + } ) + .then( newEditor => { + editor = newEditor; + model = editor.model; + root = model.document.getRoot(); + } ); + } ); + + afterEach( () => { + editor.destroy(); + } ); + + it( 'should add a paragraph to an empty table cell (on table insert)', () => { + setModelData( model, + '' + + '' + + '' + + '' + + '
' + ); + + expect( formatTable( getModelData( model, { withoutSelection: true } ) ) ).to.equal( formatTable( + '' + + '' + + '' + + '' + + '
' + ) ); + } ); + + it( 'should add a paragraph to an empty table cell (on row insert)', () => { + setModelData( model, + '' + + '' + + '' + + '' + + '
' + ); + + // Insert table row with one table cell + model.change( writer => { + writer.insertElement( 'tableRow', Position.createAfter( root.getNodeByPath( [ 0, 0 ] ) ) ); + writer.insertElement( 'tableCell', Position.createAt( root.getNodeByPath( [ 0, 1 ] ) ) ); + } ); + + expect( formatTable( getModelData( model, { withoutSelection: true } ) ) ).to.equal( formatTable( + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '
' + ) ); + } ); + + it( 'should add a paragraph to an empty table cell (on table cell insert)', () => { + setModelData( model, + '' + + '' + + '' + + '' + + '
' + ); + + // Insert table row with one table cell + model.change( writer => { + writer.insertElement( 'tableCell', Position.createAt( root.getNodeByPath( [ 0, 0 ], 'end' ) ) ); + } ); + + expect( formatTable( getModelData( model, { withoutSelection: true } ) ) ).to.equal( formatTable( + '' + + '' + + '' + + '' + + '' + + '
' + ) ); + } ); + + it( 'should add a paragraph to an empty table cell (after remove)', () => { + setModelData( model, + '' + + '' + + 'foo' + + '' + + '
' + ); + + // Remove paragraph from table cell. + model.change( writer => { + writer.remove( Range.createIn( root.getNodeByPath( [ 0, 0, 0 ] ) ) ); + } ); + + expect( formatTable( getModelData( model, { withoutSelection: true } ) ) ).to.equal( formatTable( + '' + + '' + + '' + + '' + + '
' + ) ); + } ); +} ); diff --git a/tests/converters/table-post-fixer.js b/tests/converters/table-layout-post-fixer.js similarity index 96% rename from tests/converters/table-post-fixer.js rename to tests/converters/table-layout-post-fixer.js index 1d5775eb..dee4fe9f 100644 --- a/tests/converters/table-post-fixer.js +++ b/tests/converters/table-layout-post-fixer.js @@ -12,7 +12,7 @@ import TableEditing from '../../src/tableediting'; import { formatTable, formattedModelTable, modelTable } from './../_utils/utils'; import UndoEditing from '@ckeditor/ckeditor5-undo/src/undoediting'; -describe( 'Table post-fixer', () => { +describe( 'Table layout post-fixer', () => { let editor, model, root; beforeEach( () => { @@ -181,19 +181,6 @@ describe( 'Table post-fixer', () => { } ); } ); - describe( 'on removing tableCell contents', () => { - it( 'should fix empty table cells', () => { - setModelData( model, modelTable( [ [ '11[]' ] ] ) ); - - model.change( writer => { - // Remove paragraph from table cell. - writer.remove( Range.createIn( root.getNodeByPath( [ 0, 0, 0 ] ) ) ); - } ); - - expect( formatTable( getModelData( model, { withoutSelection: true } ) ) ).to.equal( formattedModelTable( [ [ '' ] ] ) ); - } ); - } ); - describe( 'on collaboration', () => { it( 'should add missing cells to columns (remove column vs insert row)', () => { _testExternal(