From 8d01b990af74cef4983d2debf8bea35199eaaa2d Mon Sep 17 00:00:00 2001 From: Bob Ippolito Date: Sat, 7 Sep 2024 09:01:13 -0700 Subject: [PATCH] [lexical-table][lexical-playground] Bug Fix: Fix merged cell related edge cases (#6607) --- .../__tests__/e2e/Tables.spec.mjs | 304 +++++++++++++----- .../src/plugins/MarkdownTransformers/index.ts | 5 +- .../plugins/TableActionMenuPlugin/index.tsx | 55 +--- .../lexical-table/flow/LexicalTable.js.flow | 6 +- .../lexical-table/src/LexicalTableCellNode.ts | 55 ++-- .../src/LexicalTableSelection.ts | 22 +- .../lexical-table/src/LexicalTableUtils.ts | 126 +++++--- 7 files changed, 361 insertions(+), 212 deletions(-) diff --git a/packages/lexical-playground/__tests__/e2e/Tables.spec.mjs b/packages/lexical-playground/__tests__/e2e/Tables.spec.mjs index d639bc9e5b3..959d3395395 100644 --- a/packages/lexical-playground/__tests__/e2e/Tables.spec.mjs +++ b/packages/lexical-playground/__tests__/e2e/Tables.spec.mjs @@ -665,91 +665,93 @@ test.describe.parallel('Tables', () => { }); }); - test(`Can select cells using Table selection`, async ({ - page, - isPlainText, - isCollab, - }) => { - await initialize({isCollab, page}); - test.skip(isPlainText); + test( + `Can select cells using Table selection`, + { + tag: '@flaky', + }, + async ({page, isPlainText, isCollab}) => { + await initialize({isCollab, page}); + test.skip(isPlainText); - await focusEditor(page); - await insertTable(page, 2, 3); + await focusEditor(page); + await insertTable(page, 2, 3); - await fillTablePartiallyWithText(page); - await selectCellsFromTableCords( - page, - {x: 0, y: 0}, - {x: 1, y: 1}, - true, - false, - ); + await fillTablePartiallyWithText(page); + await selectCellsFromTableCords( + page, + {x: 0, y: 0}, + {x: 1, y: 1}, + true, + false, + ); - await assertHTML( - page, - html` -


- - - - - - - - - - - -
-

a

- 		-

bb

- 		-

cc

-
-

d

- 		-

e

- 		-

f

-
-


- `, - html` -


- - - - - - - - - - - -
-

a

- 		-

bb

- 		-

cc

-
-

d

- 		-

e

- 		-

f

-
-


- `, - {ignoreClasses: true}, - ); - }); + await assertHTML( + page, + html` +


+ + + + + + + + + + + +
+

a

+ 		+

bb

+ 		+

cc

+
+

d

+ 		+

e

+ 		+

f

+
+


+ `, + html` +


+ + + + + + + + + + + +
+

a

+ 		+

bb

+ 		+

cc

+
+

d

+ 		+

e

+ 		+

f

+
+


+ `, + {ignoreClasses: true}, + ); + }, + ); test(`Can select cells using Table selection via keyboard`, async ({ page, @@ -1889,9 +1891,10 @@ test.describe.parallel('Tables', () => { second

- +


- +


@@ -2252,6 +2255,139 @@ test.describe.parallel('Tables', () => { ); }); + test('Merge multiple merged cells and then unmerge', async ({ + page, + isPlainText, + isCollab, + }) => { + await initialize({isCollab, page}); + test.skip(isPlainText); + + await focusEditor(page); + + await insertTable(page, 3, 3); + + await click(page, '.PlaygroundEditorTheme__tableCell'); + await moveDown(page, 1); + await selectCellsFromTableCords( + page, + {x: 0, y: 0}, + {x: 0, y: 1}, + true, + true, + ); + await mergeTableCells(page); + + await moveRight(page, 1); + await selectCellsFromTableCords( + page, + {x: 1, y: 0}, + {x: 2, y: 0}, + true, + true, + ); + await mergeTableCells(page); + + await selectCellsFromTableCords( + page, + {x: 0, y: 0}, + {x: 1, y: 0}, + true, + true, + ); + await mergeTableCells(page); + + await assertHTML( + page, + html` +


+ + + + +
+ + + + + +
+


+
+


+ 		+


+ 		+


+
+


+ `, + ); + + await selectCellsFromTableCords( + page, + {x: 0, y: 0}, + {x: 0, y: 0}, + true, + true, + ); + await unmergeTableCell(page); + + await assertHTML( + page, + html` +


+ + + + + + + + + + + + + + + + +
+


+ 		+


+ 		+


+
+


+ 		+


+ 		+


+
+


+ 		+


+ 		+


+
+


+ `, + ); + }); + test('Insert row above (with conflicting merged cell)', async ({ page, isPlainText, diff --git a/packages/lexical-playground/src/plugins/MarkdownTransformers/index.ts b/packages/lexical-playground/src/plugins/MarkdownTransformers/index.ts index de5c1e64824..736267dee54 100644 --- a/packages/lexical-playground/src/plugins/MarkdownTransformers/index.ts +++ b/packages/lexical-playground/src/plugins/MarkdownTransformers/index.ts @@ -212,7 +212,10 @@ export const TABLE: ElementTransformer = { if (!$isTableCellNode(cell)) { return; } - cell.toggleHeaderStyle(TableCellHeaderStates.ROW); + cell.setHeaderStyles( + TableCellHeaderStates.ROW, + TableCellHeaderStates.ROW, + ); }); // Remove line diff --git a/packages/lexical-playground/src/plugins/TableActionMenuPlugin/index.tsx b/packages/lexical-playground/src/plugins/TableActionMenuPlugin/index.tsx index 0796241e5e1..60a09ab8a09 100644 --- a/packages/lexical-playground/src/plugins/TableActionMenuPlugin/index.tsx +++ b/packages/lexical-playground/src/plugins/TableActionMenuPlugin/index.tsx @@ -43,7 +43,6 @@ import { import * as React from 'react'; import {ReactPortal, useCallback, useEffect, useRef, useState} from 'react'; import {createPortal} from 'react-dom'; -import invariant from 'shared/invariant'; import useModal from '../../hooks/useModal'; import ColorPicker from '../../ui/ColorPicker'; @@ -59,48 +58,6 @@ function computeSelectionCount(selection: TableSelection): { }; } -// This is important when merging cells as there is no good way to re-merge weird shapes (a result -// of selecting merged cells and non-merged) -function isTableSelectionRectangular(selection: TableSelection): boolean { - const nodes = selection.getNodes(); - const currentRows: Array = []; - let currentRow = null; - let expectedColumns = null; - let currentColumns = 0; - for (let i = 0; i < nodes.length; i++) { - const node = nodes[i]; - if ($isTableCellNode(node)) { - const row = node.getParentOrThrow(); - invariant( - $isTableRowNode(row), - 'Expected CellNode to have a RowNode parent', - ); - if (currentRow !== row) { - if (expectedColumns !== null && currentColumns !== expectedColumns) { - return false; - } - if (currentRow !== null) { - expectedColumns = currentColumns; - } - currentRow = row; - currentColumns = 0; - } - const colSpan = node.__colSpan; - for (let j = 0; j < colSpan; j++) { - if (currentRows[currentColumns + j] === undefined) { - currentRows[currentColumns + j] = 0; - } - currentRows[currentColumns + j] += node.__rowSpan; - } - currentColumns += colSpan; - } - } - return ( - (expectedColumns === null || currentColumns === expectedColumns) && - currentRows.every((v) => v === currentRows[0]) - ); -} - function $canUnmerge(): boolean { const selection = $getSelection(); if ( @@ -208,9 +165,7 @@ function TableActionMenu({ const currentSelectionCounts = computeSelectionCount(selection); updateSelectionCounts(computeSelectionCount(selection)); setCanMergeCells( - isTableSelectionRectangular(selection) && - (currentSelectionCounts.columns > 1 || - currentSelectionCounts.rows > 1), + currentSelectionCounts.columns > 1 || currentSelectionCounts.rows > 1, ); } // Unmerge cell @@ -407,12 +362,14 @@ function TableActionMenu({ throw new Error('Expected table row'); } + const newStyle = + tableCellNode.getHeaderStyles() ^ TableCellHeaderStates.ROW; tableRow.getChildren().forEach((tableCell) => { if (!$isTableCellNode(tableCell)) { throw new Error('Expected table cell'); } - tableCell.toggleHeaderStyle(TableCellHeaderStates.ROW); + tableCell.setHeaderStyles(newStyle, TableCellHeaderStates.ROW); }); clearTableSelection(); @@ -436,6 +393,8 @@ function TableActionMenu({ throw new Error('Expected table cell to be inside of table row.'); } + const newStyle = + tableCellNode.getHeaderStyles() ^ TableCellHeaderStates.COLUMN; for (let r = 0; r < tableRows.length; r++) { const tableRow = tableRows[r]; @@ -455,7 +414,7 @@ function TableActionMenu({ throw new Error('Expected table cell'); } - tableCell.toggleHeaderStyle(TableCellHeaderStates.COLUMN); + tableCell.setHeaderStyles(newStyle, TableCellHeaderStates.COLUMN); } clearTableSelection(); onClose(); diff --git a/packages/lexical-table/flow/LexicalTable.js.flow b/packages/lexical-table/flow/LexicalTable.js.flow index 7227722befa..0c58fc56374 100644 --- a/packages/lexical-table/flow/LexicalTable.js.flow +++ b/packages/lexical-table/flow/LexicalTable.js.flow @@ -62,12 +62,12 @@ declare export class TableCellNode extends ElementNode { getRowSpan(): number; setRowSpan(rowSpan: number): this; getTag(): string; - setHeaderStyles(headerState: TableCellHeaderState): TableCellHeaderState; + setHeaderStyles(headerState: TableCellHeaderState, mask?: TableCellHeaderState): TableCellNode; getHeaderStyles(): TableCellHeaderState; - setWidth(width: number): ?number; + setWidth(width: number): TableCellNode; getWidth(): ?number; getBackgroundColor(): null | string; - setBackgroundColor(newBackgroundColor: null | string): void; + setBackgroundColor(newBackgroundColor: null | string): TableCellNode; toggleHeaderStyle(headerState: TableCellHeaderState): TableCellNode; hasHeader(): boolean; updateDOM(prevNode: TableCellNode): boolean; diff --git a/packages/lexical-table/src/LexicalTableCellNode.ts b/packages/lexical-table/src/LexicalTableCellNode.ts index 455d39bf6c9..7f752777dc0 100644 --- a/packages/lexical-table/src/LexicalTableCellNode.ts +++ b/packages/lexical-table/src/LexicalTableCellNode.ts @@ -69,15 +69,18 @@ export class TableCellNode extends ElementNode { } static clone(node: TableCellNode): TableCellNode { - const cellNode = new TableCellNode( + return new TableCellNode( node.__headerState, node.__colSpan, node.__width, node.__key, ); - cellNode.__rowSpan = node.__rowSpan; - cellNode.__backgroundColor = node.__backgroundColor; - return cellNode; + } + + afterCloneFrom(node: this): void { + super.afterCloneFrom(node); + this.__rowSpan = node.__rowSpan; + this.__backgroundColor = node.__backgroundColor; } static importDOM(): DOMConversionMap | null { @@ -96,14 +99,13 @@ export class TableCellNode extends ElementNode { static importJSON(serializedNode: SerializedTableCellNode): TableCellNode { const colSpan = serializedNode.colSpan || 1; const rowSpan = serializedNode.rowSpan || 1; - const cellNode = $createTableCellNode( + return $createTableCellNode( serializedNode.headerState, colSpan, serializedNode.width || undefined, - ); - cellNode.__rowSpan = rowSpan; - cellNode.__backgroundColor = serializedNode.backgroundColor || null; - return cellNode; + ) + .setRowSpan(rowSpan) + .setBackgroundColor(serializedNode.backgroundColor || null); } constructor( @@ -190,41 +192,46 @@ export class TableCellNode extends ElementNode { } getColSpan(): number { - return this.__colSpan; + return this.getLatest().__colSpan; } setColSpan(colSpan: number): this { - this.getWritable().__colSpan = colSpan; - return this; + const self = this.getWritable(); + self.__colSpan = colSpan; + return self; } getRowSpan(): number { - return this.__rowSpan; + return this.getLatest().__rowSpan; } setRowSpan(rowSpan: number): this { - this.getWritable().__rowSpan = rowSpan; - return this; + const self = this.getWritable(); + self.__rowSpan = rowSpan; + return self; } getTag(): string { return this.hasHeader() ? 'th' : 'td'; } - setHeaderStyles(headerState: TableCellHeaderState): TableCellHeaderState { + setHeaderStyles( + headerState: TableCellHeaderState, + mask: TableCellHeaderState = TableCellHeaderStates.BOTH, + ): this { const self = this.getWritable(); - self.__headerState = headerState; - return this.__headerState; + self.__headerState = (headerState & mask) | (self.__headerState & ~mask); + return self; } getHeaderStyles(): TableCellHeaderState { return this.getLatest().__headerState; } - setWidth(width: number): number | null | undefined { + setWidth(width: number): this { const self = this.getWritable(); self.__width = width; - return this.__width; + return self; } getWidth(): number | undefined { @@ -235,11 +242,13 @@ export class TableCellNode extends ElementNode { return this.getLatest().__backgroundColor; } - setBackgroundColor(newBackgroundColor: null | string): void { - this.getWritable().__backgroundColor = newBackgroundColor; + setBackgroundColor(newBackgroundColor: null | string): this { + const self = this.getWritable(); + self.__backgroundColor = newBackgroundColor; + return self; } - toggleHeaderStyle(headerStateToToggle: TableCellHeaderState): TableCellNode { + toggleHeaderStyle(headerStateToToggle: TableCellHeaderState): this { const self = this.getWritable(); if ((self.__headerState & headerStateToToggle) === headerStateToToggle) { diff --git a/packages/lexical-table/src/LexicalTableSelection.ts b/packages/lexical-table/src/LexicalTableSelection.ts index f4e94bdc87f..acb80d2a20d 100644 --- a/packages/lexical-table/src/LexicalTableSelection.ts +++ b/packages/lexical-table/src/LexicalTableSelection.ts @@ -157,8 +157,8 @@ export class TableSelection implements BaseSelection { focusCellNodeRect.columnIndex, ); const stopX = Math.max( - anchorCellNodeRect.columnIndex, - focusCellNodeRect.columnIndex, + anchorCellNodeRect.columnIndex + anchorCellNodeRect.colSpan - 1, + focusCellNodeRect.columnIndex + focusCellNodeRect.colSpan - 1, ); const startY = Math.min( @@ -166,8 +166,8 @@ export class TableSelection implements BaseSelection { focusCellNodeRect.rowIndex, ); const stopY = Math.max( - anchorCellNodeRect.rowIndex, - focusCellNodeRect.rowIndex, + anchorCellNodeRect.rowIndex + anchorCellNodeRect.rowSpan - 1, + focusCellNodeRect.rowIndex + focusCellNodeRect.rowSpan - 1, ); return { @@ -306,7 +306,11 @@ export class TableSelection implements BaseSelection { } } - const nodes: Array = [tableNode]; + // We use a Map here because merged cells in the grid would otherwise + // show up multiple times in the nodes array + const nodeMap: Map = new Map([ + [tableNode.getKey(), tableNode], + ]); let lastRow = null; for (let i = minRow; i <= maxRow; i++) { for (let j = minColumn; j <= maxColumn; j++) { @@ -317,12 +321,16 @@ export class TableSelection implements BaseSelection { 'Expected TableCellNode parent to be a TableRowNode', ); if (currentRow !== lastRow) { - nodes.push(currentRow); + nodeMap.set(currentRow.getKey(), currentRow); + } + nodeMap.set(cell.getKey(), cell); + for (const child of $getChildrenRecursively(cell)) { + nodeMap.set(child.getKey(), child); } - nodes.push(cell, ...$getChildrenRecursively(cell)); lastRow = currentRow; } } + const nodes = Array.from(nodeMap.values()); if (!isCurrentlyReadOnlyMode()) { this._cachedNodes = nodes; diff --git a/packages/lexical-table/src/LexicalTableUtils.ts b/packages/lexical-table/src/LexicalTableUtils.ts index 3293881e823..d3e7d5ab888 100644 --- a/packages/lexical-table/src/LexicalTableUtils.ts +++ b/packages/lexical-table/src/LexicalTableUtils.ts @@ -557,7 +557,7 @@ export function $deleteTableRow__EXPERIMENTAL(): void { const rowNode = grid.getChildAtIndex(row); invariant( $isTableRowNode(rowNode), - 'Expected GridNode childAtIndex(%s) to be RowNode', + 'Expected TableNode childAtIndex(%s) to be RowNode', String(row), ); rowNode.remove(); @@ -673,19 +673,43 @@ export function $unmergeCell(): void { const [cell, row, grid] = $getNodeTriplet(anchor); const colSpan = cell.__colSpan; const rowSpan = cell.__rowSpan; + if (colSpan === 1 && rowSpan === 1) { + return; + } + const [map, cellMap] = $computeTableMap(grid, cell, cell); + const {startColumn, startRow} = cellMap; + // Create a heuristic for what the style of the unmerged cells should be + // based on whether every row or column already had that state before the + // unmerge. + const baseColStyle = cell.__headerState & TableCellHeaderStates.COLUMN; + const colStyles = Array.from({length: colSpan}, (_v, i) => { + let colStyle = baseColStyle; + for (let rowIdx = 0; colStyle !== 0 && rowIdx < map.length; rowIdx++) { + colStyle &= map[rowIdx][i + startColumn].cell.__headerState; + } + return colStyle; + }); + const baseRowStyle = cell.__headerState & TableCellHeaderStates.ROW; + const rowStyles = Array.from({length: rowSpan}, (_v, i) => { + let rowStyle = baseRowStyle; + for (let colIdx = 0; rowStyle !== 0 && colIdx < map[0].length; colIdx++) { + rowStyle &= map[i + startRow][colIdx].cell.__headerState; + } + return rowStyle; + }); + if (colSpan > 1) { for (let i = 1; i < colSpan; i++) { cell.insertAfter( - $createTableCellNode(TableCellHeaderStates.NO_STATUS).append( + $createTableCellNode(colStyles[i] | rowStyles[0]).append( $createParagraphNode(), ), ); } cell.setColSpan(1); } + if (rowSpan > 1) { - const [map, cellMap] = $computeTableMap(grid, cell, cell); - const {startColumn, startRow} = cellMap; let currentRowNode; for (let i = 1; i < rowSpan; i++) { const currentRow = startRow + i; @@ -707,18 +731,18 @@ export function $unmergeCell(): void { } } if (insertAfterCell === null) { - for (let j = 0; j < colSpan; j++) { + for (let j = colSpan - 1; j >= 0; j--) { $insertFirst( currentRowNode, - $createTableCellNode(TableCellHeaderStates.NO_STATUS).append( + $createTableCellNode(colStyles[j] | rowStyles[i]).append( $createParagraphNode(), ), ); } } else { - for (let j = 0; j < colSpan; j++) { + for (let j = colSpan - 1; j >= 0; j--) { insertAfterCell.insertAfter( - $createTableCellNode(TableCellHeaderStates.NO_STATUS).append( + $createTableCellNode(colStyles[j] | rowStyles[i]).append( $createParagraphNode(), ), ); @@ -739,8 +763,8 @@ export function $computeTableMap( cellA, cellB, ); - invariant(cellAValue !== null, 'Anchor not found in Grid'); - invariant(cellBValue !== null, 'Focus not found in Grid'); + invariant(cellAValue !== null, 'Anchor not found in Table'); + invariant(cellBValue !== null, 'Focus not found in Table'); return [tableMap, cellAValue, cellBValue]; } @@ -752,52 +776,62 @@ export function $computeTableMapSkipCellCheck( const tableMap: TableMapType = []; let cellAValue: null | TableMapValueType = null; let cellBValue: null | TableMapValueType = null; - function write(startRow: number, startColumn: number, cell: TableCellNode) { - const value = { - cell, - startColumn, - startRow, - }; - const rowSpan = cell.__rowSpan; - const colSpan = cell.__colSpan; - for (let i = 0; i < rowSpan; i++) { - if (tableMap[startRow + i] === undefined) { - tableMap[startRow + i] = []; - } - for (let j = 0; j < colSpan; j++) { - tableMap[startRow + i][startColumn + j] = value; - } - } - if (cellA !== null && cellA.is(cell)) { - cellAValue = value; - } - if (cellB !== null && cellB.is(cell)) { - cellBValue = value; + function getMapRow(i: number) { + let row = tableMap[i]; + if (row === undefined) { + tableMap[i] = row = []; } + return row; } - function isEmpty(row: number, column: number) { - return tableMap[row] === undefined || tableMap[row][column] === undefined; - } - const gridChildren = grid.getChildren(); - for (let i = 0; i < gridChildren.length; i++) { - const row = gridChildren[i]; + for (let rowIdx = 0; rowIdx < gridChildren.length; rowIdx++) { + const row = gridChildren[rowIdx]; invariant( $isTableRowNode(row), - 'Expected GridNode children to be TableRowNode', + 'Expected TableNode children to be TableRowNode', ); - const rowChildren = row.getChildren(); - let j = 0; - for (const cell of rowChildren) { + for ( + let cell = row.getFirstChild(), colIdx = 0; + cell != null; + cell = cell.getNextSibling() + ) { invariant( $isTableCellNode(cell), 'Expected TableRowNode children to be TableCellNode', ); - while (!isEmpty(i, j)) { - j++; + // Skip past any columns that were merged from a higher row + const startMapRow = getMapRow(rowIdx); + while (startMapRow[colIdx] !== undefined) { + colIdx++; + } + const value: TableMapValueType = { + cell, + startColumn: colIdx, + startRow: rowIdx, + }; + const {__rowSpan: rowSpan, __colSpan: colSpan} = cell; + for (let j = 0; j < rowSpan; j++) { + if (rowIdx + j >= gridChildren.length) { + // The table is non-rectangular with a rowSpan + // below the last in the table. + // We should probably handle this with a node transform + // to ensure that tables are always rectangular but this + // will avoid crashes such as #6584 + // Note that there are probably still latent bugs + // regarding colSpan or general cell count mismatches. + break; + } + const mapRow = getMapRow(rowIdx + j); + for (let i = 0; i < colSpan; i++) { + mapRow[colIdx + i] = value; + } + } + if (cellA !== null && cellAValue === null && cellA.is(cell)) { + cellAValue = value; + } + if (cellB !== null && cellBValue === null && cellB.is(cell)) { + cellBValue = value; } - write(i, j, cell); - j += cell.__colSpan; } } return [tableMap, cellAValue, cellBValue]; @@ -832,7 +866,7 @@ export function $getNodeTriplet( const grid = row.getParent(); invariant( $isTableNode(grid), - 'Expected TableRowNode to have a parent GridNode', + 'Expected TableRowNode to have a parent TableNode', ); return [cell, row, grid]; }