Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

#6260: Fixed triggering of table cell refreshing when multiple elements added in one differ #6779

Merged
merged 3 commits into from
May 11, 2020
Merged

#6260: Fixed triggering of table cell refreshing when multiple elements added in one differ #6779

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4eddb20
Fixed triggering of table cell refreshing when multiple elements adde…
niegowski May 8, 2020
9f4c0e2
Add test to verify that refresh item is not called too many times.
jodator May 11, 2020
82f7ae3
Update packages/ckeditor5-table/src/converters/table-cell-refresh-pos…
jodator May 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions packages/ckeditor5-table/src/converters/table-cell-refresh-post-fixer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,21 @@ function tableCellRefreshPostFixer( model ) {
// Stores cells to be refreshed so the table cell will be refreshed once for multiple changes.
const cellsToRefresh = new Set();

// Counting the paragraph inserts to verify if it increased to more than one paragraph in the current differ.
let insertCount = 0;

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 ) ) {
if ( !parent.is( 'tableCell' ) ) {
continue;
}

if ( change.type == 'insert' ) {
insertCount++;
}

if ( checkRefresh( parent, change.type, insertCount ) ) {
cellsToRefresh.add( parent );
}
}
Expand Down Expand Up @@ -60,7 +71,8 @@ function tableCellRefreshPostFixer( model ) {
//
// @param {module:engine/model/element~Element} tableCell The table cell to check.
// @param {String} type Type of change.
function checkRefresh( tableCell, type ) {
// @param {Number} insertCount The number of inserts in differ.
function checkRefresh( tableCell, type, insertCount ) {
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.
Expand All @@ -83,5 +95,7 @@ function checkRefresh( tableCell, type ) {
//
// - 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 );
//
// Change is not needed if there were multiple blocks before change.
return tableCell.childCount <= ( type == 'insert' ? insertCount + 1 : 1 );
}
83 changes: 76 additions & 7 deletions packages/ckeditor5-table/tests/converters/table-cell-refresh-post-fixer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,18 +55,16 @@ describe( 'Table cell refresh post-fixer', () => {
return editor.destroy();
} );

it( 'should rename <span> to <p> when adding more <paragraph> elements to the same table cell', () => {
it( 'should rename <span> to <p> when adding <paragraph> element to the same table cell', () => {
editor.setData( viewTable( [ [ '<p>00</p>' ] ] ) );

const table = root.getChild( 0 );

model.change( writer => {
const nodeByPath = table.getNodeByPath( [ 0, 0, 0 ] );

const paragraph = writer.createElement( 'paragraph' );

writer.insert( paragraph, nodeByPath, 'after' );

writer.setSelection( nodeByPath.nextSibling, 0 );
} );

Expand All @@ -76,18 +74,37 @@ describe( 'Table cell refresh post-fixer', () => {
sinon.assert.calledOnce( refreshItemSpy );
} );

it( 'should rename <span> to <p> on adding other block element to the same table cell', () => {
it( 'should rename <span> to <p> when adding more <paragraph> elements to the same table cell', () => {
editor.setData( viewTable( [ [ '<p>00</p>' ] ] ) );

const table = root.getChild( 0 );

model.change( writer => {
const nodeByPath = table.getNodeByPath( [ 0, 0, 0 ] );
const paragraph1 = writer.createElement( 'paragraph' );
const paragraph2 = writer.createElement( 'paragraph' );

const paragraph = writer.createElement( 'block' );
writer.insert( paragraph1, nodeByPath, 'after' );
writer.insert( paragraph2, nodeByPath, 'after' );
writer.setSelection( nodeByPath.nextSibling, 0 );
} );

writer.insert( paragraph, nodeByPath, 'after' );
assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [
[ '<p>00</p><p></p><p></p>' ]
], { asWidget: true } ) );
sinon.assert.calledOnce( refreshItemSpy );
} );

it( 'should rename <span> to <p> on adding other block element to the same table cell', () => {
editor.setData( viewTable( [ [ '<p>00</p>' ] ] ) );

const table = root.getChild( 0 );

model.change( writer => {
const nodeByPath = table.getNodeByPath( [ 0, 0, 0 ] );
const block = writer.createElement( 'block' );

writer.insert( block, nodeByPath, 'after' );
writer.setSelection( nodeByPath.nextSibling, 0 );
} );

Expand All @@ -97,6 +114,27 @@ describe( 'Table cell refresh post-fixer', () => {
sinon.assert.calledOnce( refreshItemSpy );
} );

it( 'should rename <span> to <p> on adding multiple other block elements to the same table cell', () => {
editor.setData( viewTable( [ [ '<p>00</p>' ] ] ) );

const table = root.getChild( 0 );

model.change( writer => {
const nodeByPath = table.getNodeByPath( [ 0, 0, 0 ] );
const block1 = writer.createElement( 'block' );
const block2 = writer.createElement( 'block' );

writer.insert( block1, nodeByPath, 'after' );
writer.insert( block2, nodeByPath, 'after' );
writer.setSelection( nodeByPath.nextSibling, 0 );
} );

assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [
[ '<p>00</p><div></div><div></div>' ]
], { asWidget: true } ) );
sinon.assert.calledOnce( refreshItemSpy );
} );

it( 'should properly rename the same element on consecutive changes', () => {
editor.setData( viewTable( [ [ '<p>00</p>' ] ] ) );

Expand Down Expand Up @@ -140,7 +178,7 @@ describe( 'Table cell refresh post-fixer', () => {
sinon.assert.calledOnce( refreshItemSpy );
} );

it( 'should rename <p> to <span> when removing all but one paragraph inside table cell', () => {
it( 'should rename <p> to <span> when removing one of two paragraphs inside table cell', () => {
editor.setData( viewTable( [ [ '<p>00</p><p>foo</p>' ] ] ) );

const table = root.getChild( 0 );
Expand All @@ -155,6 +193,22 @@ describe( 'Table cell refresh post-fixer', () => {
sinon.assert.calledOnce( refreshItemSpy );
} );

it( 'should rename <p> to <span> when removing all but one paragraph inside table cell', () => {
editor.setData( viewTable( [ [ '<p>00</p><p>foo</p><p>bar</p>' ] ] ) );

const table = root.getChild( 0 );

model.change( writer => {
writer.remove( table.getNodeByPath( [ 0, 0, 1 ] ) );
writer.remove( table.getNodeByPath( [ 0, 0, 1 ] ) );
} );

assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [
[ '00' ]
], { asWidget: true } ) );
sinon.assert.calledOnce( refreshItemSpy );
} );

it( 'should rename <p> to <span> when removing attribute from <paragraph>', () => {
editor.setData( '<table><tr><td><p foo="bar">00</p></td></tr></table>' );

Expand Down Expand Up @@ -250,6 +304,21 @@ describe( 'Table cell refresh post-fixer', () => {
sinon.assert.notCalled( refreshItemSpy );
} );

it( 'should do nothing on adding <paragraph> to existing paragraphs', () => {
editor.setData( viewTable( [ [ '<p>a</p><p>b</p>' ] ] ) );

const table = root.getChild( 0 );

model.change( writer => {
writer.insertElement( 'paragraph', table.getNodeByPath( [ 0, 0, 1 ] ), 'after' );
} );

assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [
[ '<p>a</p><p>b</p><p></p>' ]
], { asWidget: true } ) );
sinon.assert.notCalled( refreshItemSpy );
} );

it( 'should do nothing when setting attribute on block item other then <paragraph>', () => {
editor.setData( viewTable( [ [ '<div>foo</div>' ] ] ) );

Expand Down