From d3a2d7682d2bcb1d46b6a36d0f539cd618d72123 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Tue, 23 Jun 2020 13:35:34 +0200 Subject: [PATCH 01/37] Use optional chaining --- packages/alfa-table/src/table.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index 09e1139ad5..2afc40a635 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -207,9 +207,7 @@ export namespace Table { } public slot(x: number, y: number): Option { - return this._slots[x] === undefined || this._slots[x][y] === undefined - ? None - : this._slots[x][y]; + return this._slots?.[x]?.[y] === undefined ? None : this._slots[x][y]; } public update(update: { From 9f83d4014112d14842206555ac3e0a63c78a83b7 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Tue, 23 Jun 2020 13:38:14 +0200 Subject: [PATCH 02/37] Make method private --- packages/alfa-table/src/table.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index 2afc40a635..a352755558 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -231,11 +231,11 @@ export namespace Table { return update.cells !== undefined ? // aggressively keep slots in sync if any cells has been modified. - table.updateSlots(update.cells) + table._updateSlots(update.cells) : table; } - public updateSlots(cells: Iterable): Builder { + private _updateSlots(cells: Iterable): Builder { for (const cell of cells) { for (let x = cell.anchor.x; x < cell.anchor.x + cell.width; x++) { if (this._slots[x] === undefined) { From ceb078590634f8e4d5a961b9434bb15e390bc53b Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Tue, 23 Jun 2020 13:48:27 +0200 Subject: [PATCH 03/37] Use null-coalescing operator in update --- packages/alfa-table/src/cell.ts | 28 +++++++++++----------------- packages/alfa-table/src/row-group.ts | 10 +++++----- packages/alfa-table/src/row.ts | 16 +++++++--------- packages/alfa-table/src/table.ts | 14 +++++++------- 4 files changed, 30 insertions(+), 38 deletions(-) diff --git a/packages/alfa-table/src/cell.ts b/packages/alfa-table/src/cell.ts index b060b9ba4c..4a654db6b8 100644 --- a/packages/alfa-table/src/cell.ts +++ b/packages/alfa-table/src/cell.ts @@ -272,23 +272,17 @@ export namespace Cell { implicitHeaders?: Iterable; }): Builder { return Builder.of( - update.kind !== undefined ? update.kind : this.kind, - update.x !== undefined ? update.x : this.anchor.x, - update.y !== undefined ? update.y : this.anchor.y, - update.width !== undefined ? update.width : this.width, - update.height !== undefined ? update.height : this.height, - update.element !== undefined ? update.element : this.element, - update.variant !== undefined ? update.variant : this.variant, - update.downwardGrowing !== undefined - ? update.downwardGrowing - : this._downwardGrowing, - update.scope !== undefined ? update.scope : this.scope, - update.explicitHeaders !== undefined - ? update.explicitHeaders - : this._explicitHeaders, - update.implicitHeaders !== undefined - ? update.implicitHeaders - : this._implicitHeaders + update.kind ?? this.kind, + update.x ?? this.anchor.x, + update.y ?? this.anchor.y, + update.width ?? this.width, + update.height ?? this.height, + update.element ?? this.element, + update.variant ?? this.variant, + update.downwardGrowing ?? this._downwardGrowing, + update.scope ?? this.scope, + update.explicitHeaders ?? this._explicitHeaders, + update.implicitHeaders ?? this._implicitHeaders ); } diff --git a/packages/alfa-table/src/row-group.ts b/packages/alfa-table/src/row-group.ts index 18d57ab250..2edab2362e 100644 --- a/packages/alfa-table/src/row-group.ts +++ b/packages/alfa-table/src/row-group.ts @@ -140,11 +140,11 @@ export namespace RowGroup { cells?: Iterable; }): Builder { return Builder.of( - update.y !== undefined ? update.y : this._rowGroup.anchor.y, - update.height !== undefined ? update.height : this._rowGroup.height, - update.element !== undefined ? update.element : this._rowGroup.element, - update.width !== undefined ? update.width : this._width, - update.cells !== undefined ? update.cells : this._cells + update.y ?? this._rowGroup.anchor.y, + update.height ?? this._rowGroup.height, + update.element ?? this._rowGroup.element, + update.width ?? this._width, + update.cells ?? this._cells ); } diff --git a/packages/alfa-table/src/row.ts b/packages/alfa-table/src/row.ts index feb98411dd..5fb44e65b0 100644 --- a/packages/alfa-table/src/row.ts +++ b/packages/alfa-table/src/row.ts @@ -98,15 +98,13 @@ export namespace Row { downwardGrowingCells?: Iterable; }): Builder { return Builder.of( - update.y !== undefined ? update.y : this._y, - update.width !== undefined ? update.width : this._width, - update.height !== undefined ? update.height : this._height, - update.element !== undefined ? update.element : this._element, - update.cells !== undefined ? update.cells : this._cells, - update.downwardGrowingCells !== undefined - ? update.downwardGrowingCells - : this._downwardGrowingCells, - update.xCurrent !== undefined ? update.xCurrent : this._xCurrent + update.y ?? this._y, + update.width ?? this._width, + update.height ?? this._height, + update.element ?? this._element, + update.cells ?? this._cells, + update.downwardGrowingCells ?? this._downwardGrowingCells, + update.xCurrent ?? this._xCurrent ); } diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index a352755558..233fe14290 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -220,13 +220,13 @@ export namespace Table { colGroups?: Iterable; }): Builder { const table = Builder.of( - update.element !== undefined ? update.element : this.element, - update.width !== undefined ? update.width : this.width, - update.height !== undefined ? update.height : this.height, - update.cells !== undefined ? update.cells : this._cells, - update.slots !== undefined ? update.slots : this._slots, - update.rowGroups !== undefined ? update.rowGroups : this.rowGroups, - update.colGroups !== undefined ? update.colGroups : this.colGroups + update.element ?? this.element, + update.width ?? this.width, + update.height ?? this.height, + update.cells ?? this._cells, + update.slots ?? this._slots, + update.rowGroups ?? this.rowGroups, + update.colGroups ?? this.colGroups ); return update.cells !== undefined From 3383b713a435e9594d007fe1b78ef385d4a60d66 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Tue, 23 Jun 2020 13:50:28 +0200 Subject: [PATCH 04/37] Make method private --- packages/alfa-table/src/row.ts | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/alfa-table/src/row.ts b/packages/alfa-table/src/row.ts index 5fb44e65b0..13f1adb544 100644 --- a/packages/alfa-table/src/row.ts +++ b/packages/alfa-table/src/row.ts @@ -88,7 +88,7 @@ export namespace Row { return this._downwardGrowingCells; } - public update(update: { + private _update(update: { y?: number; xCurrent?: number; width?: number; @@ -109,7 +109,7 @@ export namespace Row { } public growCells(y: number): Builder { - return this.update({ + return this._update({ downwardGrowingCells: this._downwardGrowingCells.map((cell) => cell.growDownward(y) ), @@ -117,11 +117,11 @@ export namespace Row { } public addNonGrowingCell(cell: Cell.Builder): Builder { - return this.update({ cells: this._cells.append(cell) }); + return this._update({ cells: this._cells.append(cell) }); } public addGrowingCell(cell: Cell.Builder): Builder { - return this.update({ + return this._update({ downwardGrowingCells: this._downwardGrowingCells.append(cell), }); } @@ -139,7 +139,7 @@ export namespace Row { // 8, 9, 10, 13 return Cell.Builder.from(currentCell, this._xCurrent, yCurrent).map( (cell) => - this.update({ + this._update({ // 11 width: Math.max(this.width, this._xCurrent + cell.width), // 12 @@ -153,11 +153,11 @@ export namespace Row { } public adjustWidth(width: number): Builder { - return this.update({ width: Math.max(this._width, width) }); + return this._update({ width: Math.max(this._width, width) }); } public adjustHeight(height: number): Builder { - return this.update({ height: Math.max(this._height, height) }); + return this._update({ height: Math.max(this._height, height) }); } /** @@ -172,7 +172,7 @@ export namespace Row { .concat(this._downwardGrowingCells) .some((cell) => cell.isCovering(this._xCurrent, yCurrent)) ) { - return this.update({ xCurrent: this._xCurrent + 1 }).skipIfCovered( + return this._update({ xCurrent: this._xCurrent + 1 }).skipIfCovered( cells, yCurrent ); @@ -188,7 +188,7 @@ export namespace Row { } public sort(): Builder { - return this.update({ + return this._update({ cells: [...this._cells].sort(compare), downwardGrowingCells: [...this._downwardGrowingCells].sort(compare), }); From ee000f5fdf251cec20546a2d6683f3d7e47d7a7b Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Tue, 23 Jun 2020 14:29:42 +0200 Subject: [PATCH 05/37] Improve default values in update --- packages/alfa-table/src/cell.ts | 36 +++++++++++++------- packages/alfa-table/src/row-group.ts | 19 +++++------ packages/alfa-table/src/row.ts | 24 +++++++++----- packages/alfa-table/src/table.ts | 49 +++++++++++++++------------- 4 files changed, 75 insertions(+), 53 deletions(-) diff --git a/packages/alfa-table/src/cell.ts b/packages/alfa-table/src/cell.ts index 4a654db6b8..6d6a6bf626 100644 --- a/packages/alfa-table/src/cell.ts +++ b/packages/alfa-table/src/cell.ts @@ -258,7 +258,19 @@ export namespace Cell { this._implicitHeaders = List.from(implicitHeaders); } - private _update(update: { + private _update({ + kind = this.kind, + x = this.anchor.x, + y = this.anchor.y, + width = this.width, + height = this.height, + element = this.element, + variant = this.variant, + downwardGrowing = this._downwardGrowing, + scope = this.scope, + explicitHeaders = this._explicitHeaders, + implicitHeaders = this._implicitHeaders, + }: { kind?: Cell.Kind; x?: number; y?: number; @@ -272,17 +284,17 @@ export namespace Cell { implicitHeaders?: Iterable; }): Builder { return Builder.of( - update.kind ?? this.kind, - update.x ?? this.anchor.x, - update.y ?? this.anchor.y, - update.width ?? this.width, - update.height ?? this.height, - update.element ?? this.element, - update.variant ?? this.variant, - update.downwardGrowing ?? this._downwardGrowing, - update.scope ?? this.scope, - update.explicitHeaders ?? this._explicitHeaders, - update.implicitHeaders ?? this._implicitHeaders + kind, + x, + y, + width, + height, + element, + variant, + downwardGrowing, + scope, + explicitHeaders, + implicitHeaders ); } diff --git a/packages/alfa-table/src/row-group.ts b/packages/alfa-table/src/row-group.ts index 2edab2362e..ab0d08d6ce 100644 --- a/packages/alfa-table/src/row-group.ts +++ b/packages/alfa-table/src/row-group.ts @@ -16,8 +16,7 @@ const { compare } = Comparable; /** * @see https://html.spec.whatwg.org/multipage/tables.html#concept-row-group */ -export class RowGroup - implements Comparable, Equatable, Serializable { +export class RowGroup implements Comparable, Equatable, Serializable { public static of(y: number, h: number, element: Element): RowGroup { return new RowGroup(y, h, element); } @@ -132,20 +131,20 @@ export namespace RowGroup { this._cells = List.from(cells); } - public update(update: { + public update({ + y = this._rowGroup.anchor.y, + width = this._width, + height = this._rowGroup.height, + element = this._rowGroup.element, + cells = this._cells, + }: { y?: number; width?: number; height?: number; element?: Element; cells?: Iterable; }): Builder { - return Builder.of( - update.y ?? this._rowGroup.anchor.y, - update.height ?? this._rowGroup.height, - update.element ?? this._rowGroup.element, - update.width ?? this._width, - update.cells ?? this._cells - ); + return Builder.of(y, height, element, width, cells); } public get rowgroup(): RowGroup { diff --git a/packages/alfa-table/src/row.ts b/packages/alfa-table/src/row.ts index 13f1adb544..88c9da0fbb 100644 --- a/packages/alfa-table/src/row.ts +++ b/packages/alfa-table/src/row.ts @@ -88,7 +88,15 @@ export namespace Row { return this._downwardGrowingCells; } - private _update(update: { + private _update({ + y = this._y, + xCurrent = this._xCurrent, + width = this._width, + height = this._height, + element = this._element, + cells = this._cells, + downwardGrowingCells = this._downwardGrowingCells, + }: { y?: number; xCurrent?: number; width?: number; @@ -98,13 +106,13 @@ export namespace Row { downwardGrowingCells?: Iterable; }): Builder { return Builder.of( - update.y ?? this._y, - update.width ?? this._width, - update.height ?? this._height, - update.element ?? this._element, - update.cells ?? this._cells, - update.downwardGrowingCells ?? this._downwardGrowingCells, - update.xCurrent ?? this._xCurrent + y, + width, + height, + element, + cells, + downwardGrowingCells, + xCurrent ); } diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index 233fe14290..4db061561f 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -210,29 +210,32 @@ export namespace Table { return this._slots?.[x]?.[y] === undefined ? None : this._slots[x][y]; } - public update(update: { - element?: Element; - width?: number; - height?: number; - cells?: Iterable; - slots?: Array>>; - rowGroups?: Iterable; - colGroups?: Iterable; - }): Builder { - const table = Builder.of( - update.element ?? this.element, - update.width ?? this.width, - update.height ?? this.height, - update.cells ?? this._cells, - update.slots ?? this._slots, - update.rowGroups ?? this.rowGroups, - update.colGroups ?? this.colGroups + public update( + { + element = this.element, + width = this.width, + height = this.height, + cells = this._cells, + slots = this._slots, + rowGroups = this.rowGroups, + colGroups = this.colGroups, + }: { + element?: Element; + width?: number; + height?: number; + cells?: Iterable; + slots?: Array>>; + rowGroups?: Iterable; + colGroups?: Iterable; + }, + modifiedCells: Iterable = cells + ): Builder { + return ( + Builder.of(element, width, height, cells, slots, rowGroups, colGroups) + // aggressively keep slots in sync. + // cells are modified during build, effectively creating a new Cell.Builder and requiring slots resync. + ._updateSlots(modifiedCells) ); - - return update.cells !== undefined - ? // aggressively keep slots in sync if any cells has been modified. - table._updateSlots(update.cells) - : table; } private _updateSlots(cells: Iterable): Builder { @@ -251,7 +254,7 @@ export namespace Table { } public addCells(cells: Iterable): Builder { - return this.update({ cells: this._cells.concat(cells) }); + return this.update({ cells: this._cells.concat(cells) }, cells); } public addRowGroupFromElement( From c0bb99fc81b75b7756848b637143480c423139af Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Tue, 23 Jun 2020 14:31:29 +0200 Subject: [PATCH 06/37] Optimize slots resync --- packages/alfa-table/src/table.ts | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index 4db061561f..72e7cfabbc 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -265,15 +265,18 @@ export namespace Table { .map((rowGroup) => rowGroup.anchorAt(yCurrent)) .map((rowGroup) => { if (rowGroup.height > 0) { - return this.update({ - // adjust table height and width - height: Math.max(this.height, this.height + rowGroup.height), - width: Math.max(this.width, rowGroup.width), - // merge in new cells - cells: this._cells.concat(rowGroup.cells), - // add new group - rowGroups: List.from(this.rowGroups).append(rowGroup.rowgroup), - }); + return this.update( + { + // adjust table height and width + height: Math.max(this.height, this.height + rowGroup.height), + width: Math.max(this.width, rowGroup.width), + // merge in new cells + cells: this._cells.concat(rowGroup.cells), + // add new group + rowGroups: List.from(this.rowGroups).append(rowGroup.rowgroup), + }, + rowGroup.cells + ); } else { return this; } @@ -429,7 +432,7 @@ export namespace Table { width: Math.max(table.width, table.width + colGroup.width), // 9.1 (1).7 and (2).3 colGroups: List.from(table.colGroups).append(colGroup), - }); + }, []); } continue; } @@ -452,7 +455,7 @@ export namespace Table { cells: List.from(table.cells).concat(row.cells), height: Math.max(table.height, yCurrent + 1), width: Math.max(table.width, row.width), - }); + }, row.cells); // row processing steps 4/16 yCurrent++; @@ -547,7 +550,7 @@ export namespace Table { slots[x].push(table.slot(x, y)); } } - table = table.update({ slots }); + table = table.update({ slots }, []); // Second, we need to compute all headers variant. // This need to be done separately so that the updated table is used in assignHeaders. From e781a4aeee7cb1d9ae85d0c35025bf0916282596 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Tue, 23 Jun 2020 14:40:11 +0200 Subject: [PATCH 07/37] Optimize slots resync --- packages/alfa-table/src/table.ts | 49 +++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 17 deletions(-) diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index 72e7cfabbc..5740d6c1dd 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -215,7 +215,8 @@ export namespace Table { element = this.element, width = this.width, height = this.height, - cells = this._cells, + // we need to remember that no cell is modified to avoid useless slots resync. + cells = undefined, slots = this._slots, rowGroups = this.rowGroups, colGroups = this.colGroups, @@ -228,13 +229,22 @@ export namespace Table { rowGroups?: Iterable; colGroups?: Iterable; }, - modifiedCells: Iterable = cells + // If we're not provided a list of modified cells, we assume none of them have been copied. + modifiedCells: Iterable | undefined = cells ): Builder { return ( - Builder.of(element, width, height, cells, slots, rowGroups, colGroups) - // aggressively keep slots in sync. + Builder.of( + element, + width, + height, + cells ?? this._cells, + slots, + rowGroups, + colGroups + ) + // aggressively keep slots in sync for the cells that have been modified. // cells are modified during build, effectively creating a new Cell.Builder and requiring slots resync. - ._updateSlots(modifiedCells) + ._updateSlots(modifiedCells ?? []) ); } @@ -427,12 +437,14 @@ export namespace Table { const colGroup = ColumnGroup.Builder.from(currentElement) .get() .anchorAt(table.width).columnGroup; - table = table.update({ - // 9.1 (1).4 (cumulative) and (2).2 - width: Math.max(table.width, table.width + colGroup.width), - // 9.1 (1).7 and (2).3 - colGroups: List.from(table.colGroups).append(colGroup), - }, []); + table = table.update( + { + // 9.1 (1).4 (cumulative) and (2).2 + width: Math.max(table.width, table.width + colGroup.width), + // 9.1 (1).7 and (2).3 + colGroups: List.from(table.colGroups).append(colGroup), + } + ); } continue; } @@ -451,11 +463,14 @@ export namespace Table { table.width ).get(); growingCellsList = [...row.downwardGrowingCells]; - table = table.update({ - cells: List.from(table.cells).concat(row.cells), - height: Math.max(table.height, yCurrent + 1), - width: Math.max(table.width, row.width), - }, row.cells); + table = table.update( + { + cells: List.from(table.cells).concat(row.cells), + height: Math.max(table.height, yCurrent + 1), + width: Math.max(table.width, row.width), + }, + row.cells + ); // row processing steps 4/16 yCurrent++; @@ -550,7 +565,7 @@ export namespace Table { slots[x].push(table.slot(x, y)); } } - table = table.update({ slots }, []); + table = table.update({ slots }); // Second, we need to compute all headers variant. // This need to be done separately so that the updated table is used in assignHeaders. From 4961e2353f64975951decdd3c6160133070beb62 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Tue, 23 Jun 2020 14:41:00 +0200 Subject: [PATCH 08/37] Optimize slots resync --- packages/alfa-table/src/table.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index 5740d6c1dd..c80528837e 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -229,7 +229,7 @@ export namespace Table { rowGroups?: Iterable; colGroups?: Iterable; }, - // If we're not provided a list of modified cells, we assume none of them have been copied. + // If we're not provided a list of modified cells, we assume all the cells provided are new and need resync. modifiedCells: Iterable | undefined = cells ): Builder { return ( From 881d28b1171fe12c219914131a6d3071f3e0f7a0 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Tue, 23 Jun 2020 15:18:58 +0200 Subject: [PATCH 09/37] Separate update/add cells --- packages/alfa-table/src/table.ts | 154 +++++++++++++++++-------------- 1 file changed, 85 insertions(+), 69 deletions(-) diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index c80528837e..e9a919e7ad 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -210,44 +210,59 @@ export namespace Table { return this._slots?.[x]?.[y] === undefined ? None : this._slots[x][y]; } - public update( - { - element = this.element, - width = this.width, - height = this.height, - // we need to remember that no cell is modified to avoid useless slots resync. - cells = undefined, - slots = this._slots, - rowGroups = this.rowGroups, - colGroups = this.colGroups, - }: { - element?: Element; - width?: number; - height?: number; - cells?: Iterable; - slots?: Array>>; - rowGroups?: Iterable; - colGroups?: Iterable; - }, - // If we're not provided a list of modified cells, we assume all the cells provided are new and need resync. - modifiedCells: Iterable | undefined = cells - ): Builder { - return ( - Builder.of( - element, - width, - height, - cells ?? this._cells, - slots, - rowGroups, - colGroups - ) - // aggressively keep slots in sync for the cells that have been modified. - // cells are modified during build, effectively creating a new Cell.Builder and requiring slots resync. - ._updateSlots(modifiedCells ?? []) + /** + * Update by getting new value. Does not keep anything in sync, hence is highly unsafe. Use at your own risks. + */ + private _updateUnsafe({ + element = this.element, + width = this.width, + height = this.height, + cells = this._cells, + slots = this._slots, + rowGroups = this.rowGroups, + colGroups = this.colGroups, + }: { + element?: Element; + width?: number; + height?: number; + cells?: Iterable; + slots?: Array>>; + rowGroups?: Iterable; + colGroups?: Iterable; + }): Builder { + return Builder.of( + element, + width, + height, + cells, + slots, + rowGroups, + colGroups ); } + /** + * Update anything but cells, because cells need to be kept in sync. + */ + public update(update: { + element?: Element; + width?: number; + height?: number; + slots?: Array>>; + rowGroups?: Iterable; + colGroups?: Iterable; + }): Builder { + return this._updateUnsafe(update); + } + + /** + * Update cells, and resync slots + * Cells are assumed to keep the same anchors, hence left/top most ones don't change. + */ + public updateCells(cells: Iterable): Builder { + return this._updateUnsafe({ cells })._updateSlots(cells); + } + private _updateSlots(cells: Iterable): Builder { for (const cell of cells) { for (let x = cell.anchor.x; x < cell.anchor.x + cell.width; x++) { @@ -263,8 +278,13 @@ export namespace Table { return this; // for chaining } + /** + * Add new cells, sync slots with the new cells and update left/top most cells + */ public addCells(cells: Iterable): Builder { - return this.update({ cells: this._cells.concat(cells) }, cells); + return this._updateUnsafe({ + cells: this._cells.concat(cells), + })._updateSlots(cells); } public addRowGroupFromElement( @@ -275,17 +295,16 @@ export namespace Table { .map((rowGroup) => rowGroup.anchorAt(yCurrent)) .map((rowGroup) => { if (rowGroup.height > 0) { - return this.update( - { + return ( + this.update({ // adjust table height and width height: Math.max(this.height, this.height + rowGroup.height), width: Math.max(this.width, rowGroup.width), - // merge in new cells - cells: this._cells.concat(rowGroup.cells), // add new group rowGroups: List.from(this.rowGroups).append(rowGroup.rowgroup), - }, - rowGroup.cells + }) + // merge in new cells + .addCells(rowGroup.cells) ); } else { return this; @@ -303,8 +322,8 @@ export namespace Table { } public addHeadersVariants(): Builder { - return this.update({ - cells: List.from( + return this.updateCells( + List.from( map(this.cells, (cell) => cell.addHeaderVariant( this.hasDataCellCoveringArea.bind(this), @@ -312,8 +331,8 @@ export namespace Table { this.height ) ) - ), - }); + ) + ); } /** @@ -366,16 +385,16 @@ export namespace Table { } public assignHeaders(): Builder { - return this.update({ - cells: map(this.cells, (cell) => + return this.updateCells( + map(this.cells, (cell) => cell.assignHeaders( this.element, (x: number, y: number) => this._slots[x][y], this.getAboveLeftGroupHeaders("row"), this.getAboveLeftGroupHeaders("column") ) - ), - }); + ) + ); } public equals(value: unknown): value is this { @@ -437,14 +456,12 @@ export namespace Table { const colGroup = ColumnGroup.Builder.from(currentElement) .get() .anchorAt(table.width).columnGroup; - table = table.update( - { - // 9.1 (1).4 (cumulative) and (2).2 - width: Math.max(table.width, table.width + colGroup.width), - // 9.1 (1).7 and (2).3 - colGroups: List.from(table.colGroups).append(colGroup), - } - ); + table = table.update({ + // 9.1 (1).4 (cumulative) and (2).2 + width: Math.max(table.width, table.width + colGroup.width), + // 9.1 (1).7 and (2).3 + colGroups: List.from(table.colGroups).append(colGroup), + }); } continue; } @@ -463,14 +480,12 @@ export namespace Table { table.width ).get(); growingCellsList = [...row.downwardGrowingCells]; - table = table.update( - { - cells: List.from(table.cells).concat(row.cells), + table = table + .update({ height: Math.max(table.height, yCurrent + 1), width: Math.max(table.width, row.width), - }, - row.cells - ); + }) + .addCells(row.cells); // row processing steps 4/16 yCurrent++; @@ -574,11 +589,12 @@ export namespace Table { table = table.assignHeaders(); // Finally, we sort lists and export the result. - table = table.update({ - cells: [...table.cells].sort(compare), - rowGroups: [...table.rowGroups].sort(compare), - colGroups: [...table.colGroups].sort(compare), - }); + table = table + .update({ + rowGroups: [...table.rowGroups].sort(compare), + colGroups: [...table.colGroups].sort(compare), + }) + .updateCells([...table.cells].sort(compare)); return Ok.of(table); } } From c376c37293d64a54944fdc27f5c555b97dbff596 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Tue, 23 Jun 2020 15:38:04 +0200 Subject: [PATCH 10/37] Store left/top most data cell --- packages/alfa-table/src/table.ts | 57 ++++++++++++++++++++++++-------- 1 file changed, 43 insertions(+), 14 deletions(-) diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index e9a919e7ad..7bb175d494 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -18,7 +18,7 @@ import { RowGroup } from "./row-group"; import { Scope } from "./scope"; const { compare } = Comparable; -const { filter, map, some } = Iterable; +const { filter, map, reduce, some } = Iterable; /** * @see https://html.spec.whatwg.org/multipage/tables.html#table-processing-model @@ -132,7 +132,9 @@ export namespace Table { cells: Iterable = List.empty(), slots: Array>> = [[]], rowGroups: Iterable = List.empty(), - colGroups: Iterable = List.empty() + colGroups: Iterable = List.empty(), + leftmostDataCell: number = -1, + topmostDataCell: number = -1 ): Builder { return new Builder( element, @@ -141,7 +143,9 @@ export namespace Table { cells, slots, rowGroups, - colGroups + colGroups, + leftmostDataCell, + topmostDataCell ); } @@ -149,6 +153,8 @@ export namespace Table { private readonly _table: Table; private readonly _cells: List; private readonly _slots: Array>>; + private readonly _leftmostDataCell: number; // x anchor of the leftmost data cell + private readonly _topmostDataCell: number; // y anchor of the topmost data cell private constructor( element: Element, @@ -157,7 +163,9 @@ export namespace Table { cells: Iterable, slots: Array>>, rowGroups: Iterable, - colGroups: Iterable + colGroups: Iterable, + leftmostDataCell: number, + topmostDataCell: number ) { this._table = Table.of( element, @@ -169,6 +177,8 @@ export namespace Table { ); this._cells = List.from(cells); this._slots = slots; + this._leftmostDataCell = leftmostDataCell; + this._topmostDataCell = topmostDataCell; } public get cells(): Iterable { @@ -221,6 +231,8 @@ export namespace Table { slots = this._slots, rowGroups = this.rowGroups, colGroups = this.colGroups, + leftmostDataCell = this._leftmostDataCell, + topmostDataCell = this._topmostDataCell, }: { element?: Element; width?: number; @@ -229,6 +241,8 @@ export namespace Table { slots?: Array>>; rowGroups?: Iterable; colGroups?: Iterable; + leftmostDataCell?: number; + topmostDataCell?: number; }): Builder { return Builder.of( element, @@ -237,7 +251,9 @@ export namespace Table { cells, slots, rowGroups, - colGroups + colGroups, + leftmostDataCell, + topmostDataCell ); } @@ -263,6 +279,28 @@ export namespace Table { return this._updateUnsafe({ cells })._updateSlots(cells); } + /** + * Add new cells, sync slots with the new cells and update left/top most cells + */ + public addCells(cells: Iterable): Builder { + const leftmostDataCell = reduce( + cells, + (x, cell) => Math.min(x, cell.anchor.x), + this._leftmostDataCell + ); + const topmostDataCell = reduce( + cells, + (y, cell) => Math.min(y, cell.anchor.y), + this._topmostDataCell + ); + + return this._updateUnsafe({ + cells: this._cells.concat(cells), + leftmostDataCell, + topmostDataCell + })._updateSlots(cells); + } + private _updateSlots(cells: Iterable): Builder { for (const cell of cells) { for (let x = cell.anchor.x; x < cell.anchor.x + cell.width; x++) { @@ -278,15 +316,6 @@ export namespace Table { return this; // for chaining } - /** - * Add new cells, sync slots with the new cells and update left/top most cells - */ - public addCells(cells: Iterable): Builder { - return this._updateUnsafe({ - cells: this._cells.concat(cells), - })._updateSlots(cells); - } - public addRowGroupFromElement( rowgroup: Element, yCurrent: number From 4030c31024aef7a6f13e2a55a72317eb49bc504b Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Tue, 23 Jun 2020 16:09:01 +0200 Subject: [PATCH 11/37] Keep memory of column/row with data cells --- packages/alfa-table/src/cell.ts | 18 ++- packages/alfa-table/src/table.ts | 62 +++++----- packages/alfa-table/test/table.spec.ts | 154 ++++++++++++------------- 3 files changed, 125 insertions(+), 109 deletions(-) diff --git a/packages/alfa-table/src/cell.ts b/packages/alfa-table/src/cell.ts index 6d6a6bf626..f1bd2892f7 100644 --- a/packages/alfa-table/src/cell.ts +++ b/packages/alfa-table/src/cell.ts @@ -418,7 +418,9 @@ export namespace Cell { h: number ) => boolean, width: number, - height: number + height: number, + columnHasData: Array, + rowHasData: Array ): Option { switch (scope) { // https://html.spec.whatwg.org/multipage/tables.html#column-group-header @@ -439,6 +441,7 @@ export namespace Cell { // Not entirely clear whether "any of the cells covering slots with y-coordinates y .. y+height-1." // means "for any x" or just for the x of the cell. Using "for all x" if ( + // this.anchor.y + this.height-1 <= topmostDataCell existsDataCellCoveringArea(0, this.anchor.y, width, this.height) ) { // there are *SOME* data cells in any of the cells covering slots with y-coordinates y .. y+height-1. @@ -466,11 +469,20 @@ export namespace Cell { h: number ) => boolean, width: number, - height: number + height: number, + columnHasData: Array, + rowHasData: Array ): Builder { return this._update({ variant: this._scope.flatMap((scope) => - this._scopeToState(scope, existsDataCellCoveringArea, width, height) + this._scopeToState( + scope, + existsDataCellCoveringArea, + width, + height, + columnHasData, + rowHasData + ) ), }); } diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index 7bb175d494..592ff65af9 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -106,6 +106,7 @@ export class Table implements Equatable, Serializable { } export namespace Table { + import Kind = Cell.Kind; const cache = Cache.empty>(); export function from(element: Element): Result { @@ -133,8 +134,8 @@ export namespace Table { slots: Array>> = [[]], rowGroups: Iterable = List.empty(), colGroups: Iterable = List.empty(), - leftmostDataCell: number = -1, - topmostDataCell: number = -1 + columnHasData: Array = [], + rowHasData: Array = [] ): Builder { return new Builder( element, @@ -144,8 +145,8 @@ export namespace Table { slots, rowGroups, colGroups, - leftmostDataCell, - topmostDataCell + columnHasData, + rowHasData ); } @@ -153,8 +154,8 @@ export namespace Table { private readonly _table: Table; private readonly _cells: List; private readonly _slots: Array>>; - private readonly _leftmostDataCell: number; // x anchor of the leftmost data cell - private readonly _topmostDataCell: number; // y anchor of the topmost data cell + private readonly _columnHasData: Array; // Does column x contains a slot covered by a data cell? + private readonly _rowHasData: Array; // Does row y contains a slot covered by a data cell? private constructor( element: Element, @@ -164,8 +165,8 @@ export namespace Table { slots: Array>>, rowGroups: Iterable, colGroups: Iterable, - leftmostDataCell: number, - topmostDataCell: number + columnHasData: Array, + rowHasData: Array ) { this._table = Table.of( element, @@ -177,8 +178,8 @@ export namespace Table { ); this._cells = List.from(cells); this._slots = slots; - this._leftmostDataCell = leftmostDataCell; - this._topmostDataCell = topmostDataCell; + this._columnHasData = columnHasData; + this._rowHasData = rowHasData; } public get cells(): Iterable { @@ -231,8 +232,8 @@ export namespace Table { slots = this._slots, rowGroups = this.rowGroups, colGroups = this.colGroups, - leftmostDataCell = this._leftmostDataCell, - topmostDataCell = this._topmostDataCell, + columnHasData = this._columnHasData, + rowHasData = this._rowHasData, }: { element?: Element; width?: number; @@ -241,8 +242,8 @@ export namespace Table { slots?: Array>>; rowGroups?: Iterable; colGroups?: Iterable; - leftmostDataCell?: number; - topmostDataCell?: number; + columnHasData?: Array; + rowHasData?: Array; }): Builder { return Builder.of( element, @@ -252,8 +253,8 @@ export namespace Table { slots, rowGroups, colGroups, - leftmostDataCell, - topmostDataCell + columnHasData, + rowHasData ); } @@ -283,21 +284,22 @@ export namespace Table { * Add new cells, sync slots with the new cells and update left/top most cells */ public addCells(cells: Iterable): Builder { - const leftmostDataCell = reduce( - cells, - (x, cell) => Math.min(x, cell.anchor.x), - this._leftmostDataCell - ); - const topmostDataCell = reduce( - cells, - (y, cell) => Math.min(y, cell.anchor.y), - this._topmostDataCell - ); + for (const cell of cells) { + if (cell.kind === Kind.Data) { + // If this is a data cell, update the memory of column/row with data cells. + for (let x = cell.anchor.x; x < cell.anchor.x + cell.width; x++) { + this._columnHasData[x] = true; + } + for (let y = cell.anchor.y; y < cell.anchor.y + cell.height; y++) { + this._columnHasData[y] = true; + } + } + } return this._updateUnsafe({ cells: this._cells.concat(cells), - leftmostDataCell, - topmostDataCell + columnHasData: this._columnHasData, + rowHasData: this._rowHasData, })._updateSlots(cells); } @@ -357,7 +359,9 @@ export namespace Table { cell.addHeaderVariant( this.hasDataCellCoveringArea.bind(this), this.width, - this.height + this.height, + this._columnHasData, + this._rowHasData ) ) ) diff --git a/packages/alfa-table/test/table.spec.ts b/packages/alfa-table/test/table.spec.ts index af2602dccd..cbb41557d5 100644 --- a/packages/alfa-table/test/table.spec.ts +++ b/packages/alfa-table/test/table.spec.ts @@ -20,13 +20,13 @@ import { import { Cell } from "../src/cell"; test("Builder.from() computes correct header variants", (t) => { - const expensesTable = Table.Builder.from(expenses.element).get(); - t.deepEqual( - [...expensesTable.cells] - .filter((cell) => cell.kind === Cell.Kind.Header) - .map((cell) => cell.variant.get()), - expenses.headerVariants - ); + // const expensesTable = Table.Builder.from(expenses.element).get(); + // t.deepEqual( + // [...expensesTable.cells] + // .filter((cell) => cell.kind === Cell.Kind.Header) + // .map((cell) => cell.variant.get()), + // expenses.headerVariants + // ); const headersTable = Table.Builder.from(headersVariant.element).get(); t.deepEqual( @@ -37,73 +37,73 @@ test("Builder.from() computes correct header variants", (t) => { ); }); -test("from() builds correct table model", (t) => { - t.deepEqual( - Table.from(smithonian.element).get().toJSON(), - smithonian.expected.toJSON() - ); - - t.deepEqual( - Table.from(apple.element).get().toJSON(), - apple.expected.toJSON() - ); - - t.deepEqual( - Table.from(expenses.element).get().toJSON(), - expenses.expected.toJSON() - ); - - t.deepEqual( - Table.from(expensesNum.element).get().toJSON(), - expensesNum.expected.toJSON() - ); -}); - -test("from() detects table model errors", (t) => { - t.deepEqual( - Table.from(errors.emptyCol), - Err.of("col 1 has no cell anchored in it") - ); - t.deepEqual( - Table.from(errors.emptyRow), - Err.of("row 1 has no cell anchored in it") - ); - t.deepEqual( - Table.from(errors.coveredTwice), - Err.of("Slot (1, 1) is covered twice") - ); -}); - -test("from() computes header association of simple headers (no groups)", (t) => { - t.deepEqual( - Table.from(explicitHeaders.element).get().toJSON(), - explicitHeaders.expected.toJSON() - ); - - t.deepEqual( - Table.from(simpleImplicitHeaders.element).get().toJSON(), - simpleImplicitHeaders.expected.toJSON() - ); - - t.deepEqual( - Table.from(duplicateIDExplicitHeaders.table).get().toJSON(), - duplicateIDExplicitHeaders.expected.toJSON() - ); -}); - -test("from() computes header association of tables with groups headers", (t) => { - t.deepEqual( - Table.from(rowGroupImplicitHeaders.element).get().toJSON(), - rowGroupImplicitHeaders.expected.toJSON() - ); - - t.deepEqual( - Table.from(colGroupImplicitHeaders.element).get().toJSON(), - colGroupImplicitHeaders.expected.toJSON() - ); - - t.deepEqual( - Table.from(allWeirdImplicitHeaders.element).get().toJSON(), - allWeirdImplicitHeaders.expected.toJSON() - ); -}); +// test("from() builds correct table model", (t) => { +// t.deepEqual( +// Table.from(smithonian.element).get().toJSON(), +// smithonian.expected.toJSON() +// ); +// +// t.deepEqual( +// Table.from(apple.element).get().toJSON(), +// apple.expected.toJSON() +// ); +// +// t.deepEqual( +// Table.from(expenses.element).get().toJSON(), +// expenses.expected.toJSON() +// ); +// +// t.deepEqual( +// Table.from(expensesNum.element).get().toJSON(), +// expensesNum.expected.toJSON() +// ); +// }); +// +// test("from() detects table model errors", (t) => { +// t.deepEqual( +// Table.from(errors.emptyCol), +// Err.of("col 1 has no cell anchored in it") +// ); +// t.deepEqual( +// Table.from(errors.emptyRow), +// Err.of("row 1 has no cell anchored in it") +// ); +// t.deepEqual( +// Table.from(errors.coveredTwice), +// Err.of("Slot (1, 1) is covered twice") +// ); +// }); +// +// test("from() computes header association of simple headers (no groups)", (t) => { +// t.deepEqual( +// Table.from(explicitHeaders.element).get().toJSON(), +// explicitHeaders.expected.toJSON() +// ); +// +// t.deepEqual( +// Table.from(simpleImplicitHeaders.element).get().toJSON(), +// simpleImplicitHeaders.expected.toJSON() +// ); +// +// t.deepEqual( +// Table.from(duplicateIDExplicitHeaders.table).get().toJSON(), +// duplicateIDExplicitHeaders.expected.toJSON() +// ); +// }); +// +// test("from() computes header association of tables with groups headers", (t) => { +// t.deepEqual( +// Table.from(rowGroupImplicitHeaders.element).get().toJSON(), +// rowGroupImplicitHeaders.expected.toJSON() +// ); +// +// t.deepEqual( +// Table.from(colGroupImplicitHeaders.element).get().toJSON(), +// colGroupImplicitHeaders.expected.toJSON() +// ); +// +// t.deepEqual( +// Table.from(allWeirdImplicitHeaders.element).get().toJSON(), +// allWeirdImplicitHeaders.expected.toJSON() +// ); +// }); From c9016a0c756be969143c7402c1a18c3188e61519 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Wed, 24 Jun 2020 09:17:40 +0200 Subject: [PATCH 12/37] Use stored data cells in rows/columns --- packages/alfa-table/src/cell.ts | 72 +++--------- packages/alfa-table/src/table.ts | 19 +-- packages/alfa-table/test/table.spec.ts | 154 ++++++++++++------------- 3 files changed, 95 insertions(+), 150 deletions(-) diff --git a/packages/alfa-table/src/cell.ts b/packages/alfa-table/src/cell.ts index f1bd2892f7..600e28cd1d 100644 --- a/packages/alfa-table/src/cell.ts +++ b/packages/alfa-table/src/cell.ts @@ -384,44 +384,24 @@ export namespace Cell { * see @https://html.spec.whatwg.org/multipage/tables.html#column-header * and following */ - private _isCoveringArea( - x: number, - y: number, - w: number, - h: number - ): boolean { - for (let col = x; col < x + w; col++) { - for (let row = y; row < y + h; row++) { - if (this.isCovering(col, row)) { - return true; - } - } - } - return false; - } - - public isDataCoveringArea( - x: number, - y: number, - w: number, - h: number - ): boolean { - return this.kind === Cell.Kind.Data && this._isCoveringArea(x, y, w, h); - } - private _scopeToState( scope: Scope, - existsDataCellCoveringArea: ( - x: number, - y: number, - w: number, - h: number - ) => boolean, - width: number, - height: number, columnHasData: Array, rowHasData: Array ): Option { + let dataInColumns = false; + let dataInRows = false; + for (let x = this.anchor.x; x < this.anchor.x + this.width; x++) { + if (columnHasData[x] ?? false) { + dataInColumns = true; + } + } + for (let y = this.anchor.y; y < this.anchor.y + this.height; y++) { + if (rowHasData[y] ?? false) { + dataInRows = true; + } + } + switch (scope) { // https://html.spec.whatwg.org/multipage/tables.html#column-group-header case Scope.ColumnGroup: @@ -440,14 +420,9 @@ export namespace Cell { case Scope.Auto: // Not entirely clear whether "any of the cells covering slots with y-coordinates y .. y+height-1." // means "for any x" or just for the x of the cell. Using "for all x" - if ( - // this.anchor.y + this.height-1 <= topmostDataCell - existsDataCellCoveringArea(0, this.anchor.y, width, this.height) - ) { + if (dataInRows) { // there are *SOME* data cells in any of the cells covering slots with y-coordinates y .. y+height-1. - if ( - existsDataCellCoveringArea(this.anchor.x, 0, this.width, height) - ) { + if (dataInColumns) { // there are *SOME* data cells in any of the cells covering slots with x-coordinates x .. x+width-1. return None; } else { @@ -462,27 +437,12 @@ export namespace Cell { } public addHeaderVariant( - existsDataCellCoveringArea: ( - x: number, - y: number, - w: number, - h: number - ) => boolean, - width: number, - height: number, columnHasData: Array, rowHasData: Array ): Builder { return this._update({ variant: this._scope.flatMap((scope) => - this._scopeToState( - scope, - existsDataCellCoveringArea, - width, - height, - columnHasData, - rowHasData - ) + this._scopeToState(scope, columnHasData, rowHasData) ), }); } diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index 592ff65af9..806698627f 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -291,7 +291,7 @@ export namespace Table { this._columnHasData[x] = true; } for (let y = cell.anchor.y; y < cell.anchor.y + cell.height; y++) { - this._columnHasData[y] = true; + this._rowHasData[y] = true; } } } @@ -343,26 +343,11 @@ export namespace Table { }); } - public hasDataCellCoveringArea( - x: number, - y: number, - w: number, - h: number - ): boolean { - return some(this.cells, (cell) => cell.isDataCoveringArea(x, y, w, h)); - } - public addHeadersVariants(): Builder { return this.updateCells( List.from( map(this.cells, (cell) => - cell.addHeaderVariant( - this.hasDataCellCoveringArea.bind(this), - this.width, - this.height, - this._columnHasData, - this._rowHasData - ) + cell.addHeaderVariant(this._columnHasData, this._rowHasData) ) ) ); diff --git a/packages/alfa-table/test/table.spec.ts b/packages/alfa-table/test/table.spec.ts index cbb41557d5..af2602dccd 100644 --- a/packages/alfa-table/test/table.spec.ts +++ b/packages/alfa-table/test/table.spec.ts @@ -20,13 +20,13 @@ import { import { Cell } from "../src/cell"; test("Builder.from() computes correct header variants", (t) => { - // const expensesTable = Table.Builder.from(expenses.element).get(); - // t.deepEqual( - // [...expensesTable.cells] - // .filter((cell) => cell.kind === Cell.Kind.Header) - // .map((cell) => cell.variant.get()), - // expenses.headerVariants - // ); + const expensesTable = Table.Builder.from(expenses.element).get(); + t.deepEqual( + [...expensesTable.cells] + .filter((cell) => cell.kind === Cell.Kind.Header) + .map((cell) => cell.variant.get()), + expenses.headerVariants + ); const headersTable = Table.Builder.from(headersVariant.element).get(); t.deepEqual( @@ -37,73 +37,73 @@ test("Builder.from() computes correct header variants", (t) => { ); }); -// test("from() builds correct table model", (t) => { -// t.deepEqual( -// Table.from(smithonian.element).get().toJSON(), -// smithonian.expected.toJSON() -// ); -// -// t.deepEqual( -// Table.from(apple.element).get().toJSON(), -// apple.expected.toJSON() -// ); -// -// t.deepEqual( -// Table.from(expenses.element).get().toJSON(), -// expenses.expected.toJSON() -// ); -// -// t.deepEqual( -// Table.from(expensesNum.element).get().toJSON(), -// expensesNum.expected.toJSON() -// ); -// }); -// -// test("from() detects table model errors", (t) => { -// t.deepEqual( -// Table.from(errors.emptyCol), -// Err.of("col 1 has no cell anchored in it") -// ); -// t.deepEqual( -// Table.from(errors.emptyRow), -// Err.of("row 1 has no cell anchored in it") -// ); -// t.deepEqual( -// Table.from(errors.coveredTwice), -// Err.of("Slot (1, 1) is covered twice") -// ); -// }); -// -// test("from() computes header association of simple headers (no groups)", (t) => { -// t.deepEqual( -// Table.from(explicitHeaders.element).get().toJSON(), -// explicitHeaders.expected.toJSON() -// ); -// -// t.deepEqual( -// Table.from(simpleImplicitHeaders.element).get().toJSON(), -// simpleImplicitHeaders.expected.toJSON() -// ); -// -// t.deepEqual( -// Table.from(duplicateIDExplicitHeaders.table).get().toJSON(), -// duplicateIDExplicitHeaders.expected.toJSON() -// ); -// }); -// -// test("from() computes header association of tables with groups headers", (t) => { -// t.deepEqual( -// Table.from(rowGroupImplicitHeaders.element).get().toJSON(), -// rowGroupImplicitHeaders.expected.toJSON() -// ); -// -// t.deepEqual( -// Table.from(colGroupImplicitHeaders.element).get().toJSON(), -// colGroupImplicitHeaders.expected.toJSON() -// ); -// -// t.deepEqual( -// Table.from(allWeirdImplicitHeaders.element).get().toJSON(), -// allWeirdImplicitHeaders.expected.toJSON() -// ); -// }); +test("from() builds correct table model", (t) => { + t.deepEqual( + Table.from(smithonian.element).get().toJSON(), + smithonian.expected.toJSON() + ); + + t.deepEqual( + Table.from(apple.element).get().toJSON(), + apple.expected.toJSON() + ); + + t.deepEqual( + Table.from(expenses.element).get().toJSON(), + expenses.expected.toJSON() + ); + + t.deepEqual( + Table.from(expensesNum.element).get().toJSON(), + expensesNum.expected.toJSON() + ); +}); + +test("from() detects table model errors", (t) => { + t.deepEqual( + Table.from(errors.emptyCol), + Err.of("col 1 has no cell anchored in it") + ); + t.deepEqual( + Table.from(errors.emptyRow), + Err.of("row 1 has no cell anchored in it") + ); + t.deepEqual( + Table.from(errors.coveredTwice), + Err.of("Slot (1, 1) is covered twice") + ); +}); + +test("from() computes header association of simple headers (no groups)", (t) => { + t.deepEqual( + Table.from(explicitHeaders.element).get().toJSON(), + explicitHeaders.expected.toJSON() + ); + + t.deepEqual( + Table.from(simpleImplicitHeaders.element).get().toJSON(), + simpleImplicitHeaders.expected.toJSON() + ); + + t.deepEqual( + Table.from(duplicateIDExplicitHeaders.table).get().toJSON(), + duplicateIDExplicitHeaders.expected.toJSON() + ); +}); + +test("from() computes header association of tables with groups headers", (t) => { + t.deepEqual( + Table.from(rowGroupImplicitHeaders.element).get().toJSON(), + rowGroupImplicitHeaders.expected.toJSON() + ); + + t.deepEqual( + Table.from(colGroupImplicitHeaders.element).get().toJSON(), + colGroupImplicitHeaders.expected.toJSON() + ); + + t.deepEqual( + Table.from(allWeirdImplicitHeaders.element).get().toJSON(), + allWeirdImplicitHeaders.expected.toJSON() + ); +}); From bcfa4444a4f5540c5edc3abff968c275766da624 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Wed, 24 Jun 2020 09:58:48 +0200 Subject: [PATCH 13/37] Move some table logic out of Cell.Builder --- packages/alfa-cli/package.json | 3 +++ packages/alfa-table/src/cell.ts | 23 +++++------------------ packages/alfa-table/src/table.ts | 23 +++++++++++++++++++---- 3 files changed, 27 insertions(+), 22 deletions(-) diff --git a/packages/alfa-cli/package.json b/packages/alfa-cli/package.json index 99fc5c6762..6d170e462d 100644 --- a/packages/alfa-cli/package.json +++ b/packages/alfa-cli/package.json @@ -46,5 +46,8 @@ "publishConfig": { "access": "public", "registry": "https://npm.pkg.github.com/" + }, + "scripts": { + "test": "node script.js" } } diff --git a/packages/alfa-table/src/cell.ts b/packages/alfa-table/src/cell.ts index 600e28cd1d..bea58442b9 100644 --- a/packages/alfa-table/src/cell.ts +++ b/packages/alfa-table/src/cell.ts @@ -386,22 +386,9 @@ export namespace Cell { */ private _scopeToState( scope: Scope, - columnHasData: Array, - rowHasData: Array + dataInColumns: boolean, // Is there a data cell in the columns covered by this? + dataInRows: boolean // Is there a data cell in the rows covered by this? ): Option { - let dataInColumns = false; - let dataInRows = false; - for (let x = this.anchor.x; x < this.anchor.x + this.width; x++) { - if (columnHasData[x] ?? false) { - dataInColumns = true; - } - } - for (let y = this.anchor.y; y < this.anchor.y + this.height; y++) { - if (rowHasData[y] ?? false) { - dataInRows = true; - } - } - switch (scope) { // https://html.spec.whatwg.org/multipage/tables.html#column-group-header case Scope.ColumnGroup: @@ -437,12 +424,12 @@ export namespace Cell { } public addHeaderVariant( - columnHasData: Array, - rowHasData: Array + dataInColumns: boolean, // Is there a data cell in the columns covered by this? + dataInRows: boolean // Is there a data cell in the rows covered by this? ): Builder { return this._update({ variant: this._scope.flatMap((scope) => - this._scopeToState(scope, columnHasData, rowHasData) + this._scopeToState(scope, dataInColumns, dataInRows) ), }); } diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index 806698627f..4ec1405252 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -274,14 +274,15 @@ export namespace Table { /** * Update cells, and resync slots - * Cells are assumed to keep the same anchors, hence left/top most ones don't change. + * Cells are assumed to keep the same anchors, width, height and kind (header/data), + * hence columnHasData and rowHasData don't change but slots need to be updated to the new version of their cell. */ public updateCells(cells: Iterable): Builder { return this._updateUnsafe({ cells })._updateSlots(cells); } /** - * Add new cells, sync slots with the new cells and update left/top most cells + * Add new cells, sync slots with the new cells and update columnHasData and rowHasData. */ public addCells(cells: Iterable): Builder { for (const cell of cells) { @@ -346,8 +347,22 @@ export namespace Table { public addHeadersVariants(): Builder { return this.updateCells( List.from( - map(this.cells, (cell) => - cell.addHeaderVariant(this._columnHasData, this._rowHasData) + map(this.cells, (cell) => { + let dataInColumns = false; + let dataInRows = false; + for (let x = cell.anchor.x; x < cell.anchor.x + cell.width; x++) { + if (this._columnHasData[x] ?? false) { + dataInColumns = true; + } + } + for (let y = cell.anchor.y; y < cell.anchor.y + cell.height; y++) { + if (this._rowHasData[y] ?? false) { + dataInRows = true; + } + } + + return cell.addHeaderVariant(dataInColumns, dataInRows) + } ) ) ); From 652f5f50c81b6896a43bc387364c3c20aab71773 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Wed, 24 Jun 2020 10:48:47 +0200 Subject: [PATCH 14/37] Store slot coverage in Row.Builder --- packages/alfa-table/src/row.ts | 113 ++++++++++++++++++++++++++++--- packages/alfa-table/src/table.ts | 2 +- 2 files changed, 104 insertions(+), 11 deletions(-) diff --git a/packages/alfa-table/src/row.ts b/packages/alfa-table/src/row.ts index 88c9da0fbb..a434715491 100644 --- a/packages/alfa-table/src/row.ts +++ b/packages/alfa-table/src/row.ts @@ -4,12 +4,16 @@ import { Equatable } from "@siteimprove/alfa-equatable"; import { Iterable } from "@siteimprove/alfa-iterable"; import { Serializable } from "@siteimprove/alfa-json"; import { List } from "@siteimprove/alfa-list"; +import { None, Option, Some } from "@siteimprove/alfa-option"; import { Err, Ok, Result } from "@siteimprove/alfa-result"; import * as json from "@siteimprove/alfa-json"; import { Cell } from "./cell"; +import { ColumnGroup } from "./column-group"; import { isHtmlElementWithName } from "./helpers"; +import { RowGroup } from "./row-group"; +// import { Builder } from "./table"; const { compare } = Comparable; const { some } = Iterable; @@ -25,6 +29,8 @@ const { some } = Iterable; * as long as they are all based in the same way… */ export namespace Row { + import concat = Iterable.concat; + export class Builder implements Equatable, Serializable { public static of( y: number, @@ -33,9 +39,19 @@ export namespace Row { element: Element, cells: Iterable = List.empty(), growing: Iterable = List.empty(), - xCurrent: number = 0 + xCurrent: number = 0, + slots: Array>> = [[]] ): Builder { - return new Builder(y, width, height, element, cells, growing, xCurrent); + return new Builder( + y, + width, + height, + element, + cells, + growing, + xCurrent, + slots + ); } private readonly _y: number; @@ -45,6 +61,8 @@ export namespace Row { private readonly _element: Element; private readonly _cells: List; private readonly _downwardGrowingCells: List; + // Cell covering a given slot, either from this._cells or this._downwardGrowingCells + private readonly _slots: Array>>; private constructor( y: number, @@ -53,7 +71,8 @@ export namespace Row { element: Element, cells: Iterable, growing: Iterable, - xCurrent: number + xCurrent: number, + slots: Array>> ) { this._y = y; this._xCurrent = xCurrent; @@ -62,6 +81,7 @@ export namespace Row { this._element = element; this._cells = List.from(cells); this._downwardGrowingCells = List.from(growing); + this._slots = slots; } public get anchor(): { y: number } { @@ -88,7 +108,14 @@ export namespace Row { return this._downwardGrowingCells; } - private _update({ + public slot(x: number, y: number): Option { + return this._slots?.[x]?.[y] === undefined ? None : this._slots[x][y]; + } + + /** + * Update by getting new values. Does not keep slots in sync, hence is highly unsafe. Use at your own risks. + */ + private _updateUnsafe({ y = this._y, xCurrent = this._xCurrent, width = this._width, @@ -96,6 +123,7 @@ export namespace Row { element = this._element, cells = this._cells, downwardGrowingCells = this._downwardGrowingCells, + slots = this._slots, }: { y?: number; xCurrent?: number; @@ -104,6 +132,7 @@ export namespace Row { element?: Element; cells?: Iterable; downwardGrowingCells?: Iterable; + slots?: Array>>; }): Builder { return Builder.of( y, @@ -112,25 +141,89 @@ export namespace Row { element, cells, downwardGrowingCells, - xCurrent + xCurrent, + slots ); } + /** + * Update anything but cells/downward growing cells, because these need to be kept in sync with slots. + */ + private _update(update: { + y?: number; + xCurrent?: number; + width?: number; + height?: number; + element?: Element; + slots?: Array>>; + }): Builder { + return this._updateUnsafe(update); + } + + /** + * Update cells, and resync slots + */ + private _updateCells({ + cells = List.empty(), + downwardGrowingCells = List.empty(), + }: { + cells?: Iterable; + downwardGrowingCells?: Iterable; + }): Builder { + return this._updateUnsafe({ cells, downwardGrowingCells })._updateSlots( + concat(cells, downwardGrowingCells) + ); + } + + /** + * Add new cells, and sync slots with the new cells. + */ + private _addCells({ + cells = List.empty(), + downwardGrowingCells = List.empty(), + }: { + cells?: Iterable; + downwardGrowingCells?: Iterable; + }): Builder { + return this._updateUnsafe({ + cells: this._cells.concat(cells), + downwardGrowingCells: this._downwardGrowingCells.concat( + downwardGrowingCells + ), + })._updateSlots(concat(cells, downwardGrowingCells)); + } + + private _updateSlots(cells: Iterable): Builder { + for (const cell of cells) { + for (let x = cell.anchor.x; x < cell.anchor.x + cell.width; x++) { + if (this._slots[x] === undefined) { + this._slots[x] = []; + } + for (let y = cell.anchor.y; y < cell.anchor.y + cell.height; y++) { + this._slots[x][y] = Some.of(cell); + } + } + } + + return this; // for chaining + } + public growCells(y: number): Builder { - return this._update({ + return this._updateCells({ downwardGrowingCells: this._downwardGrowingCells.map((cell) => cell.growDownward(y) ), }); } + // TODO make private ×n (or remove) public addNonGrowingCell(cell: Cell.Builder): Builder { - return this._update({ cells: this._cells.append(cell) }); + return this._addCells({ cells: [cell] }); } public addGrowingCell(cell: Cell.Builder): Builder { - return this._update({ - downwardGrowingCells: this._downwardGrowingCells.append(cell), + return this._addCells({ + downwardGrowingCells: [cell], }); } @@ -196,7 +289,7 @@ export namespace Row { } public sort(): Builder { - return this._update({ + return this._updateCells({ cells: [...this._cells].sort(compare), downwardGrowingCells: [...this._downwardGrowingCells].sort(compare), }); diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index 4ec1405252..fc7403007c 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -222,7 +222,7 @@ export namespace Table { } /** - * Update by getting new value. Does not keep anything in sync, hence is highly unsafe. Use at your own risks. + * Update by getting new values. Does not keep anything in sync, hence is highly unsafe. Use at your own risks. */ private _updateUnsafe({ element = this.element, From 10ccc9ed30da30bf894b9a13786f13aa0ef91014 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Wed, 24 Jun 2020 10:49:31 +0200 Subject: [PATCH 15/37] Clean up --- packages/alfa-table/src/row.ts | 3 --- packages/alfa-table/src/table.ts | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/alfa-table/src/row.ts b/packages/alfa-table/src/row.ts index a434715491..9ebfae770a 100644 --- a/packages/alfa-table/src/row.ts +++ b/packages/alfa-table/src/row.ts @@ -10,10 +10,7 @@ import { Err, Ok, Result } from "@siteimprove/alfa-result"; import * as json from "@siteimprove/alfa-json"; import { Cell } from "./cell"; -import { ColumnGroup } from "./column-group"; import { isHtmlElementWithName } from "./helpers"; -import { RowGroup } from "./row-group"; -// import { Builder } from "./table"; const { compare } = Comparable; const { some } = Iterable; diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index fc7403007c..7573f8ed03 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -18,7 +18,7 @@ import { RowGroup } from "./row-group"; import { Scope } from "./scope"; const { compare } = Comparable; -const { filter, map, reduce, some } = Iterable; +const { filter, map, some } = Iterable; /** * @see https://html.spec.whatwg.org/multipage/tables.html#table-processing-model From 77956c553b98655f68e528d9c56ad830f8c75062 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Wed, 24 Jun 2020 10:49:57 +0200 Subject: [PATCH 16/37] Clean up --- packages/alfa-table/src/row.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/alfa-table/src/row.ts b/packages/alfa-table/src/row.ts index 9ebfae770a..c0d58e93f4 100644 --- a/packages/alfa-table/src/row.ts +++ b/packages/alfa-table/src/row.ts @@ -13,7 +13,7 @@ import { Cell } from "./cell"; import { isHtmlElementWithName } from "./helpers"; const { compare } = Comparable; -const { some } = Iterable; +const { concat, some } = Iterable; /** * Build artifact, corresponds to a single element @@ -26,8 +26,6 @@ const { some } = Iterable; * as long as they are all based in the same way… */ export namespace Row { - import concat = Iterable.concat; - export class Builder implements Equatable, Serializable { public static of( y: number, From 715f84c8a949b9b0f9f7e06bbfcf6029f8ef5bfd Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Wed, 24 Jun 2020 11:20:42 +0200 Subject: [PATCH 17/37] Eta-contract call --- packages/alfa-table/src/table.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index 7573f8ed03..ee31cc21a5 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -361,9 +361,8 @@ export namespace Table { } } - return cell.addHeaderVariant(dataInColumns, dataInRows) - } - ) + return cell.addHeaderVariant(dataInColumns, dataInRows); + }) ) ); } @@ -422,7 +421,7 @@ export namespace Table { map(this.cells, (cell) => cell.assignHeaders( this.element, - (x: number, y: number) => this._slots[x][y], + this.slot.bind(this), this.getAboveLeftGroupHeaders("row"), this.getAboveLeftGroupHeaders("column") ) From d63bfe3382b73fc54f8ae55acc1eb0e707dfba87 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Wed, 24 Jun 2020 11:26:56 +0200 Subject: [PATCH 18/37] Clean up --- packages/alfa-table/src/row-group.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/alfa-table/src/row-group.ts b/packages/alfa-table/src/row-group.ts index ab0d08d6ce..8d48419844 100644 --- a/packages/alfa-table/src/row-group.ts +++ b/packages/alfa-table/src/row-group.ts @@ -211,11 +211,7 @@ export namespace RowGroup { * @see https://html.spec.whatwg.org/multipage/tables.html#algorithm-for-processing-row-groups */ export function from(group: Element): Result { - if ( - group.name !== "tfoot" && - group.name !== "tbody" && - group.name !== "thead" - ) { + if (!Element.hasName("tfoot", "tbody", "thead")(group)) { return Err.of("This element is not a row group"); } From 81e39b1a95a38cd8ee8a21f4302af4695e7a3504 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Wed, 24 Jun 2020 11:43:51 +0200 Subject: [PATCH 19/37] Store slot coverage in row groups --- packages/alfa-table/src/row-group.ts | 170 +++++++++++++++++++++------ packages/alfa-table/src/row.ts | 10 +- packages/alfa-table/src/table.ts | 5 +- 3 files changed, 147 insertions(+), 38 deletions(-) diff --git a/packages/alfa-table/src/row-group.ts b/packages/alfa-table/src/row-group.ts index 8d48419844..45f77cfe2f 100644 --- a/packages/alfa-table/src/row-group.ts +++ b/packages/alfa-table/src/row-group.ts @@ -1,8 +1,10 @@ import { Comparable, Comparison } from "@siteimprove/alfa-comparable"; import { Element } from "@siteimprove/alfa-dom"; import { Equatable } from "@siteimprove/alfa-equatable"; +import { Iterable } from "@siteimprove/alfa-iterable"; import { Serializable } from "@siteimprove/alfa-json"; import { List } from "@siteimprove/alfa-list"; +import { None, Option, Some } from "@siteimprove/alfa-option"; import { Err, Ok, Result } from "@siteimprove/alfa-result"; import * as json from "@siteimprove/alfa-json"; @@ -12,6 +14,7 @@ import { isHtmlElementWithName } from "./helpers"; import { Row } from "./row"; const { compare } = Comparable; +const { concat } = Iterable; /** * @see https://html.spec.whatwg.org/multipage/tables.html#concept-row-group @@ -105,46 +108,47 @@ export namespace RowGroup { * The row group builder contains width and cells list that will be merged with parent table once done. */ export class Builder implements Equatable, Serializable { - private readonly _width: number; - private readonly _cells: List; - private readonly _rowGroup: RowGroup; - public static of( y: number, height: number, element: Element, width: number = 0, - cells: Iterable = List.empty() + cells: Iterable = List.empty(), + downwardGrowingCells: Iterable = List.empty(), + slots: Array>> = [[]] ): Builder { - return new Builder(y, height, element, width, cells); + return new Builder( + y, + height, + element, + width, + cells, + downwardGrowingCells, + slots + ); } + private readonly _width: number; + private readonly _cells: List; + private readonly _downwardGrowingCells: List; + private readonly _rowGroup: RowGroup; + // Cell covering a given slot, either from this._cells or this._downwardGrowingCells + private readonly _slots: Array>>; + constructor( y: number, height: number, element: Element, width: number, - cells: Iterable + cells: Iterable, + downwardGrowingCells: Iterable, + slots: Array>> ) { this._rowGroup = RowGroup.of(y, height, element); this._width = width; this._cells = List.from(cells); - } - - public update({ - y = this._rowGroup.anchor.y, - width = this._width, - height = this._rowGroup.height, - element = this._rowGroup.element, - cells = this._cells, - }: { - y?: number; - width?: number; - height?: number; - element?: Element; - cells?: Iterable; - }): Builder { - return Builder.of(y, height, element, width, cells); + this._downwardGrowingCells = List.from(downwardGrowingCells); + this._slots = slots; } public get rowgroup(): RowGroup { @@ -171,14 +175,111 @@ export namespace RowGroup { return this._rowGroup.element; } + public slot(x: number, y: number): Option { + return this._slots?.[x]?.[y] === undefined ? None : this._slots[x][y]; + } + + /** + * Update by getting new values. Does not keep slots in sync, hence is highly unsafe. Use at your own risks. + */ + private _updateUnsafe({ + y = this._rowGroup.anchor.y, + width = this._width, + height = this._rowGroup.height, + element = this._rowGroup.element, + cells = this._cells, + downwardGrowingCells = this._downwardGrowingCells, + slots = this._slots, + }: { + y?: number; + width?: number; + height?: number; + element?: Element; + cells?: Iterable; + downwardGrowingCells?: Iterable; + slots?: Array>>; + }): Builder { + return Builder.of( + y, + height, + element, + width, + cells, + downwardGrowingCells, + slots + ); + } + + /** + * Update anything but cells/downward growing cells, because these need to be kept in sync with slots. + */ + public update(update: { + y?: number; + width?: number; + height?: number; + element?: Element; + }): Builder { + return this._updateUnsafe(update); + } + + /** + * Update cells/downward growing cells, and resync slots with all (updated) cells + */ + public updateCells({ + cells = List.empty(), + downwardGrowingCells = List.empty(), + }: { + cells?: Iterable; + downwardGrowingCells?: Iterable; + }): Builder { + return this._updateUnsafe({ cells, downwardGrowingCells })._updateSlots( + concat(cells, downwardGrowingCells) + ); + } + + /** + * Add new cells/downward growing cells, and sync slots with the new cells only. + */ + private _addCells({ + cells = List.empty(), + downwardGrowingCells = List.empty(), + }: { + cells?: Iterable; + downwardGrowingCells?: Iterable; + }): Builder { + return this._updateUnsafe({ + cells: this._cells.concat(cells), + downwardGrowingCells: this._downwardGrowingCells.concat( + downwardGrowingCells + ), + })._updateSlots(concat(cells, downwardGrowingCells)); + } + + /** + * Resync slots with a given list of cells. Caller need to ensure that all updated/added cells are passed. + */ + private _updateSlots(cells: Iterable): Builder { + for (const cell of cells) { + for (let x = cell.anchor.x; x < cell.anchor.x + cell.width; x++) { + if (this._slots[x] === undefined) { + this._slots[x] = []; + } + for (let y = cell.anchor.y; y < cell.anchor.y + cell.height; y++) { + this._slots[x][y] = Some.of(cell); + } + } + } + + return this; // for chaining + } + // anchoring a row group needs to move down all cells accordingly public anchorAt(y: number): Builder { - return this.update({ - y, + return this.updateCells({ cells: this._cells.map((cell) => cell.anchorAt(cell.anchor.x, y + cell.anchor.y) ), - }); + }).update({ y }); } public equals(value: unknown): value is this { @@ -230,11 +331,14 @@ export namespace RowGroup { rowgroup.width ).get(); growingCellsList = List.from(row.downwardGrowingCells); - rowgroup = rowgroup.update({ - cells: List.from(rowgroup.cells).concat(row.cells), - height: Math.max(rowgroup.height, yCurrent + row.height), - width: Math.max(rowgroup.width, row.width), - }); + rowgroup = rowgroup + .updateCells({ + cells: List.from(rowgroup.cells).concat(row.cells), + }) + .update({ + height: Math.max(rowgroup.height, yCurrent + row.height), + width: Math.max(rowgroup.width, row.width), + }); // row processing steps 4/16 yCurrent++; } @@ -245,13 +349,13 @@ export namespace RowGroup { ); // ending row group 2 // When emptying the growing cells list, we need to finally add them to the group. - rowgroup = rowgroup.update({ + rowgroup = rowgroup.updateCells({ cells: List.from(rowgroup.cells).concat(growingCellsList), }); // 3, returning the row group for the table to handle // we could check here if height>0 and return an option, to be closer to the algorithm but that would be less uniform. return Ok.of( - rowgroup.update({ cells: [...rowgroup.cells].sort(compare) }) + rowgroup.updateCells({ cells: [...rowgroup.cells].sort(compare) }) ); } } diff --git a/packages/alfa-table/src/row.ts b/packages/alfa-table/src/row.ts index c0d58e93f4..3a9d78e48c 100644 --- a/packages/alfa-table/src/row.ts +++ b/packages/alfa-table/src/row.ts @@ -142,7 +142,7 @@ export namespace Row { } /** - * Update anything but cells/downward growing cells, because these need to be kept in sync with slots. + * Update anything but cells/downward growing cells/slots, because these need to be kept in sync. */ private _update(update: { y?: number; @@ -150,13 +150,12 @@ export namespace Row { width?: number; height?: number; element?: Element; - slots?: Array>>; }): Builder { return this._updateUnsafe(update); } /** - * Update cells, and resync slots + * Update cells/downward growing cells, and resync slots with all (updated) cells */ private _updateCells({ cells = List.empty(), @@ -171,7 +170,7 @@ export namespace Row { } /** - * Add new cells, and sync slots with the new cells. + * Add new cells/downward growing cells, and sync slots with the new cells only. */ private _addCells({ cells = List.empty(), @@ -188,6 +187,9 @@ export namespace Row { })._updateSlots(concat(cells, downwardGrowingCells)); } + /** + * Resync slots with a given list of cells. Caller need to ensure that all updated/added cells are passed. + */ private _updateSlots(cells: Iterable): Builder { for (const cell of cells) { for (let x = cell.anchor.x; x < cell.anchor.x + cell.width; x++) { diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index ee31cc21a5..7c19d505fb 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -259,7 +259,7 @@ export namespace Table { } /** - * Update anything but cells, because cells need to be kept in sync. + * Update anything but cells/slots, because they need to be kept in sync. */ public update(update: { element?: Element; @@ -304,6 +304,9 @@ export namespace Table { })._updateSlots(cells); } + /** + * Resync slots with a given list of cells. Caller need to ensure that all updated/added cells are passed. + */ private _updateSlots(cells: Iterable): Builder { for (const cell of cells) { for (let x = cell.anchor.x; x < cell.anchor.x + cell.width; x++) { From 190ea6ed9d4869c7e196a2926901c7c511ee6f29 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Wed, 24 Jun 2020 13:37:43 +0200 Subject: [PATCH 20/37] Try to store growing cells list --- packages/alfa-table/src/row-group.ts | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/packages/alfa-table/src/row-group.ts b/packages/alfa-table/src/row-group.ts index 45f77cfe2f..b03172f81c 100644 --- a/packages/alfa-table/src/row-group.ts +++ b/packages/alfa-table/src/row-group.ts @@ -14,7 +14,7 @@ import { isHtmlElementWithName } from "./helpers"; import { Row } from "./row"; const { compare } = Comparable; -const { concat } = Iterable; +const { concat, map } = Iterable; /** * @see https://html.spec.whatwg.org/multipage/tables.html#concept-row-group @@ -163,6 +163,10 @@ export namespace RowGroup { return this._cells; } + public get downwardGrowingCells(): Iterable { + return this._downwardGrowingCells; + } + public get anchor(): { y: number } { return this._rowGroup.anchor; } @@ -240,7 +244,7 @@ export namespace RowGroup { /** * Add new cells/downward growing cells, and sync slots with the new cells only. */ - private _addCells({ + public addCells({ cells = List.empty(), downwardGrowingCells = List.empty(), }: { @@ -279,6 +283,9 @@ export namespace RowGroup { cells: this._cells.map((cell) => cell.anchorAt(cell.anchor.x, y + cell.anchor.y) ), + downwardGrowingCells: this._downwardGrowingCells.map((cell) => + cell.anchorAt(cell.anchor.x, y + cell.anchor.y) + ), }).update({ y }); } @@ -327,18 +334,20 @@ export namespace RowGroup { tr, rowgroup.cells, growingCellsList, + // rowgroup.downwardGrowingCells, yCurrent, rowgroup.width ).get(); + growingCellsList = List.from(row.downwardGrowingCells); rowgroup = rowgroup - .updateCells({ - cells: List.from(rowgroup.cells).concat(row.cells), - }) + .addCells({ cells: row.cells }) + // .updateCells({ downwardGrowingCells: row.downwardGrowingCells }) .update({ height: Math.max(rowgroup.height, yCurrent + row.height), width: Math.max(rowgroup.width, row.width), }); + // row processing steps 4/16 yCurrent++; } @@ -349,8 +358,11 @@ export namespace RowGroup { ); // ending row group 2 // When emptying the growing cells list, we need to finally add them to the group. - rowgroup = rowgroup.updateCells({ - cells: List.from(rowgroup.cells).concat(growingCellsList), + rowgroup = rowgroup.addCells({ + cells: growingCellsList, + // cells: map(rowgroup.downwardGrowingCells, (cell) => + // cell.growDownward(rowgroup.rowgroup.height - 1) + // ), }); // 3, returning the row group for the table to handle // we could check here if height>0 and return an option, to be closer to the algorithm but that would be less uniform. From 96f08091104d397a56fa2fa91a54294b668c1f0e Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Wed, 24 Jun 2020 13:42:55 +0200 Subject: [PATCH 21/37] Simplify code --- packages/alfa-table/src/row-group.ts | 8 +++----- packages/alfa-table/src/table.ts | 11 +++++------ 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/packages/alfa-table/src/row-group.ts b/packages/alfa-table/src/row-group.ts index b03172f81c..250cbc40c7 100644 --- a/packages/alfa-table/src/row-group.ts +++ b/packages/alfa-table/src/row-group.ts @@ -352,14 +352,12 @@ export namespace RowGroup { yCurrent++; } // 4, ending the row group - // ending row group 1 - growingCellsList = growingCellsList.map((cell) => - cell.growDownward(rowgroup.rowgroup.height - 1) - ); // ending row group 2 // When emptying the growing cells list, we need to finally add them to the group. rowgroup = rowgroup.addCells({ - cells: growingCellsList, + cells: growingCellsList + // ending row group 1 + .map((cell) => cell.growDownward(rowgroup.rowgroup.height - 1)), // cells: map(rowgroup.downwardGrowingCells, (cell) => // cell.growDownward(rowgroup.rowgroup.height - 1) // ), diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index 7c19d505fb..49ec56a0c9 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -529,14 +529,13 @@ export namespace Table { // 14 // Ending row group 1 - // Slots are NOT in sync after this step. - growingCellsList = growingCellsList.map((cell) => - cell.growDownward(table.height - 1) - ); yCurrent = table.height; // Ending row group 2 - // Slots are sync again after this step. - table = table.addCells(growingCellsList); + table = table.addCells( + growingCellsList + // Ending row group 1 + .map((cell) => cell.growDownward(table.height - 1)) + ); growingCellsList = []; if (currentElement.name === "tfoot") { From 2a6ec4fde33ea1d2b56c75150e5daad90e51ff47 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Wed, 24 Jun 2020 14:03:12 +0200 Subject: [PATCH 22/37] Keep growing cells in Builder instance --- packages/alfa-table/src/row-group.ts | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/packages/alfa-table/src/row-group.ts b/packages/alfa-table/src/row-group.ts index 250cbc40c7..230ab5dd46 100644 --- a/packages/alfa-table/src/row-group.ts +++ b/packages/alfa-table/src/row-group.ts @@ -186,7 +186,7 @@ export namespace RowGroup { /** * Update by getting new values. Does not keep slots in sync, hence is highly unsafe. Use at your own risks. */ - private _updateUnsafe({ + public _updateUnsafe({ y = this._rowGroup.anchor.y, width = this._width, height = this._rowGroup.height, @@ -323,7 +323,6 @@ export namespace RowGroup { return Err.of("This element is not a row group"); } - let growingCellsList: List = List.empty(); let rowgroup = Builder.of(-1, 0, group); let yCurrent = 0; // y position inside the rowgroup // 1 @@ -333,16 +332,14 @@ export namespace RowGroup { const row = Row.Builder.from( tr, rowgroup.cells, - growingCellsList, - // rowgroup.downwardGrowingCells, + rowgroup.downwardGrowingCells, yCurrent, rowgroup.width ).get(); - growingCellsList = List.from(row.downwardGrowingCells); rowgroup = rowgroup .addCells({ cells: row.cells }) - // .updateCells({ downwardGrowingCells: row.downwardGrowingCells }) + ._updateUnsafe({ downwardGrowingCells: row.downwardGrowingCells }) .update({ height: Math.max(rowgroup.height, yCurrent + row.height), width: Math.max(rowgroup.width, row.width), @@ -355,12 +352,9 @@ export namespace RowGroup { // ending row group 2 // When emptying the growing cells list, we need to finally add them to the group. rowgroup = rowgroup.addCells({ - cells: growingCellsList - // ending row group 1 - .map((cell) => cell.growDownward(rowgroup.rowgroup.height - 1)), - // cells: map(rowgroup.downwardGrowingCells, (cell) => - // cell.growDownward(rowgroup.rowgroup.height - 1) - // ), + cells: map(rowgroup.downwardGrowingCells, (cell) => + cell.growDownward(rowgroup.rowgroup.height - 1) + ), }); // 3, returning the row group for the table to handle // we could check here if height>0 and return an option, to be closer to the algorithm but that would be less uniform. From 203dff6b3edaceb7f9b9f1d78c7ebf429cd00465 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Wed, 24 Jun 2020 14:10:25 +0200 Subject: [PATCH 23/37] Fix bug and use updateCells --- packages/alfa-table/src/row-group.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/alfa-table/src/row-group.ts b/packages/alfa-table/src/row-group.ts index 230ab5dd46..5afa4f3aef 100644 --- a/packages/alfa-table/src/row-group.ts +++ b/packages/alfa-table/src/row-group.ts @@ -186,7 +186,7 @@ export namespace RowGroup { /** * Update by getting new values. Does not keep slots in sync, hence is highly unsafe. Use at your own risks. */ - public _updateUnsafe({ + private _updateUnsafe({ y = this._rowGroup.anchor.y, width = this._width, height = this._rowGroup.height, @@ -230,8 +230,8 @@ export namespace RowGroup { * Update cells/downward growing cells, and resync slots with all (updated) cells */ public updateCells({ - cells = List.empty(), - downwardGrowingCells = List.empty(), + cells = this._cells, + downwardGrowingCells = this._downwardGrowingCells, }: { cells?: Iterable; downwardGrowingCells?: Iterable; @@ -339,7 +339,7 @@ export namespace RowGroup { rowgroup = rowgroup .addCells({ cells: row.cells }) - ._updateUnsafe({ downwardGrowingCells: row.downwardGrowingCells }) + .updateCells({ downwardGrowingCells: row.downwardGrowingCells }) .update({ height: Math.max(rowgroup.height, yCurrent + row.height), width: Math.max(rowgroup.width, row.width), From cb364f56fde7a687811f78691578058bca069a46 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Wed, 24 Jun 2020 14:12:05 +0200 Subject: [PATCH 24/37] Clean up after self --- packages/alfa-table/src/row-group.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/alfa-table/src/row-group.ts b/packages/alfa-table/src/row-group.ts index 5afa4f3aef..64a9fe5bad 100644 --- a/packages/alfa-table/src/row-group.ts +++ b/packages/alfa-table/src/row-group.ts @@ -359,7 +359,10 @@ export namespace RowGroup { // 3, returning the row group for the table to handle // we could check here if height>0 and return an option, to be closer to the algorithm but that would be less uniform. return Ok.of( - rowgroup.updateCells({ cells: [...rowgroup.cells].sort(compare) }) + rowgroup.updateCells({ + cells: [...rowgroup.cells].sort(compare), + downwardGrowingCells: [], + }) ); } } From bedb0f8a7eed17ce90d0b05f10e37f2a85a0dd4e Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Wed, 24 Jun 2020 14:16:36 +0200 Subject: [PATCH 25/37] Prepare for storing downward growing cells --- packages/alfa-table/src/table.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index 49ec56a0c9..a3c0f7591b 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -131,6 +131,7 @@ export namespace Table { width: number = 0, height: number = 0, cells: Iterable = List.empty(), + downwardGrowingCells: Iterable = List.empty(), slots: Array>> = [[]], rowGroups: Iterable = List.empty(), colGroups: Iterable = List.empty(), @@ -142,6 +143,7 @@ export namespace Table { width, height, cells, + downwardGrowingCells, slots, rowGroups, colGroups, @@ -153,6 +155,7 @@ export namespace Table { // The product will always have empty cells list as it's stored here private readonly _table: Table; private readonly _cells: List; + private readonly _downwardgrowingCells: List; private readonly _slots: Array>>; private readonly _columnHasData: Array; // Does column x contains a slot covered by a data cell? private readonly _rowHasData: Array; // Does row y contains a slot covered by a data cell? @@ -162,6 +165,7 @@ export namespace Table { width: number, height: number, cells: Iterable, + downwardGrowingCells: Iterable, slots: Array>>, rowGroups: Iterable, colGroups: Iterable, @@ -177,6 +181,7 @@ export namespace Table { colGroups ); this._cells = List.from(cells); + this._downwardgrowingCells = List.from(downwardGrowingCells); this._slots = slots; this._columnHasData = columnHasData; this._rowHasData = rowHasData; @@ -229,6 +234,7 @@ export namespace Table { width = this.width, height = this.height, cells = this._cells, + downwardGrowingCells = this._downwardgrowingCells, slots = this._slots, rowGroups = this.rowGroups, colGroups = this.colGroups, @@ -239,6 +245,7 @@ export namespace Table { width?: number; height?: number; cells?: Iterable; + downwardGrowingCells?: Iterable; slots?: Array>>; rowGroups?: Iterable; colGroups?: Iterable; @@ -250,6 +257,7 @@ export namespace Table { width, height, cells, + downwardGrowingCells, slots, rowGroups, colGroups, From c66714ac646ee9ad3a64a1c7b87ca80d87489f1b Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Wed, 24 Jun 2020 14:18:20 +0200 Subject: [PATCH 26/37] Fix updateCells --- packages/alfa-table/src/row.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/alfa-table/src/row.ts b/packages/alfa-table/src/row.ts index 3a9d78e48c..7f84b04be4 100644 --- a/packages/alfa-table/src/row.ts +++ b/packages/alfa-table/src/row.ts @@ -158,8 +158,8 @@ export namespace Row { * Update cells/downward growing cells, and resync slots with all (updated) cells */ private _updateCells({ - cells = List.empty(), - downwardGrowingCells = List.empty(), + cells = this._cells, + downwardGrowingCells = this._downwardGrowingCells, }: { cells?: Iterable; downwardGrowingCells?: Iterable; From 761c5bc2a6e5e02fcc455d9b69918e215906674d Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Wed, 24 Jun 2020 14:18:28 +0200 Subject: [PATCH 27/37] Typo --- packages/alfa-table/src/row-group.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/alfa-table/src/row-group.ts b/packages/alfa-table/src/row-group.ts index 64a9fe5bad..2a7ab19045 100644 --- a/packages/alfa-table/src/row-group.ts +++ b/packages/alfa-table/src/row-group.ts @@ -215,7 +215,7 @@ export namespace RowGroup { } /** - * Update anything but cells/downward growing cells, because these need to be kept in sync with slots. + * Update anything but cells/downward growing cells/slots, because these need to be kept in sync. */ public update(update: { y?: number; From b321b6b570ddfab70d1e47f1c4cd0a9788862986 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Wed, 24 Jun 2020 14:23:01 +0200 Subject: [PATCH 28/37] Allow update of downward growing cells --- packages/alfa-table/src/table.ts | 40 +++++++++++++++++++------------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index a3c0f7591b..104addbb96 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -18,7 +18,7 @@ import { RowGroup } from "./row-group"; import { Scope } from "./scope"; const { compare } = Comparable; -const { filter, map, some } = Iterable; +const { concat, filter, map, some } = Iterable; /** * @see https://html.spec.whatwg.org/multipage/tables.html#table-processing-model @@ -155,7 +155,7 @@ export namespace Table { // The product will always have empty cells list as it's stored here private readonly _table: Table; private readonly _cells: List; - private readonly _downwardgrowingCells: List; + private readonly _downwardGrowingCells: List; private readonly _slots: Array>>; private readonly _columnHasData: Array; // Does column x contains a slot covered by a data cell? private readonly _rowHasData: Array; // Does row y contains a slot covered by a data cell? @@ -181,7 +181,7 @@ export namespace Table { colGroups ); this._cells = List.from(cells); - this._downwardgrowingCells = List.from(downwardGrowingCells); + this._downwardGrowingCells = List.from(downwardGrowingCells); this._slots = slots; this._columnHasData = columnHasData; this._rowHasData = rowHasData; @@ -234,7 +234,7 @@ export namespace Table { width = this.width, height = this.height, cells = this._cells, - downwardGrowingCells = this._downwardgrowingCells, + downwardGrowingCells = this._downwardGrowingCells, slots = this._slots, rowGroups = this.rowGroups, colGroups = this.colGroups, @@ -267,7 +267,7 @@ export namespace Table { } /** - * Update anything but cells/slots, because they need to be kept in sync. + * Update anything but cells/downward growing cells/slots, because these need to be kept in sync. */ public update(update: { element?: Element; @@ -285,8 +285,16 @@ export namespace Table { * Cells are assumed to keep the same anchors, width, height and kind (header/data), * hence columnHasData and rowHasData don't change but slots need to be updated to the new version of their cell. */ - public updateCells(cells: Iterable): Builder { - return this._updateUnsafe({ cells })._updateSlots(cells); + public updateCells({ + cells = this._cells, + downwardGrowingCells = this._downwardGrowingCells, + }: { + cells?: Iterable; + downwardGrowingCells?: Iterable; + }): Builder { + return this._updateUnsafe({ cells, downwardGrowingCells })._updateSlots( + concat(cells, downwardGrowingCells) + ); } /** @@ -356,8 +364,8 @@ export namespace Table { } public addHeadersVariants(): Builder { - return this.updateCells( - List.from( + return this.updateCells({ + cells: List.from( map(this.cells, (cell) => { let dataInColumns = false; let dataInRows = false; @@ -374,8 +382,8 @@ export namespace Table { return cell.addHeaderVariant(dataInColumns, dataInRows); }) - ) - ); + ), + }); } /** @@ -428,16 +436,16 @@ export namespace Table { } public assignHeaders(): Builder { - return this.updateCells( - map(this.cells, (cell) => + return this.updateCells({ + cells: map(this.cells, (cell) => cell.assignHeaders( this.element, this.slot.bind(this), this.getAboveLeftGroupHeaders("row"), this.getAboveLeftGroupHeaders("column") ) - ) - ); + ), + }); } public equals(value: unknown): value is this { @@ -636,7 +644,7 @@ export namespace Table { rowGroups: [...table.rowGroups].sort(compare), colGroups: [...table.colGroups].sort(compare), }) - .updateCells([...table.cells].sort(compare)); + .updateCells({ cells: [...table.cells].sort(compare) }); return Ok.of(table); } } From 31a15cf15b389fe05c23e376306fc9651c1e8623 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Wed, 24 Jun 2020 14:28:08 +0200 Subject: [PATCH 29/37] Allow adding downward growing cells --- packages/alfa-table/src/table.ts | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index 104addbb96..bad8cf11c4 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -300,8 +300,14 @@ export namespace Table { /** * Add new cells, sync slots with the new cells and update columnHasData and rowHasData. */ - public addCells(cells: Iterable): Builder { - for (const cell of cells) { + public addCells({ + cells = List.empty(), + downwardGrowingCells = List.empty(), + }: { + cells?: Iterable; + downwardGrowingCells?: Iterable; + }): Builder { + for (const cell of concat(cells, downwardGrowingCells)) { if (cell.kind === Kind.Data) { // If this is a data cell, update the memory of column/row with data cells. for (let x = cell.anchor.x; x < cell.anchor.x + cell.width; x++) { @@ -315,6 +321,9 @@ export namespace Table { return this._updateUnsafe({ cells: this._cells.concat(cells), + downwardGrowingCells: this._downwardGrowingCells.concat( + downwardGrowingCells + ), columnHasData: this._columnHasData, rowHasData: this._rowHasData, })._updateSlots(cells); @@ -355,7 +364,7 @@ export namespace Table { rowGroups: List.from(this.rowGroups).append(rowGroup.rowgroup), }) // merge in new cells - .addCells(rowGroup.cells) + .addCells({ cells: rowGroup.cells }) ); } else { return this; @@ -536,7 +545,7 @@ export namespace Table { height: Math.max(table.height, yCurrent + 1), width: Math.max(table.width, row.width), }) - .addCells(row.cells); + .addCells({cells: row.cells}); // row processing steps 4/16 yCurrent++; @@ -547,11 +556,11 @@ export namespace Table { // Ending row group 1 yCurrent = table.height; // Ending row group 2 - table = table.addCells( - growingCellsList + table = table.addCells({ + cells: growingCellsList // Ending row group 1 - .map((cell) => cell.growDownward(table.height - 1)) - ); + .map((cell) => cell.growDownward(table.height - 1)), + }); growingCellsList = []; if (currentElement.name === "tfoot") { From 4b47cb40cb5f243f827e34e3d269bbc64bbe4ef1 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Wed, 24 Jun 2020 15:14:27 +0200 Subject: [PATCH 30/37] Only compute column/rowHasData once before using it --- packages/alfa-table/src/table.ts | 60 ++++++++++++-------------------- 1 file changed, 23 insertions(+), 37 deletions(-) diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index bad8cf11c4..28520e32af 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -134,9 +134,7 @@ export namespace Table { downwardGrowingCells: Iterable = List.empty(), slots: Array>> = [[]], rowGroups: Iterable = List.empty(), - colGroups: Iterable = List.empty(), - columnHasData: Array = [], - rowHasData: Array = [] + colGroups: Iterable = List.empty() ): Builder { return new Builder( element, @@ -146,9 +144,7 @@ export namespace Table { downwardGrowingCells, slots, rowGroups, - colGroups, - columnHasData, - rowHasData + colGroups ); } @@ -157,8 +153,6 @@ export namespace Table { private readonly _cells: List; private readonly _downwardGrowingCells: List; private readonly _slots: Array>>; - private readonly _columnHasData: Array; // Does column x contains a slot covered by a data cell? - private readonly _rowHasData: Array; // Does row y contains a slot covered by a data cell? private constructor( element: Element, @@ -168,9 +162,7 @@ export namespace Table { downwardGrowingCells: Iterable, slots: Array>>, rowGroups: Iterable, - colGroups: Iterable, - columnHasData: Array, - rowHasData: Array + colGroups: Iterable ) { this._table = Table.of( element, @@ -183,8 +175,6 @@ export namespace Table { this._cells = List.from(cells); this._downwardGrowingCells = List.from(downwardGrowingCells); this._slots = slots; - this._columnHasData = columnHasData; - this._rowHasData = rowHasData; } public get cells(): Iterable { @@ -238,8 +228,6 @@ export namespace Table { slots = this._slots, rowGroups = this.rowGroups, colGroups = this.colGroups, - columnHasData = this._columnHasData, - rowHasData = this._rowHasData, }: { element?: Element; width?: number; @@ -249,8 +237,6 @@ export namespace Table { slots?: Array>>; rowGroups?: Iterable; colGroups?: Iterable; - columnHasData?: Array; - rowHasData?: Array; }): Builder { return Builder.of( element, @@ -260,9 +246,7 @@ export namespace Table { downwardGrowingCells, slots, rowGroups, - colGroups, - columnHasData, - rowHasData + colGroups ); } @@ -307,25 +291,11 @@ export namespace Table { cells?: Iterable; downwardGrowingCells?: Iterable; }): Builder { - for (const cell of concat(cells, downwardGrowingCells)) { - if (cell.kind === Kind.Data) { - // If this is a data cell, update the memory of column/row with data cells. - for (let x = cell.anchor.x; x < cell.anchor.x + cell.width; x++) { - this._columnHasData[x] = true; - } - for (let y = cell.anchor.y; y < cell.anchor.y + cell.height; y++) { - this._rowHasData[y] = true; - } - } - } - return this._updateUnsafe({ cells: this._cells.concat(cells), downwardGrowingCells: this._downwardGrowingCells.concat( downwardGrowingCells ), - columnHasData: this._columnHasData, - rowHasData: this._rowHasData, })._updateSlots(cells); } @@ -373,18 +343,34 @@ export namespace Table { } public addHeadersVariants(): Builder { + // We need to know which column/row contains at least one data cell. Computing this once and for all now. + const columnHasData: Array = []; + const rowHasData: Array = []; + for (const cell of this._cells) { + if (cell.kind === Kind.Data) { + // If this is a data cell, update the memory of column/row with data cells. + for (let x = cell.anchor.x; x < cell.anchor.x + cell.width; x++) { + columnHasData[x] = true; + } + for (let y = cell.anchor.y; y < cell.anchor.y + cell.height; y++) { + rowHasData[y] = true; + } + } + } + return this.updateCells({ cells: List.from( map(this.cells, (cell) => { + // We need to know if the cell is covering a slot with a data cell in the same column/row let dataInColumns = false; let dataInRows = false; for (let x = cell.anchor.x; x < cell.anchor.x + cell.width; x++) { - if (this._columnHasData[x] ?? false) { + if (columnHasData[x] ?? false) { dataInColumns = true; } } for (let y = cell.anchor.y; y < cell.anchor.y + cell.height; y++) { - if (this._rowHasData[y] ?? false) { + if (rowHasData[y] ?? false) { dataInRows = true; } } @@ -545,7 +531,7 @@ export namespace Table { height: Math.max(table.height, yCurrent + 1), width: Math.max(table.width, row.width), }) - .addCells({cells: row.cells}); + .addCells({ cells: row.cells }); // row processing steps 4/16 yCurrent++; From 8275de3d348e822c0ff7194504ddbc68c878defb Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Wed, 24 Jun 2020 15:29:09 +0200 Subject: [PATCH 31/37] Keep downward growing cells in Builder --- packages/alfa-table/src/table.ts | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index 28520e32af..415966b3c8 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -181,6 +181,10 @@ export namespace Table { return this._cells; } + public get downwardGrowingCells(): Iterable { + return this._downwardGrowingCells; + } + public get width(): number { return this._table.width; } @@ -490,8 +494,6 @@ export namespace Table { let yCurrent = 0; // 11 - let growingCellsList: Array = []; - let processCG = true; for (const currentElement of children) { // loop control is 7 + 9.2 + 13 (advance) + 15 (advance) + 17 + 18 @@ -521,16 +523,16 @@ export namespace Table { const row = Row.Builder.from( currentElement, table.cells, - growingCellsList, + table.downwardGrowingCells, yCurrent, table.width ).get(); - growingCellsList = [...row.downwardGrowingCells]; table = table .update({ height: Math.max(table.height, yCurrent + 1), width: Math.max(table.width, row.width), }) + .updateCells({ downwardGrowingCells: row.downwardGrowingCells }) .addCells({ cells: row.cells }); // row processing steps 4/16 yCurrent++; @@ -542,12 +544,15 @@ export namespace Table { // Ending row group 1 yCurrent = table.height; // Ending row group 2 - table = table.addCells({ - cells: growingCellsList - // Ending row group 1 - .map((cell) => cell.growDownward(table.height - 1)), - }); - growingCellsList = []; + table = table + .addCells({ + cells: map( + table.downwardGrowingCells, + // Ending row group 1 + (cell) => cell.growDownward(table.height - 1) + ), + }) + .updateCells({ downwardGrowingCells: [] }); if (currentElement.name === "tfoot") { // 15 (add to list) From 7e75611fc7131df6bbf153debeb35555b173aaf7 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Wed, 24 Jun 2020 16:14:08 +0200 Subject: [PATCH 32/37] Use stored coverage info for skipIfCovered --- packages/alfa-table/src/row-group.ts | 3 +- packages/alfa-table/src/row.ts | 43 ++++++++++++++-------------- packages/alfa-table/src/table.ts | 3 +- 3 files changed, 25 insertions(+), 24 deletions(-) diff --git a/packages/alfa-table/src/row-group.ts b/packages/alfa-table/src/row-group.ts index 2a7ab19045..a11aecb6c9 100644 --- a/packages/alfa-table/src/row-group.ts +++ b/packages/alfa-table/src/row-group.ts @@ -334,7 +334,8 @@ export namespace RowGroup { rowgroup.cells, rowgroup.downwardGrowingCells, yCurrent, - rowgroup.width + rowgroup.width, + rowgroup.slot.bind(rowgroup) ).get(); rowgroup = rowgroup diff --git a/packages/alfa-table/src/row.ts b/packages/alfa-table/src/row.ts index 7f84b04be4..e82f5ea49e 100644 --- a/packages/alfa-table/src/row.ts +++ b/packages/alfa-table/src/row.ts @@ -213,23 +213,13 @@ export namespace Row { }); } - // TODO make private ×n (or remove) - public addNonGrowingCell(cell: Cell.Builder): Builder { - return this._addCells({ cells: [cell] }); - } - - public addGrowingCell(cell: Cell.Builder): Builder { + private _addCell(cell: Cell.Builder): Builder { return this._addCells({ - downwardGrowingCells: [cell], + cells: cell.downwardGrowing ? [] : [cell], + downwardGrowingCells: cell.downwardGrowing ? [cell] : [], }); } - public addCell(cell: Cell.Builder): Builder { - return cell.downwardGrowing - ? this.addGrowingCell(cell) - : this.addNonGrowingCell(cell); - } - public addCellFromElement( currentCell: Element, yCurrent: number @@ -244,7 +234,7 @@ export namespace Row { height: Math.max(this.height, cell.height), }) // 14 - .addCell(cell) + ._addCell(cell) ); // 13 // Double coverage check made at the end of table building to de-entangle code @@ -262,17 +252,25 @@ export namespace Row { * moves xCurrent to the first slot which is not already covered by one of the cells from the row or its context * step 6 */ - public skipIfCovered(cells: List, yCurrent: number): Builder { + public skipIfCovered( + cells: List, + yCurrent: number, + externalCover: (x: number, y: number) => Option + ): Builder { + const covering = this.slot(this._xCurrent, yCurrent).or(externalCover(this._xCurrent, yCurrent)); + if ( this._xCurrent < this._width && - cells - .concat(this._cells) - .concat(this._downwardGrowingCells) - .some((cell) => cell.isCovering(this._xCurrent, yCurrent)) + covering.isSome() + // cells + // .concat(this._cells) + // .concat(this._downwardGrowingCells) + // .some((cell) => cell.isCovering(this._xCurrent, yCurrent)) ) { return this._update({ xCurrent: this._xCurrent + 1 }).skipIfCovered( cells, - yCurrent + yCurrent, + externalCover ); } else { return this; @@ -327,7 +325,8 @@ export namespace Row { cells: Iterable = List.empty(), growingCells: Iterable = List.empty(), yCurrent: number = 0, - w: number = 0 + w: number = 0, + externalCover: (x: number, y: number) => Option = (x, y) => None ): Result { if (tr.name !== "tr") { return Err.of("This element is not a table row"); @@ -352,7 +351,7 @@ export namespace Row { (row, currentCell) => row // 6 (Cells) - .skipIfCovered(List.from(cells), yCurrent) + .skipIfCovered(List.from(cells), yCurrent, externalCover) // 7 .enlargeIfNeeded() // 8-14 diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index 415966b3c8..aca723f83e 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -525,7 +525,8 @@ export namespace Table { table.cells, table.downwardGrowingCells, yCurrent, - table.width + table.width, + table.slot.bind(table) ).get(); table = table .update({ From 90f099f7ee44bcdc9638c88602ee4740c9ca4f15 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Wed, 24 Jun 2020 16:17:57 +0200 Subject: [PATCH 33/37] Clean up --- packages/alfa-table/src/row-group.ts | 1 - packages/alfa-table/src/row.ts | 36 ++++++++++------------------ packages/alfa-table/src/table.ts | 1 - 3 files changed, 13 insertions(+), 25 deletions(-) diff --git a/packages/alfa-table/src/row-group.ts b/packages/alfa-table/src/row-group.ts index a11aecb6c9..2c18b3d20a 100644 --- a/packages/alfa-table/src/row-group.ts +++ b/packages/alfa-table/src/row-group.ts @@ -331,7 +331,6 @@ export namespace RowGroup { for (const tr of group.children().filter(isHtmlElementWithName("tr"))) { const row = Row.Builder.from( tr, - rowgroup.cells, rowgroup.downwardGrowingCells, yCurrent, rowgroup.width, diff --git a/packages/alfa-table/src/row.ts b/packages/alfa-table/src/row.ts index e82f5ea49e..c14e505e46 100644 --- a/packages/alfa-table/src/row.ts +++ b/packages/alfa-table/src/row.ts @@ -253,22 +253,17 @@ export namespace Row { * step 6 */ public skipIfCovered( - cells: List, yCurrent: number, + // Non-growing cells from out of the current row that may cover it. + // These are essentially cells with a fixed rowspan from previous rows. externalCover: (x: number, y: number) => Option ): Builder { - const covering = this.slot(this._xCurrent, yCurrent).or(externalCover(this._xCurrent, yCurrent)); - - if ( - this._xCurrent < this._width && - covering.isSome() - // cells - // .concat(this._cells) - // .concat(this._downwardGrowingCells) - // .some((cell) => cell.isCovering(this._xCurrent, yCurrent)) - ) { + const covering = this.slot(this._xCurrent, yCurrent).or( + externalCover(this._xCurrent, yCurrent) + ); + + if (this._xCurrent < this._width && covering.isSome()) { return this._update({ xCurrent: this._xCurrent + 1 }).skipIfCovered( - cells, yCurrent, externalCover ); @@ -322,24 +317,19 @@ export namespace Row { */ export function from( tr: Element, - cells: Iterable = List.empty(), + // downward growing cells that extend into the row growingCells: Iterable = List.empty(), yCurrent: number = 0, w: number = 0, - externalCover: (x: number, y: number) => Option = (x, y) => None + // Non-growing cells from out of the current row that may cover it. + // These are essentially cells with a fixed rowspan from previous rows. + externalCover: (x: number, y: number) => Option = (x, y) => + None ): Result { if (tr.name !== "tr") { return Err.of("This element is not a table row"); } - if ( - some(cells, (cell) => - some(growingCells, (growingCell) => cell.equals(growingCell)) - ) - ) { - return Err.of("Cells and growing cells must be disjoints"); - } - const children = tr.children().filter(isHtmlElementWithName("th", "td")); // 1 @@ -351,7 +341,7 @@ export namespace Row { (row, currentCell) => row // 6 (Cells) - .skipIfCovered(List.from(cells), yCurrent, externalCover) + .skipIfCovered(yCurrent, externalCover) // 7 .enlargeIfNeeded() // 8-14 diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index aca723f83e..3011594847 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -522,7 +522,6 @@ export namespace Table { const row = Row.Builder.from( currentElement, - table.cells, table.downwardGrowingCells, yCurrent, table.width, From 28bd823362654e05343d3089f7ce8a9614c00649 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Wed, 24 Jun 2020 16:30:54 +0200 Subject: [PATCH 34/37] Clean up --- packages/alfa-cli/package.json | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/alfa-cli/package.json b/packages/alfa-cli/package.json index 6d170e462d..99fc5c6762 100644 --- a/packages/alfa-cli/package.json +++ b/packages/alfa-cli/package.json @@ -46,8 +46,5 @@ "publishConfig": { "access": "public", "registry": "https://npm.pkg.github.com/" - }, - "scripts": { - "test": "node script.js" } } From 9eddac33792aa010161b28b977527ff4168d6225 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Thu, 25 Jun 2020 09:08:55 +0200 Subject: [PATCH 35/37] Add TODO comments --- packages/alfa-table/src/table.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index 3011594847..5bada930e8 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -388,6 +388,7 @@ export namespace Table { /** * If principal cell is in a group, get all group headers that are in this group and above+lift of principal cell. */ + // TODO: optimize, don't filter on all cells public getAboveLeftGroupHeaders( kind: "row" | "column" ): (principalCell: Cell.Builder) => Iterable { @@ -579,6 +580,7 @@ export namespace Table { // Of course, errors are more or less caught and repaired by browsers. // Note that having a rowspan that extends out of the row group is not a table error per se! // checking for rows + // TODO: optimize. don't some on cells for (let row = 0; row < table.height; row++) { let rowCovered = false; for (let col = 0; !rowCovered && col < table.width; col++) { @@ -592,6 +594,7 @@ export namespace Table { if (!rowCovered) return Err.of(`row ${row} has no cell anchored in it`); } // checking for cols + // TODO: optimize. don't some on cells for (let col = 0; col < table.width; col++) { let colCovered = false; for (let row = 0; !colCovered && row < table.height; row++) { @@ -604,6 +607,7 @@ export namespace Table { } if (!colCovered) return Err.of(`col ${col} has no cell anchored in it`); } +// TODO: optimize. Don't filter on cells. // Checking for row forming algorithm step 13 (slot covered twice) for (let x = 0; x < table.width; x++) { for (let y = 0; y < table.height; y++) { From 437d0b6363e9e45be5ff5bb233e11420312c6d5f Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Thu, 25 Jun 2020 09:27:18 +0200 Subject: [PATCH 36/37] Optimize row/column group headers detection --- packages/alfa-table/src/table.ts | 69 +++++++++++++++++++++++--------- 1 file changed, 50 insertions(+), 19 deletions(-) diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index 5bada930e8..d094b963f0 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -134,7 +134,9 @@ export namespace Table { downwardGrowingCells: Iterable = List.empty(), slots: Array>> = [[]], rowGroups: Iterable = List.empty(), - colGroups: Iterable = List.empty() + colGroups: Iterable = List.empty(), + rowGroupHeaders: Iterable = List.empty(), + columnGroupHeaders: Iterable = List.empty() ): Builder { return new Builder( element, @@ -144,7 +146,9 @@ export namespace Table { downwardGrowingCells, slots, rowGroups, - colGroups + colGroups, + rowGroupHeaders, + columnGroupHeaders ); } @@ -153,6 +157,8 @@ export namespace Table { private readonly _cells: List; private readonly _downwardGrowingCells: List; private readonly _slots: Array>>; + private readonly _rowGroupHeaders: List; + private readonly _columnGroupHeaders: List; private constructor( element: Element, @@ -162,7 +168,9 @@ export namespace Table { downwardGrowingCells: Iterable, slots: Array>>, rowGroups: Iterable, - colGroups: Iterable + colGroups: Iterable, + rowGroupHeaders: Iterable, + columnGroupHeaders: Iterable ) { this._table = Table.of( element, @@ -175,6 +183,8 @@ export namespace Table { this._cells = List.from(cells); this._downwardGrowingCells = List.from(downwardGrowingCells); this._slots = slots; + this._rowGroupHeaders = List.from(rowGroupHeaders); + this._columnGroupHeaders = List.from(columnGroupHeaders); } public get cells(): Iterable { @@ -232,6 +242,8 @@ export namespace Table { slots = this._slots, rowGroups = this.rowGroups, colGroups = this.colGroups, + rowGroupHeaders = this._rowGroupHeaders, + columnGroupHeaders = this._columnGroupHeaders, }: { element?: Element; width?: number; @@ -241,6 +253,8 @@ export namespace Table { slots?: Array>>; rowGroups?: Iterable; colGroups?: Iterable; + rowGroupHeaders?: Iterable; + columnGroupHeaders?: Iterable; }): Builder { return Builder.of( element, @@ -250,7 +264,9 @@ export namespace Table { downwardGrowingCells, slots, rowGroups, - colGroups + colGroups, + rowGroupHeaders, + columnGroupHeaders ); } @@ -264,6 +280,8 @@ export namespace Table { slots?: Array>>; rowGroups?: Iterable; colGroups?: Iterable; + rowGroupHeaders?: Iterable; + columnGroupHeaders?: Iterable; }): Builder { return this._updateUnsafe(update); } @@ -385,10 +403,25 @@ export namespace Table { }); } + public findGroupHeaders(): Builder { + const rowGroupHeaders: Array = []; + const columnGroupHeaders: Array = []; + + for (const cell of this._cells) { + if (cell.variant.equals(Some.of(Scope.RowGroup))) { + rowGroupHeaders.push(cell); + } + if (cell.variant.equals(Some.of(Scope.ColumnGroup))) { + columnGroupHeaders.push(cell); + } + } + + return this.update({ rowGroupHeaders, columnGroupHeaders }); + } + /** * If principal cell is in a group, get all group headers that are in this group and above+lift of principal cell. */ - // TODO: optimize, don't filter on all cells public getAboveLeftGroupHeaders( kind: "row" | "column" ): (principalCell: Cell.Builder) => Iterable { @@ -400,16 +433,12 @@ export namespace Table { case "row": anchor = "y"; groups = this.rowGroups; - groupHeaders = filter(this.cells, (cell) => - cell.variant.equals(Some.of(Scope.RowGroup)) - ); + groupHeaders = this._rowGroupHeaders; break; case "column": anchor = "x"; groups = this.colGroups; - groupHeaders = filter(this.cells, (cell) => - cell.variant.equals(Some.of(Scope.ColumnGroup)) - ); + groupHeaders = this._columnGroupHeaders; break; } @@ -607,7 +636,7 @@ export namespace Table { } if (!colCovered) return Err.of(`col ${col} has no cell anchored in it`); } -// TODO: optimize. Don't filter on cells. + // TODO: optimize. Don't filter on cells. // Checking for row forming algorithm step 13 (slot covered twice) for (let x = 0; x < table.width; x++) { for (let y = 0; y < table.height; y++) { @@ -634,13 +663,15 @@ export namespace Table { slots[x].push(table.slot(x, y)); } } - table = table.update({ slots }); - - // Second, we need to compute all headers variant. - // This need to be done separately so that the updated table is used in assignHeaders. - table = table.addHeadersVariants(); - // Third, we assign headers to cells - table = table.assignHeaders(); + + table = table + .update({ slots }) + // Second, we need to compute all headers variant. + // This need to be done separately so that the updated table is used in assignHeaders. + .addHeadersVariants() + // Third, we assign headers to cells + .findGroupHeaders() + .assignHeaders(); // Finally, we sort lists and export the result. table = table From 3beb1aeffed439aa6c12a216a492acb23ccd3756 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Thu, 25 Jun 2020 11:20:29 +0200 Subject: [PATCH 37/37] Optimize coverage checks (table model errors detection) --- packages/alfa-table/src/table.ts | 68 ++++++++++++++++---------------- 1 file changed, 33 insertions(+), 35 deletions(-) diff --git a/packages/alfa-table/src/table.ts b/packages/alfa-table/src/table.ts index d094b963f0..cb5e70b990 100644 --- a/packages/alfa-table/src/table.ts +++ b/packages/alfa-table/src/table.ts @@ -605,47 +605,45 @@ export namespace Table { table = table.addRowGroupFromElement(tfoot, yCurrent).get(); yCurrent = table.height; } - // 20 + // 20, 21 // Of course, errors are more or less caught and repaired by browsers. // Note that having a rowspan that extends out of the row group is not a table error per se! - // checking for rows - // TODO: optimize. don't some on cells - for (let row = 0; row < table.height; row++) { - let rowCovered = false; - for (let col = 0; !rowCovered && col < table.width; col++) { - rowCovered = - rowCovered || - some( - table.cells, - (cell) => cell.anchor.x === col && cell.anchor.y === row - ); + + // Is this slot covered? If more than once => error. + const slotCovering: Array> = [[]]; + // Does this row/column have a cell anchored in it? + const rowCovering: Array = []; + const columnCovering: Array = []; + + for (const cell of table.cells) { + columnCovering[cell.anchor.x] = true; + rowCovering[cell.anchor.y] = true; + + for (let x = cell.anchor.x; x < cell.anchor.x + cell.width; x++) { + for (let y = cell.anchor.y; y < cell.anchor.y + cell.height; y++) { + // Checking for row forming algorithm step 13 (slot covered twice) + if (slotCovering?.[x]?.[y] ?? false) { + return Err.of(`Slot (${x}, ${y}) is covered twice`); + } else { + if (slotCovering[x] === undefined) { + slotCovering[x] = []; + } + slotCovering[x][y] = true; + } + } } - if (!rowCovered) return Err.of(`row ${row} has no cell anchored in it`); } - // checking for cols - // TODO: optimize. don't some on cells - for (let col = 0; col < table.width; col++) { - let colCovered = false; - for (let row = 0; !colCovered && row < table.height; row++) { - colCovered = - colCovered || - some( - table.cells, - (cell) => cell.anchor.x === col && cell.anchor.y === row - ); + + // checking for rows + for (let y = 0; y < table.height; y++) { + if (!(rowCovering[y] ?? false)) { + return Err.of(`row ${y} has no cell anchored in it`); } - if (!colCovered) return Err.of(`col ${col} has no cell anchored in it`); } - // TODO: optimize. Don't filter on cells. - // Checking for row forming algorithm step 13 (slot covered twice) + // checking for cols for (let x = 0; x < table.width; x++) { - for (let y = 0; y < table.height; y++) { - if ( - List.from(table.cells).filter((cell) => cell.isCovering(x, y)) - .size > 1 - ) { - return Err.of(`Slot (${x}, ${y}) is covered twice`); - } + if (!(columnCovering[x] ?? false)) { + return Err.of(`col ${x} has no cell anchored in it`); } } @@ -663,7 +661,7 @@ export namespace Table { slots[x].push(table.slot(x, y)); } } - + table = table .update({ slots }) // Second, we need to compute all headers variant.