feat(Block Editor): implement table handles and popovers

[rjvelazco]

Proposed Changes

Implements accessibility-aware table editing in libs/new-block-editor per #35720, plus the surrounding UX.

Schema additions on the table node (all stored as JSON attrs, headless-consumable):

• caption (string) — renders as <caption> in the output
• ariaLabel — renders as aria-label on <table>
• ariaLabelledby — renders as aria-labelledby on <table>
• scope on tableHeader cells (col / row / colgroup / rowgroup) — auto-assigned by a ProseMirror plugin (col on first row, row on first column), preserving any explicit author override

UX — Notion-style contextual table editing:

• The old table sub-toolbar (10+ buttons cluttering the main toolbar) is gone. The toolbar now keeps only "Insert table" + a new "Table properties" (table_edit) button for the a11y form (caption + aria-label + aria-labelledby).
• Per-cell handles rendered inline via addNodeView() on tableCell/tableHeader — pure CSS positioning, no floating-ui:
  • Column handle (top of column) — Insert left/right, Toggle column header, scope override (when in <th>), Delete column
  • Row handle (left of row) — Insert above/below, Toggle row header, Delete row
  • Selection handle (right edge of multi-cell selection, vertically centered) — Merge cells / Split cell with reactive disabled from editor.can(). Handles vertical centering correctly across rowspan'd merged cells.
• The three popovers were consolidated into a single TableHandlePopoverComponent driven by COLUMN_ACTIONS / ROW_ACTIONS / SELECTION_ACTIONS constants — adding a new action is one entry in the ACTIONS registry + one append to the right list.

Checklist

• Tests (skipped per current new-block-editor convention — verified via nx lint + tsc --noEmit)
• Translations (14 new i18n keys in Language.properties)
• Security Implications Contemplated (no new input surfaces beyond plain-text attribute writes that round-trip through standard TipTap serialization)

Additional Info

Still outstanding (tracked in libs/new-block-editor/todo.md):

• Update the VTL file to render the new table attributes (caption, aria-label, aria-labelledby, scope) for server-rendered (non-headless) consumers so they get accessible markup too.

Legacy libs/block-editor: out of scope for this PR. The schema is forward-compatible — content authored here remains readable by the legacy editor (unknown attrs are preserved). A separate decision on backporting will follow.

Video

video.mov

This PR fixes: #35720

[claude]

Claude finished @rjvelazco's task in 4m 0s —— View job

---

Review

Main finding is downstream — the SDK renderers don't honor the cell-level tableHeader type, which makes the new scope attribute meaningless for headless consumers in the most common a11y case (row-header tables).

🔴 Blocker — SDK renderers ignore cell type and force header purely by row index

core-web/libs/sdk/angular/src/lib/components/dotcms-block-editor-renderer/blocks/table.component.ts:17-51 and the React equivalent at core-web/libs/sdk/react/src/lib/next/components/DotCMSBlockEditorRenderer/components/blocks/Table.tsx:43-69 both do content.slice(0, 1) → <th> inside <thead>, and content.slice(1) → <td> inside <tbody>. The actual cellNode.type (tableHeader vs tableCell) is ignored.

Practical impact, given this PR explicitly adds scope on tableHeader and auto-assigns scope="row" for the first column:

• Toggle a column header (the canonical row-header table for screen readers): cells in rows 1..N are stored as tableHeader with scope="row", but the SDK still emits them as <td>. The new accessibility attribute round-trips through the editor but vanishes for any headless renderer. The feature is broken end-to-end for the row-header use case.
• A table without any header row: row 0's tableCell nodes get force-rendered as <th> inside <thead>. Wrong markup, wrong semantics.
• Mixed headers (corner cell <th>, others <td>): the corner pattern is unrepresentable.

The VTL renderer at render.vtl:57-60 does this correctly (branches on element.type == "tableHeader"). The SDK renderers should do the same — pick tag per cell, and use the row's first cell type (or any heuristic that matches the data) to decide thead/tbody, or just emit a flat <tbody> with per-cell <th>/<td>. The latter is closer to what TipTap stores and what most accessibility guides recommend.

Fix this →

🟠 Selection handle vertical-centering misplaces for rowspan > 2 inside even-row selections

table-active-cells.plugin.ts:155-166 only checks whether the anchor cell extends one row up:

const anchorCellSpansBoundary =
    !isOdd &&
    anchorRow > rect.top &&
    map.map[(anchorRow - 1) * map.width + anchorCol] === cellOffset;

If the anchor cell is merged across only a subset of the selection (e.g. 4-row selection, anchor cell spans rows 0..2 → 3 rows), the cell at anchorRow=2 IS the merged cell, the cell at anchorRow-1=1 is also the merged cell → check returns true → centered variant → handle lands at the merged cell's visual middle (somewhere in row 1), not at the selection's center (the boundary between rows 1 and 2). The simpler invariant is "is the merged cell's vertical span the same as the selection's" — anything else needs explicit per-row math against map.map.

This is an edge case (multi-row rowspan + even selection height), but it's the exact case the comment claims to handle.

🟡 i18n — hardcoded English keys

table-properties-popover.component.ts:108 and :115 pass 'Cancel' / 'Apply' literals to the dm pipe. Those keys do exist in Language.properties (lines 332/555) so it resolves, but the rest of the editor uses dot.common.cancel / dot.common.apply. Inconsistent; will get flagged by future i18n audits.

🟡 Auto-scope plugin walks the full doc on every doc-changing transaction

table-scope-auto-assign.plugin.ts:79-83 calls buildAutoScopeTransaction(newState) for any transaction with docChanged, which descendants() the whole doc looking for tables. Fine for normal docs; will start to show up for very long block-editor fields that get heavy typing. A cheap mitigation is to skip when no transaction touched a table — tr.mapping.maps ranges intersected with table positions — but it's a perf nice-to-have, not a correctness issue.

🟡 Synchronous dispatch inside the plugin's view init

table-scope-auto-assign.plugin.ts:74-78 dispatches a transaction from the view(view) constructor. ProseMirror tolerates this in practice but it's the kind of thing that bites you when another plugin's init runs after this one and reads a now-stale state snapshot. A microtask-deferred dispatch (queueMicrotask(() => view.dispatch(...))) is the safer pattern for "first paint normalization."

🟢 Things that look right

• The popover-id-array support on <dot-editor-popover> is a clean way to host three variants under one shell.
• Closure capture in the cell NodeView (currentNode updated in update()) is the right pattern for keeping click handlers reading current attrs without re-binding listeners.
• VTL renderer (render.vtl:52, table.vtl:3) escapes the new string attrs via $esc.html — XSS-safe.
• Schema additions on tableHeader.scope default to null and renderHTML skips when empty — backward-compatible with existing stored content.
• runColumnCommand/runRowCommand dispatch two transactions per action (selection-set, then the actual command). Selection-only transactions don't enter the undo history so it's not user-visible — just code smell; collapsing into one chain would be cleaner.

Out-of-scope but worth noting

• The PR body mentions tests are skipped per new-block-editor convention. The two plugins (table-active-cells.plugin.ts and table-scope-auto-assign.plugin.ts) have non-trivial math that would benefit from unit tests — especially the merged-cell anchor selection code. computeAutoScope is even already exported "for unit testing" but has no test. Worth adding before this surface grows.

---

[rjvelazco]

@claude Review these changes, please

[claude]

Claude finished @rjvelazco's task in 4m 17s —— View job

---

Review

The popover / NodeView consolidation since the prior review reads cleanly — one shell + an ACTIONS registry is a real improvement over the three-component setup. The merged-cell anchor fix lands a partial improvement. The SDK blocker from last review is still unaddressed; surfacing it again with reduced detail since the analysis hasn't changed.

🔴 Blocker — SDK renderers still ignore cellNode.type (unchanged from prior review)

core-web/libs/sdk/angular/src/lib/components/dotcms-block-editor-renderer/blocks/table.component.ts:17-51 and core-web/libs/sdk/react/src/lib/next/components/DotCMSBlockEditorRenderer/components/blocks/Table.tsx:42-70 still split rows by index — content.slice(0, 1) → <th> inside <thead>, content.slice(1) → <td> inside <tbody> — and never branch on cellNode.type. Result: the row-header table that this PR's auto-scope plugin actively produces (toggle column header → cells in rows 1..N stored as tableHeader with scope="row") still serializes to <td> for any consumer of the SDK renderers. The new scope attribute round-trips through the editor and disappears for headless consumers in the most common a11y case the feature is designed for.

Reference behaviour already exists in this PR — render.vtl:57-60 branches on element.type == "tableHeader". The SDK should mirror it: per-cell tag selection (<th> when cellNode.type === 'tableHeader', <td> otherwise) and decide thead/tbody split — or skip the split and emit a flat <tbody> — based on the actual cell data rather than row index.

The PR body's "outstanding: update the VTL file" line is already done by commit 8f19a71; what remains outstanding is the SDK side, which is the part that breaks for headless consumers.

Fix this →

🟠 Selection-anchor fix is correct for full-coverage merged cells, incomplete for partial rowspans

The new check at table-active-cells.plugin.ts:158-161 handles the simple case (anchor cell merged across rows N-1 and N inside an even-row selection → use centered variant) — that closes the visually obvious bug. But for an anchor cell whose rowspan covers more rows than just the two middle ones, the centered variant still places the handle at the merged cell's own visual middle, which only coincides with the selection's middle when the rowspan is symmetric around the selection's center.

Concrete failure case — 4-row selection, anchor column has a rowspan-3 cell covering rows 0..2:

• rect.top=0, rect.bottom=4, rowCount=4, anchorRow = 0 + 2 = 2.
• map.map[1 * width + col] and map.map[2 * width + col] both resolve to the same merged-cell offset → anchorCellSpansBoundary = true → centered variant.
• Centered variant puts the handle at the merged cell's vertical middle = middle of row 1.
• Selection's vertical middle = boundary between rows 1 and 2.
• Handle lands ~half a row above where it should.

The robust invariant is "place the handle at the y-coordinate halfway between rect.top and rect.bottom, in the rightmost column" — computed directly from the cell rects rather than picking one cell + a CSS variant. Anything cell-based has to enumerate every cell in the rightmost column of the selection and pick one whose top/bottom edge sits at the selection's vertical midpoint; the current heuristic doesn't.

Still an edge case (multi-row rowspan + even selection height), but it's the exact scenario the fix was meant to cover.

🟡 Auto-scope plugin still walks the full doc on every doc-changing transaction

table-scope-auto-assign.plugin.ts:79-83 runs buildAutoScopeTransaction(newState) on every docChanged transaction; buildAutoScopeTransaction then descendants()s the whole doc looking for tables. Acceptable for typical docs, will start to show up on long block-editor fields with heavy typing. Cheap mitigation: pre-filter transactions by checking whether any of their mapping.maps intersect a table range; only then re-scan. Not correctness, just a perf nice-to-have.

🟡 Synchronous view.dispatch from plugin's view(view) constructor

table-scope-auto-assign.plugin.ts:74-78 still dispatches from view(view). ProseMirror tolerates this in practice, but it's the kind of init-time foot-gun that bites when a later plugin's view() reads a stale state snapshot it captured before the dispatch landed. queueMicrotask(() => view.dispatch(...)) is the safer pattern for "first-paint normalization."

🟡 i18n inconsistency — 'Cancel' / 'Apply' literal keys

table-properties-popover.component.ts:108 and :115 use 'Cancel' / 'Apply' literal keys (resolve via Language.properties:332, 555). Everywhere else in the editor uses dot.common.cancel / dot.common.apply (Language.properties:1291, 1293). Resolves correctly today, just looks out-of-pattern; an i18n audit will flag it.

🟡 NodeView handles aren't keyboard-accessible

table-extensions.ts:173 sets tabindex="-1" on each handle button and click is bound on mousedown only — keyboard users have no path to the column/row/selection popovers. Given this PR's theme is a11y, worth a follow-up: either let the handles enter the tab order when their cell becomes active, or add a keyboard alternative (e.g. the toolbar Table properties button hosts the same actions when the caret is inside a table).

🟡 Per-cell DOM cost — three buttons per cell, always

Every <td> / <th> NodeView constructs three <button> elements + icon <span>s on creation (table-extensions.ts:79-90). For a 20×20 table that's 1,200 DOM nodes whose only purpose is to react to a CSS visibility class. Lazy-creating handles only when the cell first enters the is-active-* state (and pooling them across cells) would cut the baseline cost, but not urgent.

🟢 Looks right

• Single popover shell with popoverId: PopoverId | readonly PopoverId[] is a clean carrier for the three-variant unification — editor-popover.component.ts:53,62-66 makes the multi-id semantics explicit, no behavior regression vs the per-variant components.
• Closure-over-currentNode in the NodeView (table-extensions.ts:131,138) is the right pattern for letting click handlers see the latest attrs without rebinding listeners.
• DotTable.addAttributes (table-extensions.ts:23-39) renders nothing when the value is empty/null — keeps stored content backward-compatible.
• VTL renderer (render.vtl:52, 58, table.vtl:3-4) $esc.html-escapes all three new string attrs — XSS-safe.
• TableScopeAutoAssign.appendTransaction (table-scope-auto-assign.plugin.ts:79-82) gates on tr.docChanged AND skips its own self-tagged transactions — no recursive amplification.

Out-of-scope but worth flagging

• Pre-existing in the SDK Angular renderer (not introduced by this PR): track rowNode.type / track cellNode.type at table.component.ts:18,20,36,38. All sibling cells share the same type string, which is a duplicate-key violation under Angular's @for track. NG0955 in dev. Whoever fixes the cell-type renderer bug should also switch these to track $index (or compose with the cellNode reference).
• computeAutoScope is exported "for unit-testing" but there's still no test. The auto-scope rules + the merged-cell anchor math in table-active-cells.plugin.ts are the two pieces of non-trivial logic that genuinely benefit from a vitest spec, even if the broader new-block-editor convention is no tests.
  · Branch