diff --git a/specs/GH13652/tables/product.md b/specs/GH13652/tables/product.md new file mode 100644 index 0000000000..b9c14c5c7d --- /dev/null +++ b/specs/GH13652/tables/product.md @@ -0,0 +1,115 @@ +# PRODUCT.md — Markdown viewer: raw-HTML `` support + +Issue: https://github.com/warpdotdev/warp/issues/13652 +Preceded by: `` sizing (`specs/GH13652/`, PR #13656) and `
/` +(`specs/GH13652/details-summary/`). + +## Summary + +Warp's Markdown viewer already renders GFM pipe-tables (behind the `MarkdownTables` +feature flag), but drops raw-HTML `
` markup. The issue asks for HTML tables +specifically for the two things GFM pipe-table syntax **cannot** express: cells that +need a hard line break (`
`), and cells that span multiple columns or rows +(`colspan`/`rowspan`). + +This spec covers teaching the Markdown viewer to render a block-level HTML +`
` (``/``/``/`
`/``) by mapping it onto Warp's existing +table model and render path, and to honor `
` inside cells. It scopes +`colspan`/`rowspan` as an **explicit non-goal for this slice**, because those fight the +existing flat rectangular-grid model at every layer and warrant their own effort. + +A deliberate framing note for reviewers: a *simple* HTML table (no `
`, no spans) maps +onto the existing `FormattedTable` and renders with zero editor changes — but it delivers +little beyond GFM pipe tables. The value HTML tables add is `
`-in-cell and spans; +this slice targets the first (`
`) and honestly defers the second (spans). + +Figma: none provided. + +## Goals / Non-goals + +In scope: + +- Recognize a block-level HTML `` in a Markdown document (on its own lines) and + render it through Warp's existing table layout/render path. +- Support ``/`` grouping, `` rows, `` row) becomes the table header; `` or as the first `` — becomes the + table's header row. Subsequent `` rows of `` renders its first `` as the header (matching the existing + model, which always has a header row), unless the design chooses to synthesize an empty + header; the tech spec picks one and documents it. + +3. Cell content renders with the viewer's inline formatting: bold, italic, inline code, + strikethrough, links, and inline images (per the `` spec) all render inside a cell + exactly as they do in a GFM table cell. + +4. A `
` inside a cell renders as a hard line break, so the cell's content occupies + multiple lines within its row. The row's height grows to fit the tallest cell. This is + distinct from the automatic word-wrapping the viewer already does for long cell content + — `
` is an author-specified break that is honored regardless of column width. + +5. Column alignment follows `align`/`text-align` on the header (or per-cell) where present, + defaulting to left. Alignment renders identically to GFM table alignment. + +6. A ragged HTML table (rows with differing cell counts) is normalized to a uniform column + count (short rows padded with empty cells), matching the existing table model's + `normalize_shape` behavior. It must not panic or misalign columns. + +7. A `` with no cells is dropped or padded, not a crash. + +10. Only structural tags (`table`/`thead`/`tbody`/`tr`/`th`/`td`) and the `align`/ + `text-align`/`colspan`/`rowspan` attributes are read (the last two only to decide the + degraded single-cell behavior of invariant 7). All other attributes (`onclick`, + `style` beyond text-align, `class`, `id`, …) are ignored. No attribute is executed or + navigated to. + +11. Copy / export of a document containing an HTML table preserves the tabular content. + Because the internal representation is the shared table model, export may canonicalize + to GFM pipe-table syntax where the content fits it; content that GFM cannot express + (a `
`-bearing multi-line cell) must round-trip in a form that preserves the line + break (the tech spec defines the serialization). Span attributes, being ignored, are + not preserved. diff --git a/specs/GH13652/tables/tech.md b/specs/GH13652/tables/tech.md new file mode 100644 index 0000000000..efdcf04ad5 --- /dev/null +++ b/specs/GH13652/tables/tech.md @@ -0,0 +1,225 @@ +# TECH.md — Markdown viewer: raw-HTML `
` header cells, and `` data + cells. A `` row (or a `
` rows become + data rows. +- Parse inline content inside cells (bold, italic, code, links, and inline images per the + `` spec) using the viewer's existing inline parsing. +- Honor `
` inside a cell as a hard line break, producing a genuinely multi-line cell + (the primary capability GFM pipe tables lack). +- Honor per-column alignment when expressed via `align="left|center|right"` on `		`/ + `` (or the equivalent `style="text-align:…"`), defaulting to left. +- Degrade gracefully: a malformed/unterminated ``, or a ragged table with + inconsistent cell counts, renders without panicking — either as a best-effort normalized + grid (padded to a uniform shape, as the existing model already does via + `normalize_shape`) or, if unparseable, as literal text. + +Out of scope (explicit non-goals): + +- **`colspan` / `rowspan`.** Spanning cells require a non-rectangular grid model that the + current data model, layout, render, and selection code do not support. A ``/``, and nested tables inside a cell. +- Inline `
` + or `` is handled as a **degraded** single-cell (the span attribute is + ignored and the cell occupies one slot), never a panic or corrupt layout. Full span + support is a documented follow-up. +- `
`, `
` mixed with other text on the same line. +- Arbitrary CSS beyond the discrete `align` / `text-align` used for alignment. +- Any change to GFM pipe-table behavior. HTML tables are an additional input path that + produces the same internal table representation. +- Script execution / event handlers / navigation from table markup. + +## Behavior + +1. A Markdown document region delimited by `
` … `
` on their own lines + renders as a table in the viewer, using Warp's existing table appearance (borders, + dividers, alignment, horizontal scrolling for wide tables), identical to how a GFM + pipe-table of the same content renders. + +2. A row of `		` cells — whether inside `
` cells become data rows. A table + with no ``/`
` or `` is rendered as a single ordinary cell (the + span attribute is ignored for this slice). The table remains a well-formed rectangular + grid; no cell visually merges. This is a known limitation (non-goal), surfaced so the + behavior is predictable rather than a crash or corrupt grid. + +8. A malformed or unterminated `` (missing `
`, or content the parser can't + form into a grid) falls back to rendering as literal text without swallowing the rest of + the document or panicking. + +9. Empty structures degrade cleanly: `
` renders as an empty (or + single-empty-row) table or nothing, per the tech spec's chosen convention, never a + panic; a `
` support + +Product spec: `specs/GH13652/tables/product.md` +GitHub issue: https://github.com/warpdotdev/warp/issues/13652 +Preceding specs in the chain: `specs/GH13652/` (``, PR #13656), +`specs/GH13652/details-summary/`. + +## Context + +Warp **already has a table model and render path**; the work here is a new *input path* +(HTML `
`) into it, plus one model extension (`
`-in-cell). + +The shared model — `crates/markdown_parser/src/lib.rs:354-359`: + +```rust +pub struct FormattedTable { + pub headers: Vec, + pub alignments: Vec, // Left / Center / Right (:344-351) + pub rows: Vec>, +} +``` + +A flat, rectangular headers+rows grid. A cell is `FormattedTextInline = Vec` +(`:501`) — a run of styled inline fragments, **structurally single-line**: `parse_table_cell` +rejects `\n`/`\r` (`markdown_parser.rs:596-597`), and no code path produces a line break +inside a cell. There is **no colspan/rowspan/caption** field anywhere (`grep colspan|rowspan` +across `crates/editor/` is empty). + +GFM tables enter via `parse_table` (`markdown_parser.rs:535-674`), gated by +`FeatureFlag::MarkdownTables` in `crates/editor/src/content/buffer.rs:850-855` +(`parse_markdown_with_gfm_tables` vs `parse_markdown`). There is also a fenced-code +internal round-trip format `warp-markdown-table` (`markdown_parser.rs:39`, +`FormattedTable::from_internal_format`/`to_internal_format`, `lib.rs:363-411`) that is +**strictly tab-separated flat cells, newline-separated rows** — it cannot represent an +intra-cell newline (a `\n` reads as a new row) and has no slot for spans. + +Render path (all already built, reused as-is for the simple case): + +- Layout: `layout_table_block` / `measure_table_cells` (`crates/editor/src/content/edit.rs:1197-1339`, + `:260-323`), column widths clamped `[MIN_TABLE_CELL_CONTENT_WIDTH_EMS=1.0 (:132), + MAX_TABLE_CELL_CONTENT_WIDTH_PX=500.0 (:133)]`; cells already **wrap** to multiple visual + lines when content exceeds the clamp (pass-2 re-layout at fixed width, `:1250-1289`). +- Render model: `TableBlockConfig` (`render/model/mod.rs:1478-1482`), `LaidOutTable` + (`:1623-1644`), and crucially `CellLayout { line_heights, line_y_offsets, line_char_ranges, + line_widths, line_caret_positions }` (`:1487-1493`) — **multi-line cells are already a + first-class rendering concept**, sourced from wrapped text frames (`from_text_frame`, + `:1496-1533`). +- Element: `crates/editor/src/render/element/table.rs` (`RenderableTable`) paints a uniform + grid + per-table horizontal scroll. +- Selection/copy correctness across the horizontal viewport relies on per-cell source↔ + rendered offset maps (`table_offset_map.rs`, `content/text.rs` `table_cell_offset_maps`), + introduced by `specs/zachlloyd/wide-markdown-table-scrolling/`. + +The HTML paste parser (`crates/markdown_parser/src/html_parser.rs`) currently **skips** +`
` (`TOP_LEVEL_ELEMENT_TAGS_TO_SKIP` at `:23-25`) and flattens `/
/` into +paragraph lines (they fall to the `_ =>` arm at `:337-347`) — so it produces **no** +`FormattedTable` from HTML today. But it already has the reusable pieces: an `html5ever` +DOM (`parse_document`, `:126`), attribute helpers (`get_attribute` `:107`), and inline +phrasing parsing (`parse_phrasing_content` `:410`). `html5ever` is already a dependency of +`crates/markdown_parser`. + +### Constraints from existing table specs (must respect) + +- `specs/zachlloyd/markdown-table-consistency/` — table chrome must come from the shared + `TableStyle`/appearance helper; "keep layout, selection, cursor, and alignment code + unchanged." A new input path must produce a normal `FormattedTable` and not fork the + render chrome. +- `specs/zachlloyd/wide-markdown-table-scrolling/` — per-cell source↔rendered offset maps + and the `MAX_TABLE_CELL_CONTENT_WIDTH_PX` wrap clamp must stay correct. Any change to + cell content structure (the `
` line-break work) must keep these offset maps correct. + +## Feasibility summary (three slices, honestly sized) + +- **(i) Simple HTML `` → `FormattedTable`: SMALL.** A simple HTML table *is* a + rectangular headers+rows grid of inline cells — exactly `FormattedTable`. Build one from + the DOM and emit `FormattedTextLine::Table`; the entire existing layout/render/scroll/ + selection path works unchanged. But this alone barely beats GFM pipe tables. +- **(ii) `
` in cells: MEDIUM.** The renderer already does multi-line cells (via wrap), + but the cell *type* can't represent an authored hard break, and the internal round-trip + format uses `\n` as the row delimiter. Needs a cell-type/serialization change, not new + rendering. This is the capability that makes HTML tables worth having. +- **(iii) `colspan`/`rowspan`: LARGE — explicit non-goal here.** The flat rectangular grid + (one width per column, one height per row, `[row][col]` indexing) is baked into + `FormattedTable`, `measure_table_cells`, `LaidOutTable`, the painter, and every offset + map. Spans need a non-rectangular grid model across all of them. Deferred to a follow-up. + +This spec implements **(i) + (ii)** and degrades **(iii)** to single-cell (product +invariant 7). + +## Proposed changes + +### 1. Cell model: allow an authored line break (`
`) + +The minimal, least-disruptive change is to make a cell a sequence of lines rather than a +single inline run. Two options: + +- **Option A (recommended): change the cell type to `Vec`** (a list of + lines) in `FormattedTable.headers`/`rows`. This is explicit and makes multi-line cells + first-class end to end. It ripples through `from/to_internal_format`, `normalize_shape`, + `to_plain_text`, and every editor consumer of `FormattedTable` cells. +- **Option B (smaller, hackier): keep `FormattedTextInline` but introduce a line-break + sentinel fragment** (e.g. a `FormattedTextFragment` flagged as a hard break) that the + layout inserts as a forced newline. Less type churn, but every consumer must know to + treat the sentinel specially, and it's easy to miss a site. + +Recommend Option A for correctness, but call the choice out for maintainer review since it +touches the shared `FormattedTable` type. Whichever is chosen: + +- Layout (`measure_table_cells` / pass-2 in `edit.rs:260-323,1250-1289`) inserts the + authored break so the cell lays out to ≥2 lines; the existing multi-line `CellLayout` + machinery (`render/model/mod.rs:1487-1533`) then handles heights/selection. +- Serialization: `to_internal_format` (`lib.rs:396-411`) must escape an intra-cell break + (it can't be a literal `\n`); encode it as `
` (or an escape marker) within the + tab-separated cell, and decode symmetrically in `from_internal_format`. `to_plain_text` + (GFM export) likewise encodes the break as `
` since GFM has no intra-cell newline. +- Keep the per-cell offset maps (`table_cell_offset_maps`) correct across the added break. + +### 2. HTML table reader in `markdown_parser` + +Add a `
` reader that produces a `FormattedTable`. Two placement options: + +- Extend `html_parser.rs`: remove `"table"` from `TOP_LEVEL_ELEMENT_TAGS_TO_SKIP` (`:23-25`) + and add explicit handling that walks `///` row, or a `` whose cells are +`` become `rows`. If there is no header row, +promote the first `` to the header (the model always has a header) — documented +behavior. + +`colspan`/`rowspan` (product invariant 7): read the attributes only to **ignore** them — +each `
/`, builds rows of + cells via the existing `parse_phrasing_content` (`:410`) for inline content, reads + `align`/`text-align` for `TableAlignment`, maps `
` (already `→ LineBreak` at `:335`) + into the new intra-cell break, and assembles a `FormattedTable`. Reuses the DOM + helpers + already there. +- Because `html_parser.rs` is currently paste-oriented (whole-document), the file-viewer + path needs the block Markdown grammar to *detect* a raw `` block and route its text + through this reader. Add a block-level detector in `markdown_parser.rs` (near the image + block branch) that recognizes an own-line `
` … `
` region, extracts the raw + HTML, and calls the table reader — emitting `FormattedTextLine::Table(FormattedTable)`. + This mirrors how the `` and `
` specs add own-line raw-HTML block detectors. + +Header determination (product invariant 2): a `
`, becomes `headers`; remaining `
`/`` occupies exactly one grid slot regardless of span. Ragged rows are fixed +by the existing `FormattedTable::normalize_shape` (`lib.rs:414-429`) (invariant 6). + +Fallback (invariant 8): if the region has no `
` or can't form a grid, the block +detector fails and the text is parsed as ordinary Markdown/literal text. + +### 3. Feature gating + +HTML tables should ride the **existing `FeatureFlag::MarkdownTables`** gate +(`buffer.rs:850-855`) so they light up exactly where GFM tables do, and stay dark where +tables are disabled. No new flag. The `
`-in-cell model change (item 1) is behind the +same table code paths, so it only affects tables. + +### 4. Security + +Only structural tags and `align`/`text-align` (plus `colspan`/`rowspan` read solely to +ignore) are consulted; all other attributes are dropped (invariant 10). Cell content is +parsed as inline Markdown/phrasing content and inherits the viewer's existing trust +boundary. Inline images inside cells resolve through the same asset-source resolver as the +`` spec — no new source path. No script/event-handler surface. + +## Testing and validation + +### Parser unit tests (`crates/markdown_parser/src/html_parser_tests.rs`, `markdown_parser_tests.rs`) + +- Simple `` with ``/`` → `FormattedTable` with expected headers/rows + (invariants 1, 2). +- `` → first row is header (invariant 2). +- Inline formatting inside cells (bold/link/`code`/inline image) → parsed fragments + (invariant 3). +- `
` in a cell → multi-line cell (invariant 4); assert the cell holds ≥2 lines under + the chosen cell model. +- `align`/`text-align` on cells → `TableAlignment` (invariant 5). +- Ragged rows → normalized to uniform columns (invariant 6). +- `` → no panic (invariant 9). +- Ignored attributes (`onclick`, `class`) → not consulted (invariant 10). + +### Round-trip (`crates/markdown_parser` + `crates/editor/src/content/text_tests.rs`) + +- HTML table without `
` → internal format → back, content preserved; canonicalizes to + GFM where it fits (invariant 11). +- HTML table **with** `
` in a cell → round-trips preserving the line break (encoded as + `
` in the internal/GFM forms), not collapsed and not turned into a new row + (invariant 11 + 4). + +### Layout / render tests (`crates/editor/src/render/model/mod_tests.rs`) + +- A `
`-bearing cell increases its row height to fit the extra line; neighbor columns + unaffected. +- Column widths still honor the `[MIN, MAX]` clamp; wide HTML tables still scroll + horizontally (no regression to the wide-table viewport). +- Selection/copy across a multi-line cell stays correct (offset maps). + +### Integration / manual + +Per CONTRIBUTING, before/after screenshots + a short recording rendering the issue's +motivating case — an HTML table whose cell contains a `
`-separated multi-line value — +alongside a GFM table for comparison, and a `colspan` table showing the documented +degraded (span-ignored) rendering. Add `crates/integration/` coverage for opening a +Markdown file containing an HTML table if exercisable there. + +## Risks and follow-ups + +- **The valuable capability is the medium-cost one.** A simple HTML table (small) barely + improves on GFM pipe tables; the reason to do HTML tables at all is `
`-in-cell (and, + later, spans). This slice therefore commits to the `
` cell-model change rather than + shipping only the near-free simple-table reader. If maintainers would rather ship the + simple reader first and defer `
`, the cell-model change (item 1) can be split into its + own follow-up — noted as an option. +- **Cell-type change touches the shared `FormattedTable`.** Whether Option A (`Vec`) + or Option B (sentinel fragment), this ripples through parser round-trip, editor layout, + and offset maps. It's the main risk surface; the tests above target each site. If it + starts to sprawl, that's the signal to split simple-table and `
` into two PRs. +- **`colspan`/`rowspan` is a genuine model change** (non-rectangular grid) and is an + explicit non-goal here (invariant 7 degrades it). It deserves its own spec/PR — likely + the largest single piece of the whole #13652 effort — and should be scoped separately + once simple + `
` tables land. +- **Interaction with the other tier-zero specs:** inline images inside cells depend on the + `` spec's inline-image support; an HTML table inside a `
` body should work + under that spec's Option-A model since the table is an ordinary top-level block. Verify + once the chain lands.
`-first-row table with no `
` / `rowspan` → single ordinary cell, span ignored, grid rectangular + (invariant 7). +- Unterminated `` / non-grid content → literal-text fallback, document below intact + (invariant 8). +- `
` / empty `