From e7f2806a19c6b7511ab7d5c64e73baf1028d3a91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 22 Aug 2019 11:16:08 +0200 Subject: [PATCH 01/15] Table cell post-fixer will refresh a cell only when it is needed. --- .../table-cell-refresh-post-fixer.js | 45 ++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/src/converters/table-cell-refresh-post-fixer.js b/src/converters/table-cell-refresh-post-fixer.js index 5f4bfbaf..9005eac3 100644 --- a/src/converters/table-cell-refresh-post-fixer.js +++ b/src/converters/table-cell-refresh-post-fixer.js @@ -30,7 +30,7 @@ function tableCellRefreshPostFixer( model ) { for ( const change of differ.getChanges() ) { const parent = change.type == 'insert' || change.type == 'remove' ? change.position.parent : change.range.start.parent; - if ( parent.is( 'tableCell' ) ) { + if ( parent.is( 'tableCell' ) && checkRefresh( parent, change.type ) ) { differ.refreshItem( parent ); fixed = true; @@ -39,3 +39,46 @@ function tableCellRefreshPostFixer( model ) { return fixed; } + +// Check if a table cell in the view requires refreshing. +// +// This methods detects changes that will require: +// - to

rename in the view, +// - adding a missing , +// - or wrapping a text node in +// +// thus requiring refreshing the table cell view. +// +// @param {module:engine/model/element~Element} tableCell Table cell to check. +// @param {String} type Type of change. +function checkRefresh( tableCell, type ) { + // If children of a table cell were removed - refresh it. + if ( !tableCell.childCount ) { + return true; + } + + const children = Array.from( tableCell.getChildren() ); + const hasInnerText = children.some( child => child.is( 'text' ) ); + + // If a bare text node (not wrapped in a paragraph) was added - refresh it. + if ( hasInnerText ) { + return true; + } + + const hasInnerParagraph = children.some( child => child.is( 'paragraph' ) ); + + // If there is no paragraph in table cell then the view doesn't require refreshing. + if ( !hasInnerParagraph ) { + return false; + } + + // For attribute change we only refresh single paragraphs as they might trigger to

change in the view. + if ( type == 'attribute' ) { + return tableCell.childCount === 1; + } + + // For other changes (insert/remove) the to

change can occur when: + // - sibling is added to a single paragraph (childCount == 2) + // - sibling is removed and single paragraph is left (childCount == 1) + return tableCell.childCount < 3; +} From fda2a6976c13abd957210105bccd318891b8d5fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 22 Aug 2019 11:55:46 +0200 Subject: [PATCH 02/15] Add tests for table cell paragraph post-fixer run count. --- .../table-cell-paragraph-post-fixer.js | 66 ++++++++++++++++++- tests/manual/tableblockcontent.js | 7 +- 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/tests/converters/table-cell-paragraph-post-fixer.js b/tests/converters/table-cell-paragraph-post-fixer.js index f4877865..afd8d05b 100644 --- a/tests/converters/table-cell-paragraph-post-fixer.js +++ b/tests/converters/table-cell-paragraph-post-fixer.js @@ -12,12 +12,20 @@ import { formatTable } from './../_utils/utils'; import UndoEditing from '@ckeditor/ckeditor5-undo/src/undoediting'; describe( 'Table cell paragraph post-fixer', () => { - let editor, model, root; + let editor, model, root, postFixerSpy; beforeEach( () => { return VirtualTestEditor .create( { - plugins: [ TableEditing, Paragraph, UndoEditing ] + plugins: [ function( editor ) { + editor.model.schema.register( 'block', { inheritAllFrom: '$block' } ); + + // Add a post-fixer spy before any other plugin so it will be always run if other post-fixer returns true. + // Use spy.resetHistory() before checking a case when post fixer should be run. + // It should be called once if no other post-fixer fixed the model or more then one if another post-fixer changed model. + postFixerSpy = sinon.spy(); + editor.model.document.registerPostFixer( postFixerSpy ); + }, TableEditing, Paragraph, UndoEditing ] } ) .then( newEditor => { editor = newEditor; @@ -221,4 +229,58 @@ describe( 'Table cell paragraph post-fixer', () => { '' ) ); } ); + + it( 'should not be run on changing attribute of other element in a table cell', () => { + setModelData( model, + '' + + '' + + '' + + '' + + '
' + ); + + postFixerSpy.resetHistory(); + + // Insert table row with one table cell + model.change( writer => { + writer.setAttribute( 'foo', 'bar', root.getNodeByPath( [ 0, 0, 0, 0 ] ) ); + } ); + + expect( formatTable( getModelData( model, { withoutSelection: true } ) ) ).to.equal( formatTable( + '' + + '' + + '' + + '' + + '
' + ) ); + + sinon.assert.calledOnce( postFixerSpy ); + } ); + + it( 'should be run on changing attribute of an paragraph in a table cell', () => { + setModelData( model, + '' + + '' + + '' + + '' + + '
' + ); + + postFixerSpy.resetHistory(); + + // Insert table row with one table cell + model.change( writer => { + writer.setAttribute( 'foo', 'bar', root.getNodeByPath( [ 0, 0, 0, 0 ] ) ); + } ); + + expect( formatTable( getModelData( model, { withoutSelection: true } ) ) ).to.equal( formatTable( + '' + + '' + + '' + + '' + + '
' + ) ); + + sinon.assert.calledTwice( postFixerSpy ); + } ); } ); diff --git a/tests/manual/tableblockcontent.js b/tests/manual/tableblockcontent.js index eb03add8..27633fc9 100644 --- a/tests/manual/tableblockcontent.js +++ b/tests/manual/tableblockcontent.js @@ -8,13 +8,16 @@ import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classiceditor'; import ArticlePluginSet from '@ckeditor/ckeditor5-core/tests/_utils/articlepluginset'; import Alignment from '@ckeditor/ckeditor5-alignment/src/alignment'; +import EasyImage from '@ckeditor/ckeditor5-easy-image/src/easyimage'; +import { CS_CONFIG } from '@ckeditor/ckeditor5-cloud-services/tests/_utils/cloud-services-config'; ClassicEditor .create( document.querySelector( '#editor' ), { - plugins: [ ArticlePluginSet, Alignment ], + cloudServices: CS_CONFIG, + plugins: [ ArticlePluginSet, Alignment, EasyImage ], toolbar: [ 'heading', '|', 'insertTable', '|', 'bold', 'italic', 'bulletedList', 'numberedList', 'blockQuote', - 'alignment', '|', 'undo', 'redo' + 'alignment', '|', 'imageUpload', 'undo', 'redo' ], image: { toolbar: [ 'imageStyle:full', 'imageStyle:side' ] From f309bd1d6bfe556bc9e3a7a2314dcfc22dbf4eb8 Mon Sep 17 00:00:00 2001 From: Maciej Date: Thu, 22 Aug 2019 12:55:16 +0200 Subject: [PATCH 03/15] Update tests/converters/table-cell-paragraph-post-fixer.js Co-Authored-By: Szymon Cofalik --- tests/converters/table-cell-paragraph-post-fixer.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/converters/table-cell-paragraph-post-fixer.js b/tests/converters/table-cell-paragraph-post-fixer.js index afd8d05b..da1b28b1 100644 --- a/tests/converters/table-cell-paragraph-post-fixer.js +++ b/tests/converters/table-cell-paragraph-post-fixer.js @@ -257,7 +257,7 @@ describe( 'Table cell paragraph post-fixer', () => { sinon.assert.calledOnce( postFixerSpy ); } ); - it( 'should be run on changing attribute of an paragraph in a table cell', () => { + it( 'should be run on changing attribute of a paragraph in a table cell', () => { setModelData( model, '' + '' + From 377c985656594a7e1ec1940c357c25a41acfb523 Mon Sep 17 00:00:00 2001 From: Maciej Date: Thu, 22 Aug 2019 12:55:34 +0200 Subject: [PATCH 04/15] Update src/converters/table-cell-refresh-post-fixer.js Co-Authored-By: Szymon Cofalik --- src/converters/table-cell-refresh-post-fixer.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/converters/table-cell-refresh-post-fixer.js b/src/converters/table-cell-refresh-post-fixer.js index 9005eac3..2fa28f77 100644 --- a/src/converters/table-cell-refresh-post-fixer.js +++ b/src/converters/table-cell-refresh-post-fixer.js @@ -52,7 +52,7 @@ function tableCellRefreshPostFixer( model ) { // @param {module:engine/model/element~Element} tableCell Table cell to check. // @param {String} type Type of change. function checkRefresh( tableCell, type ) { - // If children of a table cell were removed - refresh it. + // If all children of a table cell were removed - refresh it. if ( !tableCell.childCount ) { return true; } From 8be62548f3f2d6374b2c964ea3dc7ac6c4f6a130 Mon Sep 17 00:00:00 2001 From: Maciej Date: Thu, 22 Aug 2019 12:55:58 +0200 Subject: [PATCH 05/15] Update src/converters/table-cell-refresh-post-fixer.js Co-Authored-By: Szymon Cofalik --- src/converters/table-cell-refresh-post-fixer.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/converters/table-cell-refresh-post-fixer.js b/src/converters/table-cell-refresh-post-fixer.js index 2fa28f77..1be20194 100644 --- a/src/converters/table-cell-refresh-post-fixer.js +++ b/src/converters/table-cell-refresh-post-fixer.js @@ -40,7 +40,7 @@ function tableCellRefreshPostFixer( model ) { return fixed; } -// Check if a table cell in the view requires refreshing. +// Check if the model table cell requires refreshing to be re-rendered to a proper state in the view. // // This methods detects changes that will require: // - to

rename in the view, From eafd8d8c7bf7bc17383b3bfaaccf5fde1227ab23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 22 Aug 2019 13:43:30 +0200 Subject: [PATCH 06/15] Rewrite tests to use spy on differ.refreshItem(). --- .../table-cell-paragraph-post-fixer.js | 109 +++++++++++++----- 1 file changed, 83 insertions(+), 26 deletions(-) diff --git a/tests/converters/table-cell-paragraph-post-fixer.js b/tests/converters/table-cell-paragraph-post-fixer.js index da1b28b1..ad01d49d 100644 --- a/tests/converters/table-cell-paragraph-post-fixer.js +++ b/tests/converters/table-cell-paragraph-post-fixer.js @@ -11,26 +11,21 @@ import TableEditing from '../../src/tableediting'; import { formatTable } from './../_utils/utils'; import UndoEditing from '@ckeditor/ckeditor5-undo/src/undoediting'; -describe( 'Table cell paragraph post-fixer', () => { - let editor, model, root, postFixerSpy; +describe.only( 'Table cell paragraph post-fixer', () => { + let editor, model, root, refreshItemSpy; beforeEach( () => { return VirtualTestEditor .create( { - plugins: [ function( editor ) { - editor.model.schema.register( 'block', { inheritAllFrom: '$block' } ); - - // Add a post-fixer spy before any other plugin so it will be always run if other post-fixer returns true. - // Use spy.resetHistory() before checking a case when post fixer should be run. - // It should be called once if no other post-fixer fixed the model or more then one if another post-fixer changed model. - postFixerSpy = sinon.spy(); - editor.model.document.registerPostFixer( postFixerSpy ); - }, TableEditing, Paragraph, UndoEditing ] + plugins: [ TableEditing, Paragraph, UndoEditing ] } ) .then( newEditor => { editor = newEditor; model = editor.model; root = model.document.getRoot(); + + editor.model.schema.register( 'block', { inheritAllFrom: '$block' } ); + refreshItemSpy = sinon.spy( model.document.differ, 'refreshItem' ); } ); } ); @@ -38,7 +33,7 @@ describe( 'Table cell paragraph post-fixer', () => { editor.destroy(); } ); - it( 'should add a paragraph to an empty table cell (on table insert)', () => { + it( 'should not be called on an empty table cell (on table insert)', () => { setModelData( model, '

' + '' + @@ -54,9 +49,11 @@ describe( 'Table cell paragraph post-fixer', () => { '' + '
' ) ); + + sinon.assert.notCalled( refreshItemSpy ); } ); - it( 'should add a paragraph to an empty table cell (on row insert)', () => { + it( 'should not be called on an empty table cell (on row insert)', () => { setModelData( model, '' + '' + @@ -81,9 +78,11 @@ describe( 'Table cell paragraph post-fixer', () => { '' + '
' ) ); + + sinon.assert.notCalled( refreshItemSpy ); } ); - it( 'should add a paragraph to an empty table cell (on table cell insert)', () => { + it( 'should not be called on an empty table cell (on table cell insert)', () => { setModelData( model, '' + '' + @@ -105,9 +104,11 @@ describe( 'Table cell paragraph post-fixer', () => { '' + '
' ) ); + + sinon.assert.notCalled( refreshItemSpy ); } ); - it( 'should add a paragraph to an empty table cell (after remove)', () => { + it( 'should be called on inserting paragraph to an empty table cell (after remove)', () => { setModelData( model, '' + '' + @@ -128,9 +129,12 @@ describe( 'Table cell paragraph post-fixer', () => { '' + '
' ) ); + + // TODO - it should not. + sinon.assert.calledOnce( refreshItemSpy ); } ); - it( 'should wrap in paragraph $text nodes placed directly in tableCell (on table cell modification) ', () => { + it( 'should be called on when wrapping in paragraph $text nodes placed directly in tableCell (on table cell modification) ', () => { setModelData( model, '' + '' + @@ -162,9 +166,11 @@ describe( 'Table cell paragraph post-fixer', () => { '' + '
' ) ); + + sinon.assert.calledOnce( refreshItemSpy ); } ); - it( 'should wrap in paragraph $text nodes placed directly in tableCell (on inserting table cell)', () => { + it( 'should not be called on wrapping in paragraph $text nodes placed directly in tableCell (on inserting table cell)', () => { setModelData( model, '' + '' + @@ -193,9 +199,11 @@ describe( 'Table cell paragraph post-fixer', () => { '' + '
' ) ); + + sinon.assert.notCalled( refreshItemSpy ); } ); - it( 'should wrap in paragraph $text nodes placed directly in tableCell (on inserting table rows)', () => { + it( 'should not be called on wrapping in paragraph $text nodes placed directly in tableCell (on inserting table rows)', () => { setModelData( model, '' + '' + @@ -228,9 +236,11 @@ describe( 'Table cell paragraph post-fixer', () => { '' + '
' ) ); + + sinon.assert.notCalled( refreshItemSpy ); } ); - it( 'should not be run on changing attribute of other element in a table cell', () => { + it( 'should not be called on changing attribute of other element in a table cell', () => { setModelData( model, '' + '' + @@ -239,8 +249,6 @@ describe( 'Table cell paragraph post-fixer', () => { '
' ); - postFixerSpy.resetHistory(); - // Insert table row with one table cell model.change( writer => { writer.setAttribute( 'foo', 'bar', root.getNodeByPath( [ 0, 0, 0, 0 ] ) ); @@ -254,10 +262,10 @@ describe( 'Table cell paragraph post-fixer', () => { '' ) ); - sinon.assert.calledOnce( postFixerSpy ); + sinon.assert.notCalled( refreshItemSpy ); } ); - it( 'should be run on changing attribute of a paragraph in a table cell', () => { + it( 'should be called on changing attribute of a paragraph in a table cell', () => { setModelData( model, '' + '' + @@ -266,8 +274,6 @@ describe( 'Table cell paragraph post-fixer', () => { '
' ); - postFixerSpy.resetHistory(); - // Insert table row with one table cell model.change( writer => { writer.setAttribute( 'foo', 'bar', root.getNodeByPath( [ 0, 0, 0, 0 ] ) ); @@ -281,6 +287,57 @@ describe( 'Table cell paragraph post-fixer', () => { '' ) ); - sinon.assert.calledTwice( postFixerSpy ); + sinon.assert.calledOnce( refreshItemSpy ); + } ); + + it( 'should not be called on adding attribute of a paragraph with other attribute set in a table cell', () => { + setModelData( model, + '' + + '' + + '' + + '' + + '
' + ); + + // Insert table row with one table cell + model.change( writer => { + writer.setAttribute( 'foo', 'bar', root.getNodeByPath( [ 0, 0, 0, 0 ] ) ); + } ); + + expect( formatTable( getModelData( model, { withoutSelection: true } ) ) ).to.equal( formatTable( + '' + + '' + + '' + + '' + + '
' + ) ); + + sinon.assert.notCalled( refreshItemSpy ); + } ); + + it( 'should not be called on adding attribute and removing attribute of a paragraph with other attribute set in a table cell', () => { + setModelData( model, + '' + + '' + + '' + + '' + + '
' + ); + + // Insert table row with one table cell + model.change( writer => { + writer.setAttribute( 'foo', 'bar', root.getNodeByPath( [ 0, 0, 0, 0 ] ) ); + writer.removeAttribute( 'bar', root.getNodeByPath( [ 0, 0, 0, 0 ] ) ); + } ); + + expect( formatTable( getModelData( model, { withoutSelection: true } ) ) ).to.equal( formatTable( + '' + + '' + + '' + + '' + + '
' + ) ); + + sinon.assert.notCalled( refreshItemSpy ); } ); } ); From 058f422ededc82979a719d53c90e62eadf6756d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 22 Aug 2019 13:44:26 +0200 Subject: [PATCH 07/15] Table cell refresh post-fixer will refresh a cell once. --- src/converters/table-cell-refresh-post-fixer.js | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/converters/table-cell-refresh-post-fixer.js b/src/converters/table-cell-refresh-post-fixer.js index 1be20194..0b572837 100644 --- a/src/converters/table-cell-refresh-post-fixer.js +++ b/src/converters/table-cell-refresh-post-fixer.js @@ -25,19 +25,26 @@ export default function injectTableCellRefreshPostFixer( model ) { function tableCellRefreshPostFixer( model ) { const differ = model.document.differ; - let fixed = false; + // Stores cells to be refreshed so the table cell will be refreshed once for multiple changes. + const cellToRefresh = new Set(); for ( const change of differ.getChanges() ) { const parent = change.type == 'insert' || change.type == 'remove' ? change.position.parent : change.range.start.parent; if ( parent.is( 'tableCell' ) && checkRefresh( parent, change.type ) ) { - differ.refreshItem( parent ); + cellToRefresh.add( parent ); + } + } - fixed = true; + if ( cellToRefresh.size ) { + for ( const tableCell of cellToRefresh.values() ) { + differ.refreshItem( tableCell ); } + + return true; } - return fixed; + return false; } // Check if the model table cell requires refreshing to be re-rendered to a proper state in the view. @@ -74,7 +81,7 @@ function checkRefresh( tableCell, type ) { // For attribute change we only refresh single paragraphs as they might trigger to

change in the view. if ( type == 'attribute' ) { - return tableCell.childCount === 1; + return tableCell.childCount === 1 && Array.from( tableCell.getChild( 0 ).getAttributeKeys() ).length < 2; } // For other changes (insert/remove) the to

change can occur when: From b13214d610ce729013c27173c4f898bb9c4c64dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 22 Aug 2019 14:23:26 +0200 Subject: [PATCH 08/15] Revert a table cell paragraph post-fixer. --- .../table-cell-paragraph-post-fixer.js | 137 ++---------------- 1 file changed, 9 insertions(+), 128 deletions(-) diff --git a/tests/converters/table-cell-paragraph-post-fixer.js b/tests/converters/table-cell-paragraph-post-fixer.js index ad01d49d..f4877865 100644 --- a/tests/converters/table-cell-paragraph-post-fixer.js +++ b/tests/converters/table-cell-paragraph-post-fixer.js @@ -11,8 +11,8 @@ import TableEditing from '../../src/tableediting'; import { formatTable } from './../_utils/utils'; import UndoEditing from '@ckeditor/ckeditor5-undo/src/undoediting'; -describe.only( 'Table cell paragraph post-fixer', () => { - let editor, model, root, refreshItemSpy; +describe( 'Table cell paragraph post-fixer', () => { + let editor, model, root; beforeEach( () => { return VirtualTestEditor @@ -23,9 +23,6 @@ describe.only( 'Table cell paragraph post-fixer', () => { editor = newEditor; model = editor.model; root = model.document.getRoot(); - - editor.model.schema.register( 'block', { inheritAllFrom: '$block' } ); - refreshItemSpy = sinon.spy( model.document.differ, 'refreshItem' ); } ); } ); @@ -33,7 +30,7 @@ describe.only( 'Table cell paragraph post-fixer', () => { editor.destroy(); } ); - it( 'should not be called on an empty table cell (on table insert)', () => { + it( 'should add a paragraph to an empty table cell (on table insert)', () => { setModelData( model, '' + '' + @@ -49,11 +46,9 @@ describe.only( 'Table cell paragraph post-fixer', () => { '' + '
' ) ); - - sinon.assert.notCalled( refreshItemSpy ); } ); - it( 'should not be called on an empty table cell (on row insert)', () => { + it( 'should add a paragraph to an empty table cell (on row insert)', () => { setModelData( model, '' + '' + @@ -78,11 +73,9 @@ describe.only( 'Table cell paragraph post-fixer', () => { '' + '
' ) ); - - sinon.assert.notCalled( refreshItemSpy ); } ); - it( 'should not be called on an empty table cell (on table cell insert)', () => { + it( 'should add a paragraph to an empty table cell (on table cell insert)', () => { setModelData( model, '' + '' + @@ -104,11 +97,9 @@ describe.only( 'Table cell paragraph post-fixer', () => { '' + '
' ) ); - - sinon.assert.notCalled( refreshItemSpy ); } ); - it( 'should be called on inserting paragraph to an empty table cell (after remove)', () => { + it( 'should add a paragraph to an empty table cell (after remove)', () => { setModelData( model, '' + '' + @@ -129,12 +120,9 @@ describe.only( 'Table cell paragraph post-fixer', () => { '' + '
' ) ); - - // TODO - it should not. - sinon.assert.calledOnce( refreshItemSpy ); } ); - it( 'should be called on when wrapping in paragraph $text nodes placed directly in tableCell (on table cell modification) ', () => { + it( 'should wrap in paragraph $text nodes placed directly in tableCell (on table cell modification) ', () => { setModelData( model, '' + '' + @@ -166,11 +154,9 @@ describe.only( 'Table cell paragraph post-fixer', () => { '' + '
' ) ); - - sinon.assert.calledOnce( refreshItemSpy ); } ); - it( 'should not be called on wrapping in paragraph $text nodes placed directly in tableCell (on inserting table cell)', () => { + it( 'should wrap in paragraph $text nodes placed directly in tableCell (on inserting table cell)', () => { setModelData( model, '' + '' + @@ -199,11 +185,9 @@ describe.only( 'Table cell paragraph post-fixer', () => { '' + '
' ) ); - - sinon.assert.notCalled( refreshItemSpy ); } ); - it( 'should not be called on wrapping in paragraph $text nodes placed directly in tableCell (on inserting table rows)', () => { + it( 'should wrap in paragraph $text nodes placed directly in tableCell (on inserting table rows)', () => { setModelData( model, '' + '' + @@ -236,108 +220,5 @@ describe.only( 'Table cell paragraph post-fixer', () => { '' + '
' ) ); - - sinon.assert.notCalled( refreshItemSpy ); - } ); - - it( 'should not be called on changing attribute of other element in a table cell', () => { - setModelData( model, - '' + - '' + - '' + - '' + - '
' - ); - - // Insert table row with one table cell - model.change( writer => { - writer.setAttribute( 'foo', 'bar', root.getNodeByPath( [ 0, 0, 0, 0 ] ) ); - } ); - - expect( formatTable( getModelData( model, { withoutSelection: true } ) ) ).to.equal( formatTable( - '' + - '' + - '' + - '' + - '
' - ) ); - - sinon.assert.notCalled( refreshItemSpy ); - } ); - - it( 'should be called on changing attribute of a paragraph in a table cell', () => { - setModelData( model, - '' + - '' + - '' + - '' + - '
' - ); - - // Insert table row with one table cell - model.change( writer => { - writer.setAttribute( 'foo', 'bar', root.getNodeByPath( [ 0, 0, 0, 0 ] ) ); - } ); - - expect( formatTable( getModelData( model, { withoutSelection: true } ) ) ).to.equal( formatTable( - '' + - '' + - '' + - '' + - '
' - ) ); - - sinon.assert.calledOnce( refreshItemSpy ); - } ); - - it( 'should not be called on adding attribute of a paragraph with other attribute set in a table cell', () => { - setModelData( model, - '' + - '' + - '' + - '' + - '
' - ); - - // Insert table row with one table cell - model.change( writer => { - writer.setAttribute( 'foo', 'bar', root.getNodeByPath( [ 0, 0, 0, 0 ] ) ); - } ); - - expect( formatTable( getModelData( model, { withoutSelection: true } ) ) ).to.equal( formatTable( - '' + - '' + - '' + - '' + - '
' - ) ); - - sinon.assert.notCalled( refreshItemSpy ); - } ); - - it( 'should not be called on adding attribute and removing attribute of a paragraph with other attribute set in a table cell', () => { - setModelData( model, - '' + - '' + - '' + - '' + - '
' - ); - - // Insert table row with one table cell - model.change( writer => { - writer.setAttribute( 'foo', 'bar', root.getNodeByPath( [ 0, 0, 0, 0 ] ) ); - writer.removeAttribute( 'bar', root.getNodeByPath( [ 0, 0, 0, 0 ] ) ); - } ); - - expect( formatTable( getModelData( model, { withoutSelection: true } ) ) ).to.equal( formatTable( - '' + - '' + - '' + - '' + - '
' - ) ); - - sinon.assert.notCalled( refreshItemSpy ); } ); } ); From da8d40e0e5c138d03cc9b3f1692cfa9d169555d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 22 Aug 2019 15:05:45 +0200 Subject: [PATCH 09/15] Update proper tests for table cell refresh post-fixer. --- .../table-cell-refresh-post-fixer.js | 55 ++++++++++++++++++- 1 file changed, 52 insertions(+), 3 deletions(-) diff --git a/tests/converters/table-cell-refresh-post-fixer.js b/tests/converters/table-cell-refresh-post-fixer.js index ee9a5a20..a9e3a7d6 100644 --- a/tests/converters/table-cell-refresh-post-fixer.js +++ b/tests/converters/table-cell-refresh-post-fixer.js @@ -17,7 +17,7 @@ import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictest import Delete from '@ckeditor/ckeditor5-typing/src/delete'; describe( 'Table cell refresh post-fixer', () => { - let editor, model, doc, root, view; + let editor, model, doc, root, view, refreshItemSpy; testUtils.createSinonSandbox(); @@ -44,10 +44,12 @@ describe( 'Table cell refresh post-fixer', () => { } ); editor.conversion.elementToElement( { model: 'block', view: 'div' } ); - model.schema.extend( '$block', { allowAttributes: 'foo' } ); + model.schema.extend( '$block', { allowAttributes: [ 'foo', 'bar' ] } ); editor.conversion.attributeToAttribute( { model: 'foo', view: 'foo' } ); + editor.conversion.attributeToAttribute( { model: 'bar', view: 'bar' } ); injectTableCellRefreshPostFixer( model ); + refreshItemSpy = sinon.spy( model.document.differ, 'refreshItem' ); } ); } ); @@ -69,6 +71,7 @@ describe( 'Table cell refresh post-fixer', () => { expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [ [ '

00

' ] ], { asWidget: true } ) ); + sinon.assert.calledOnce( refreshItemSpy ); } ); it( 'should rename to

on adding other block element to the same table cell', () => { @@ -89,6 +92,7 @@ describe( 'Table cell refresh post-fixer', () => { expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [ [ '

00

' ] ], { asWidget: true } ) ); + sinon.assert.calledOnce( refreshItemSpy ); } ); it( 'should properly rename the same element on consecutive changes', () => { @@ -107,6 +111,7 @@ describe( 'Table cell refresh post-fixer', () => { expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [ [ '

00

' ] ], { asWidget: true } ) ); + sinon.assert.calledOnce( refreshItemSpy ); model.change( writer => { writer.remove( table.getNodeByPath( [ 0, 0, 1 ] ) ); @@ -115,6 +120,7 @@ describe( 'Table cell refresh post-fixer', () => { expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [ [ '00' ] ], { asWidget: true } ) ); + sinon.assert.calledTwice( refreshItemSpy ); } ); it( 'should rename to

when setting attribute on ', () => { @@ -129,6 +135,7 @@ describe( 'Table cell refresh post-fixer', () => { expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [ [ '

00

' ] ], { asWidget: true } ) ); + sinon.assert.calledOnce( refreshItemSpy ); } ); it( 'should rename

to when removing all but one paragraph inside table cell', () => { @@ -143,6 +150,7 @@ describe( 'Table cell refresh post-fixer', () => { expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [ [ '00' ] ], { asWidget: true } ) ); + sinon.assert.calledOnce( refreshItemSpy ); } ); it( 'should rename

to when removing attribute from ', () => { @@ -157,6 +165,7 @@ describe( 'Table cell refresh post-fixer', () => { expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [ [ '00' ] ], { asWidget: true } ) ); + sinon.assert.calledOnce( refreshItemSpy ); } ); it( 'should keep

in the view when attribute value is changed', () => { @@ -171,6 +180,42 @@ describe( 'Table cell refresh post-fixer', () => { expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [ [ '

00

' ] ], { asWidget: true } ) ); + // False positive: should not be called. + sinon.assert.calledOnce( refreshItemSpy ); + } ); + + it( 'should keep

in the view when adding another attribute to a with other attributes', () => { + editor.setData( viewTable( [ [ '

00

' ] ] ) ); + + const table = root.getChild( 0 ); + + model.change( writer => { + writer.setAttribute( 'bar', 'bar', table.getNodeByPath( [ 0, 0, 0 ] ) ); + } ); + + expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [ + [ '

00

' ] + ], { asWidget: true } ) ); + + // False positive + sinon.assert.notCalled( refreshItemSpy ); + } ); + + it( 'should keep

in the view when adding another attribute to a and removing attribute that is already set', () => { + editor.setData( viewTable( [ [ '

00

' ] ] ) ); + + const table = root.getChild( 0 ); + + model.change( writer => { + writer.setAttribute( 'bar', 'bar', table.getNodeByPath( [ 0, 0, 0 ] ) ); + writer.removeAttribute( 'foo', table.getNodeByPath( [ 0, 0, 0 ] ) ); + } ); + + expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [ + [ '

00

' ] + ], { asWidget: true } ) ); + // False positive: should not be called. + sinon.assert.calledOnce( refreshItemSpy ); } ); it( 'should keep

in the view when attribute value is changed (table cell with multiple blocks)', () => { @@ -185,6 +230,7 @@ describe( 'Table cell refresh post-fixer', () => { expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [ [ '

00

00

' ] ], { asWidget: true } ) ); + sinon.assert.notCalled( refreshItemSpy ); } ); it( 'should do nothing on rename to other block', () => { @@ -199,6 +245,7 @@ describe( 'Table cell refresh post-fixer', () => { expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [ [ '
00
' ] ], { asWidget: true } ) ); + sinon.assert.notCalled( refreshItemSpy ); } ); it( 'should do nothing when setting attribute on block item other then ', () => { @@ -213,9 +260,10 @@ describe( 'Table cell refresh post-fixer', () => { expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [ [ '
foo
' ] ], { asWidget: true } ) ); + sinon.assert.notCalled( refreshItemSpy ); } ); - it( 'should keep

in the view when attribute value is changed (table cell with multiple blocks)', () => { + it( 'should rename

in to when removing (table cell with 2 paragraphs)', () => { editor.setData( viewTable( [ [ '

00

00

' ] ] ) ); const table = root.getChild( 0 ); @@ -227,6 +275,7 @@ describe( 'Table cell refresh post-fixer', () => { expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [ [ '00' ] ], { asWidget: true } ) ); + sinon.assert.calledOnce( refreshItemSpy ); } ); it( 'should update view selection after deleting content', () => { From e614403c08c689136fd71e95f9bebd8db54e0c4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 22 Aug 2019 15:06:10 +0200 Subject: [PATCH 10/15] Improve docs & check in table cell refresh post-fixer. --- src/converters/table-cell-refresh-post-fixer.js | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/converters/table-cell-refresh-post-fixer.js b/src/converters/table-cell-refresh-post-fixer.js index 0b572837..852ef50c 100644 --- a/src/converters/table-cell-refresh-post-fixer.js +++ b/src/converters/table-cell-refresh-post-fixer.js @@ -49,12 +49,14 @@ function tableCellRefreshPostFixer( model ) { // Check if the model table cell requires refreshing to be re-rendered to a proper state in the view. // -// This methods detects changes that will require: -// - to

rename in the view, -// - adding a missing , -// - or wrapping a text node in +// This methods detects changes that will require renaming to

(or vice versa) in the view, // -// thus requiring refreshing the table cell view. +// This method is simple heuristic that check only single change and will give false positive result when many changes will result +// in a state that does not require renaming in the view. +// +// For instance: The to

should be renamed when adding an attribute to a . +// But adding one attribute and removing another will result in false positive: the check for added attribute will see one attribute +// on a paragraph and will falsy qualify such change as adding attribute to a paragraph without any attribute. // // @param {module:engine/model/element~Element} tableCell Table cell to check. // @param {String} type Type of change. @@ -87,5 +89,5 @@ function checkRefresh( tableCell, type ) { // For other changes (insert/remove) the to

change can occur when: // - sibling is added to a single paragraph (childCount == 2) // - sibling is removed and single paragraph is left (childCount == 1) - return tableCell.childCount < 3; + return tableCell.childCount <= ( type == 'insert' ? 2 : 1 ); } From 4fb432b55ccaca7172842865fe40852921578693 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 22 Aug 2019 15:12:11 +0200 Subject: [PATCH 11/15] Revert table manual test. --- tests/manual/tableblockcontent.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/tests/manual/tableblockcontent.js b/tests/manual/tableblockcontent.js index 27633fc9..eb03add8 100644 --- a/tests/manual/tableblockcontent.js +++ b/tests/manual/tableblockcontent.js @@ -8,16 +8,13 @@ import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classiceditor'; import ArticlePluginSet from '@ckeditor/ckeditor5-core/tests/_utils/articlepluginset'; import Alignment from '@ckeditor/ckeditor5-alignment/src/alignment'; -import EasyImage from '@ckeditor/ckeditor5-easy-image/src/easyimage'; -import { CS_CONFIG } from '@ckeditor/ckeditor5-cloud-services/tests/_utils/cloud-services-config'; ClassicEditor .create( document.querySelector( '#editor' ), { - cloudServices: CS_CONFIG, - plugins: [ ArticlePluginSet, Alignment, EasyImage ], + plugins: [ ArticlePluginSet, Alignment ], toolbar: [ 'heading', '|', 'insertTable', '|', 'bold', 'italic', 'bulletedList', 'numberedList', 'blockQuote', - 'alignment', '|', 'imageUpload', 'undo', 'redo' + 'alignment', '|', 'undo', 'redo' ], image: { toolbar: [ 'imageStyle:full', 'imageStyle:side' ] From 347d8522e3061e592f1efcdf8e44643343d6d4ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 22 Aug 2019 15:20:46 +0200 Subject: [PATCH 12/15] Fix table cell paragraph post-fixer. --- src/converters/table-cell-paragraph-post-fixer.js | 6 ++++++ src/converters/table-cell-refresh-post-fixer.js | 6 ------ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/converters/table-cell-paragraph-post-fixer.js b/src/converters/table-cell-paragraph-post-fixer.js index a675dd3b..00a1c579 100644 --- a/src/converters/table-cell-paragraph-post-fixer.js +++ b/src/converters/table-cell-paragraph-post-fixer.js @@ -55,6 +55,12 @@ function tableCellContentsPostFixer( writer, model ) { if ( entry.name == 'tableCell' ) { wasFixed = fixTableCellContent( entry.position.nodeAfter, writer ) || wasFixed; } + + // Check table cell children for directly placed text nodes. + // Temporary check. See https://github.com/ckeditor/ckeditor5/issues/1464. + if ( entry.name == '$text' && entry.position.parent.is( 'tableCell' ) ) { + wasFixed = fixTableCellContent( entry.position.parent, writer ) || wasFixed; + } } } diff --git a/src/converters/table-cell-refresh-post-fixer.js b/src/converters/table-cell-refresh-post-fixer.js index 852ef50c..68ff8cfe 100644 --- a/src/converters/table-cell-refresh-post-fixer.js +++ b/src/converters/table-cell-refresh-post-fixer.js @@ -67,12 +67,6 @@ function checkRefresh( tableCell, type ) { } const children = Array.from( tableCell.getChildren() ); - const hasInnerText = children.some( child => child.is( 'text' ) ); - - // If a bare text node (not wrapped in a paragraph) was added - refresh it. - if ( hasInnerText ) { - return true; - } const hasInnerParagraph = children.some( child => child.is( 'paragraph' ) ); From 9c4b1dd937fea82b2da42353533b0c45802092ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 22 Aug 2019 15:21:24 +0200 Subject: [PATCH 13/15] Remove unnecessary constant. --- src/converters/table-cell-refresh-post-fixer.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/converters/table-cell-refresh-post-fixer.js b/src/converters/table-cell-refresh-post-fixer.js index 68ff8cfe..4ae4a865 100644 --- a/src/converters/table-cell-refresh-post-fixer.js +++ b/src/converters/table-cell-refresh-post-fixer.js @@ -66,9 +66,7 @@ function checkRefresh( tableCell, type ) { return true; } - const children = Array.from( tableCell.getChildren() ); - - const hasInnerParagraph = children.some( child => child.is( 'paragraph' ) ); + const hasInnerParagraph = Array.from( tableCell.getChildren() ).some( child => child.is( 'paragraph' ) ); // If there is no paragraph in table cell then the view doesn't require refreshing. if ( !hasInnerParagraph ) { From 5dcbe0f006e7ee929d499df7a9b0516fb17d47e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 22 Aug 2019 16:08:46 +0200 Subject: [PATCH 14/15] Move adding empty paragraph to a table cell on remove to table cell paragraph post-fixer. --- .../table-cell-paragraph-post-fixer.js | 47 +++++++++++-------- .../table-cell-refresh-post-fixer.js | 5 -- 2 files changed, 28 insertions(+), 24 deletions(-) diff --git a/src/converters/table-cell-paragraph-post-fixer.js b/src/converters/table-cell-paragraph-post-fixer.js index 00a1c579..6291432f 100644 --- a/src/converters/table-cell-paragraph-post-fixer.js +++ b/src/converters/table-cell-paragraph-post-fixer.js @@ -42,25 +42,20 @@ function tableCellContentsPostFixer( writer, model ) { let wasFixed = false; for ( const entry of changes ) { - // 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; - } - - // Check table cell children for directly placed text nodes. - // Temporary check. See https://github.com/ckeditor/ckeditor5/issues/1464. - if ( entry.name == '$text' && entry.position.parent.is( 'tableCell' ) ) { - wasFixed = fixTableCellContent( entry.position.parent, writer ) || wasFixed; - } + if ( entry.type == 'insert' && entry.name == 'table' ) { + wasFixed = fixTable( entry.position.nodeAfter, writer ) || wasFixed; + } + + if ( entry.type == 'insert' && entry.name == 'tableRow' ) { + wasFixed = fixTableRow( entry.position.nodeAfter, writer ) || wasFixed; + } + + if ( entry.type == 'insert' && entry.name == 'tableCell' ) { + wasFixed = fixTableCellContent( entry.position.nodeAfter, writer ) || wasFixed; + } + + if ( checkTableCellChange( entry ) ) { + wasFixed = fixTableCellContent( entry.position.parent, writer ) || wasFixed; } } @@ -121,3 +116,17 @@ function fixTableCellContent( tableCell, writer ) { // Return true when there were text nodes to fix. return !!textNodes.length; } + +// Check if differ change should fix table cell. This happens on: +// - removing content from table cell (ie tableCell can be left empty). +// - adding text node directly into a table cell. +// +// @param {Object} differ change entry +// @returns {Boolean} +function checkTableCellChange( entry ) { + if ( !entry.position || !entry.position.parent.is( 'tableCell' ) ) { + return false; + } + + return entry.type == 'insert' && entry.name == '$text' || entry.type == 'remove'; +} diff --git a/src/converters/table-cell-refresh-post-fixer.js b/src/converters/table-cell-refresh-post-fixer.js index 4ae4a865..05ee7ddb 100644 --- a/src/converters/table-cell-refresh-post-fixer.js +++ b/src/converters/table-cell-refresh-post-fixer.js @@ -61,11 +61,6 @@ function tableCellRefreshPostFixer( model ) { // @param {module:engine/model/element~Element} tableCell Table cell to check. // @param {String} type Type of change. function checkRefresh( tableCell, type ) { - // If all children of a table cell were removed - refresh it. - if ( !tableCell.childCount ) { - return true; - } - const hasInnerParagraph = Array.from( tableCell.getChildren() ).some( child => child.is( 'paragraph' ) ); // If there is no paragraph in table cell then the view doesn't require refreshing. From 37c524b6efa886206ea5079d74c181d0ee5d0413 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Thu, 22 Aug 2019 16:19:44 +0200 Subject: [PATCH 15/15] Cosmetic changes. --- .../table-cell-refresh-post-fixer.js | 39 +++++++++++-------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/src/converters/table-cell-refresh-post-fixer.js b/src/converters/table-cell-refresh-post-fixer.js index 05ee7ddb..08748304 100644 --- a/src/converters/table-cell-refresh-post-fixer.js +++ b/src/converters/table-cell-refresh-post-fixer.js @@ -26,18 +26,18 @@ function tableCellRefreshPostFixer( model ) { const differ = model.document.differ; // Stores cells to be refreshed so the table cell will be refreshed once for multiple changes. - const cellToRefresh = new Set(); + const cellsToRefresh = new Set(); for ( const change of differ.getChanges() ) { const parent = change.type == 'insert' || change.type == 'remove' ? change.position.parent : change.range.start.parent; if ( parent.is( 'tableCell' ) && checkRefresh( parent, change.type ) ) { - cellToRefresh.add( parent ); + cellsToRefresh.add( parent ); } } - if ( cellToRefresh.size ) { - for ( const tableCell of cellToRefresh.values() ) { + if ( cellsToRefresh.size ) { + for ( const tableCell of cellsToRefresh.values() ) { differ.refreshItem( tableCell ); } @@ -47,16 +47,16 @@ function tableCellRefreshPostFixer( model ) { return false; } -// Check if the model table cell requires refreshing to be re-rendered to a proper state in the view. +// Checks if the model table cell requires refreshing to be re-rendered to a proper state in the view. // -// This methods detects changes that will require renaming to

(or vice versa) in the view, +// This methods detects changes that will require renaming to

(or vice versa) in the view. // -// This method is simple heuristic that check only single change and will give false positive result when many changes will result -// in a state that does not require renaming in the view. +// This method is a simple heuristic that checks only a single change and will sometimes give a false positive result when multiple changes +// will result in a state that does not require renaming in the view (but will be seen as requiring a refresh). // -// For instance: The to

should be renamed when adding an attribute to a . -// But adding one attribute and removing another will result in false positive: the check for added attribute will see one attribute -// on a paragraph and will falsy qualify such change as adding attribute to a paragraph without any attribute. +// For instance: a `` should be renamed to `

` when adding an attribute to a ``. +// But adding one attribute and removing another one will result in a false positive: the check for added attribute will see one attribute +// on a paragraph and will falsy qualify such change as adding an attribute to a paragraph without any attribute. // // @param {module:engine/model/element~Element} tableCell Table cell to check. // @param {String} type Type of change. @@ -64,17 +64,24 @@ function checkRefresh( tableCell, type ) { const hasInnerParagraph = Array.from( tableCell.getChildren() ).some( child => child.is( 'paragraph' ) ); // If there is no paragraph in table cell then the view doesn't require refreshing. + // + // Why? What we really want to achieve is to make all the old paragraphs (which weren't added in this batch) to be + // converted once again, so that the paragraph-in-table-cell converter can correctly create a `

` or a `` element. + // If there are no paragraphs in the table cell, we don't care. if ( !hasInnerParagraph ) { return false; } - // For attribute change we only refresh single paragraphs as they might trigger to

change in the view. + // For attribute change we only refresh if there is a single paragraph as in this case we may want to change existing `` to `

`. if ( type == 'attribute' ) { - return tableCell.childCount === 1 && Array.from( tableCell.getChild( 0 ).getAttributeKeys() ).length < 2; + const attributesCount = Array.from( tableCell.getChild( 0 ).getAttributeKeys() ).length; + + return tableCell.childCount === 1 && attributesCount < 2; } - // For other changes (insert/remove) the to

change can occur when: - // - sibling is added to a single paragraph (childCount == 2) - // - sibling is removed and single paragraph is left (childCount == 1) + // For other changes (insert, remove) the `` to `

` change is needed when: + // + // - another element is added to a single paragraph (childCount becomes >= 2) + // - another element is removed and a single paragraph is left (childCount == 1) return tableCell.childCount <= ( type == 'insert' ? 2 : 1 ); }