Skip to content

[lexical-table] Bug Fix: Infer column header state from position during DOM import#8259

Merged
etrepum merged 3 commits intofacebook:mainfrom
Sathvik-Chowdary-Veerapaneni:table-column-header-import-8090
Mar 28, 2026
Merged

[lexical-table] Bug Fix: Infer column header state from position during DOM import#8259
etrepum merged 3 commits intofacebook:mainfrom
Sathvik-Chowdary-Veerapaneni:table-column-header-import-8090

Conversation

@Sathvik-Chowdary-Veerapaneni
Copy link
Copy Markdown
Contributor

@Sathvik-Chowdary-Veerapaneni Sathvik-Chowdary-Veerapaneni commented Mar 26, 2026

Description

When importing a <th> element without a scope attribute, the header state is now inferred from position instead of always defaulting to ROW:

  • First row <th>ROW header state
  • First column <th>COLUMN header state
  • First row + first column → BOTH

This fixes the issue where column headers were not correctly propagated when inserting new rows via $insertTableRowAtSelection, because <th> elements in header columns were incorrectly marked as ROW instead of COLUMN.

Closes #8090

Test plan

Before

  • <th> elements imported via DOM always defaulted to ROW header type regardless of position
  • Inserting new rows lost column header state

After

  • Added 4 new unit tests for positioned <th> elements:
    • <th> in first row first column → BOTH
    • <th> in first column of non-first row → COLUMN
    • <th> in <thead> non-first column → ROW
    • Detached <th>ROW (unchanged fallback)
  • All existing unit tests pass
  • pnpm run ci-check passes

When a <th> element has no scope attribute, infer header state from
its position in the table instead of always defaulting to ROW:
- First row <th> → ROW header state
- First column <th> → COLUMN header state
- First row + first column → BOTH

This ensures that column headers imported from HTML are correctly
propagated when inserting new rows via $insertTableRowAtSelection.

Closes facebook#8090
Add tests verifying that <th> elements get correct header state
based on position: BOTH for first-row first-column, COLUMN for
first-column in non-first row, and ROW for non-first-column in thead.
@vercel
Copy link
Copy Markdown

vercel bot commented Mar 26, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
lexical Ready Ready Preview, Comment Mar 26, 2026 8:35pm
lexical-playground Ready Ready Preview, Comment Mar 26, 2026 8:35pm

Request Review

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 26, 2026
// Infer header state from position when no scope is specified
const parentRow = domNode_.parentElement;
const isFirstRow =
parentRow instanceof HTMLTableRowElement &&
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's best to avoid using instanceof for DOM because it won't work across certain contexts which is why we have isHTMLElement and comparisons with tagName instead.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, switched to isHTMLElement() with tagName checks

const parentRow = domNode_.parentElement;
const isFirstRow =
parentRow instanceof HTMLTableRowElement &&
(parentRow.parentElement?.nodeName.toLowerCase() === 'thead' ||
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For mostly historical reasons nullish coalescing isn't used in lexical outside of tests and the playground

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, replaced with an explicit isHTMLElement() guard

headerState = TableCellHeaderStates.ROW;
// Infer header state from position when no scope is specified
const parentRow = domNode_.parentElement;
const isFirstRow =
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name is a bit misleading because if it's in a thead it may not be the first row

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, renamed to isInHeaderRow

Comment on lines +368 to +371
// Fallback for detached elements
if (headerState === TableCellHeaderStates.NO_STATUS) {
headerState = TableCellHeaderStates.ROW;
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to worry about detached elements, the unit test can be updated to have semantically correct input with a table ancestor. The extension and legacy plugin for tables has transforms that would remove any TableCellNode or TableRowNode that doesn't have the correct TableNode -> TableRowNode -> TableCellNode hierarchy.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated the test to use a proper table > tr > th structure

@etrepum
Copy link
Copy Markdown
Collaborator

etrepum commented Mar 26, 2026

@etrepum
Copy link
Copy Markdown
Collaborator

etrepum commented Mar 26, 2026

pnpm run ci-check is failing and must pass for any PR before it can be accepted

… structure

- Replace instanceof HTMLTableRowElement with isHTMLElement() and tagName checks
- Remove nullish coalescing in favor of explicit isHTMLElement guards
- Rename isFirstRow to isInHeaderRow for clarity
- Remove unnecessary comments
- Update test to use proper table > tr > th structure instead of detached element
@Sathvik-Chowdary-Veerapaneni Sathvik-Chowdary-Veerapaneni changed the title table: infer column header state from position during DOM import [lexical-table] Bug Fix: Infer column header state from position during DOM import Mar 26, 2026
@etrepum etrepum added the extended-tests Run extended e2e tests on a PR label Mar 27, 2026
@Sathvik-Chowdary-Veerapaneni
Copy link
Copy Markdown
Contributor Author

updated the PR body to match the template (Description + Before/After test plan). ci-check passes locally — tsc, flow, prettier, lint all clean.

@etrepum etrepum added this pull request to the merge queue Mar 28, 2026
Merged via the queue into facebook:main with commit 7f061d3 Mar 28, 2026
64 checks passed
@etrepum etrepum mentioned this pull request Apr 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. extended-tests Run extended e2e tests on a PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Table header status is not set correctly on table header columns

2 participants