Skip to content

Commit

Permalink
Merge pull request #16821 from ckeditor/ck/16819
Browse files Browse the repository at this point in the history
Fix (engine): No longer crash editor when trying to style a very long paragraph. Closes #16819
  • Loading branch information
niegowski authored Aug 2, 2024
2 parents b50ddbb + 67f5943 commit 248d64c
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 1 deletion.
12 changes: 11 additions & 1 deletion packages/ckeditor5-engine/src/model/differ.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1520,7 +1520,17 @@ function _generateDiffInstructionsFromChanges( oldChildrenLength: number, change
// We removed `howMany` old nodes, update `oldChildrenHandled`.
oldChildrenHandled += change.howMany;
} else {
diff.push( ...'a'.repeat( change.howMany ).split( '' ) );
// Total maximum amount of arguments that can be passed to `Array.prototype.push` may be limited so we need to
// add them manually one by one to avoid this limit. However loop might be a bit slower than `push` method on
// smaller changesets so we need to decide which method to use based on the size of the change.
// See: https://github.com/ckeditor/ckeditor5/issues/16819
if ( change.howMany > 1500 ) {
for ( let i = 0; i < change.howMany; i++ ) {
diff.push( 'a' );
}
} else {
diff.push( ...'a'.repeat( change.howMany ).split( '' ) );
}

// The last handled offset is at the position after the changed range.
offset = change.offset + change.howMany;
Expand Down
56 changes: 56 additions & 0 deletions packages/ckeditor5-engine/tests/model/differ.js
Original file line number Diff line number Diff line change
Expand Up @@ -1707,6 +1707,62 @@ describe( 'Differ', () => {
expectChanges( [] );
} );
} );

// See: https://github.com/ckeditor/ckeditor5/issues/16819
it( 'on element with very long text should have batched instructions together during generating diff from changes', () => {
const MAX_PUSH_CALL_STACK_ARGS = 1500;

const p = root.getChild( 0 );
const veryLongString = 'a'.repeat( 300 );

model.change( () => {
insert( veryLongString, Position._createAt( p, 0 ) );
} );

const pushSpy = sinon.spy( Array.prototype, 'push' );

model.change( writer => {
writer.setAttribute( attributeKey, true, writer.createRangeIn( p ) );
} );

// Let's check if appended instructions has been batched together in single `.push()` call.
const instructionsDiff = pushSpy.args.find( args => (
args.length >= 300 &&
args.length < MAX_PUSH_CALL_STACK_ARGS &&
args.every( ch => ch === 'a' )
) );

expect( instructionsDiff ).not.to.be.undefined;
pushSpy.restore();
} );

// See: https://github.com/ckeditor/ckeditor5/issues/16819
it( 'on element with very long text should not have batched instructions together during generating diff from changes ' +
'that are larger than max push call stack args count', () => {
const MAX_PUSH_CALL_STACK_ARGS = 1500;

const p = root.getChild( 0 );
const veryLongString = 'a'.repeat( MAX_PUSH_CALL_STACK_ARGS + 10 );

model.change( () => {
insert( veryLongString, Position._createAt( p, 0 ) );
} );

const pushSpy = sinon.spy( Array.prototype, 'push' );

model.change( writer => {
writer.setAttribute( attributeKey, true, writer.createRangeIn( p ) );
} );

// Let's check if appended instructions has been NOT batched together in single `.push()` call.
const instructionsDiff = pushSpy.args.find( args => (
args.length > MAX_PUSH_CALL_STACK_ARGS &&
args.every( ch => ch === 'a' )
) );

expect( instructionsDiff ).to.be.undefined;
pushSpy.restore();
} );
} );

describe( 'split', () => {
Expand Down

0 comments on commit 248d64c

Please sign in to comment.