[lexical-table] Bug Fix: Shift+Arrow no longer throws when the anchor is off the rect corner - #9045
[lexical-table] Bug Fix: Shift+Arrow no longer throws when the anchor is off the rect corner#9045LeSingh1 wants to merge 1 commit into
Conversation
… is off the rect corner
## Description
`$adjustFocusInDirection` needs to know which corner of the selection rect the
anchor sits on, so it can move the opposite corner:
```ts
const rect = $computeTableCellRectBoundary(tableMap, anchorCellValue, focusCellValue);
...
const anchorCorner = getCornerOrThrow(rect, anchorCellValue);
```
`$computeTableCellRectBoundary` does not return the rect through the two cells
— it *grows* that rect until it contains every merged cell that straddles an
edge, so a merged cell can push an edge out past the anchor. When it does,
`getCorner` returns `null` for the third case in its {minColumn, maxColumn,
neither} test and `getCornerOrThrow` fires its invariant, killing the editor
update mid-keystroke. The anchor and focus of a `TableSelection` are never
normalised to rect corners — `$setAnchorCellForSelection` and
`$createTableSelectionFrom` just record the two cell keys — so nothing upstream
prevents it.
Repro: in a 3x3 table merge grid columns 0–1 of row 1, drag-select from
(row 0, column 1) to (row 1, column 2) — the highlight correctly shows the
expanded block — then press Shift+ArrowDown.
`$extractRectCorners`, 24 lines below, asks the identical question about the
same expanded rect and handles the `null` case: it falls back to the corner
opposite the focus, then to the top-left. This gives `$adjustFocusInDirection`
the same three-tier resolution via a shared `getAnchorCorner` helper.
`getCornerOrThrow` had no other call site and is removed with it.
## Test plan
`npx vitest run packages/lexical-table/src/__tests__/unit/ShiftArrowOffCornerAnchor.test.ts`
(The natural test file, `LexicalTableSelectionHelpers.test.ts`, is already
being edited by an open PR, so the repro lives in its own file.)
### Before
```
FAIL ... > Shift+Arrow with an anchor that is not on a rect corner > extends the selection instead of throwing
AssertionError: expected [Function] to not throw an error but 'Error: getCornerOrThrow: cell 7 is no…' was thrown
- Expected:
undefined
+ Received:
"Error: getCornerOrThrow: cell 7 is not at a corner of rect"
```
### After
```
Test Files 1 passed (1)
Tests 1 passed (1)
```
Also green:
- `npx vitest run packages/lexical-table` — 13 files, 158 tests
- `E2E_BROWSER=chromium npx playwright test --project=chromium Tables.spec.mjs` — 88 passed, 1 skipped
(this suite includes the shift-selection groups that exercise `$adjustFocusInDirection`)
Only chromium was exercised locally for the e2e run.
|
@LeSingh1 is attempting to deploy a commit to the Meta Open Source Team on Vercel. A member of the Team first needs to authorize it. |
|
Confirmed the bug on |
| editor.read('latest', () => { | ||
| const selection = $getSelection(); | ||
| assert($isTableSelection(selection), 'expected a TableSelection'); | ||
| expect(selection.isValid()).toBe(true); |
There was a problem hiding this comment.
Two coverage gaps here:
isValid()passes for any coherentTableSelection, so a future change to the fallback branch would go undetected. The opposite-of-focus path produces deterministic cells worth pinning:
const [map] = $computeTableMapSkipCellCheck($table(), null, null);
expect(selection.anchor.key).toBe(map[0][0].cell.getKey()); // top-left (oppositeCorner of focus's maxColumn/maxRow)
expect(selection.focus.key).toBe(map[2][2].cell.getKey()); // bottom-right after one row down(Verified — these are the actual post-keystroke values.)
- Only
KEY_ARROW_DOWN_COMMANDis exercised. Each of the four arrow directions takes a distinct branch in$adjustFocusInDirection— Up, Left, and Right should also be covered.
| if (focusCorner) { | ||
| return oppositeCorner(focusCorner); | ||
| } | ||
| return ['minColumn', 'minRow']; |
There was a problem hiding this comment.
When both anchor and focus are off-corner, getAnchorCorner returns ['minColumn', 'minRow'], making $adjustFocusInDirection treat maxColumn/maxRow as the focus side. For direction === 'up' and direction === 'backward' this means focus moves inward rather than outward. $extractRectCorners carries a TODO acknowledging the same arbitrariness ("use the closest corner instead") — worth mirroring that note here so the limitation is visible at this call site too.
| import {afterEach, assert, beforeEach, describe, expect, test} from 'vitest'; | ||
|
|
||
| let container: HTMLDivElement; | ||
| let editor: ReturnType<typeof buildEditorFromExtensions>; |
There was a problem hiding this comment.
LexicalTableSelectionHelpers.test.ts (the sibling file in this package) declares the editor as let editor: LexicalEditorWithDispose — the named export from @lexical/extension. ReturnType<typeof buildEditorFromExtensions> is equivalent but inconsistent with the local pattern.
… grid indices, cell state and boundary selection ## Description Eleven small table bugs, consolidated into one branch per the review feedback on facebook#9027 and facebook#9035. They fall into three groups. ### 1. Grid geometry Code that uses a row's child index where the table *grid* column index is required, or that ignores a span. These agree only while every cell has `colSpan === 1`/`rowSpan === 1`, so they are invisible until a table has merged cells. - `$insertTableColumnAtNode` (`packages/lexical-table/src/LexicalTableUtils.ts`) walked leftwards from the insertion column looking for the cell to insert after. When every grid position at or before the insertion column is covered by a `rowSpan` from an earlier row, the walk fell off the left edge and appended the new cell to the *end* of the row, so the inserted column did not line up. Replaced with the same left-to-right map scan `$unmergeCellNode` uses: track the last row-map entry with `startRow === i` at or before the insertion column, `insertAfter` it, or `$insertFirst` when the row owns nothing to the left. This also removes the labeled `continue`. (facebook#8836) - `toggleTableColumnIsHeader` (`packages/lexical-playground/src/plugins/TableActionMenuPlugin/index.tsx`) fed `$setTableColumnIsHeader`, which indexes `gridMap[row][columnIndex]`, the result of `$getTableColumnIndexFromTableCellNode` — the cell's index among its row's *children*. With an earlier spanning cell in the row, "Toggle column header" applied to a column left of the one clicked. It now reads `cellMap.startColumn` from `$computeTableMap`. (facebook#9006) ### 2. Cell and table state Transforms that rebuild a cell or its paragraph and drop what it carried, plus the `dir` attribute being written onto an element that the export throws away. - `$insertTableIntoGrid` (`LexicalTableUtils.ts`) copies the template cell's `backgroundColor` onto the destination cell but not `verticalAlign`, which was added to `TableCellNode` after that loop (facebook#7077). Pasting a table with vertically aligned cells over an existing table dropped the alignment. (facebook#9015) - `$tableTransform` (`LexicalTablePluginHelpers.ts`) pads short rows out to the table width with `$createTableCellNode()` and no header state, so a short header row was repaired into a mix of `<th>` and `<td>`. The padding cells now inherit the `TableCellHeaderStates.ROW` bit from the row's last cell — the same reference `$insertTableColumnAtNode` uses when it appends a column. (facebook#9023) - `$unmergeCellNode` (`LexicalTableUtils.ts`) recomputes the header state of the cells it splits off, correctly, but gives them nothing else, so a filled merged cell unmerged into one filled cell and a run of blank ones. The split cells now keep the original's `backgroundColor` and `verticalAlign`, which is the cell's own presentation and not a description of its row or column. (facebook#9025) - `TableNode.exportDOM` (`LexicalTableNode.ts`) narrows the export to `element.querySelector('table')`. When scrollable tables are active `createDOM` returns the scroll wrapper `<div>`, which is where `ElementNode.exportDOM` writes `dir` — so the wrapper, and the direction with it, was thrown away. The direction is now re-applied to the `<table>`, which is where `$convertTableElement` reads it back. (facebook#9029) - `TableObserver.$clearText` (`LexicalTableObserver.ts`) emptied each selected cell by constructing a fresh `ParagraphNode`, so Backspace over a multi-cell selection also reset the format, style, direction and indent of those cells. It now `$copyNode`s the cell's existing paragraph, which carries the element state and returns a childless copy. (facebook#9041) ### 3. Selection and caret at the table boundary - The `KEY_ARROW_LEFT_COMMAND`/`KEY_ARROW_RIGHT_COMMAND` handlers in `registerRichText` (`packages/lexical-rich-text/src/index.ts`) had no equivalent of the `$isSelectionAtEndOfRoot`/`$isSelectionAtStartOfRoot` guards the up/down handlers already use. With a table as the last node of the document the key fell through to the native caret, which walks around the block cursor element, so the caret cycled: last cell → block cursor → root offset before the table → back into the last cell. A new `$isBlockCursorAtRootEdge` consumes the key when the collapsed element point sits at the root edge beside a child that needs a block cursor. Non-collapsed selections and block cursors that still have a sibling to move into are untouched. (facebook#8949, fixes facebook#7999) - `$handleArrowKey` (`LexicalTableSelectionHelpers.ts`) armed the Firefox scrollable-table workaround (`setShouldCheckSelectionForTable`) on every ArrowDown. From the block cursor below a trailing table ArrowDown moves nothing, so the flag survived to be consumed by the next selection change — an ArrowUp back into the table — which snapped the caret to the first cell. A new `$isSelectionBeforeTable` arms it only when the focus is before the table in document order, the only side ArrowDown can enter from. (facebook#8963, fixes facebook#6822) - `$deleteTableRowAtSelection` (`LexicalTableUtils.ts`) removes the table outright when the selection covers every row, but never moved the selection first, leaving a `TableSelection` anchored to detached nodes. Added the `grid.selectPrevious()` that `$deleteTableColumnAtSelection` and `TableObserver.$clearText` already do in their identical branches. (facebook#9037) - `$adjustFocusInDirection` (`LexicalTableSelectionHelpers.ts`) called `getCornerOrThrow` on the rect from `$computeTableCellRectBoundary`, which *grows* the rect until it contains every merged cell straddling an edge — so the anchor is not guaranteed to be at a corner, and Shift+Arrow killed the editor update with an invariant. `getCornerOrThrow` is replaced by `getAnchorCorner`, which falls back the same way `$extractRectCorners` 24 lines below already does: to the corner opposite the focus, then to the top-left. (facebook#9045) ## Test plan Unit tests cover ten of the eleven fixes, two browser tests cover the caret/arrow-key behaviour at the table boundary, and facebook#9006 is a Playwright spec (`packages/lexical-playground/__tests__/regression/7266-column-header-merged-cells.spec.mjs`). ### Before Source fixes reverted, new tests kept: ``` $ npx vitest run --project unit packages/lexical-table packages/lexical-rich-text × pasting a table carries the cell backgroundColor and verticalAlign 6ms × a short header row is padded with header cells 5ms × extends the selection instead of throwing 73ms × keeps the paragraph format, style and direction of each cell 89ms × inserts the new cell in the correct column for rows spanned by a rowSpan cell 7ms × exports dir with scrollable tables active 31ms × leaves the selection outside the removed table 13ms × ArrowRight at the block cursor after the last block stays put 5ms × ArrowLeft at the block cursor before the first block stays put 1ms × the cells a merged cell splits into keep its backgroundColor and verticalAlign 13ms ⎯⎯⎯⎯⎯⎯⎯ Failed Tests 10 ⎯⎯⎯⎯⎯⎯⎯ Test Files 8 failed | 23 passed (31) Tests 10 failed | 291 passed (301) $ npx vitest run --project browser packages/lexical-table/src/__tests__/browser/TableTrailingCaret.test.ts packages/lexical-table/src/__tests__/browser/Issue6822BlockCursorArrowKeys.test.ts × the caret stops beneath a trailing table instead of cycling around it 172ms × ArrowUp from the block cursor below a trailing table returns to the last row 380ms × ArrowUp from the block cursor below a table that is the only root child returns to the last row 355ms Test Files 2 failed (2) Tests 3 failed | 2 passed (5) ``` The remaining new cases pass on both sides of the change — the unstyled-unmerge, short-body-row, non-wrapped-export, delete-every-column and still-owns-a-cell-to-the-left cases — which is what pins each asymmetry rather than the fix itself. ### After ``` $ npx vitest run --project unit packages/lexical-table packages/lexical-rich-text Test Files 31 passed (31) Tests 301 passed (301) $ npx vitest run --project browser packages/lexical-table/src/__tests__/browser/TableTrailingCaret.test.ts packages/lexical-table/src/__tests__/browser/Issue6822BlockCursorArrowKeys.test.ts Test Files 2 passed (2) Tests 5 passed (5) $ npx playwright test --project=chromium Tables.spec.mjs TablesHTMLCopyAndPaste.spec.mjs 7266-column-header-merged-cells.spec.mjs 99 passed, 1 skipped $ npx tsc --noEmit -p . (clean) ``` Supersedes facebook#8836, facebook#8949, facebook#8963, facebook#9006, facebook#9015, facebook#9023, facebook#9025, facebook#9029, facebook#9037, facebook#9041, facebook#9045, consolidated per the review feedback on facebook#9027 and facebook#9035. facebook#9000 and facebook#9011 were part of an earlier revision of this PR and have been dropped, because each contradicted an existing e2e expectation rather than extending it: - facebook#9000 imported `dir` in the `DOMImportExtension` table rules. Google Sheets emits `<table dir="ltr">` unconditionally, so `Copy + paste (Table - Google Sheets)` in `TablesHTMLCopyAndPaste.spec.mjs` went from `dir="auto"` to a pinned `dir="ltr"` on the table, and the rows below it stopped emitting `dir="auto"` at all. Honouring a pasted `dir` is a deliberate behaviour change for third-party HTML and needs its own PR. - facebook#9011 made the right-edge resizer widen the *last* grid column a merged cell spans instead of its first. `Resize merged cells width (2)` in `Tables.spec.mjs` pins the existing behaviour, and both variants widen the merged cell identically — they differ only in which unmerged column absorbs the change, so this is a behaviour preference that needs a maintainer call.
Description
$adjustFocusInDirectionneeds to know which corner of the selection rect theanchor sits on, so it can move the opposite corner:
$computeTableCellRectBoundarydoes not return the rect through the two cells— it grows that rect until it contains every merged cell that straddles an
edge, so a merged cell can push an edge out past the anchor. When it does,
getCornerreturnsnullfor the third case in its {minColumn, maxColumn,neither} test and
getCornerOrThrowfires its invariant, killing the editorupdate mid-keystroke. The anchor and focus of a
TableSelectionare nevernormalised to rect corners —
$setAnchorCellForSelectionand$createTableSelectionFromjust record the two cell keys — so nothing upstreamprevents it.
Repro: in a 3x3 table merge grid columns 0–1 of row 1, drag-select from
(row 0, column 1) to (row 1, column 2) — the highlight correctly shows the
expanded block — then press Shift+ArrowDown.
$extractRectCorners, 24 lines below, asks the identical question about thesame expanded rect and handles the
nullcase: it falls back to the corneropposite the focus, then to the top-left. This gives
$adjustFocusInDirectionthe same three-tier resolution via a shared
getAnchorCornerhelper.getCornerOrThrowhad no other call site and is removed with it.Test plan
npx vitest run packages/lexical-table/src/__tests__/unit/ShiftArrowOffCornerAnchor.test.ts(The natural test file,
LexicalTableSelectionHelpers.test.ts, is alreadybeing edited by an open PR, so the repro lives in its own file.)
Before
After
Also green:
npx vitest run packages/lexical-table— 13 files, 158 testsE2E_BROWSER=chromium npx playwright test --project=chromium Tables.spec.mjs— 88 passed, 1 skipped(this suite includes the shift-selection groups that exercise
$adjustFocusInDirection)Only chromium was exercised locally for the e2e run.