Skip to content

Commit

Permalink
Fix RangeError when handling big tables (#14843)
Browse files Browse the repository at this point in the history
Fix (table): Fix crashing when handling tables with over 2500 rows. Closes #14785.

Other (table): Improve performance when handling large tables. See #14785.
  • Loading branch information
filipsobol authored Aug 29, 2023
1 parent fec3d4b commit c2d4af6
Show file tree
Hide file tree
Showing 2 changed files with 105 additions and 2 deletions.
67 changes: 67 additions & 0 deletions packages/ckeditor5-table/src/tablewalker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,11 @@ export default class TableWalker implements IterableIterator<TableSlot> {
*/
private _nextCellAtColumn: number;

/**
* Indicates whether the iterator jumped to (or close to) the start row, ignoring rows that don't need to be traversed.
*/
private _jumpedToStartRow = false;

/**
* Creates an instance of the table walker.
*
Expand Down Expand Up @@ -243,6 +248,10 @@ export default class TableWalker implements IterableIterator<TableSlot> {
* @returns The next table walker's value.
*/
public next(): IteratorResult<TableSlot, undefined> {
if ( this._canJumpToStartRow() ) {
this._jumpToNonSpannedRowClosestToStartRow();
}

const row = this._table.getChild( this._rowIndex );

// Iterator is done when there's no row (table ended) or the row is after `endRow` limit.
Expand Down Expand Up @@ -423,6 +432,64 @@ export default class TableWalker implements IterableIterator<TableSlot> {

rowSpans.set( column, data );
}

/**
* Checks if part of the table can be skipped.
*/
private _canJumpToStartRow(): boolean {
return !!this._startRow &&
this._startRow > 0 &&
!this._jumpedToStartRow;
}

/**
* Sets the current row to `this._startRow` or the first row before it that has the number of cells
* equal to the number of columns in the table.
*
* Example:
* +----+----+----+
* | 00 | 01 | 02 |
* |----+----+----+
* | 10 | 12 |
* | +----+
* | | 22 |
* | +----+
* | | 32 | <--- Start row
* +----+----+----+
* | 40 | 41 | 42 |
* +----+----+----+
*
* If the 4th row is a `this._startRow`, this method will:
* 1.) Count the number of columns this table has based on the first row (3 columns in this case).
* 2.) Check if the 4th row contains 3 cells. It doesn't, so go to the row before it.
* 3.) Check if the 3rd row contains 3 cells. It doesn't, so go to the row before it.
* 4.) Check if the 2nd row contains 3 cells. It does, so set the current row to that row.
*
* Setting the current row this way is necessary to let the `next()` method loop over the cells
* spanning multiple rows or columns and update the `this._spannedCells` property.
*/
private _jumpToNonSpannedRowClosestToStartRow(): void {
const firstRowLength = this._getRowLength( 0 );

for ( let i = this._startRow!; !this._jumpedToStartRow; i-- ) {
if ( firstRowLength === this._getRowLength( i ) ) {
this._row = i;
this._rowIndex = i;
this._jumpedToStartRow = true;
}
}
}

/**
* Returns a number of columns in a row taking `colspan` into consideration.
*/
private _getRowLength( rowIndex: number ): number {
const row = this._table.getChild( rowIndex ) as Element;

return [ ...row.getChildren() ].reduce( ( cols, row ) => {
return cols + parseInt( row.getAttribute( 'colspan' ) as string || '1' );
}, 0 );
}
}

/**
Expand Down
40 changes: 38 additions & 2 deletions packages/ckeditor5-table/tests/tablewalker.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,10 @@ import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
import { setData, parse } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';

import TableWalker from '../src/tablewalker';
import TableEditing from '../src/tableediting';
import { modelTable } from './_utils/utils';

import TableWalker from '../src/tablewalker';

describe( 'TableWalker', () => {
let editor, model, doc, root;

Expand Down Expand Up @@ -195,6 +194,43 @@ describe( 'TableWalker', () => {
expect( tableWalker[ 3 ].rowIndex ).to.equal( 2 );
} );

it( 'does not cause the "RangeError: Maximum call stack size exceeded" error when handling big tables. ', () => {
const data = Array( 3000 ).fill( [ '1', 'Example content', '3' ] );
const table = parse(
modelTable( data ),
model.schema
);

function getAllItems() {
return Array.from(
new TableWalker( table, { row: 2999 } )
);
}

expect( getAllItems ).to.not.throw( RangeError, 'Maximum call stack size exceeded' );
} ).timeout( 5000 );

it( 'does not cause the "RangeError: Maximum call stack size exceeded" error when handling big tables with rowspan. ', () => {
const data = [
...Array( 2000 ).fill( [ '1', 'Example content', '3' ] ),
[ '1', { contents: 'Cell with rowspan', rowspan: 1000 }, '3' ],
...Array( 999 ).fill( [ '1', '3' ] )
];

const table = parse(
modelTable( data ),
model.schema
);

function getAllItems() {
return Array.from(
new TableWalker( table, { row: 2999 } )
);
}

expect( getAllItems ).to.not.throw( RangeError, 'Maximum call stack size exceeded' );
} ).timeout( 5000 );

describe( 'option.startRow', () => {
it( 'should start iterating from given row but with cell spans properly calculated', () => {
// +----+----+----+
Expand Down

0 comments on commit c2d4af6

Please sign in to comment.