[lexical-table] Bug Fix: export the table's dir when scrollable tables are active - #9029
Closed
LeSingh1 wants to merge 1 commit into
Closed
[lexical-table] Bug Fix: export the table's dir when scrollable tables are active#9029LeSingh1 wants to merge 1 commit into
LeSingh1 wants to merge 1 commit into
Conversation
…s are active
## Description
`$convertTableElement` reads `dir` back off the `<table>` on import. On the way
out it is written by `ElementNode.exportDOM`, which sets it on whatever
`createDOM` returned — and when scrollable tables are active `TableNode.createDOM`
returns the scroll **wrapper** `<div>`, not the `<table>`.
`TableNode.exportDOM` then narrows the export down to `element.querySelector('table')`
and throws the wrapper away, taking the `dir` with it. So the direction survives
only when `hasHorizontalScroll` is off; with the default config (and in the
playground) copying an RTL table produces `<table>` with no direction, and
re-importing that HTML no longer produces the node it came from.
This resolves the exported element once and re-applies the node's direction to
it when the wrapper was dropped. A table with no explicit direction, and the
non-wrapped case where `ElementNode.exportDOM` already wrote `dir` onto the
`<table>` itself, are both unaffected.
## Test plan
`npx vitest run packages/lexical-table/src/__tests__/unit/TableExportDirection.test.ts`
The two cases are the same table exported with `hasHorizontalScroll` on and off;
the second passes on both sides of the change, which is what pins the wrapper as
the cause.
### Before
```
❯ |unit| packages/lexical-table/src/__tests__/unit/TableExportDirection.test.ts (2 tests | 1 failed) 33ms
× exports dir with scrollable tables active 29ms
FAIL ... > TableNode.exportDOM direction > exports dir with scrollable tables active
AssertionError: expected '<table><tbody><tr><td style="border: …' to contain '<table dir="rtl">'
Expected: "<table dir="rtl">"
Received: "<table><tbody><tr><td style="border: 1px solid black; width: 75px; vertical-align: top; text-align: start;"><p><br></p></td></tr></tbody></table>"
```
### After
```
Test Files 1 passed (1)
Tests 2 passed (2)
```
Also green:
- `npx vitest run packages/lexical-table` — 12 files, 159 tests
- `npx vitest run packages/lexical-clipboard packages/lexical-html packages/lexical-markdown` — 15 files, 541 tests
LeSingh1
requested review from
acywatson,
etrepum,
fantactuka,
ivailop7,
potatowagon and
zurfyx
as code owners
August 9, 2026 05:46
|
@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. |
meta-cla
Bot
added
the
CLA Signed
LeSingh1
added a commit
to LeSingh1/lexical
that referenced
this pull request
Aug 10, 2026
… 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
$convertTableElementreadsdirback off the<table>on import. On the wayout it is written by
ElementNode.exportDOM, which sets it on whatevercreateDOMreturned — and when scrollable tables are activeTableNode.createDOMreturns the scroll wrapper
<div>, not the<table>.TableNode.exportDOMthen narrows the export down toelement.querySelector('table')and throws the wrapper away, taking the
dirwith it. So the direction survivesonly when
hasHorizontalScrollis off; with the default config (and in theplayground) copying an RTL table produces
<table>with no direction, andre-importing that HTML no longer produces the node it came from.
This resolves the exported element once and re-applies the node's direction to
it when the wrapper was dropped. A table with no explicit direction, and the
non-wrapped case where
ElementNode.exportDOMalready wrotedironto the<table>itself, are both unaffected.Test plan
npx vitest run packages/lexical-table/src/__tests__/unit/TableExportDirection.test.tsThe two cases are the same table exported with
hasHorizontalScrollon and off;the second passes on both sides of the change, which is what pins the wrapper as
the cause.
Before
After
Also green:
npx vitest run packages/lexical-table— 12 files, 159 testsnpx vitest run packages/lexical-clipboard packages/lexical-html packages/lexical-markdown— 15 files, 541 tests