Skip to content

feat(Block Editor): implement table handles and popovers#35740

Open
rjvelazco wants to merge 4 commits into
mainfrom
issue-35720-block-editor-add-accessibility-attributes-to-tables-scope-on-th-caption-aria-label-aria-labelledby
Open

feat(Block Editor): implement table handles and popovers#35740
rjvelazco wants to merge 4 commits into
mainfrom
issue-35720-block-editor-add-accessibility-attributes-to-tables-scope-on-th-caption-aria-label-aria-labelledby

Conversation

@rjvelazco
Copy link
Copy Markdown
Member

@rjvelazco rjvelazco commented May 18, 2026

WIP

TODO: I need to update the vtl file to render the new attributes in the table node

This PR fixes: #35720

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 18, 2026

Claude finished @rjvelazco's task in 3m 57s —— View job


PR Review — table handles & a11y popovers (#35720)

This is flagged WIP and the PR description already lists the VTL render gap; below are the issues I'd want addressed before merge, ordered by severity.

Blockers

1. caption is parsed in but never rendered out. DotTable.addAttributes().caption.renderHTML returns {} and nothing in the extension emits a <caption> child node. (table-extensions.ts:21) Result: editing the caption via the popover stores the attribute, but the user sees nothing change in the editor surface, and editor.getHTML() drops it on the next round-trip. This is bigger than the VTL gap called out in the PR body — the round-trip is broken on the client too, so a JSON-stored caption survives only as long as the user never re-parses through HTML.

2. Auto-scope plugin mutates the doc on load and emits a dirty event. The view: initializer dispatches a tx synchronously when legacy tables have unset scopes (table-scope-auto-assign.plugin.ts:74-78). That tx flows through onUpdatevalueChange.emit → CVA onChange, marking every existing form dirty on open with no user action. For customers with hundreds of legacy story-blocks, this'll trip "unsaved changes" prompts everywhere. Options: gate the initial pass behind setMeta('addToHistory', false) and dispatch with a meta that the editor's onUpdate recognizes to skip the emit, or perform the rewrite during the doc's initial parse via parseHTML defaults rather than a tx.

3. TableActiveCellsPlugin uses child index, not visual column index. In buildDecorations, row.forEach's third arg (colIndex) is the index of the cell among the row's children; activeCol is computed via TableMap and is the visual column. For tables with colspan/rowspan, those two diverge, so the wrong cells get .is-active-column / .is-active-row, and the handle appears on the wrong cell. (table-active-cells.plugin.ts:71-77) The fix is to derive each cell's visual column from TableMap (map.colCount(cellPos - tablePos - 1) or iterating map.map), not from forEach's positional index. Even though merge/split isn't surfaced yet, legacy content can contain merged cells.

High

4. Row/column actions are keyboard-inaccessible. The handle buttons get tabindex="-1" and only appear via CSS on cursor proximity (table-extensions.ts:157). There's no keyboard path to delete a row or column, and the previous bubble-menu/toolbar entries were removed. For an a11y-themed PR, removing keyboard reach for table manipulation is a regression. Either restore the toolbar entries or add a shortcut / context-menu fallback.

5. Action buttons use (mousedown) instead of (click). Both popovers wire actions on mousedown (table-column-popover.component.ts:41, table-row-popover.component.ts:29). Once the popover is open, Tab/Enter/Space won't activate items because native button activation goes through click, not mousedown. Net effect of #4 + #5: the entire row/column action surface is mouse-only. Move actions back to (click) (focus loss isn't an issue once the popover is the focused surface — and you've already got SELECTION_PRESERVE_KEY for the editor highlight).

6. Stale cellPos between open and apply. TableColumnPayload.cellPos and TableRowPayload.cellPos are snapshots (editor-popover.service.ts:37-48). If anything mutates the doc while the popover is open (column resize tx, async upload placeholder finishing, an undo from a keyboard event the popover doesn't swallow), setTextSelection(payload.cellPos + 1) lands somewhere wrong. The comment says "snapshotting keeps the popover acting on the same cell," but a number snapshot doesn't survive position shifts. Either map the position through subsequent transactions (tr.mapping.map(cellPos)) or sanity-check state.doc.nodeAt(cellPos) is still a cell before applying.

Medium

7. applyHTMLAttributes doesn't sync attribute removal on update(). CELL_ATTRS_TO_SYNC covers re-writing known attrs from the new node, but the initial applyHTMLAttributes(cell, HTMLAttributes) blindly sets everything passed in (e.g., classes added by other plugins). If a downstream plugin adds a class via the cell's HTMLAttributes and later removes it, update() never sees that removal because the sync loop is limited to the static CELL_ATTRS_TO_SYNC list. Decorations from TableActiveCellsPlugin are applied via ProseMirror's decoration mechanism, so they're fine — but anything else that mutates HTMLAttributes post-creation would persist.

8. currentNode is captured AFTER the mousedown listeners that read it. Lines 91-112 register handlers that read currentNode.attrs['scope']; currentNode is declared on line 116. JS hoists let into the TDZ — synchronous reads would throw. It works because the handler fires async. Re-order for clarity: initialize currentNode = node before adding listeners.

9. editor.commands.setContent(parsed, { emitUpdate: false }) is set on init paths, but the auto-scope tx that follows still emits. Related to #2 — even when setContent is silent, the next appended tx from the plugin isn't. Worth verifying whether onCreate runs the auto-scope view init before TipTap considers the editor "settled."

Minor

10. todo.md checked in. Lib-level TODO file isn't a great place for these — either put them in the issue or in PR-description checklist. If kept, the merge-cells gap is meaningful: the legacy editor supports merge/split and the i18n keys for them were already added in Language.properties (dotCMS/.../Language.properties:5972-5973, 5999-6000) but never used in this PR — dead i18n.

11. Hardcoded ariaLabel defaults in extension options. 'Column actions' / 'Row actions' in addOptions() (table-extensions.ts:181-182, 195-196) are English literals, used if a consumer forgets to pass labels. createEditorExtensions always passes the translated values, so this is a fallback — but the fallback won't ever be reached by intended callers and the defaults could just be '' to make missed-injection obvious.

12. :host ::ng-deep everywhere in CSS. Acceptable given TipTap's DOM lives outside Angular's view encapsulation, but the new .dot-cell-handle rules (editor.component.css:261-312) are component-scoped to the editor shell, so a host page that already provides .dot-cell-handle would lose. Class name is specific enough that this is unlikely.

Not flagged — looks fine

  • EditorPopoverService additions are consistent with the existing pattern.
  • The toolbar simplification (172 lines of HTML deleted) is a clean removal of the old per-cell sub-toolbar.
  • forwardRef removal on the toolbar component wasn't needed.
  • The popover form for TableProperties uses signals + effect + untracked correctly to seed values on open.

Tasks

@github-actions github-actions Bot added Area : Backend PR changes Java/Maven backend code Area : Frontend PR changes Angular/TypeScript frontend code labels May 18, 2026
@rjvelazco rjvelazco changed the title feat(table-handles): implement table handles and popovers for column … feat(Block Editor): implement table handles and popovers May 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Backend PR changes Java/Maven backend code Area : Frontend PR changes Angular/TypeScript frontend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Block Editor: Add accessibility attributes to Tables: scope on <th>, <caption>, aria-label, aria-labelledby

1 participant