From 737c166b53ea31747642e998b5ec86179636d951 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 8 Apr 2020 10:19:48 +0200 Subject: [PATCH 01/11] Duplicate beforeEach call to make test independent. See ckeditor/ckeditor5#6574. --- tests/converters/downcast.js | 125 ++++++++++++++++++++++++++++++----- 1 file changed, 108 insertions(+), 17 deletions(-) diff --git a/tests/converters/downcast.js b/tests/converters/downcast.js index e40ded71..5904017d 100644 --- a/tests/converters/downcast.js +++ b/tests/converters/downcast.js @@ -42,24 +42,25 @@ describe( 'downcast converters', () => { testUtils.createSinonSandbox(); - beforeEach( () => { - // Most tests assume non-edge environment but we do not set `contenteditable=false` on Edge so stub `env.isEdge`. - testUtils.sinon.stub( env, 'isEdge' ).get( () => false ); - - return VirtualTestEditor.create() - .then( newEditor => { - editor = newEditor; - model = editor.model; - doc = model.document; - root = doc.getRoot( 'main' ); - view = editor.editing.view; - - defaultSchema( model.schema ); - defaultConversion( editor.conversion ); - } ); - } ); - describe( 'downcastInsertTable()', () => { + // The beforeEach is duplicated due to ckeditor/ckeditor5#6574. New test are written using TableEdting. + beforeEach( () => { + // Most tests assume non-edge environment but we do not set `contenteditable=false` on Edge so stub `env.isEdge`. + testUtils.sinon.stub( env, 'isEdge' ).get( () => false ); + + return VirtualTestEditor.create() + .then( newEditor => { + editor = newEditor; + model = editor.model; + doc = model.document; + root = doc.getRoot( 'main' ); + view = editor.editing.view; + + defaultSchema( model.schema ); + defaultConversion( editor.conversion ); + } ); + } ); + it( 'should create table with tbody', () => { setModelData( model, modelTable( [ [ '' ] ] ) ); @@ -355,6 +356,24 @@ describe( 'downcast converters', () => { } ); describe( 'downcastInsertRow()', () => { + // The beforeEach is duplicated due to ckeditor/ckeditor5#6574. New test are written using TableEdting. + beforeEach( () => { + // Most tests assume non-edge environment but we do not set `contenteditable=false` on Edge so stub `env.isEdge`. + testUtils.sinon.stub( env, 'isEdge' ).get( () => false ); + + return VirtualTestEditor.create() + .then( newEditor => { + editor = newEditor; + model = editor.model; + doc = model.document; + root = doc.getRoot( 'main' ); + view = editor.editing.view; + + defaultSchema( model.schema ); + defaultConversion( editor.conversion ); + } ); + } ); + it( 'should react to changed rows', () => { setModelData( model, modelTable( [ [ '00', '01' ] @@ -592,6 +611,24 @@ describe( 'downcast converters', () => { } ); describe( 'downcastInsertCell()', () => { + // The beforeEach is duplicated due to ckeditor/ckeditor5#6574. New test are written using TableEdting. + beforeEach( () => { + // Most tests assume non-edge environment but we do not set `contenteditable=false` on Edge so stub `env.isEdge`. + testUtils.sinon.stub( env, 'isEdge' ).get( () => false ); + + return VirtualTestEditor.create() + .then( newEditor => { + editor = newEditor; + model = editor.model; + doc = model.document; + root = doc.getRoot( 'main' ); + view = editor.editing.view; + + defaultSchema( model.schema ); + defaultConversion( editor.conversion ); + } ); + } ); + it( 'should add tableCell on proper index in tr', () => { setModelData( model, modelTable( [ [ '00', '01' ] @@ -736,6 +773,24 @@ describe( 'downcast converters', () => { } ); describe( 'downcastTableHeadingColumnsChange()', () => { + // The beforeEach is duplicated due to ckeditor/ckeditor5#6574. New test are written using TableEdting. + beforeEach( () => { + // Most tests assume non-edge environment but we do not set `contenteditable=false` on Edge so stub `env.isEdge`. + testUtils.sinon.stub( env, 'isEdge' ).get( () => false ); + + return VirtualTestEditor.create() + .then( newEditor => { + editor = newEditor; + model = editor.model; + doc = model.document; + root = doc.getRoot( 'main' ); + view = editor.editing.view; + + defaultSchema( model.schema ); + defaultConversion( editor.conversion ); + } ); + } ); + it( 'should work for adding heading columns', () => { setModelData( model, modelTable( [ [ '00', '01' ], @@ -912,6 +967,24 @@ describe( 'downcast converters', () => { } ); describe( 'downcastTableHeadingRowsChange()', () => { + // The beforeEach is duplicated due to ckeditor/ckeditor5#6574. New test are written using TableEdting. + beforeEach( () => { + // Most tests assume non-edge environment but we do not set `contenteditable=false` on Edge so stub `env.isEdge`. + testUtils.sinon.stub( env, 'isEdge' ).get( () => false ); + + return VirtualTestEditor.create() + .then( newEditor => { + editor = newEditor; + model = editor.model; + doc = model.document; + root = doc.getRoot( 'main' ); + view = editor.editing.view; + + defaultSchema( model.schema ); + defaultConversion( editor.conversion ); + } ); + } ); + it( 'should work for adding heading rows', () => { setModelData( model, modelTable( [ [ '00', '01' ], @@ -1126,6 +1199,24 @@ describe( 'downcast converters', () => { } ); describe( 'downcastRemoveRow()', () => { + // The beforeEach is duplicated due to ckeditor/ckeditor5#6574. New test are written using TableEdting. + beforeEach( () => { + // Most tests assume non-edge environment but we do not set `contenteditable=false` on Edge so stub `env.isEdge`. + testUtils.sinon.stub( env, 'isEdge' ).get( () => false ); + + return VirtualTestEditor.create() + .then( newEditor => { + editor = newEditor; + model = editor.model; + doc = model.document; + root = doc.getRoot( 'main' ); + view = editor.editing.view; + + defaultSchema( model.schema ); + defaultConversion( editor.conversion ); + } ); + } ); + it( 'should react to removed row from the beginning of a tbody', () => { setModelData( model, modelTable( [ [ '00[]', '01' ], From 4c47e32718bf4570d5cbd91ccc3d67dbcb371c71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 8 Apr 2020 10:43:41 +0200 Subject: [PATCH 02/11] Use full view markup in downcast remove row conversion assertions. Using full markup in tests without helper functions acts as documentation for this part of table conversion. --- tests/converters/downcast.js | 257 +++++++++++++++++++++++------------ 1 file changed, 170 insertions(+), 87 deletions(-) diff --git a/tests/converters/downcast.js b/tests/converters/downcast.js index 5904017d..37084bc0 100644 --- a/tests/converters/downcast.js +++ b/tests/converters/downcast.js @@ -4,13 +4,15 @@ */ import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor'; -import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view'; -import { setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; +import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; -import { defaultConversion, defaultSchema, modelTable, viewTable } from '../_utils/utils'; +import TableEditing from '../../src/tableediting'; import env from '@ckeditor/ckeditor5-utils/src/env'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; +import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view'; +import { setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; import { assertEqualMarkup } from '@ckeditor/ckeditor5-utils/tests/_utils/utils'; +import { defaultConversion, defaultSchema, modelTable, viewTable } from '../_utils/utils'; function paragraphInTableCell() { return dispatcher => dispatcher.on( 'insert:paragraph', ( evt, data, conversionApi ) => { @@ -1200,124 +1202,205 @@ describe( 'downcast converters', () => { describe( 'downcastRemoveRow()', () => { // The beforeEach is duplicated due to ckeditor/ckeditor5#6574. New test are written using TableEdting. - beforeEach( () => { + beforeEach( async () => { // Most tests assume non-edge environment but we do not set `contenteditable=false` on Edge so stub `env.isEdge`. testUtils.sinon.stub( env, 'isEdge' ).get( () => false ); - return VirtualTestEditor.create() - .then( newEditor => { - editor = newEditor; - model = editor.model; - doc = model.document; - root = doc.getRoot( 'main' ); - view = editor.editing.view; + editor = await VirtualTestEditor.create( { plugins: [ Paragraph, TableEditing ] } ); - defaultSchema( model.schema ); - defaultConversion( editor.conversion ); - } ); + model = editor.model; + root = model.document.getRoot( 'main' ); + view = editor.editing.view; } ); - it( 'should react to removed row from the beginning of a tbody', () => { - setModelData( model, modelTable( [ - [ '00[]', '01' ], - [ '10', '11' ] - ] ) ); + // The remove row downcast conversion is not executed in data pipeline. + describe( 'editing pipeline', () => { + it( 'should react to removed row from the beginning of a body rows (no heading rows)', () => { + setModelData( model, modelTable( [ + [ '00[]', '01' ], + [ '10', '11' ] + ] ) ); - const table = root.getChild( 0 ); + const table = root.getChild( 0 ); - model.change( writer => { - writer.remove( table.getChild( 1 ) ); + model.change( writer => { + writer.remove( table.getChild( 1 ) ); + } ); + + assertEqualMarkup( getViewData( view, { withoutSelection: true } ), + '
' + + '
' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '
' + + '00' + + '' + + '01' + + '
' + + '
' + ); } ); - assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [ - [ '00', '01' ] - ] ) ); - } ); + it( 'should react to removed row form the end of a body rows (no heading rows)', () => { + setModelData( model, modelTable( [ + [ '00[]', '01' ], + [ '10', '11' ] + ] ) ); - it( 'should react to removed row form the end of a tbody', () => { - setModelData( model, modelTable( [ - [ '00[]', '01' ], - [ '10', '11' ] - ] ) ); + const table = root.getChild( 0 ); - const table = root.getChild( 0 ); + model.change( writer => { + writer.remove( table.getChild( 0 ) ); + } ); - model.change( writer => { - writer.remove( table.getChild( 0 ) ); + assertEqualMarkup( getViewData( view, { withoutSelection: true } ), + '
' + + '
' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '
' + + '10' + + '' + + '11' + + '
' + + '
' + ); } ); - assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [ - [ '10', '11' ] - ] ) ); - } ); + it( 'should react to removed row from the beginning of a heading rows (no body rows)', () => { + setModelData( model, modelTable( [ + [ '00[]', '01' ], + [ '10', '11' ] + ], { headingRows: 2 } ) ); - it( 'should react to removed row from the beginning of a thead', () => { - setModelData( model, modelTable( [ - [ '00[]', '01' ], - [ '10', '11' ] - ], { headingRows: 2 } ) ); + const table = root.getChild( 0 ); - const table = root.getChild( 0 ); + model.change( writer => { + writer.remove( table.getChild( 0 ) ); + } ); - model.change( writer => { - writer.remove( table.getChild( 1 ) ); + assertEqualMarkup( getViewData( view, { withoutSelection: true } ), + '
' + + '
' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '
' + + '10' + + '' + + '11' + + '
' + + '
' + ); } ); - assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [ - [ '00', '01' ] - ], { headingRows: 2 } ) ); - } ); + it( 'should react to removed row form the end of a heading rows (no body rows)', () => { + setModelData( model, modelTable( [ + [ '00[]', '01' ], + [ '10', '11' ] + ], { headingRows: 2 } ) ); - it( 'should react to removed row form the end of a thead', () => { - setModelData( model, modelTable( [ - [ '00[]', '01' ], - [ '10', '11' ] - ], { headingRows: 2 } ) ); + const table = root.getChild( 0 ); - const table = root.getChild( 0 ); + model.change( writer => { + writer.remove( table.getChild( 1 ) ); + } ); - model.change( writer => { - writer.remove( table.getChild( 0 ) ); + assertEqualMarkup( getViewData( view, { withoutSelection: true } ), + '
' + + '
' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '
' + + '00' + + '' + + '01' + + '
' + + '
' + ); } ); - assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [ - [ '10', '11' ] - ], { headingRows: 2 } ) ); - } ); + it( 'should remove empty thead if a last row was removed from a heading rows (has heading and body)', () => { + setModelData( model, modelTable( [ + [ '00[]', '01' ], + [ '10', '11' ] + ], { headingRows: 1 } ) ); - it( 'should remove empty thead section if a last row was removed from thead', () => { - setModelData( model, modelTable( [ - [ '00[]', '01' ], - [ '10', '11' ] - ], { headingRows: 1 } ) ); + const table = root.getChild( 0 ); - const table = root.getChild( 0 ); + model.change( writer => { + writer.setAttribute( 'headingRows', 0, table ); + writer.remove( table.getChild( 0 ) ); + } ); - model.change( writer => { - writer.setAttribute( 'headingRows', 0, table ); - writer.remove( table.getChild( 0 ) ); + assertEqualMarkup( getViewData( view, { withoutSelection: true } ), + '
' + + '
' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '
' + + '10' + + '' + + '11' + + '
' + + '
' + ); } ); - assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [ - [ '10', '11' ] - ] ) ); - } ); + it( 'should remove empty tbody if a last row was removed a body rows (has heading and body)', () => { + setModelData( model, modelTable( [ + [ '00[]', '01' ], + [ '10', '11' ] + ], { headingRows: 1 } ) ); - it( 'should remove empty tbody section if a last row was removed from tbody', () => { - setModelData( model, modelTable( [ - [ '00[]', '01' ], - [ '10', '11' ] - ], { headingRows: 1 } ) ); + const table = root.getChild( 0 ); - const table = root.getChild( 0 ); + model.change( writer => { + writer.remove( table.getChild( 1 ) ); + } ); - model.change( writer => { - writer.remove( table.getChild( 1 ) ); + assertEqualMarkup( getViewData( view, { withoutSelection: true } ), + '
' + + '
' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '
' + + '00' + + '' + + '01' + + '
' + + '
' + ); } ); - - assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [ - [ '00', '01' ] - ], { headingRows: 1 } ) ); } ); } ); From df80eec873d72d22458a57d2e7d40c0ffb7ee8ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 8 Apr 2020 10:49:13 +0200 Subject: [PATCH 03/11] Fix code used to remove rows from heading section of a table. This change reflects how the code is used by commands. --- tests/converters/downcast.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/converters/downcast.js b/tests/converters/downcast.js index 37084bc0..c5c70425 100644 --- a/tests/converters/downcast.js +++ b/tests/converters/downcast.js @@ -1286,6 +1286,8 @@ describe( 'downcast converters', () => { const table = root.getChild( 0 ); model.change( writer => { + // Removing row from a heading section changes requires changing heading rows attribute. + writer.setAttribute( 'headingRows', 1, table ); writer.remove( table.getChild( 0 ) ); } ); @@ -1317,6 +1319,8 @@ describe( 'downcast converters', () => { const table = root.getChild( 0 ); model.change( writer => { + // Removing row from a heading section changes requires changing heading rows attribute. + writer.setAttribute( 'headingRows', 1, table ); writer.remove( table.getChild( 1 ) ); } ); @@ -1348,7 +1352,8 @@ describe( 'downcast converters', () => { const table = root.getChild( 0 ); model.change( writer => { - writer.setAttribute( 'headingRows', 0, table ); + // Removing row from a heading section changes requires changing heading rows attribute. + writer.removeAttribute( 'headingRows', table ); writer.remove( table.getChild( 0 ) ); } ); From 242a2e0b367da00e2f85929829e022f3c8c1318d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 8 Apr 2020 10:58:35 +0200 Subject: [PATCH 04/11] Removing row from a heading section while changing heading rows attribute will now properly clean empty table sections. --- src/converters/downcast.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/converters/downcast.js b/src/converters/downcast.js index a32f8998..79daf561 100644 --- a/src/converters/downcast.js +++ b/src/converters/downcast.js @@ -298,6 +298,7 @@ export function downcastRemoveRow() { const viewStart = mapper.toViewPosition( data.position ).getLastMatchingPosition( value => !value.item.is( 'tr' ) ); const viewItem = viewStart.nodeAfter; const tableSection = viewItem.parent; + const viewTable = tableSection.parent; // Remove associated from the view. const removeRange = viewWriter.createRangeOn( viewItem ); @@ -307,11 +308,10 @@ export function downcastRemoveRow() { mapper.unbindViewElement( child ); } - // Check if table section has any children left - if not remove it from the view. - if ( !tableSection.childCount ) { - // No need to unbind anything as table section is not represented in the model. - viewWriter.remove( viewWriter.createRangeOn( tableSection ) ); - } + // Cleanup: Ensure that thead & tbody sections are removed if left empty. See ckeditor/ckeditor5#6437. + // This happens if removing a row is associated with changing table's headingRows attribute (removing row from a heading section). + removeTableSectionIfEmpty( 'thead', viewTable, conversionApi ); + removeTableSectionIfEmpty( 'tbody', viewTable, conversionApi ); }, { priority: 'higher' } ); } From 1ec631c5628faadd1761b631baba867b1d3184c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 15 Apr 2020 10:33:20 +0200 Subject: [PATCH 05/11] Use TableEditing in RemoveRowCommand tests. See ckeditor/ckeditor5#6574. --- tests/commands/removerowcommand.js | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/tests/commands/removerowcommand.js b/tests/commands/removerowcommand.js index 21ccb315..a27d4715 100644 --- a/tests/commands/removerowcommand.js +++ b/tests/commands/removerowcommand.js @@ -9,22 +9,19 @@ import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils import RemoveRowCommand from '../../src/commands/removerowcommand'; import TableSelection from '../../src/tableselection'; -import { defaultConversion, defaultSchema, modelTable, viewTable } from '../_utils/utils'; +import { modelTable, viewTable } from '../_utils/utils'; import { assertEqualMarkup } from '@ckeditor/ckeditor5-utils/tests/_utils/utils'; +import TableEditing from '../../src/tableediting'; +import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; describe( 'RemoveRowCommand', () => { let editor, model, command; - beforeEach( () => { - return VirtualTestEditor.create( { plugins: [ TableSelection ] } ) - .then( newEditor => { - editor = newEditor; - model = editor.model; - command = new RemoveRowCommand( editor ); + beforeEach( async () => { + editor = await VirtualTestEditor.create( { plugins: [ Paragraph, TableEditing, TableSelection ] } ); - defaultSchema( model.schema ); - defaultConversion( editor.conversion ); - } ); + model = editor.model; + command = new RemoveRowCommand( editor ); } ); afterEach( () => { @@ -263,11 +260,11 @@ describe( 'RemoveRowCommand', () => { [ '[]40', '41' ] ], { headingRows: 1 } ) ); - // The view should also be properly downcasted. + // The editing view should also be properly downcasted. assertEqualMarkup( getViewData( editor.editing.view, { withoutSelection: true } ), viewTable( [ [ '00', '01' ], [ '40', '41' ] - ], { headingRows: 1 } ) ); + ], { headingRows: 1, asWidget: true } ) ); } ); it( 'should support removing mixed heading and cell rows', () => { @@ -447,8 +444,8 @@ describe( 'RemoveRowCommand', () => { setData( model, modelTable( [ [ { rowspan: 4, contents: '00' }, { rowspan: 3, contents: '01' }, { rowspan: 2, contents: '02' }, '03', '04' ], [ { rowspan: 2, contents: '13' }, '14' ], - [ '22[]', '23', '24' ], - [ '30', '31', '32', '33', '34' ] + [ '22[]', '24' ], + [ '31', '32', '33', '34' ] ] ) ); command.execute(); @@ -456,7 +453,7 @@ describe( 'RemoveRowCommand', () => { assertEqualMarkup( getData( model ), modelTable( [ [ { rowspan: 3, contents: '00' }, { rowspan: 2, contents: '01' }, { rowspan: 2, contents: '02' }, '03', '04' ], [ '13', '14' ], - [ '30', '31', '[]32', '33', '34' ] + [ '31', '32', '[]33', '34' ] ] ) ); } ); From 39380f93d73e9fcc0db5e695e3ff39baeeb7e6b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 15 Apr 2020 10:36:10 +0200 Subject: [PATCH 06/11] Add empty tbody/thead section cleanup also when downcasting heading rows. --- src/converters/downcast.js | 13 +++++------- tests/converters/downcast.js | 38 ++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/src/converters/downcast.js b/src/converters/downcast.js index 79daf561..cbd9ad0a 100644 --- a/src/converters/downcast.js +++ b/src/converters/downcast.js @@ -209,9 +209,6 @@ export function downcastTableHeadingRowsChange( options = {} ) { renameViewTableCell( tableCell, 'th', conversionApi, asWidget ); } } - - // Cleanup: this will remove any empty section from the view which may happen when moving all rows from a table section. - removeTableSectionIfEmpty( 'tbody', viewTable, conversionApi ); } // The head section has shrunk so move rows from to . else { @@ -234,11 +231,12 @@ export function downcastTableHeadingRowsChange( options = {} ) { for ( const tableWalkerValue of tableWalker ) { renameViewTableCellIfRequired( tableWalkerValue, tableAttributes, conversionApi, asWidget ); } - - // Cleanup: this will remove any empty section from the view which may happen when moving all rows from a table section. - removeTableSectionIfEmpty( 'thead', viewTable, conversionApi ); } + // Cleanup: Ensure that thead & tbody sections are removed if left empty after moving rows. See #6437, #6391. + removeTableSectionIfEmpty( 'thead', viewTable, conversionApi ); + removeTableSectionIfEmpty( 'tbody', viewTable, conversionApi ); + function isBetween( index, lower, upper ) { return index > lower && index < upper; } @@ -308,8 +306,7 @@ export function downcastRemoveRow() { mapper.unbindViewElement( child ); } - // Cleanup: Ensure that thead & tbody sections are removed if left empty. See ckeditor/ckeditor5#6437. - // This happens if removing a row is associated with changing table's headingRows attribute (removing row from a heading section). + // Cleanup: Ensure that thead & tbody sections are removed if left empty after removing rows. See #6437, #6391. removeTableSectionIfEmpty( 'thead', viewTable, conversionApi ); removeTableSectionIfEmpty( 'tbody', viewTable, conversionApi ); }, { priority: 'higher' } ); diff --git a/tests/converters/downcast.js b/tests/converters/downcast.js index c5c70425..2f937c73 100644 --- a/tests/converters/downcast.js +++ b/tests/converters/downcast.js @@ -1310,6 +1310,7 @@ describe( 'downcast converters', () => { ); } ); + // Might be a misuse of API or other operation - must work either wey. See #6437, #6391. it( 'should react to removed row form the end of a heading rows (no body rows)', () => { setModelData( model, modelTable( [ [ '00[]', '01' ], @@ -1343,6 +1344,43 @@ describe( 'downcast converters', () => { ); } ); + // Might be a misuse of API or other operation - must work either wey. See #6437, #6391. + it( 'should react to removed row form the end of a heading rows (no body rows, using enqueueChange())', () => { + setModelData( model, modelTable( [ + [ '00[]', '01' ], + [ '10', '11' ] + ], { headingRows: 2 } ) ); + + const table = root.getChild( 0 ); + + model.change( writer => { + // Removing row from a heading section changes requires changing heading rows attribute. + writer.remove( table.getChild( 1 ) ); + + model.enqueueChange( writer => { + writer.setAttribute( 'headingRows', 1, table ); + } ); + } ); + + assertEqualMarkup( getViewData( view, { withoutSelection: true } ), + '
' + + '
' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '
' + + '00' + + '' + + '01' + + '
' + + '
' + ); + } ); + it( 'should remove empty thead if a last row was removed from a heading rows (has heading and body)', () => { setModelData( model, modelTable( [ [ '00[]', '01' ], From 53cc8f79a1983ae525fe492088d4d15b9e7b4185 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 15 Apr 2020 10:59:57 +0200 Subject: [PATCH 07/11] Use TableEditing in downcast heading rows change tests. See ckeditor/ckeditor5#6574. --- tests/converters/downcast.js | 317 +++++++++++++++++------------------ 1 file changed, 155 insertions(+), 162 deletions(-) diff --git a/tests/converters/downcast.js b/tests/converters/downcast.js index 2f937c73..cfb793f5 100644 --- a/tests/converters/downcast.js +++ b/tests/converters/downcast.js @@ -45,7 +45,7 @@ describe( 'downcast converters', () => { testUtils.createSinonSandbox(); describe( 'downcastInsertTable()', () => { - // The beforeEach is duplicated due to ckeditor/ckeditor5#6574. New test are written using TableEdting. + // The beforeEach is duplicated due to ckeditor/ckeditor5#6574. New test are written using TableEditing. beforeEach( () => { // Most tests assume non-edge environment but we do not set `contenteditable=false` on Edge so stub `env.isEdge`. testUtils.sinon.stub( env, 'isEdge' ).get( () => false ); @@ -358,7 +358,7 @@ describe( 'downcast converters', () => { } ); describe( 'downcastInsertRow()', () => { - // The beforeEach is duplicated due to ckeditor/ckeditor5#6574. New test are written using TableEdting. + // The beforeEach is duplicated due to ckeditor/ckeditor5#6574. New test are written using TableEditing. beforeEach( () => { // Most tests assume non-edge environment but we do not set `contenteditable=false` on Edge so stub `env.isEdge`. testUtils.sinon.stub( env, 'isEdge' ).get( () => false ); @@ -613,7 +613,7 @@ describe( 'downcast converters', () => { } ); describe( 'downcastInsertCell()', () => { - // The beforeEach is duplicated due to ckeditor/ckeditor5#6574. New test are written using TableEdting. + // The beforeEach is duplicated due to ckeditor/ckeditor5#6574. New test are written using TableEditing. beforeEach( () => { // Most tests assume non-edge environment but we do not set `contenteditable=false` on Edge so stub `env.isEdge`. testUtils.sinon.stub( env, 'isEdge' ).get( () => false ); @@ -775,7 +775,7 @@ describe( 'downcast converters', () => { } ); describe( 'downcastTableHeadingColumnsChange()', () => { - // The beforeEach is duplicated due to ckeditor/ckeditor5#6574. New test are written using TableEdting. + // The beforeEach is duplicated due to ckeditor/ckeditor5#6574. New test are written using TableEditing. beforeEach( () => { // Most tests assume non-edge environment but we do not set `contenteditable=false` on Edge so stub `env.isEdge`. testUtils.sinon.stub( env, 'isEdge' ).get( () => false ); @@ -953,15 +953,15 @@ describe( 'downcast converters', () => { assertEqualMarkup( getViewData( view, { withoutSelection: true } ), '
' + '
' + - '' + - '' + - '' + - '' + - '' + - '' + - '
' + - '00' + - '
' + + '' + + '' + + '' + + '' + + '' + + '' + + '
' + + '00' + + '
' + '
' ); } ); @@ -969,208 +969,201 @@ describe( 'downcast converters', () => { } ); describe( 'downcastTableHeadingRowsChange()', () => { - // The beforeEach is duplicated due to ckeditor/ckeditor5#6574. New test are written using TableEdting. + // The beforeEach is duplicated due to ckeditor/ckeditor5#6574. New test are written using TableEditing. beforeEach( () => { // Most tests assume non-edge environment but we do not set `contenteditable=false` on Edge so stub `env.isEdge`. testUtils.sinon.stub( env, 'isEdge' ).get( () => false ); - return VirtualTestEditor.create() + return VirtualTestEditor.create( { plugins: [ Paragraph, TableEditing ] } ) .then( newEditor => { editor = newEditor; model = editor.model; doc = model.document; root = doc.getRoot( 'main' ); view = editor.editing.view; - - defaultSchema( model.schema ); - defaultConversion( editor.conversion ); } ); } ); - it( 'should work for adding heading rows', () => { - setModelData( model, modelTable( [ - [ '00', '01' ], - [ '10', '11' ], - [ '20', '21' ] - ] ) ); + // The heading rows change downcast conversion is not executed in data pipeline. + describe( 'editing pipeline', () => { + it( 'should work for adding heading rows', () => { + setModelData( model, modelTable( [ + [ '00', '01' ], + [ '10', '11' ], + [ '20', '21' ] + ] ) ); - const table = root.getChild( 0 ); + const table = root.getChild( 0 ); - model.change( writer => { - writer.setAttribute( 'headingRows', 2, table ); + model.change( writer => { + writer.setAttribute( 'headingRows', 2, table ); + } ); + + assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [ + [ '00', '01' ], + [ '10', '11' ], + [ '20', '21' ] + ], { headingRows: 2, asWidget: true } ) ); } ); - assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [ - [ '00', '01' ], - [ '10', '11' ], - [ '20', '21' ] - ], { headingRows: 2 } ) ); - } ); + it( 'should work for changing number of heading rows to a bigger number', () => { + setModelData( model, modelTable( [ + [ '00', '01' ], + [ '10', '11' ], + [ '20', '21' ] + ], { headingRows: 1 } ) ); - it( 'should work for changing number of heading rows to a bigger number', () => { - setModelData( model, modelTable( [ - [ '00', '01' ], - [ '10', '11' ], - [ '20', '21' ] - ], { headingRows: 1 } ) ); + const table = root.getChild( 0 ); - const table = root.getChild( 0 ); + model.change( writer => { + writer.setAttribute( 'headingRows', 2, table ); + } ); - model.change( writer => { - writer.setAttribute( 'headingRows', 2, table ); + assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [ + [ '00', '01' ], + [ '10', '11' ], + [ '20', '21' ] + ], { headingRows: 2, asWidget: true } ) ); } ); - assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [ - [ '00', '01' ], - [ '10', '11' ], - [ '20', '21' ] - ], { headingRows: 2 } ) ); - } ); + it( 'should work for changing number of heading rows to a smaller number', () => { + setModelData( model, modelTable( [ + [ '00', '01' ], + [ '10', '11' ], + [ '20', '21' ], + [ '30', '31' ] + ], { headingRows: 3 } ) ); - it( 'should work for changing number of heading rows to a smaller number', () => { - setModelData( model, modelTable( [ - [ '00', '01' ], - [ '10', '11' ], - [ '20', '21' ], - [ '30', '31' ] - ], { headingRows: 3 } ) ); + const table = root.getChild( 0 ); - const table = root.getChild( 0 ); + model.change( writer => { + writer.setAttribute( 'headingRows', 2, table ); + } ); - model.change( writer => { - writer.setAttribute( 'headingRows', 2, table ); + assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [ + [ '00', '01' ], + [ '10', '11' ], + [ '20', '21' ], + [ '30', '31' ] + ], { headingRows: 2, asWidget: true } ) ); } ); - assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [ - [ '00', '01' ], - [ '10', '11' ], - [ '20', '21' ], - [ '30', '31' ] - ], { headingRows: 2 } ) ); - } ); + it( 'should work for removing heading rows', () => { + setModelData( model, modelTable( [ + [ '00', '01' ], + [ '10', '11' ] + ], { headingRows: 2 } ) ); - it( 'should work for removing heading rows', () => { - setModelData( model, modelTable( [ - [ '00', '01' ], - [ '10', '11' ] - ], { headingRows: 2 } ) ); + const table = root.getChild( 0 ); - const table = root.getChild( 0 ); + model.change( writer => { + writer.removeAttribute( 'headingRows', table ); + } ); - model.change( writer => { - writer.removeAttribute( 'headingRows', table ); + assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [ + [ '00', '01' ], + [ '10', '11' ] + ], { asWidget: true } ) ); } ); - assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [ - [ '00', '01' ], - [ '10', '11' ] - ] ) ); - } ); + it( 'should work for making heading rows without tbody', () => { + setModelData( model, modelTable( [ + [ '00', '01' ], + [ '10', '11' ] + ] ) ); - it( 'should work for making heading rows without tbody', () => { - setModelData( model, modelTable( [ - [ '00', '01' ], - [ '10', '11' ] - ] ) ); + const table = root.getChild( 0 ); - const table = root.getChild( 0 ); + model.change( writer => { + writer.setAttribute( 'headingRows', 2, table ); + } ); - model.change( writer => { - writer.setAttribute( 'headingRows', 2, table ); + assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [ + [ '00', '01' ], + [ '10', '11' ] + ], { headingRows: 2, asWidget: true } ) ); } ); - assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [ - [ '00', '01' ], - [ '10', '11' ] - ], { headingRows: 2 } ) ); - } ); + it( 'should be possible to overwrite', () => { + editor.conversion.attributeToAttribute( { + model: 'headingRows', + view: 'headingRows', + converterPriority: 'high' + } ); + setModelData( model, modelTable( [ [ '00' ] ] ) ); - it( 'should be possible to overwrite', () => { - editor.conversion.attributeToAttribute( { model: 'headingRows', view: 'headingRows', converterPriority: 'high' } ); - setModelData( model, modelTable( [ [ '00' ] ] ) ); + const table = root.getChild( 0 ); - const table = root.getChild( 0 ); + model.change( writer => { + writer.setAttribute( 'headingRows', 1, table ); + } ); - model.change( writer => { - writer.setAttribute( 'headingRows', 1, table ); + assertEqualMarkup( getViewData( view, { withoutSelection: true } ), + '
' + + '
' + + '' + + '' + + '' + + '' + + '' + + '' + + '
' + + '00' + + '
' + + '
' + ); } ); - assertEqualMarkup( getViewData( view, { withoutSelection: true } ), - '
' + - '' + - '' + - '' + - '' + - '
00
' + - '
' - ); - } ); + it( 'should work with adding table rows at the beginning of a table', () => { + setModelData( model, modelTable( [ + [ '00', '01' ], + [ '10', '11' ] + ], { headingRows: 1 } ) ); - it( 'should work with adding table rows at the beginning of a table', () => { - setModelData( model, modelTable( [ - [ '00', '01' ], - [ '10', '11' ] - ], { headingRows: 1 } ) ); + const table = root.getChild( 0 ); - const table = root.getChild( 0 ); + model.change( writer => { + writer.setAttribute( 'headingRows', 2, table ); - model.change( writer => { - writer.setAttribute( 'headingRows', 2, table ); + const tableRow = writer.createElement( 'tableRow' ); - const tableRow = writer.createElement( 'tableRow' ); + writer.insert( tableRow, table, 0 ); + writer.insertElement( 'tableCell', tableRow, 'end' ); + writer.insertElement( 'tableCell', tableRow, 'end' ); + } ); - writer.insert( tableRow, table, 0 ); - writer.insertElement( 'tableCell', tableRow, 'end' ); - writer.insertElement( 'tableCell', tableRow, 'end' ); + assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [ + [ '', '' ], + [ '00', '01' ], + [ '10', '11' ] + ], { headingRows: 2, asWidget: true } ) ); } ); - assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [ - [ '', '' ], - [ '00', '01' ], - [ '10', '11' ] - ], { headingRows: 2 } ) ); - } ); - - it( 'should work with adding a table row and expanding heading', () => { - setModelData( model, modelTable( [ - [ '00', '01' ], - [ '10', '11' ], - [ '20', '21' ] - ], { headingRows: 1 } ) ); - - const table = root.getChild( 0 ); - - model.change( writer => { - writer.setAttribute( 'headingRows', 2, table ); + it( 'should work with adding a table row and expanding heading', () => { + setModelData( model, modelTable( [ + [ '00', '01' ], + [ '10', '11' ], + [ '20', '21' ] + ], { headingRows: 1 } ) ); - const tableRow = writer.createElement( 'tableRow' ); + const table = root.getChild( 0 ); - writer.insert( tableRow, table, 1 ); - writer.insertElement( 'tableCell', tableRow, 'end' ); - writer.insertElement( 'tableCell', tableRow, 'end' ); - } ); + model.change( writer => { + writer.setAttribute( 'headingRows', 2, table ); - assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [ - [ '00', '01' ], - [ '', '' ], - [ '10', '11' ], - [ '20', '21' ] - ], { headingRows: 2 } ) ); - } ); + const tableRow = writer.createElement( 'tableRow' ); - describe( 'options.asWidget=true', () => { - beforeEach( () => { - return VirtualTestEditor.create() - .then( newEditor => { - editor = newEditor; - model = editor.model; - doc = model.document; - root = doc.getRoot( 'main' ); - view = editor.editing.view; + writer.insert( tableRow, table, 1 ); + writer.insertElement( 'tableCell', tableRow, 'end' ); + writer.insertElement( 'tableCell', tableRow, 'end' ); + } ); - defaultSchema( model.schema ); - defaultConversion( editor.conversion, true ); - } ); + assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [ + [ '00', '01' ], + [ '', '' ], + [ '10', '11' ], + [ '20', '21' ] + ], { headingRows: 2, asWidget: true } ) ); } ); it( 'should create renamed cell as a widget', () => { @@ -1201,7 +1194,7 @@ describe( 'downcast converters', () => { } ); describe( 'downcastRemoveRow()', () => { - // The beforeEach is duplicated due to ckeditor/ckeditor5#6574. New test are written using TableEdting. + // The beforeEach is duplicated due to ckeditor/ckeditor5#6574. New test are written using TableEditing. beforeEach( async () => { // Most tests assume non-edge environment but we do not set `contenteditable=false` on Edge so stub `env.isEdge`. testUtils.sinon.stub( env, 'isEdge' ).get( () => false ); From 343f3312d189ffd5062a9d34558ae8fb2c79a62f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 15 Apr 2020 11:02:29 +0200 Subject: [PATCH 08/11] Fix code style in tests. --- tests/converters/downcast.js | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/tests/converters/downcast.js b/tests/converters/downcast.js index cfb793f5..de7d9a6d 100644 --- a/tests/converters/downcast.js +++ b/tests/converters/downcast.js @@ -952,16 +952,16 @@ describe( 'downcast converters', () => { assertEqualMarkup( getViewData( view, { withoutSelection: true } ), '
' + - '
' + - '' + - '' + - '' + - '' + - '' + - '' + - '
' + - '00' + - '
' + + '
' + + '' + + '' + + '' + + '' + + '' + + '' + + '
' + + '00' + + '
' + '
' ); } ); From 8b934b3aa210f94dec80e040006b5b192ef24fde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Fri, 17 Apr 2020 12:42:40 +0200 Subject: [PATCH 09/11] Update remove row + change headings tests. --- tests/converters/downcast.js | 48 +++++++++++++++++++++--------------- tests/tableutils.js | 6 +++-- 2 files changed, 32 insertions(+), 22 deletions(-) diff --git a/tests/converters/downcast.js b/tests/converters/downcast.js index de7d9a6d..8160f4b0 100644 --- a/tests/converters/downcast.js +++ b/tests/converters/downcast.js @@ -1303,7 +1303,6 @@ describe( 'downcast converters', () => { ); } ); - // Might be a misuse of API or other operation - must work either wey. See #6437, #6391. it( 'should react to removed row form the end of a heading rows (no body rows)', () => { setModelData( model, modelTable( [ [ '00[]', '01' ], @@ -1337,38 +1336,47 @@ describe( 'downcast converters', () => { ); } ); - // Might be a misuse of API or other operation - must work either wey. See #6437, #6391. - it( 'should react to removed row form the end of a heading rows (no body rows, using enqueueChange())', () => { + it( 'should react to removed row form the end of a heading rows (first cell in body has colspan)', () => { setModelData( model, modelTable( [ - [ '00[]', '01' ], - [ '10', '11' ] - ], { headingRows: 2 } ) ); + [ '00[]', '01', '02', '03' ], + [ { rowspan: 2, colspan: 2, contents: '10' }, '12', '13' ], + [ '22', '23' ] + ], { headingRows: 1 } ) ); const table = root.getChild( 0 ); model.change( writer => { // Removing row from a heading section changes requires changing heading rows attribute. - writer.remove( table.getChild( 1 ) ); - - model.enqueueChange( writer => { - writer.setAttribute( 'headingRows', 1, table ); - } ); + writer.remove( table.getChild( 0 ) ); + writer.setAttribute( 'headingRows', 0, table ); } ); assertEqualMarkup( getViewData( view, { withoutSelection: true } ), '
' + - '
' + + '
' + '' + - '' + + '' + '' + - '' + - '' + + '' + + '' + + '' + '' + - '' + + '' + + '' + + '' + + '' + + '' + '
' + - '00' + - '' + - '01' + - '' + + '10' + + '' + + '12' + + '' + + '13' + + '
' + + '22' + + '' + + '23' + + '
' + '
' ); diff --git a/tests/tableutils.js b/tests/tableutils.js index ae1c4f56..641f946d 100644 --- a/tests/tableutils.js +++ b/tests/tableutils.js @@ -938,7 +938,7 @@ describe( 'TableUtils', () => { ] ) ); } ); - it( 'should create one undo step (1 batch)', () => { + it( 'should re-use batch to create one undo step', () => { setData( model, modelTable( [ [ '00', '01' ], [ '10', '11' ], @@ -954,7 +954,9 @@ describe( 'TableUtils', () => { createdBatches.add( operation.batch ); } ); - tableUtils.removeRows( root.getChild( 0 ), { at: 0, rows: 2 } ); + const batch = model.createBatch(); + + tableUtils.removeRows( root.getChild( 0 ), { at: 0, rows: 2, batch } ); expect( createdBatches.size ).to.equal( 1 ); } ); From 238279fe0e409ad813f945579a4a31a877a45b10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Fri, 17 Apr 2020 12:46:43 +0200 Subject: [PATCH 10/11] Add batch option to TableUtils.removeRows() method to re-use it during table modifications. --- src/tableutils.js | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/tableutils.js b/src/tableutils.js index a1e1b17b..74a14b18 100644 --- a/src/tableutils.js +++ b/src/tableutils.js @@ -287,22 +287,23 @@ export default class TableUtils extends Plugin { const rowsToRemove = options.rows || 1; const last = first + rowsToRemove - 1; + const batch = options.batch || 'default'; - model.change( writer => { - for ( let i = last; i >= first; i-- ) { - removeRow( table, i, writer ); - } + const headingRows = table.getAttribute( 'headingRows' ) || 0; - const headingRows = table.getAttribute( 'headingRows' ) || 0; + if ( headingRows && first < headingRows ) { + const newRows = getNewHeadingRowsValue( first, last, headingRows ); - if ( headingRows && first < headingRows ) { - const newRows = getNewHeadingRowsValue( first, last, headingRows ); + // Must be done after the changes in table structure (removing rows). + // Otherwise the downcast converter for headingRows attribute will fail. ckeditor/ckeditor5#6391. + model.enqueueChange( batch, writer => { + updateNumericAttribute( 'headingRows', newRows, table, writer, 0 ); + } ); + } - // Must be done after the changes in table structure (removing rows). - // Otherwise the downcast converter for headingRows attribute will fail. ckeditor/ckeditor5#6391. - model.enqueueChange( writer.batch, writer => { - updateNumericAttribute( 'headingRows', newRows, table, writer, 0 ); - } ); + model.enqueueChange( batch, writer => { + for ( let i = last; i >= first; i-- ) { + removeRow( table, i, writer ); } } ); } From 13b9ce854502d8e32e3e90148f1f6753cefa0a2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Fri, 17 Apr 2020 12:49:09 +0200 Subject: [PATCH 11/11] Use single batch to modify table. --- src/commands/removerowcommand.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/commands/removerowcommand.js b/src/commands/removerowcommand.js index 641d5b25..e4737791 100644 --- a/src/commands/removerowcommand.js +++ b/src/commands/removerowcommand.js @@ -60,7 +60,10 @@ export default class RemoveRowCommand extends Command { const columnIndexToFocus = this.editor.plugins.get( 'TableUtils' ).getCellLocation( firstCell ).column; - model.change( writer => { + // Use single batch to modify table in steps but in one undo step. + const batch = model.createBatch(); + + model.enqueueChange( batch, writer => { // This prevents the "model-selection-range-intersects" error, caused by removing row selected cells. writer.setSelection( writer.createSelection( table, 'on' ) ); @@ -68,9 +71,12 @@ export default class RemoveRowCommand extends Command { this.editor.plugins.get( 'TableUtils' ).removeRows( table, { at: removedRowIndexes.first, - rows: rowsToRemove + rows: rowsToRemove, + batch } ); + } ); + model.enqueueChange( batch, writer => { const cellToFocus = getCellToFocus( table, removedRowIndexes.first, columnIndexToFocus ); writer.setSelection( writer.createPositionAt( cellToFocus, 0 ) );