From a7056bdf5db781bc9bdc04104c43fced9867a5af Mon Sep 17 00:00:00 2001 From: Hampton Lintorn-Catlin Date: Fri, 11 Sep 2026 14:49:05 -0500 Subject: [PATCH 1/4] Expand tables into a spreadsheet, and let diagrams be explored MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Markdown tables used to flow off the page. They're now framed: columns size to their content, long cells wrap, and anything wider than the column scrolls inside its own bordered frame with edge fades that say so. A table big enough to be worth it offers to expand into a spreadsheet takeover — both axes pinned, an arrow-driven cell cursor with a row/column crosshair, a value bar for cells the grid clips, numeric-aware column sorting with a reset, a wrap toggle, and an A1-style address readout. ⌘C copies the focused cell. Sorting works on a clone, so the document's own table — and the comment anchors that depend on its rendered text — never moves. Diagrams get the exploration they were missing. The old lightbox closed on any click, so a drag could never pan; it's replaced by a real pan/zoom surface: drag, scroll or pinch to zoom at the cursor, double-click to toggle fit and close-up, +/-/0/1 and arrows from the keyboard. Labels are legible now too — 16px in the app's own face, thicker strokes, and fonts loaded before Mermaid measures its boxes. Fitting stops before it becomes a thumbnail: inline, a diagram that would shrink past 80% keeps its real size and scrolls; expanded, fit floors at 55% and pans instead. Both features sit on one takeover surface (coplan/expander) and one affordance, with pan/zoom factored out separately — a spreadsheet wants real scrolling, not a transform. Decks opt out of table framing; a slide is a fixed artifact whose typography the deck layout already owns. Co-Authored-By: Claude Opus 5 --- db/seeds/development.rb | 40 +- .../assets/stylesheets/coplan/application.css | 628 ++++++++++++++++-- engine/app/helpers/coplan/markdown_helper.rb | 38 +- .../app/helpers/coplan/slideshows_helper.rb | 3 +- .../coplan/data_grid_controller.js | 371 +++++++++++ .../controllers/coplan/mermaid_controller.js | 190 ++++-- engine/app/javascript/coplan/expander.js | 163 +++++ engine/app/javascript/coplan/pan_zoom.js | 212 ++++++ engine/config/importmap.rb | 5 + spec/helpers/markdown_helper_spec.rb | 47 ++ spec/system/comment_ux_spec.rb | 10 +- spec/system/data_grid_spec.rb | 226 +++++++ spec/system/deck_ux_spec.rb | 4 +- spec/system/diagram_expand_spec.rb | 153 +++++ 14 files changed, 1979 insertions(+), 111 deletions(-) create mode 100644 engine/app/javascript/controllers/coplan/data_grid_controller.js create mode 100644 engine/app/javascript/coplan/expander.js create mode 100644 engine/app/javascript/coplan/pan_zoom.js create mode 100644 spec/system/data_grid_spec.rb create mode 100644 spec/system/diagram_expand_spec.rb diff --git a/db/seeds/development.rb b/db/seeds/development.rb index 43f5951b..ab6afc96 100644 --- a/db/seeds/development.rb +++ b/db/seeds/development.rb @@ -156,12 +156,26 @@ module DevelopmentSeed AGENT_ORGANIZE_RUN_ID = "development-seed-organize-run" CONTENT_FIXTURES = { + # Deliberately wider than a document column: this is the fixture that + # demonstrates the legibility floor (the diagram keeps its real size + # and scrolls rather than shrinking to unreadable) and the pan/zoom + # expanded view. flowchart: <<~MARKDOWN, ```mermaid flowchart LR - Client --> Gateway - Gateway --> API - API --> DB[(Database)] + Client[Merchant client] --> Gateway[Edge gateway] + Gateway --> Auth[Token exchange] + Auth --> API[Orders API] + API --> Rules[Eligibility rules] + Rules --> Engine[Pricing engine] + Engine --> Ledger[(Price ledger)] + Engine --> Risk[Risk scoring] + Risk --> Capture[Capture service] + Capture --> Settle[Settlement batch] + Settle --> Payout[(Payout store)] + API --> Events[Event feed] + Events --> Search[(Search index)] + Events --> Warehouse[(Warehouse)] ``` MARKDOWN state_diagram: <<~MARKDOWN, @@ -182,11 +196,23 @@ module DevelopmentSeed S-->>A: Receipt ID ``` MARKDOWN + # Wide and long on purpose — the fixture that shows a table framed and + # wrapped in the document, and opened as a spreadsheet. table: <<~MARKDOWN, - | Metric | Control | Treatment | - | --- | ---: | ---: | - | Success | 61.2% | 62.9% | - | Errors | 18.4% | 17.7% | + | Variant | Cohort | Sessions | Success | Errors | p95 latency | Revenue / session | Notes | + | --- | --- | ---: | ---: | ---: | ---: | ---: | --- | + | Control | All | 41,208 | 61.2% | 18.4% | 412ms | $2.14 | Baseline ranker, unchanged since March | + | Treatment A | All | 40,876 | 62.9% | 17.7% | 438ms | $2.31 | Recall boost on long-tail queries | + | Treatment B | All | 12,004 | 59.8% | 21.3% | 507ms | $1.96 | Reranker on top; latency cost too high | + | Control | New | 8,912 | 54.1% | 22.8% | 401ms | $1.42 | New accounts see fewer personalized results | + | Treatment A | New | 8,844 | 58.6% | 19.9% | 430ms | $1.71 | Largest lift in the whole experiment | + | Treatment B | New | 2,610 | 55.2% | 24.1% | 512ms | $1.38 | Same latency story as the pooled cohort | + | Control | Returning | 32,296 | 63.1% | 17.2% | 414ms | $2.34 | | + | Treatment A | Returning | 32,032 | 64.1% | 17.1% | 440ms | $2.47 | Small but consistent across every week | + | Treatment B | Returning | 9,394 | 61.1% | 20.5% | 505ms | $2.12 | | + | Control | Mobile | 19,441 | 57.8% | 20.6% | 486ms | $1.88 | Mobile is latency-bound before it is ranking-bound | + | Treatment A | Mobile | 19,388 | 59.9% | 19.4% | 511ms | $2.02 | Lift holds, but the latency budget is nearly spent | + | Treatment B | Mobile | 5,702 | 54.3% | 25.7% | 604ms | $1.61 | Rejected on mobile outright | MARKDOWN code_walkthrough: <<~'MARKDOWN', How the discount engine decides what every line item costs. Each stage is diff --git a/engine/app/assets/stylesheets/coplan/application.css b/engine/app/assets/stylesheets/coplan/application.css index 21052f47..e591176c 100644 --- a/engine/app/assets/stylesheets/coplan/application.css +++ b/engine/app/assets/stylesheets/coplan/application.css @@ -2011,17 +2011,32 @@ img.avatar { .markdown-rendered .mermaid-diagram { position: relative; - display: flex; - justify-content: center; - overflow-x: auto; margin-bottom: var(--space-md); - padding: var(--space-md); background: var(--color-surface); border: 1px solid var(--color-border); border-radius: var(--radius); cursor: zoom-in; } +/* The diagram's scroll box, separate from the frame so the expand control + stays pinned to the corner instead of scrolling away with a wide + diagram. */ +.markdown-rendered .mermaid-diagram__canvas { + display: flex; + justify-content: center; + overflow-x: auto; + padding: var(--space-md); + border-radius: inherit; + scrollbar-width: thin; +} + +/* Past the legibility floor a diagram stops shrinking to fit and keeps its + real size — the controller pins the SVG's pixel width and this box + scrolls instead, so labels stay readable. */ +.markdown-rendered .mermaid-diagram--scrolling .mermaid-diagram__canvas { + justify-content: flex-start; +} + .markdown-rendered .mermaid-diagram__expand { position: absolute; top: var(--space-sm); @@ -2044,51 +2059,21 @@ img.avatar { opacity: 1; } -.mermaid-lightbox { - width: 94vw; - height: 92vh; - max-width: 94vw; - max-height: 92vh; - padding: var(--space-lg); - margin: auto; - border: 1px solid var(--color-border); - border-radius: 12px; - background: var(--color-surface); - color: var(--color-text); - box-shadow: 0 24px 64px rgba(0, 0, 0, 0.25); - cursor: zoom-out; -} - -.mermaid-lightbox::backdrop { - background: rgba(0, 0, 0, 0.35); -} - -.mermaid-lightbox__close { - position: absolute; - top: var(--space-sm); - right: var(--space-sm); - display: flex; - align-items: center; - justify-content: center; - padding: var(--space-xs); - background: var(--color-surface); - border: 1px solid var(--color-border); - border-radius: var(--radius); - color: var(--color-text-muted); - cursor: pointer; -} - -.mermaid-lightbox__close:hover { - color: var(--color-text); -} - /* The diagram's own SVG only — the expand button's icon is an svg inside the same container, and it must keep its own 16px geometry. */ -.markdown-rendered .mermaid-diagram > svg { +.markdown-rendered .mermaid-diagram__canvas > svg { max-width: 100%; height: auto; } +/* `flex: none` is load-bearing: the canvas is a flex container, and a flex + item shrinks to fit by default — which would undo the pixel width the + controller just pinned and put the diagram straight back to unreadable. */ +.markdown-rendered .mermaid-diagram--scrolling .mermaid-diagram__canvas > svg { + max-width: none; + flex: none; +} + /* Mermaid sizes node boxes assuming its own label line-height (~1.2); the page's inherited line-height (1.6) makes wrapped labels taller than the boxes mermaid measured, clipping the second line. Pin labels back to the @@ -2096,9 +2081,9 @@ img.avatar { .markdown-rendered .mermaid-diagram foreignObject div, .markdown-rendered .mermaid-diagram foreignObject span, .markdown-rendered .mermaid-diagram foreignObject p, -.mermaid-lightbox foreignObject div, -.mermaid-lightbox foreignObject span, -.mermaid-lightbox foreignObject p { +.expander--diagram foreignObject div, +.expander--diagram foreignObject span, +.expander--diagram foreignObject p { line-height: 1.2; } @@ -2139,6 +2124,153 @@ img.avatar { font-weight: 600; } +/* ── Data grid: a markdown table in the document ────────────────────────── + The bare rules above still apply wherever a table isn't wrapped (deck + slides own their own tabular typography). Everywhere else the table sits + in a frame it can scroll inside, instead of a block it overflows. */ +.markdown-rendered .data-grid { + position: relative; + margin-bottom: var(--space-md); + border: 1px solid var(--color-border); + border-radius: var(--radius); + background: var(--color-surface); +} + +.markdown-rendered .data-grid__frame { + overflow: auto; + /* Tall tables scroll inside the frame so the header can pin; short ones + never reach this and are unaffected. */ + max-height: 75vh; + border-radius: inherit; + scrollbar-width: thin; +} + +.markdown-rendered .data-grid table { + /* Columns size to their content and the table still fills a narrow + frame. width:100% squeezed columns to nothing instead, which is how a + wide table ended up unreadable *and* off the page. */ + width: max-content; + min-width: 100%; + margin: 0; + /* Collapsed borders don't travel with a sticky header — the header's own + border scrolls out from under it. Separate borders plus inset shadows + stay put. */ + border-collapse: separate; + border-spacing: 0; + font-size: var(--text-sm); +} + +.markdown-rendered .data-grid th, +.markdown-rendered .data-grid td { + border: none; + border-right: 1px solid var(--color-border); + border-bottom: 1px solid var(--color-border); + padding: var(--space-sm) var(--space-md); + text-align: left; + vertical-align: top; + min-width: 6ch; + max-width: 34ch; + overflow-wrap: break-word; +} + +.markdown-rendered .data-grid tr > *:last-child { + border-right: none; +} + +.markdown-rendered .data-grid tbody tr:last-child > * { + border-bottom: none; +} + +.markdown-rendered .data-grid thead th { + position: sticky; + top: 0; + z-index: 1; + background: var(--color-surface-muted); + white-space: nowrap; + font-weight: 600; + box-shadow: inset 0 -1px 0 var(--color-border); +} + +.markdown-rendered .data-grid tbody tr:nth-child(even) > * { + background: var(--color-surface-muted); +} + +/* Edge fades: the honest signal that the frame is holding more than it's + showing. Only lit when there really is more in that direction. */ +.markdown-rendered .data-grid::before, +.markdown-rendered .data-grid::after { + content: ""; + position: absolute; + top: 1px; + bottom: 1px; + width: 2rem; + z-index: 2; + pointer-events: none; + opacity: 0; + transition: opacity 0.15s ease; +} + +.markdown-rendered .data-grid::before { + left: 1px; + border-radius: var(--radius) 0 0 var(--radius); + background: linear-gradient(to right, var(--color-surface), transparent); +} + +.markdown-rendered .data-grid::after { + right: 1px; + border-radius: 0 var(--radius) var(--radius) 0; + background: linear-gradient(to left, var(--color-surface), transparent); +} + +.markdown-rendered .data-grid.is-scrolled-start::before, +.markdown-rendered .data-grid.is-scrolled-end::after { + opacity: 1; +} + +.markdown-rendered .data-grid__expand { + position: absolute; + top: var(--space-sm); + right: var(--space-sm); + z-index: 3; + display: none; + align-items: center; + justify-content: center; + padding: var(--space-xs); + background: var(--color-surface); + border: 1px solid var(--color-border); + border-radius: var(--radius); + color: var(--color-text-muted); + box-shadow: var(--shadow-pop); + cursor: zoom-in; + opacity: 0; + transition: opacity 0.15s ease; +} + +/* Offered only where it buys something — a four-column, five-row table is + already entirely visible. */ +.markdown-rendered .data-grid.is-expandable .data-grid__expand { + display: flex; +} + +.markdown-rendered .data-grid.is-expandable:hover .data-grid__expand, +.markdown-rendered .data-grid.is-expandable:focus-within .data-grid__expand, +.markdown-rendered .data-grid__expand:focus-visible { + opacity: 1; +} + +.markdown-rendered .data-grid__expand:hover { + color: var(--color-text); + border-color: var(--color-border-strong); +} + +/* Nothing hovers on a touch screen, and tap-to-expand isn't available + here — selecting table text is how a comment gets anchored. */ +@media (hover: none) { + .markdown-rendered .data-grid.is-expandable .data-grid__expand { + opacity: 1; + } +} + .markdown-rendered hr { border: none; border-top: 1px solid var(--color-border); @@ -6544,3 +6676,407 @@ img.avatar { font-size: 0.75rem; } } + +/* ═══ Expanded view ═══════════════════════════════════════════════════════ + The shared takeover surface (coplan/expander.js) behind an expanded + Mermaid diagram and an expanded data table. The chrome — title bar, + toolbar, status line, dismissal — is identical for both; only the body + differs. */ + +/* showModal() dims the page but doesn't stop it scrolling behind the + overlay. */ +.expander-open { + overflow: hidden; +} + +.expander { + display: flex; + flex-direction: column; + width: 94vw; + height: 92vh; + max-width: 94vw; + max-height: 92vh; + padding: 0; + margin: auto; + overflow: hidden; + background: var(--color-surface); + color: var(--color-text); + border: 1px solid var(--color-border); + border-radius: var(--radius-lg); + box-shadow: var(--shadow-lg); +} + +.expander::backdrop { + background: rgba(8, 12, 24, 0.55); + backdrop-filter: blur(2px); +} + +.expander__bar { + display: flex; + align-items: center; + gap: var(--space-sm); + flex: none; + padding: var(--space-sm) var(--space-sm) var(--space-sm) var(--space-md); + background: var(--color-surface-muted); + border-bottom: 1px solid var(--color-border); +} + +.expander__title { + flex: 1; + min-width: 0; + margin: 0; + font-size: var(--text-base); + font-weight: 600; + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis; +} + +.expander__tools { + display: flex; + align-items: center; + gap: var(--space-xs); +} + +.expander__tool, +.expander__close { + display: flex; + align-items: center; + justify-content: center; + min-width: 2rem; + height: 2rem; + padding: 0 var(--space-xs); + background: var(--color-surface); + color: var(--color-text-muted); + border: 1px solid var(--color-border); + border-radius: var(--radius); + cursor: pointer; +} + +.expander__tool:hover, +.expander__close:hover { + color: var(--color-text); + border-color: var(--color-border-strong); +} + +.expander__tool.is-active { + color: var(--color-primary); + background: var(--color-primary-light); + border-color: var(--color-primary); +} + +.expander__tool--hidden { + display: none; +} + +.expander__close { + margin-left: var(--space-xs); +} + +.expander__readout { + min-width: 3.5rem; + text-align: center; + font-size: var(--text-sm); + font-variant-numeric: tabular-nums; + color: var(--color-text-muted); +} + +.expander__body { + display: flex; + flex-direction: column; + flex: 1; + min-height: 0; +} + +.expander__status { + display: flex; + align-items: center; + gap: var(--space-sm); + flex: none; + padding: var(--space-xs) var(--space-md); + background: var(--color-surface-muted); + border-top: 1px solid var(--color-border); + font-size: var(--text-sm); + color: var(--color-text-muted); +} + +.expander__hint { + margin-left: auto; + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis; +} + +/* ── Diagram body: the pan/zoom viewport ──────────────────────────────── */ + +.expander__canvas { + position: relative; + flex: 1; + min-height: 0; + overflow: hidden; + /* The browser's own pan and pinch would fight the controller's. */ + touch-action: none; + cursor: grab; +} + +.expander__canvas.is-grabbing { + cursor: grabbing; +} + +.expander__canvas:focus-visible { + outline: 2px solid var(--color-primary); + outline-offset: -2px; +} + +.expander__canvas > svg { + display: block; +} + +/* ── Table body: the spreadsheet ──────────────────────────────────────── */ + +.data-sheet { + display: flex; + flex-direction: column; + flex: 1; + min-height: 0; +} + +/* The value bar. Cells stay clipped to one line so a wide table is + scannable; this is where the focused cell is shown in full. */ +.data-sheet__value { + display: flex; + align-items: baseline; + gap: var(--space-sm); + flex: none; + min-height: 2.75rem; + padding: var(--space-sm) var(--space-md); + background: var(--color-bg-muted); + border-bottom: 1px solid var(--color-border); +} + +.data-sheet__value-label { + flex: none; + max-width: 12rem; + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis; + font-size: var(--text-sm); + font-weight: 600; + color: var(--color-text-muted); +} + +.data-sheet__value-content { + flex: 1; + min-width: 0; + max-height: 5.5rem; + overflow-y: auto; +} + +.data-sheet__value-content > *:last-child { + margin-bottom: 0; +} + +.data-sheet__value.is-empty .data-sheet__value-content::after { + content: "empty"; + color: var(--color-text-muted); + font-style: italic; +} + +.data-sheet__frame { + flex: 1; + min-height: 0; + overflow: auto; + scrollbar-width: thin; +} + +.data-sheet__frame:focus { + outline: none; +} + +.data-sheet__table { + width: max-content; + min-width: 100%; + background: var(--color-surface); + border-collapse: separate; + border-spacing: 0; + font-size: var(--text-sm); +} + +/* Body cells are transparent on purpose: it lets a background on the + column's paint the crosshair band for free, however many rows the + table has. */ +.data-sheet__table th, +.data-sheet__table td { + position: relative; + max-width: 40ch; + padding: var(--space-sm) var(--space-md); + background: transparent; + border-right: 1px solid var(--color-border); + border-bottom: 1px solid var(--color-border); + text-align: left; + vertical-align: top; + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis; +} + +.data-sheet__frame.is-wrapped th, +.data-sheet__frame.is-wrapped td { + white-space: normal; + overflow: visible; + overflow-wrap: break-word; +} + +.data-sheet__table thead th { + position: sticky; + top: 0; + z-index: 3; + padding-right: calc(var(--space-md) + 0.75rem); + background: var(--color-surface-muted); + font-weight: 600; + cursor: pointer; + user-select: none; + box-shadow: inset 0 -1px 0 var(--color-border); +} + +.data-sheet__table thead th:first-child { + left: 0; + z-index: 4; +} + +/* The row-label column pins too — the header alone doesn't tell you which + row you're reading once you've scrolled sideways. Opaque, so the cells + it slides over disappear under it cleanly. */ +.data-sheet__table tbody tr > *:first-child { + position: sticky; + left: 0; + z-index: 2; + background: var(--color-surface); + font-weight: 500; + box-shadow: inset -1px 0 0 var(--color-border); +} + +/* The crosshair. Mixed from --color-primary rather than using + --color-primary-light, which is near-white in light mode and would make + the bands invisible on exactly the theme that needs them most. + Translucent on purpose: the row band paints over the column band, so + the cell where they cross deepens on its own. */ +.data-sheet__table col.is-cursor-column, +.data-sheet__table tbody tr.is-cursor-row > * { + background: color-mix(in srgb, var(--color-primary) 12%, transparent); +} + +/* The pinned cells can't be translucent — rows scrolling underneath would + show through them — so they take the same tint, mixed solid. */ +.data-sheet__table tbody tr.is-cursor-row > *:first-child { + background: color-mix(in srgb, var(--color-primary) 12%, var(--color-surface)); +} + +.data-sheet__table thead th.is-cursor-column { + color: var(--color-primary); + background: color-mix(in srgb, var(--color-primary) 14%, var(--color-surface-muted)); +} + +/* Written the long way round so it outranks the pinned first column's own + box-shadow — the cursor has to be visible in column A too. */ +.data-sheet__table tbody tr > td.is-cursor, +.data-sheet__table tbody tr > th.is-cursor { + z-index: 5; + box-shadow: inset 0 0 0 2px var(--color-primary); + outline: none; +} + +/* Sort state: a triangle in the header's reserved right-hand gutter. */ +.data-sheet__table thead th::after { + content: ""; + position: absolute; + right: var(--space-sm); + top: calc(50% - 2px); + border-left: 4px solid transparent; + border-right: 4px solid transparent; + opacity: 0; + transition: opacity 0.12s ease; +} + +.data-sheet__table thead th:hover::after { + border-top: 5px solid currentColor; + opacity: 0.4; +} + +.data-sheet__table thead th.is-sorted-asc::after { + border-top: none; + border-bottom: 5px solid var(--color-primary); + opacity: 1; +} + +.data-sheet__table thead th.is-sorted-desc::after { + border-bottom: none; + border-top: 5px solid var(--color-primary); + opacity: 1; +} + +/* A comment anchor inside a cell still shows, but as a mark, not as + something you can open — the sheet is a copy, and its threads live back + in the document. */ +.data-sheet mark { + background: transparent; + color: inherit; + text-decoration: underline wavy var(--color-highlight-open-border); + text-decoration-skip-ink: none; + pointer-events: none; +} + +.data-sheet__address { + min-width: 3.25rem; + font-family: var(--font-mono); + font-variant-numeric: tabular-nums; + font-weight: 600; + color: var(--color-text); +} + +.data-sheet__address.is-copied { + color: var(--color-success); +} + +.data-sheet__address.is-copied::after { + content: " copied"; + font-family: var(--font-sans); + font-weight: 400; +} + +.data-sheet__column { + font-weight: 600; + color: var(--color-text); +} + +@media (max-width: 640px) { + /* A phone has no room for a framed dialog: the takeover takes the screen. + Sized by `inset` rather than 100vw/100dvh — a modal dialog's default + `margin: auto` centres it, so any disagreement between the viewport units + and the actual layout width leaves a sliver of backdrop down one edge. */ + .expander { + inset: 0; + width: auto; + height: auto; + max-width: none; + max-height: none; + margin: 0; + border: none; + border-radius: 0; + } + + /* No room for keyboard instructions beside the cell address — but the + diagram's hint is the only place the pan/zoom gestures are named, and + it's short enough to keep. */ + .expander__hint { + display: none; + } + + .expander--diagram .expander__hint { + display: block; + } + + .data-sheet__value-label { + max-width: 7rem; + } +} diff --git a/engine/app/helpers/coplan/markdown_helper.rb b/engine/app/helpers/coplan/markdown_helper.rb index b9120cbb..56e2c434 100644 --- a/engine/app/helpers/coplan/markdown_helper.rb +++ b/engine/app/helpers/coplan/markdown_helper.rb @@ -34,7 +34,7 @@ module MarkdownHelper # version. Bump it whenever the rendering pipeline changes output for the # same input (new tags, attribute changes, checkbox wiring, etc.), or # stale HTML will be served from cache. - RENDER_CACHE_VERSION = 12 + RENDER_CACHE_VERSION = 13 # Matches `[@username](mention:username)` where the bracket text and link # target encode the same username. Username allows letters, digits, dots, @@ -51,7 +51,12 @@ module MarkdownHelper # document when rendering a slice of a larger plan (slideshow slides). # Checkbox toggles write to source lines by number, so their data-line # must stay document-absolute even when the render sees only a fragment. - def render_markdown(content, interactive: true, footnote_prefix: nil, footnotes: :inline, line_offset: 0) + # + # data_tables: wrap tables in their scroll frame and give them the + # spreadsheet expander. Decks opt out — a slide is a fixed, scaled + # artifact whose typography the deck layout engine already owns, and a + # nested scroll frame inside a transformed slide belongs to nobody. + def render_markdown(content, interactive: true, footnote_prefix: nil, footnotes: :inline, line_offset: 0, data_tables: true) render_options = { unsafe: true } # Sourcepos is only needed to wire checkboxes to their source lines; # make_checkboxes_interactive strips it from the final output. @@ -65,6 +70,7 @@ def render_markdown(content, interactive: true, footnote_prefix: nil, footnotes: result = select_footnotes(result, footnotes) return result.html_safe if footnotes == :only + result = wrap_data_tables(result) if data_tables tag.div(result.html_safe, class: "markdown-rendered", data: { controller: "coplan--mermaid coplan--syntax-highlight" }) end @@ -244,6 +250,34 @@ def make_checkboxes_interactive(html, content, line_offset: 0) doc.to_html end + # A table is a block of data, not a paragraph. Each one gets a scroll + # frame — so a wide table can't run off the page the way it does bare — + # and the Stimulus controller that pins its header, fades its edges, and + # opens it as a full-screen spreadsheet. + # + # This runs after sanitization on purpose: the frame carries + # data-controller and data-*-target attributes that document markup is + # deliberately never allowed to write for itself. It adds structure + # only, never text, so comment anchors — which count occurrences in the + # rendered text — see exactly what they saw before. + def wrap_data_tables(html) + doc = Nokogiri::HTML::DocumentFragment.parse(html) + tables = doc.css("table").reject { |table| table.ancestors("table").any? } + return html if tables.empty? + + tables.each do |table| + grid = doc.document.create_element("div", class: "data-grid", + "data-controller" => "coplan--data-grid") + frame = doc.document.create_element("div", class: "data-grid__frame", + "data-coplan--data-grid-target" => "frame") + table.add_previous_sibling(grid) + grid.add_child(frame) + frame.add_child(table) + end + + doc.to_html + end + # Rewrites commonmarker's per-document footnote ids (#fn-N / #fnref-N) # and the hrefs that point at them so multiple fragments can coexist in # one DOM. Only ids/fragments with the fn-/fnref- shape are touched — diff --git a/engine/app/helpers/coplan/slideshows_helper.rb b/engine/app/helpers/coplan/slideshows_helper.rb index d5834c5b..dfb9fd6a 100644 --- a/engine/app/helpers/coplan/slideshows_helper.rb +++ b/engine/app/helpers/coplan/slideshows_helper.rb @@ -31,6 +31,7 @@ def render_slideshow(content, interactive: true, theme: "coplan") classification = Slideshows::Classify.call(preamble + slide.source) lead_by_slide[slide.index.to_s] = classification.lead inner = render_markdown(preamble + slide.source, interactive:, footnotes: :exclude, + data_tables: false, line_offset: slide.start_line - 1 - preamble.count("\n")) tag.section(inner, class: [ "deck-slide", "deck-slide--#{classification.pattern}", @@ -44,7 +45,7 @@ def render_slideshow(content, interactive: true, theme: "coplan") # on every slide. The document-mode render is the ground truth readers # and agents link against; these passes rewrite the deck to match it. deck = Nokogiri::HTML::DocumentFragment.parse(safe_join(sections)) - document = Nokogiri::HTML::DocumentFragment.parse(render_markdown(content, interactive: false)) + document = Nokogiri::HTML::DocumentFragment.parse(render_markdown(content, interactive: false, data_tables: false)) renumber_deck_footnotes(deck, document) align_heading_ids(deck, document) mirror_section_link_enhancement(deck, document) diff --git a/engine/app/javascript/controllers/coplan/data_grid_controller.js b/engine/app/javascript/controllers/coplan/data_grid_controller.js new file mode 100644 index 00000000..eed82421 --- /dev/null +++ b/engine/app/javascript/controllers/coplan/data_grid_controller.js @@ -0,0 +1,371 @@ +import { Controller } from "@hotwired/stimulus" +import { openExpander, attachExpandAffordance, nearestHeading, ICONS } from "coplan/expander" + +// A markdown table, twice over. +// +// In the document it stays compact and well-behaved: columns sized to their +// content, long cells wrapped, a scroll frame so a wide table can never run +// off the page, a header that pins itself when the table is tall enough to +// scroll, and fades at the edges so it's obvious there's more. +// +// Expanded it becomes a spreadsheet: header row and first column pinned, a +// cell cursor you drive with the arrow keys, a crosshair on the current row +// and column, a value bar showing the focused cell in full, and sortable +// columns. The surface itself is shared with Mermaid diagrams — see +// coplan/expander. + +// Below these, a table is small enough that offering to expand it is noise. +const WORTH_EXPANDING_ROWS = 8 +const WORTH_EXPANDING_COLUMNS = 4 + +export default class extends Controller { + static targets = [ "frame" ] + + connect() { + this.table = this.frameTarget.querySelector("table") + if (!this.table) return + + this.onScroll = () => this.measure() + this.frameTarget.addEventListener("scroll", this.onScroll, { passive: true }) + this.observer = new ResizeObserver(() => this.measure()) + this.observer.observe(this.frameTarget) + this.observer.observe(this.table) + + this.affordance = attachExpandAffordance(this.element, { + label: "Expand table", + hint: "Open as a spreadsheet", + className: "data-grid__expand", + onExpand: () => this.expand() + }) + + this.measure() + } + + disconnect() { + this.frameTarget?.removeEventListener("scroll", this.onScroll) + this.observer?.disconnect() + this.expanded?.close() + } + + // Edge fades and the expand affordance both depend on whether the frame + // actually has more to show than it's showing. + measure() { + const frame = this.frameTarget + const overflowX = frame.scrollWidth - frame.clientWidth > 1 + const overflowY = frame.scrollHeight - frame.clientHeight > 1 + + this.element.classList.toggle("is-scrolled-start", frame.scrollLeft > 1) + this.element.classList.toggle("is-scrolled-end", + overflowX && Math.ceil(frame.scrollLeft + frame.clientWidth) < frame.scrollWidth - 1) + this.element.classList.toggle("is-tall", overflowY) + + const rows = this.table.rows.length + const columns = this.table.rows[0]?.cells.length || 0 + this.element.classList.toggle("is-expandable", + overflowX || overflowY || rows > WORTH_EXPANDING_ROWS || columns > WORTH_EXPANDING_COLUMNS) + } + + expand() { + if (this.expanded) return + + const expander = openExpander({ + title: nearestHeading(this.element) || "Table", + label: "Expanded table", + variant: "grid", + status: true, + onClose: () => { this.expanded = null; this.sheet = null } + }) + this.expanded = expander + this.sheet = new Sheet(this.table, expander) + } +} + +// The expanded spreadsheet. Owns a clone of the document's table, so +// sorting and the cursor never touch what the page (or a comment anchor) +// is looking at. +class Sheet { + constructor(sourceTable, expander) { + this.expander = expander + this.table = sourceTable.cloneNode(true) + this.table.className = "data-sheet__table" + this.table.setAttribute("role", "grid") + + this.headerCells = Array.from(this.table.tHead?.rows[0]?.cells || []) + this.bodyRows = Array.from(this.table.tBodies[0]?.rows || []) + this.sourceOrder = this.bodyRows.slice() + this.columnCount = this.headerCells.length || this.bodyRows[0]?.cells.length || 0 + this.sort = null + this.cursor = null + + this.build() + this.wire() + if (this.bodyRows.length > 0) this.moveTo(0, 0) + else this.frame.focus({ preventScroll: true }) + } + + build() { + // A per column is what makes the crosshair free: a class on one + // paints the whole column, with no per-cell bookkeeping on a + // table that could be hundreds of rows long. + const group = document.createElement("colgroup") + this.columns = Array.from({ length: this.columnCount }, () => { + const col = document.createElement("col") + group.append(col) + return col + }) + this.table.prepend(group) + + this.headerCells.forEach((cell, index) => { + cell.setAttribute("scope", "col") + cell.dataset.column = index + cell.setAttribute("aria-sort", "none") + cell.tabIndex = -1 + }) + + this.bodyRows.forEach(row => { + Array.from(row.cells).forEach((cell, index) => { + cell.dataset.column = index + cell.tabIndex = -1 + cell.setAttribute("role", "gridcell") + }) + }) + + this.valueBar = node("div", "data-sheet__value") + this.valueLabel = node("span", "data-sheet__value-label") + this.valueContent = node("div", "data-sheet__value-content") + this.valueBar.append(this.valueLabel, this.valueContent) + + this.frame = node("div", "data-sheet__frame") + this.frame.tabIndex = 0 + this.frame.append(this.table) + + const sheet = node("div", "data-sheet") + sheet.append(this.valueBar, this.frame) + this.expander.body.append(sheet) + + this.wrapButton = this.expander.addTool({ + label: "Wrap cell text", + hint: "Wrap cell text", + icon: ICONS.wrap, + onClick: () => this.toggleWrap() + }) + this.resetButton = this.expander.addTool({ + label: "Reset sort order", + hint: "Back to the document's order", + icon: ICONS.unsort, + className: "expander__tool expander__tool--hidden", + onClick: () => this.applySort(null) + }) + + this.address = node("span", "data-sheet__address") + this.column = node("span", "data-sheet__column") + this.dimensions = node("span", "data-sheet__dimensions") + this.dimensions.textContent = + `${count(this.bodyRows.length, "row")} × ${count(this.columnCount, "column")}` + const hint = node("span", "expander__hint") + hint.textContent = "Arrows move · Home/End jump · ⌘C copies the cell" + this.expander.setStatus([ this.address, this.column, this.dimensions, hint ]) + } + + wire() { + this.table.addEventListener("click", event => { + const cell = event.target.closest("td, th") + if (!cell) return + if (cell.parentElement.parentElement === this.table.tHead) { + this.applySort(this.nextSortFor(Number(cell.dataset.column))) + return + } + const row = this.bodyRows.indexOf(cell.parentElement) + if (row >= 0) this.moveTo(row, Number(cell.dataset.column)) + }) + + this.frame.addEventListener("keydown", event => this.handleKey(event)) + } + + handleKey(event) { + if (event.altKey) return + + if ((event.metaKey || event.ctrlKey) && event.key.toLowerCase() === "c") { + if (!this.cursor) return + event.preventDefault() + navigator.clipboard?.writeText(this.cursor.textContent.trim()) + this.flashCopied() + return + } + if (event.metaKey || event.ctrlKey) return + + const lastRow = this.bodyRows.length - 1 + const lastColumn = this.columnCount - 1 + const page = Math.max(1, Math.floor(this.frame.clientHeight / (this.cursor?.offsetHeight || 32)) - 1) + let { row, column } = this.position || { row: 0, column: 0 } + + switch (event.key) { + case "ArrowUp": row -= 1; break + case "ArrowDown": row += 1; break + case "ArrowLeft": column -= 1; break + case "ArrowRight": column += 1; break + case "PageUp": row -= page; break + case "PageDown": row += page; break + // Home/End move along the row; with shift they jump to the corners of + // the whole table, the way a spreadsheet's ctrl+Home does (ctrl is + // already spoken for by copy). + case "Home": + column = 0 + if (event.shiftKey) row = 0 + break + case "End": + column = lastColumn + if (event.shiftKey) row = lastRow + break + default: return + } + + event.preventDefault() + this.moveTo(clamp(row, 0, lastRow), clamp(column, 0, lastColumn)) + } + + moveTo(row, column) { + const cell = this.bodyRows[row]?.cells[column] + if (!cell) return + + this.cursor?.classList.remove("is-cursor") + this.cursor?.removeAttribute("aria-selected") + this.bodyRows.forEach(candidate => candidate.classList.remove("is-cursor-row")) + this.columns.forEach(candidate => candidate.classList.remove("is-cursor-column")) + this.headerCells.forEach(candidate => candidate.classList.remove("is-cursor-column")) + + this.position = { row, column } + this.cursor = cell + cell.classList.add("is-cursor") + cell.setAttribute("aria-selected", "true") + this.bodyRows[row].classList.add("is-cursor-row") + this.columns[column]?.classList.add("is-cursor-column") + this.headerCells[column]?.classList.add("is-cursor-column") + + this.address.textContent = `${columnName(column)}${row + 1}` + const header = this.headerCells[column]?.textContent.trim() + this.column.textContent = header || "" + this.column.hidden = !header + + this.valueLabel.textContent = header || columnName(column) + this.valueContent.replaceChildren(...Array.from(cell.cloneNode(true).childNodes)) + this.valueBar.classList.toggle("is-empty", cell.textContent.trim() === "") + + cell.focus({ preventScroll: true }) + this.reveal(cell) + } + + // Focus scrolls a cell into view on its own, but it doesn't know the + // pinned header and first column are sitting on top of the region it + // scrolled to — so it happily parks the cursor underneath them. + reveal(cell) { + const frame = this.frame.getBoundingClientRect() + const box = cell.getBoundingClientRect() + const headerHeight = this.table.tHead?.getBoundingClientRect().height || 0 + const gutter = this.position.column === 0 ? 0 : (this.bodyRows[0]?.cells[0]?.getBoundingClientRect().width || 0) + + let left = 0 + let top = 0 + if (box.top < frame.top + headerHeight) top = box.top - frame.top - headerHeight + else if (box.bottom > frame.bottom) top = box.bottom - frame.bottom + if (box.left < frame.left + gutter) left = box.left - frame.left - gutter + else if (box.right > frame.right) left = box.right - frame.right + + if (left || top) this.frame.scrollBy({ left, top, behavior: "instant" }) + } + + nextSortFor(column) { + if (this.sort?.column !== column) return { column, direction: "asc" } + return this.sort.direction === "asc" ? { column, direction: "desc" } : null + } + + applySort(sort) { + this.sort = sort + const body = this.table.tBodies[0] + if (!body) return + + if (sort) { + const values = new Map(this.sourceOrder.map(row => + [ row, (row.cells[sort.column]?.textContent || "").trim() ])) + const numeric = this.sourceOrder.every(row => isNumeric(values.get(row))) + const order = sort.direction === "asc" ? 1 : -1 + this.bodyRows = this.sourceOrder.slice().sort((a, b) => { + const left = values.get(a) + const right = values.get(b) + // Blanks sort last in both directions — a missing value isn't + // "smallest", it's absent. + if (left === "" || right === "") return left === right ? 0 : left === "" ? 1 : -1 + if (numeric) return (numberOf(left) - numberOf(right)) * order + return left.localeCompare(right, undefined, { numeric: true, sensitivity: "base" }) * order + }) + } else { + this.bodyRows = this.sourceOrder.slice() + } + + body.append(...this.bodyRows) + this.headerCells.forEach((cell, index) => { + const active = sort?.column === index + cell.setAttribute("aria-sort", active ? (sort.direction === "asc" ? "ascending" : "descending") : "none") + cell.classList.toggle("is-sorted-asc", active && sort.direction === "asc") + cell.classList.toggle("is-sorted-desc", active && sort.direction === "desc") + }) + this.resetButton.classList.toggle("expander__tool--hidden", !sort) + + // The cursor follows its cell, not its coordinates — the value you were + // looking at is the thing you care about after a re-sort. + if (this.cursor) { + const row = this.bodyRows.indexOf(this.cursor.parentElement) + if (row >= 0) this.moveTo(row, this.position.column) + } + } + + toggleWrap() { + const wrapped = this.frame.classList.toggle("is-wrapped") + this.wrapButton.classList.toggle("is-active", wrapped) + this.wrapButton.setAttribute("aria-pressed", String(wrapped)) + if (this.cursor) this.reveal(this.cursor) + } + + flashCopied() { + this.address.classList.add("is-copied") + setTimeout(() => this.address.classList.remove("is-copied"), 600) + } +} + +function node(name, className) { + const element = document.createElement(name) + element.className = className + return element +} + +function clamp(value, low, high) { + return Math.min(high, Math.max(low, value)) +} + +function count(n, noun) { + return `${n} ${noun}${n === 1 ? "" : "s"}` +} + +// Spreadsheet column names: A, B, … Z, AA, AB. +function columnName(index) { + let name = "" + let n = index + do { + name = String.fromCharCode(65 + (n % 26)) + name + n = Math.floor(n / 26) - 1 + } while (n >= 0) + return name +} + +// Table numbers in plans wear units: "$1,200", "38%", "12ms", "~4". A column +// counts as numeric only if every value in it reads as one, so a column of +// names never gets sorted by the digits inside it. +const NUMERIC = /^[^\d-]{0,3}-?[\d,]+(\.\d+)?[^\d]{0,4}$/ + +function isNumeric(value) { + return value === "" || NUMERIC.test(value) +} + +function numberOf(value) { + return Number.parseFloat(value.replace(/[^\d.-]/g, "")) || 0 +} diff --git a/engine/app/javascript/controllers/coplan/mermaid_controller.js b/engine/app/javascript/controllers/coplan/mermaid_controller.js index e8fd7059..4a881555 100644 --- a/engine/app/javascript/controllers/coplan/mermaid_controller.js +++ b/engine/app/javascript/controllers/coplan/mermaid_controller.js @@ -1,9 +1,22 @@ import { Controller } from "@hotwired/stimulus" +import { openExpander, attachExpandAffordance, nearestHeading, ICONS } from "coplan/expander" +import { createPanZoom } from "coplan/pan_zoom" let diagramId = 0 let mermaidPromise let configuredTheme +// How far a diagram may be shrunk to fit the document column before +// legibility loses the argument. Past this, the diagram keeps its real size +// and its frame scrolls sideways instead — a label you can't read is worse +// than a scrollbar. +const MIN_INLINE_SCALE = 0.8 + +// The same bargain in the expanded surface, where there's somewhere to pan +// to: a wide diagram "fitted" to a phone would land at 15%, so fit stops at +// roughly 9px labels and lets you drag the rest into view. +const MIN_EXPANDED_SCALE = 0.55 + export default class extends Controller { connect() { this.renderGeneration ||= 0 @@ -11,6 +24,9 @@ export default class extends Controller { this.colorSchemeQuery = window.matchMedia("(prefers-color-scheme: dark)") window.addEventListener("coplan:theme-changed", this.boundThemeChange) this.colorSchemeQuery.addEventListener("change", this.boundThemeChange) + this.sizeObserver = new ResizeObserver(entries => { + entries.forEach(entry => this.sizeDiagram(entry.target.closest(".mermaid-diagram"))) + }) this.renderDiagrams() } @@ -18,7 +34,8 @@ export default class extends Controller { this.renderGeneration += 1 window.removeEventListener("coplan:theme-changed", this.boundThemeChange) this.colorSchemeQuery.removeEventListener("change", this.boundThemeChange) - this.lightbox?.close() + this.sizeObserver?.disconnect() + this.closeExpanded() } async renderDiagrams() { @@ -38,6 +55,10 @@ export default class extends Controller { try { const mermaid = await loadMermaid() + // Mermaid measures label text in the DOM to size its node boxes. Lexend + // arrives as a swapped webfont, so measuring before it lands sizes every + // box for the fallback face and the real labels then overflow them. + await document.fonts?.ready if (!this.element.isConnected || generation !== this.renderGeneration) return for (const { container, source } of sources) { @@ -70,10 +91,20 @@ export default class extends Controller { diagram.setAttribute("aria-label", "Mermaid diagram") diagram.dataset.mermaidSource = source diagram.dataset.mermaidTheme = theme - diagram.innerHTML = svg + + // The SVG lives in its own scroll box so the expand affordance stays + // pinned to the frame's corner instead of scrolling away with a wide + // diagram. + const canvas = document.createElement("div") + canvas.className = "mermaid-diagram__canvas" + canvas.innerHTML = svg + diagram.append(canvas) + this.makeExpandable(diagram) sourceContainer.replaceWith(diagram) - bindFunctions?.(diagram) + bindFunctions?.(canvas) + this.sizeDiagram(diagram) + this.sizeObserver?.observe(canvas) } catch { document.getElementById(id)?.remove() document.getElementById(`d${id}`)?.remove() @@ -83,57 +114,99 @@ export default class extends Controller { } } + // Decides between fitting the diagram to the column and letting it keep + // its real size behind a horizontal scroll. + sizeDiagram(diagram) { + const canvas = diagram?.querySelector(".mermaid-diagram__canvas") + const svg = canvas?.querySelector("svg") + if (!svg) return + + const { width, height } = naturalSize(svg) + const available = canvas.clientWidth + if (!width || !available) return + + const scrolling = available / width < MIN_INLINE_SCALE + diagram.classList.toggle("mermaid-diagram--scrolling", scrolling) + const nextWidth = scrolling ? `${Math.round(width)}px` : "" + const nextHeight = scrolling ? `${Math.round(height)}px` : "" + // Only write when the value changes: the observer watches this element, + // and an unconditional write would keep re-triggering itself. + if (svg.style.width !== nextWidth) svg.style.width = nextWidth + if (svg.style.height !== nextHeight) svg.style.height = nextHeight + } + makeExpandable(diagram) { - const expand = document.createElement("button") - expand.type = "button" - expand.className = "mermaid-diagram__expand" - expand.setAttribute("aria-label", "Expand diagram") - expand.title = "Expand diagram" - expand.innerHTML = EXPAND_ICON - diagram.append(expand) + attachExpandAffordance(diagram, { + label: "Expand diagram", + hint: "Expand diagram", + className: "mermaid-diagram__expand", + onExpand: () => this.openExpanded(diagram) + }) diagram.addEventListener("click", event => { // Let clicks on interactive nodes inside the diagram behave normally. // Comment marks open their thread popover — expanding here would // force-hide it (showModal closes every open popover). if (event.target.closest("a, mark.anchor-highlight")) return - this.openLightbox(diagram) + this.openExpanded(diagram) }) } - openLightbox(diagram) { + openExpanded(diagram) { const svg = diagram.querySelector("svg") - if (!svg || this.lightbox) return + if (!svg || this.expanded) return - const lightbox = document.createElement("dialog") - lightbox.className = "mermaid-lightbox" - lightbox.setAttribute("aria-label", "Expanded Mermaid diagram") - lightbox.dataset.turboTemporary = "" - - const close = document.createElement("button") - close.type = "button" - close.className = "mermaid-lightbox__close" - close.setAttribute("aria-label", "Close expanded diagram") - close.title = "Close" - close.innerHTML = CLOSE_ICON + const { width, height } = naturalSize(svg) + const expander = openExpander({ + title: nearestHeading(diagram) || "Diagram", + label: "Expanded Mermaid diagram", + variant: "diagram", + status: true, + onClose: () => { + this.panZoom?.destroy() + this.panZoom = null + this.expanded = null + } + }) + this.expanded = expander + const viewport = document.createElement("div") + viewport.className = "expander__canvas" + viewport.tabIndex = 0 const content = svg.cloneNode(true) content.removeAttribute("width") content.removeAttribute("height") - content.style.maxWidth = "none" - content.style.width = "100%" - content.style.height = "100%" - - lightbox.append(close, content) - lightbox.addEventListener("click", () => lightbox.close()) - lightbox.addEventListener("close", () => { - lightbox.remove() - if (this.lightbox === lightbox) this.lightbox = null + viewport.append(content) + expander.body.append(viewport) + + let readout + this.panZoom = createPanZoom(viewport, content, { + width, + height, + minFit: MIN_EXPANDED_SCALE, + onChange: scale => { if (readout) readout.textContent = `${Math.round(scale * 100)}%` } }) - this.lightbox = lightbox - document.body.append(lightbox) - lightbox.showModal() + expander.addTool({ label: "Zoom out", hint: "Zoom out (−)", icon: ICONS.zoomOut, onClick: () => this.panZoom.zoomOut() }) + readout = expander.addToolReadout(`${Math.round(this.panZoom.scale * 100)}%`) + expander.addTool({ label: "Zoom in", hint: "Zoom in (+)", icon: ICONS.zoomIn, onClick: () => this.panZoom.zoomIn() }) + expander.addTool({ label: "Fit to screen", hint: "Fit to screen (0)", icon: ICONS.fit, onClick: () => this.panZoom.fit() }) + expander.addTool({ label: "Actual size", hint: "Actual size (1)", icon: ICONS.actual, onClick: () => this.panZoom.actualSize() }) + + const hint = document.createElement("span") + hint.className = "expander__hint" + // On a touch screen the gestures are the whole interface — and the + // keyboard shortcuts are not available — so say the touch ones instead. + hint.textContent = window.matchMedia("(hover: none)").matches + ? "Drag to pan · pinch to zoom · double-tap to fit" + : "Drag to pan · scroll or pinch to zoom · double-click to fit · 0 fit · 1 actual size" + expander.setStatus(hint) + + viewport.focus({ preventScroll: true }) + } + + closeExpanded() { + this.expanded?.close() } showError(sourceContainer) { @@ -149,18 +222,6 @@ export default class extends Controller { } } -const EXPAND_ICON = ` - -` - -const CLOSE_ICON = ` - -` - function loadMermaid() { if (mermaidPromise) return mermaidPromise @@ -168,6 +229,16 @@ function loadMermaid() { return mermaidPromise } +// A mermaid SVG carries its laid-out size in the viewBox; the width/height +// attributes are the responsive "100%" mermaid applies on top. +function naturalSize(svg) { + const box = svg.viewBox?.baseVal + if (box?.width) return { width: box.width, height: box.height } + + const rect = svg.getBoundingClientRect() + return { width: rect.width, height: rect.height } +} + // A deck is a fixed artifact — its diagrams follow the deck theme, not // the reader's app scheme, so a dark-mode reader sees the same slides as // a light-mode one. Outside a deck, diagrams follow the app. The theme @@ -186,17 +257,38 @@ function configureMermaid(mermaid, dark) { const theme = dark ? "dark" : "light" if (theme === configuredTheme) return theme + const fontFamily = appFontStack() mermaid.initialize({ startOnLoad: false, securityLevel: "strict", suppressErrorRendering: true, theme: dark ? "base" : "default", - ...(dark && { themeVariables: darkThemeVariables() }) + // Mermaid's 14px default reads small next to 16px body copy, and it's + // the label size that survives (or doesn't) being scaled to fit. + fontFamily, + themeVariables: { fontSize: "16px", fontFamily, ...(dark && darkThemeVariables()) }, + themeCSS: THEME_CSS, + flowchart: { padding: 14, nodeSpacing: 55, rankSpacing: 60, useMaxWidth: true }, + sequence: { actorFontSize: 15, messageFontSize: 15, noteFontSize: 14 }, + gantt: { fontSize: 14 } }) configuredTheme = theme return theme } +// Thin hairlines are the other half of "hard to read" — at a reduced scale +// a 1px edge disappears well before its label does. +const THEME_CSS = ` + .flowchart-link, .relationshipLine, .messageLine0, .messageLine1 { stroke-width: 1.75px; } + .edgeLabel { font-size: 14px; } + .cluster rect { stroke-width: 1.25px; } +` + +function appFontStack() { + const declared = getComputedStyle(document.documentElement).getPropertyValue("--font-sans").trim() + return declared || "system-ui, sans-serif" +} + function darkThemeVariables() { return { darkMode: true, diff --git a/engine/app/javascript/coplan/expander.js b/engine/app/javascript/coplan/expander.js new file mode 100644 index 00000000..056d390b --- /dev/null +++ b/engine/app/javascript/coplan/expander.js @@ -0,0 +1,163 @@ +// The shared takeover surface. Two very different things in a plan — a +// Mermaid diagram and a data table — both need the identical outer +// experience: a modal that owns the viewport, one title/toolbar/close +// chrome, disciplined dismissal, and the keyboard to itself while open. +// Only what happens *inside* the body differs (pan-zoom for a diagram, a +// cell cursor for a table), so that part is the caller's business. +// +// Dismissal is deliberately narrow: backdrop or Escape. The Mermaid +// lightbox this replaces closed on any click anywhere, which is precisely +// why it could never support panning — every drag ended in a close. + +let current = null + +export const ICONS = { + expand: icon(''), + close: icon(''), + zoomIn: icon(''), + zoomOut: icon(''), + fit: icon(''), + actual: icon(''), + wrap: icon(''), + unsort: icon('') +} + +// Opens the surface. Returns a handle: fill `body`, hang controls off +// `addTool`, write a one-line readout with `setStatus`. +export function openExpander({ title = "", label = "Expanded view", variant = null, status = false, onClose = null } = {}) { + current?.close() + + const dialog = document.createElement("dialog") + dialog.className = [ "expander", variant && `expander--${variant}` ].filter(Boolean).join(" ") + dialog.setAttribute("aria-label", label) + // Turbo caches the page on navigation; a cached open dialog would come + // back as a dead overlay with no controller behind it. + dialog.dataset.turboTemporary = "" + + const bar = element("header", "expander__bar") + const heading = element("h2", "expander__title") + heading.textContent = title + const tools = element("div", "expander__tools") + const closeButton = toolButton({ label: "Close", hint: "Close (Esc)", icon: ICONS.close, className: "expander__close" }) + closeButton.addEventListener("click", () => dialog.close()) + bar.append(heading, tools, closeButton) + + const body = element("div", "expander__body") + dialog.append(bar, body) + + const statusBar = status ? element("footer", "expander__status") : null + if (statusBar) dialog.append(statusBar) + + dialog.addEventListener("pointerdown", event => { + // The bar, body and status bar tile the dialog completely, so the + // dialog itself is only ever the event target for a backdrop press. + if (event.target === dialog) dialog.close() + }) + + // While a takeover is open it owns the keyboard: the page's hotkey + // controllers all listen on `document`, so stopping the event here — on + // the way out, after everything inside the dialog has seen it — keeps + // `[`/`]`/`j`/Backspace/Ctrl+Space from firing against the document + // hidden behind the overlay. Escape still closes: that's the UA's + // default action, which propagation doesn't govern. + dialog.addEventListener("keydown", event => { + if (!isTyping(event.target)) event.stopPropagation() + }) + + const handle = { + dialog, + body, + addTool: spec => { + const button = toolButton(spec) + if (spec.onClick) button.addEventListener("click", spec.onClick) + tools.append(button) + return button + }, + addToolReadout: text => { + const readout = element("span", "expander__readout") + readout.textContent = text + tools.append(readout) + return readout + }, + setStatus: nodes => { + if (!statusBar) return + statusBar.replaceChildren(...(Array.isArray(nodes) ? nodes : [ nodes ])) + }, + setTitle: text => { heading.textContent = text }, + close: () => dialog.close() + } + + dialog.addEventListener("close", () => { + document.documentElement.classList.remove("expander-open") + dialog.remove() + if (current === handle) current = null + onClose?.() + }) + + current = handle + document.documentElement.classList.add("expander-open") + document.body.append(dialog) + dialog.showModal() + return handle +} + +// The corner control that opens the surface. Shared so a table and a +// diagram present the same affordance in the same place. +export function attachExpandAffordance(container, { label, hint, onExpand, className }) { + const button = toolButton({ label, hint: hint || label, icon: ICONS.expand, className }) + button.addEventListener("click", event => { + event.preventDefault() + event.stopPropagation() + onExpand() + }) + container.append(button) + return button +} + +export function closeExpander() { + current?.close() +} + +// The heading a block sits under, so an expanded view can say what you're +// looking at instead of just "Table" or "Diagram". Walks backwards through +// preceding siblings, then out of the enclosing section, and stops at the +// rendered-markdown root. +export function nearestHeading(element) { + let node = element + while (node && !node.classList?.contains("markdown-rendered")) { + if (/^H[1-6]$/.test(node.tagName)) return node.textContent.trim() + node = node.previousElementSibling || node.parentElement + } + return null +} + +function toolButton({ label, hint, icon: markup, text, className }) { + const button = document.createElement("button") + button.type = "button" + button.className = className || "expander__tool" + button.setAttribute("aria-label", label) + if (hint) button.title = hint + button.innerHTML = markup || "" + if (text) { + const span = document.createElement("span") + span.textContent = text + button.append(span) + } + return button +} + +function element(name, className) { + const node = document.createElement(name) + node.className = className + return node +} + +function icon(paths) { + return `` +} + +function isTyping(target) { + const tag = target?.tagName + return tag === "INPUT" || tag === "TEXTAREA" || tag === "SELECT" || target?.isContentEditable +} diff --git a/engine/app/javascript/coplan/pan_zoom.js b/engine/app/javascript/coplan/pan_zoom.js new file mode 100644 index 00000000..f6866fa7 --- /dev/null +++ b/engine/app/javascript/coplan/pan_zoom.js @@ -0,0 +1,212 @@ +// Pan and zoom over a fixed-size piece of content inside a viewport. +// Written for Mermaid SVGs but deliberately knows nothing about them: it +// takes a viewport, a content element, and the content's natural size, and +// drives a transform. +// +// Gestures: drag to pan, wheel to zoom at the cursor, two-finger pinch to +// zoom, shift+wheel to pan sideways, double-click to toggle fit and close- +// up, and +/-/0/1/arrows from the keyboard. + +const KEYS = new Set([ "+", "=", "-", "_", "0", "1", "ArrowUp", "ArrowDown", "ArrowLeft", "ArrowRight" ]) + +export function createPanZoom(viewport, content, { + width, + height, + min = 0.1, + max = 8, + padding = 24, + // Fitting a three-node diagram to a 1400px screen would blow it up to a + // cartoon. Fit is allowed to enlarge, but only so far. + maxFit = 2, + // ...and the other end: a wide diagram "fitted" to a phone is a field of + // specks. Past this floor, fit stops shrinking and the surface pans + // instead. Zooming out by hand still goes all the way to `min`. + minFit = min, + onChange = null +} = {}) { + const abort = new AbortController() + const signal = abort.signal + const pointers = new Map() + + let scale = 1 + let x = 0 + let y = 0 + let fitScale = 1 + + content.style.position = "absolute" + content.style.top = "0" + content.style.left = "0" + content.style.transformOrigin = "0 0" + content.style.width = `${width}px` + content.style.height = `${height}px` + content.style.maxWidth = "none" + + function apply() { + const box = viewport.getBoundingClientRect() + x = clampAxis(x, width * scale, box.width) + y = clampAxis(y, height * scale, box.height) + content.style.transform = `translate3d(${x}px, ${y}px, 0) scale(${scale})` + onChange?.(scale) + } + + function setScale(next, anchorX, anchorY) { + const clamped = Math.min(max, Math.max(min, next)) + if (clamped === scale) return + const ratio = clamped / scale + // Hold the content point under the anchor still. + x = anchorX - ratio * (anchorX - x) + y = anchorY - ratio * (anchorY - y) + scale = clamped + apply() + } + + function viewportPoint(event) { + const box = viewport.getBoundingClientRect() + return [ event.clientX - box.left, event.clientY - box.top ] + } + + function center() { + const box = viewport.getBoundingClientRect() + return [ box.width / 2, box.height / 2 ] + } + + function fit() { + const box = viewport.getBoundingClientRect() + const available = { width: Math.max(1, box.width - padding * 2), height: Math.max(1, box.height - padding * 2) } + fitScale = Math.min(maxFit, Math.max(minFit, Math.min(available.width / width, available.height / height))) + scale = fitScale + // Centre on whichever axis the whole thing does fit; on an axis held up + // by the floor, start at the beginning of the content rather than its + // middle — `apply` clamps these to the edge. + x = width * scale <= box.width ? (box.width - width * scale) / 2 : 0 + y = height * scale <= box.height ? (box.height - height * scale) / 2 : 0 + apply() + } + + function actualSize() { + const [ cx, cy ] = center() + setScale(1, cx, cy) + } + + function zoomBy(factor, anchor) { + const [ ax, ay ] = anchor || center() + setScale(scale * factor, ax, ay) + } + + viewport.addEventListener("wheel", event => { + event.preventDefault() + if (event.shiftKey && !event.ctrlKey) { + x -= event.deltaY || event.deltaX + apply() + return + } + // A trackpad pinch arrives as ctrl+wheel with much smaller deltas than + // a mouse wheel notch, so it needs a stronger multiplier to feel 1:1. + const intensity = event.ctrlKey ? 0.012 : 0.0022 + const step = event.deltaMode === 1 ? event.deltaY * 16 : event.deltaY + setScale(scale * Math.exp(-step * intensity), ...viewportPoint(event)) + }, { passive: false, signal }) + + viewport.addEventListener("pointerdown", event => { + if (event.button !== 0 && event.pointerType === "mouse") return + viewport.setPointerCapture(event.pointerId) + pointers.set(event.pointerId, { x: event.clientX, y: event.clientY }) + viewport.classList.add("is-grabbing") + }, { signal }) + + viewport.addEventListener("pointermove", event => { + const previous = pointers.get(event.pointerId) + if (!previous) return + const current = { x: event.clientX, y: event.clientY } + + if (pointers.size === 1) { + x += current.x - previous.x + y += current.y - previous.y + pointers.set(event.pointerId, current) + apply() + return + } + + if (pointers.size === 2) { + const other = [ ...pointers.entries() ].find(([ id ]) => id !== event.pointerId)?.[1] + if (!other) return + const box = viewport.getBoundingClientRect() + const before = distance(previous, other) + const after = distance(current, other) + const midBefore = midpoint(previous, other, box) + const midAfter = midpoint(current, other, box) + pointers.set(event.pointerId, current) + + // Pan by how far the grip moved, then scale about where it now is. + x += midAfter[0] - midBefore[0] + y += midAfter[1] - midBefore[1] + if (before > 0) setScale(scale * (after / before), ...midAfter) + else apply() + } + }, { signal }) + + const release = event => { + pointers.delete(event.pointerId) + if (pointers.size === 0) viewport.classList.remove("is-grabbing") + } + viewport.addEventListener("pointerup", release, { signal }) + viewport.addEventListener("pointercancel", release, { signal }) + + viewport.addEventListener("dblclick", event => { + event.preventDefault() + // Near the fit scale, a double-click means "let me look closer"; from + // anywhere else it means "show me the whole thing again". + if (Math.abs(scale - fitScale) < 0.01) setScale(Math.max(1, fitScale * 2), ...viewportPoint(event)) + else fit() + }, { signal }) + + viewport.addEventListener("keydown", event => { + if (event.metaKey || event.ctrlKey || !KEYS.has(event.key)) return + event.preventDefault() + const step = event.shiftKey ? 200 : 60 + switch (event.key) { + case "+": case "=": zoomBy(1.25); break + case "-": case "_": zoomBy(0.8); break + case "0": fit(); break + case "1": actualSize(); break + case "ArrowUp": y += step; apply(); break + case "ArrowDown": y -= step; apply(); break + case "ArrowLeft": x += step; apply(); break + case "ArrowRight": x -= step; apply(); break + } + }, { signal }) + + const resize = new ResizeObserver(() => apply()) + resize.observe(viewport) + + fit() + + return { + fit, + actualSize, + zoomIn: () => zoomBy(1.25), + zoomOut: () => zoomBy(0.8), + get scale() { return scale }, + destroy: () => { + abort.abort() + resize.disconnect() + } + } +} + +// Keeps the content anchored to the viewport: when it's larger, no empty +// gap can be dragged in; when it's smaller, it stays fully visible. +function clampAxis(position, scaledLength, viewportLength) { + if (scaledLength <= viewportLength) { + return Math.min(Math.max(position, 0), viewportLength - scaledLength) + } + return Math.min(Math.max(position, viewportLength - scaledLength), 0) +} + +function distance(a, b) { + return Math.hypot(a.x - b.x, a.y - b.y) +} + +function midpoint(a, b, box) { + return [ (a.x + b.x) / 2 - box.left, (a.y + b.y) / 2 - box.top ] +} diff --git a/engine/config/importmap.rb b/engine/config/importmap.rb index 40134bd5..a6f260eb 100644 --- a/engine/config/importmap.rb +++ b/engine/config/importmap.rb @@ -1,5 +1,10 @@ pin "@rails/actioncable", to: "actioncable.esm.js" pin "coplan/web_push", to: "coplan/web_push.js" pin "coplan/deck_ink", to: "coplan/deck_ink.js" +# Statically imported by the mermaid and data-grid controllers, which are +# themselves preloaded — preload these too or every page pays a round trip +# to discover them. +pin "coplan/expander", to: "coplan/expander.js", preload: true +pin "coplan/pan_zoom", to: "coplan/pan_zoom.js", preload: true pin "mermaid", to: "https://cdn.jsdelivr.net/npm/mermaid@11.16.0/dist/mermaid.esm.min.mjs", preload: false pin_all_from CoPlan::Engine.root.join("app/javascript/controllers/coplan"), under: "controllers/coplan", preload: true diff --git a/spec/helpers/markdown_helper_spec.rb b/spec/helpers/markdown_helper_spec.rb index abeb2ab0..0923791e 100644 --- a/spec/helpers/markdown_helper_spec.rb +++ b/spec/helpers/markdown_helper_spec.rb @@ -246,4 +246,51 @@ expect(html).not_to include('class="mention"') end end + + describe "data tables" do + let(:markdown) do + <<~MD + | Phase | Owner | + |---|---| + | One | sam | + MD + end + + it "frames a table so a wide one scrolls instead of running off the page" do + doc = Nokogiri::HTML::DocumentFragment.parse(helper.render_markdown(markdown)) + + grid = doc.at_css("div.data-grid") + expect(grid["data-controller"]).to eq("coplan--data-grid") + frame = grid.at_css("div.data-grid__frame") + expect(frame["data-coplan--data-grid-target"]).to eq("frame") + expect(frame.element_children.map(&:name)).to eq([ "table" ]) + end + + it "adds no visible text, so comment anchors count what they counted before" do + framed = Nokogiri::HTML::DocumentFragment.parse(helper.render_markdown(markdown)).text + bare = Nokogiri::HTML::DocumentFragment.parse(helper.render_markdown(markdown, data_tables: false)).text + + expect(framed).to eq(bare) + end + + it "frames every table in a document" do + doc = Nokogiri::HTML::DocumentFragment.parse(helper.render_markdown("#{markdown}\n#{markdown}")) + + expect(doc.css("div.data-grid").size).to eq(2) + expect(doc.css("table").all? { |table| table.parent.classes.include?("data-grid__frame") }).to be(true) + end + + it "leaves a document without tables alone" do + html = helper.render_markdown("Just a paragraph.") + + expect(html).not_to include("data-grid") + end + + it "can be turned off, for decks that own their own tabular layout" do + html = helper.render_markdown(markdown, data_tables: false) + + expect(html).not_to include("data-grid") + expect(html).to include("") + end + end end diff --git a/spec/system/comment_ux_spec.rb b/spec/system/comment_ux_spec.rb index c36baeee..27891c02 100644 --- a/spec/system/comment_ux_spec.rb +++ b/spec/system/comment_ux_spec.rb @@ -175,7 +175,7 @@ def create_anchored_thread(plan:, anchor_text:, body:, user:) expect(page).to have_css('.mermaid-diagram[data-mermaid-theme="dark"]', wait: 10) end - it "expands a Mermaid diagram into a lightbox on click" do + it "expands a Mermaid diagram into the takeover surface on click" do plan.current_plan_version.update!(content_markdown: <<~MARKDOWN) ```mermaid flowchart LR @@ -187,10 +187,12 @@ def create_anchored_thread(plan:, anchor_text:, body:, user:) expect(page).to have_css(".mermaid-diagram svg", wait: 10) find(".mermaid-diagram").click - expect(page).to have_css(".mermaid-lightbox svg") + expect(page).to have_css(".expander--diagram .expander__canvas > svg") - find(".mermaid-lightbox").click - expect(page).not_to have_css(".mermaid-lightbox") + # Dismissal is the backdrop or Escape — a click inside is a pan, not + # a close. + find(".expander__close").click + expect(page).not_to have_css(".expander--diagram") end it "keeps invalid Mermaid source readable" do diff --git a/spec/system/data_grid_spec.rb b/spec/system/data_grid_spec.rb new file mode 100644 index 00000000..4fbc754c --- /dev/null +++ b/spec/system/data_grid_spec.rb @@ -0,0 +1,226 @@ +require "rails_helper" + +# A markdown table in a plan, in both of its forms: the framed, compact one +# that sits in the document, and the spreadsheet it expands into. +RSpec.describe "Data tables", type: :system do + let(:author) { create(:coplan_user, email: "author@example.com") } + + let(:plan_content) do + <<~MARKDOWN + # Rollout + + ## Phases + + | Phase | Owner | Region | Traffic | Latency | Notes | + |---|---|---|---|---|---| + | Pilot | sam | us-east | 2% | 180ms | Internal merchants only, behind the flag | + | Ramp | kim | us-west | 25% | 165ms | Waiting on the ledger backfill to finish | + | Wide | ada | eu-central | 60% | 210ms | Needs a second region before it can go on | + | Full | lee | global | 100% | 150ms | Flag removed, config baked in | + | Hold | raj | ap-south | 0% | 195ms | Paused pending a compliance review | + | Sunset | tom | us-east | 0% | 172ms | Old path retired after two clean weeks | + | Audit | joe | global | 5% | 188ms | Sampled traffic for the quarterly review | + | Repair | pat | us-west | 10% | 205ms | Retry path rebuilt, watching error rates | + | Verify | nia | eu-central | 40% | 169ms | Shadow reads compared against the legacy | + + The table above is the plan of record. + MARKDOWN + end + + let(:plan) do + p = CoPlan::Plan.create!(title: "Rollout Plan", created_by_user: author) + version = CoPlan::PlanVersion.create!( + plan: p, revision: 1, + content_markdown: plan_content, actor_type: "human", actor_id: author.id + ) + p.update!(current_plan_version: version, current_revision: 1) + p + end + + def sign_in(user) + visit sign_in_path + fill_in "Email address", with: user.email + click_button "Sign In" + expect(page).to have_current_path(root_path) + expect(page).to have_button("Menu") + end + + # The affordance only shows on hover, so a click has to be preceded by + # one — and the controller only offers it once it has measured the table. + def open_spreadsheet + expect(page).to have_css(".data-grid.is-expandable") + find(".data-grid").hover + find(".data-grid__expand").click + expect(page).to have_css(".expander--grid .data-sheet__table") + end + + def computed(selector, property) + page.evaluate_script( + "getComputedStyle(document.querySelector(#{selector.to_json})).#{property}" + ) + end + + def frame_overflows? + page.evaluate_script(<<~JS) + (() => { + const frame = document.querySelector(".data-grid__frame") + return frame.scrollWidth - frame.clientWidth > 1 + })() + JS + end + + # Sixteen columns of unbreakable tokens: no amount of wrapping saves this + # one, so it has to scroll. + let(:wide_plan) do + header = (1..16).map { |n| "Column#{n}" } + row = (1..16).map { |n| "value-#{n}-unbreakable" } + markdown = [ + "# Wide", "", + "| #{header.join(' | ')} |", + "|#{([ '---' ] * 16).join('|')}|", + "| #{row.join(' | ')} |", "" + ].join("\n") + + p = CoPlan::Plan.create!(title: "Wide Plan", created_by_user: author) + version = CoPlan::PlanVersion.create!( + plan: p, revision: 1, + content_markdown: markdown, actor_type: "human", actor_id: author.id + ) + p.update!(current_plan_version: version, current_revision: 1) + p + end + + before { sign_in(author) } + + describe "in the document" do + it "wraps long cells so a realistic table fits the column" do + visit plan_page_path(plan) + + expect(page).to have_css(".data-grid__frame table") + expect(computed(".data-grid__frame", "overflowX")).to eq("auto") + expect(frame_overflows?).to be(false) + expect(page).to have_no_css(".data-grid.is-scrolled-end") + end + + it "keeps a table too wide to wrap inside its own frame, not off the page" do + visit plan_page_path(wide_plan) + + # This is the reported bug: the table used to widen the document. + expect(page.evaluate_script("document.documentElement.scrollWidth <= window.innerWidth")).to be(true) + expect(frame_overflows?).to be(true) + # ...and it says so, rather than hiding the rest silently. + expect(page).to have_css(".data-grid.is-scrolled-end") + end + + it "pins the header row inside the frame" do + visit plan_page_path(plan) + + expect(page).to have_css(".data-grid thead th") + expect(computed(".data-grid thead th", "position")).to eq("sticky") + end + + it "offers to expand a table big enough to be worth it" do + visit plan_page_path(plan) + + expect(page).to have_css(".data-grid.is-expandable") + expect(page).to have_css(".data-grid__expand", visible: :all) + end + end + + describe "expanded" do + before do + visit plan_page_path(plan) + open_spreadsheet + end + + it "titles itself with the heading the table sits under" do + expect(page).to have_css(".expander__title", text: "Phases") + expect(page).to have_css(".data-sheet__dimensions", text: "9 rows × 6 columns") + end + + it "pins both the header row and the row-label column" do + expect(computed(".data-sheet__table thead th", "position")).to eq("sticky") + expect(computed(".data-sheet__table tbody tr td:first-child", "position")).to eq("sticky") + end + + it "starts the cursor on the first cell and moves it with the arrow keys" do + expect(page).to have_css(".data-sheet__address", text: "A1") + expect(page).to have_css(".data-sheet__value-label", text: "Phase") + + find(".data-sheet__table td.is-cursor").send_keys(:arrow_right, :arrow_down) + + expect(page).to have_css(".data-sheet__address", text: "B2") + expect(page).to have_css(".data-sheet__column", text: "Owner") + expect(page).to have_css(".data-sheet__value-content", text: "kim") + expect(page).to have_css(".data-sheet__table tr.is-cursor-row td", text: "Ramp") + end + + it "shows a long value in full even though the cell itself is clipped" do + find(".data-sheet__table td.is-cursor").send_keys(:end) + + expect(page).to have_css(".data-sheet__value-label", text: "Notes") + expect(page).to have_css(".data-sheet__value-content", + text: "Internal merchants only, behind the flag") + end + + it "keeps the cursor inside the table at the edges" do + find(".data-sheet__table td.is-cursor").send_keys(:arrow_up, :arrow_left) + + expect(page).to have_css(".data-sheet__address", text: "A1") + end + + it "sorts a column on click and returns to the document's order" do + first_owner = -> { page.evaluate_script('document.querySelector(".data-sheet__table tbody td:nth-child(2)").textContent.trim()') } + expect(first_owner.call).to eq("sam") + + find(".data-sheet__table thead th", text: "Owner").click + expect(page).to have_css(".data-sheet__table thead th.is-sorted-asc", text: "Owner") + expect(first_owner.call).to eq("ada") + + find(".data-sheet__table thead th", text: "Owner").click + expect(page).to have_css(".data-sheet__table thead th.is-sorted-desc", text: "Owner") + expect(first_owner.call).to eq("tom") + + find("button[aria-label='Reset sort order']").click + expect(page).to have_no_css(".data-sheet__table thead th.is-sorted-asc") + expect(first_owner.call).to eq("sam") + end + + it "sorts a column of numbers by value, not by its digits as text" do + find(".data-sheet__table thead th", text: "Traffic").click + + order = page.evaluate_script( + 'Array.from(document.querySelectorAll(".data-sheet__table tbody td:nth-child(4)")).map(c => c.textContent.trim())' + ) + expect(order).to eq([ "0%", "0%", "2%", "5%", "10%", "25%", "40%", "60%", "100%" ]) + end + + it "stays open when you click inside it" do + find(".data-sheet__table tbody td", text: "Ramp").click + + expect(page).to have_css(".expander--grid") + expect(page).to have_css(".data-sheet__address", text: "A2") + end + + it "closes on Escape, leaving the document's own table untouched" do + find(".data-sheet__table td.is-cursor").send_keys(:escape) + + expect(page).to have_no_css(".expander--grid") + expect(page).to have_css(".data-grid__frame table") + end + end + + it "leaves a table small enough to read as it is" do + small = CoPlan::Plan.create!(title: "Small Plan", created_by_user: author) + version = CoPlan::PlanVersion.create!( + plan: small, revision: 1, actor_type: "human", actor_id: author.id, + content_markdown: "| A | B |\n|---|---|\n| 1 | 2 |\n" + ) + small.update!(current_plan_version: version, current_revision: 1) + + visit plan_page_path(small) + + expect(page).to have_css(".data-grid__frame table") + expect(page).to have_no_css(".data-grid.is-expandable") + end +end diff --git a/spec/system/deck_ux_spec.rb b/spec/system/deck_ux_spec.rb index 974c95db..53559430 100644 --- a/spec/system/deck_ux_spec.rb +++ b/spec/system/deck_ux_spec.rb @@ -316,14 +316,14 @@ def attachments_on_screen? describe "Mermaid diagrams on a slide" do it "keeps the expand control chip-sized instead of scaling it to the canvas" do visit plan_page_path(plan) - expect(page).to have_css(".deck-slide .mermaid-diagram > svg", wait: 15) + expect(page).to have_css(".deck-slide .mermaid-diagram__canvas > svg", wait: 15) sizes = page.evaluate_script(<<~JS) (() => { const diagram = document.querySelector(".deck-slide .mermaid-diagram"); const icon = diagram.querySelector(".mermaid-diagram__expand svg"); const box = el => Math.round(el.getBoundingClientRect().width); - return { diagram: box(diagram.querySelector(":scope > svg")), icon: box(icon) }; + return { diagram: box(diagram.querySelector(".mermaid-diagram__canvas > svg")), icon: box(icon) }; })() JS diff --git a/spec/system/diagram_expand_spec.rb b/spec/system/diagram_expand_spec.rb new file mode 100644 index 00000000..cf2c46c9 --- /dev/null +++ b/spec/system/diagram_expand_spec.rb @@ -0,0 +1,153 @@ +require "rails_helper" + +# Expanding a Mermaid diagram. These specs drive the real pipeline — Mermaid +# loads from the CDN pinned in the importmap and renders in the browser — so +# they need network access, same as CI. +RSpec.describe "Expanding a Mermaid diagram", type: :system do + let(:author) { create(:coplan_user, email: "author@example.com") } + + # Wide enough that fitting it to the document column would shrink it past + # the legibility floor. + let(:wide_diagram) do + chain = (1..14).map { |n| "S#{n}[Stage number #{n}]" }.each_cons(2).map { |a, b| " #{a} --> #{b}" } + "```mermaid\nflowchart LR\n#{chain.join("\n")}\n```" + end + + let(:plan_content) do + <<~MARKDOWN + # Payment Flow + + ## The pipeline + + #{wide_diagram} + + The ledger records every movement. + MARKDOWN + end + + let(:plan) do + p = CoPlan::Plan.create!(title: "Diagram Plan", created_by_user: author) + version = CoPlan::PlanVersion.create!( + plan: p, revision: 1, + content_markdown: plan_content, actor_type: "human", actor_id: author.id + ) + p.update!(current_plan_version: version, current_revision: 1) + p + end + + def sign_in(user) + visit sign_in_path + fill_in "Email address", with: user.email + click_button "Sign In" + expect(page).to have_current_path(root_path) + expect(page).to have_button("Menu") + end + + def wait_for_diagram + expect(page).to have_css(".mermaid-diagram__canvas svg", wait: 20) + end + + def zoom_percent + find(".expander__readout").text.to_i + end + + def canvas_transform + page.evaluate_script('document.querySelector(".expander__canvas > svg").style.transform') + end + + before do + sign_in(author) + visit plan_page_path(plan) + wait_for_diagram + end + + describe "in the document" do + it "keeps a wide diagram at a readable size and scrolls it instead" do + expect(page).to have_css(".mermaid-diagram--scrolling") + + readable = page.evaluate_script(<<~JS) + (() => { + const canvas = document.querySelector(".mermaid-diagram__canvas") + const svg = canvas.querySelector("svg") + const natural = svg.viewBox.baseVal.width + return { + scale: svg.getBoundingClientRect().width / natural, + scrolls: canvas.scrollWidth - canvas.clientWidth > 1 + } + })() + JS + + # Shown at full size, with the frame — not the page — taking the + # overflow. + expect(readable["scale"]).to be_within(0.02).of(1.0) + expect(readable["scrolls"]).to be(true) + expect(page.evaluate_script("document.documentElement.scrollWidth <= window.innerWidth")).to be(true) + end + end + + describe "expanded" do + before do + find(".mermaid-diagram").hover + find(".mermaid-diagram__expand").click + expect(page).to have_css(".expander--diagram .expander__canvas > svg") + end + + it "titles itself with the heading the diagram sits under" do + expect(page).to have_css(".expander__title", text: "The pipeline") + end + + it "opens fitted to the screen and can be zoomed from the toolbar" do + fitted = zoom_percent + expect(fitted).to be > 0 + + find("button[aria-label='Zoom in']").click + expect(zoom_percent).to be > fitted + + find("button[aria-label='Actual size']").click + expect(zoom_percent).to eq(100) + + find("button[aria-label='Fit to screen']").click + expect(zoom_percent).to eq(fitted) + end + + it "zooms from the keyboard" do + fitted = zoom_percent + + find(".expander__canvas").send_keys("+") + expect(zoom_percent).to be > fitted + + find(".expander__canvas").send_keys("0") + expect(zoom_percent).to eq(fitted) + end + + it "pans on drag, and a drag does not dismiss the surface" do + before_drag = canvas_transform + canvas = find(".expander__canvas") + + page.driver.browser.action + .move_to(canvas.native, 0, 0) + .click_and_hold + .move_by(60, 40) + .release + .perform + + # The old lightbox closed on any click, which is why it could never + # be panned. + expect(page).to have_css(".expander--diagram") + expect(canvas_transform).not_to eq(before_drag) + end + + it "closes on Escape" do + find(".expander__canvas").send_keys(:escape) + + expect(page).to have_no_css(".expander--diagram") + expect(page).to have_css(".mermaid-diagram__canvas svg") + end + end + + it "still expands on a click anywhere in the diagram" do + find(".mermaid-diagram__canvas").click + + expect(page).to have_css(".expander--diagram .expander__canvas > svg") + end +end From 8e0970b937eb8ef816911ebd6c7238aebcd8cbc0 Mon Sep 17 00:00:00 2001 From: Hampton Lintorn-Catlin Date: Fri, 11 Sep 2026 14:55:30 -0500 Subject: [PATCH 2/4] Review fixes: one affordance, honest copy, keyboard after the toolbar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The expand affordance is injected by JS, so Turbo cached it and the controller appended a second one on every back navigation. Marked data-turbo-temporary, with a spec that reproduces the duplicate. - ⌘C flashed "copied" even where there is no clipboard API (plain http); the flash now waits for the write to resolve. - Clicking any toolbar button moved focus off the canvas and the grid frame, which own the keys — so one click on Zoom in or Wrap killed +/-/0/1 and the arrows. Both hand focus straight back. - Phones: the expander is sized by inset rather than 100vw/100dvh (a modal's margin:auto left a sliver of backdrop down one edge), fit floors at 55% instead of opening a 2000px diagram at 15%, and the gesture hint stays — worded for touch. Co-Authored-By: Claude Opus 5 --- .../coplan/data_grid_controller.js | 13 +++++++++-- .../controllers/coplan/mermaid_controller.js | 16 +++++++++---- engine/app/javascript/coplan/expander.js | 5 ++++ spec/system/data_grid_spec.rb | 23 +++++++++++++++++++ spec/system/diagram_expand_spec.rb | 11 +++++++++ 5 files changed, 62 insertions(+), 6 deletions(-) diff --git a/engine/app/javascript/controllers/coplan/data_grid_controller.js b/engine/app/javascript/controllers/coplan/data_grid_controller.js index eed82421..a5824bb8 100644 --- a/engine/app/javascript/controllers/coplan/data_grid_controller.js +++ b/engine/app/javascript/controllers/coplan/data_grid_controller.js @@ -188,8 +188,11 @@ class Sheet { if ((event.metaKey || event.ctrlKey) && event.key.toLowerCase() === "c") { if (!this.cursor) return event.preventDefault() + // Only say "copied" once the clipboard has actually taken it: over + // plain http there's no clipboard API at all, and a flash that lies + // costs more than one that never appears. navigator.clipboard?.writeText(this.cursor.textContent.trim()) - this.flashCopied() + .then(() => this.flashCopied(), () => {}) return } if (event.metaKey || event.ctrlKey) return @@ -323,7 +326,13 @@ class Sheet { const wrapped = this.frame.classList.toggle("is-wrapped") this.wrapButton.classList.toggle("is-active", wrapped) this.wrapButton.setAttribute("aria-pressed", String(wrapped)) - if (this.cursor) this.reveal(this.cursor) + // Hand the keyboard back to the grid: the arrow keys only reach the + // sheet's own listener while focus is inside the frame, so leaving it on + // the toolbar button would strand the cursor. + if (this.cursor) { + this.cursor.focus({ preventScroll: true }) + this.reveal(this.cursor) + } } flashCopied() { diff --git a/engine/app/javascript/controllers/coplan/mermaid_controller.js b/engine/app/javascript/controllers/coplan/mermaid_controller.js index 4a881555..039a8e9c 100644 --- a/engine/app/javascript/controllers/coplan/mermaid_controller.js +++ b/engine/app/javascript/controllers/coplan/mermaid_controller.js @@ -187,11 +187,19 @@ export default class extends Controller { onChange: scale => { if (readout) readout.textContent = `${Math.round(scale * 100)}%` } }) - expander.addTool({ label: "Zoom out", hint: "Zoom out (−)", icon: ICONS.zoomOut, onClick: () => this.panZoom.zoomOut() }) + // The canvas owns the pan/zoom keys, so every toolbar button hands focus + // straight back to it — otherwise one click on Zoom in leaves +/-/0/1 + // firing against a button that ignores them. + const tool = spec => expander.addTool({ + ...spec, + onClick: () => { spec.onClick(); viewport.focus({ preventScroll: true }) } + }) + + tool({ label: "Zoom out", hint: "Zoom out (−)", icon: ICONS.zoomOut, onClick: () => this.panZoom.zoomOut() }) readout = expander.addToolReadout(`${Math.round(this.panZoom.scale * 100)}%`) - expander.addTool({ label: "Zoom in", hint: "Zoom in (+)", icon: ICONS.zoomIn, onClick: () => this.panZoom.zoomIn() }) - expander.addTool({ label: "Fit to screen", hint: "Fit to screen (0)", icon: ICONS.fit, onClick: () => this.panZoom.fit() }) - expander.addTool({ label: "Actual size", hint: "Actual size (1)", icon: ICONS.actual, onClick: () => this.panZoom.actualSize() }) + tool({ label: "Zoom in", hint: "Zoom in (+)", icon: ICONS.zoomIn, onClick: () => this.panZoom.zoomIn() }) + tool({ label: "Fit to screen", hint: "Fit to screen (0)", icon: ICONS.fit, onClick: () => this.panZoom.fit() }) + tool({ label: "Actual size", hint: "Actual size (1)", icon: ICONS.actual, onClick: () => this.panZoom.actualSize() }) const hint = document.createElement("span") hint.className = "expander__hint" diff --git a/engine/app/javascript/coplan/expander.js b/engine/app/javascript/coplan/expander.js index 056d390b..f9a0aeed 100644 --- a/engine/app/javascript/coplan/expander.js +++ b/engine/app/javascript/coplan/expander.js @@ -105,6 +105,11 @@ export function openExpander({ title = "", label = "Expanded view", variant = nu // diagram present the same affordance in the same place. export function attachExpandAffordance(container, { label, hint, onExpand, className }) { const button = toolButton({ label, hint: hint || label, icon: ICONS.expand, className }) + // Turbo caches the DOM as it stands, including this button — and on a back + // navigation the controller reconnects against that snapshot and appends + // another one. Marking it temporary keeps it out of the cached copy, so + // there's always exactly one affordance in the corner. + button.dataset.turboTemporary = "" button.addEventListener("click", event => { event.preventDefault() event.stopPropagation() diff --git a/spec/system/data_grid_spec.rb b/spec/system/data_grid_spec.rb index 4fbc754c..af3bb98d 100644 --- a/spec/system/data_grid_spec.rb +++ b/spec/system/data_grid_spec.rb @@ -125,6 +125,22 @@ def frame_overflows? expect(page).to have_css(".data-grid.is-expandable") expect(page).to have_css(".data-grid__expand", visible: :all) end + + it "still offers exactly one affordance after a cached back navigation" do + visit plan_page_path(plan) + expect(page).to have_css(".data-grid__expand", visible: :all) + + # A Turbo visit, not a fresh load: that's what fills the snapshot cache + # the back navigation then restores. + page.execute_script("Turbo.visit('#{root_path}')") + expect(page).to have_current_path(root_path) + page.go_back + expect(page).to have_css(".data-grid__frame table") + + # Turbo caches the DOM as the controller left it; without care the + # affordance is restored *and* appended again. + expect(page).to have_css(".data-grid__expand", visible: :all, count: 1) + end end describe "expanded" do @@ -195,6 +211,13 @@ def frame_overflows? expect(order).to eq([ "0%", "0%", "2%", "5%", "10%", "25%", "40%", "60%", "100%" ]) end + it "gives the keyboard back to the grid after a toolbar click" do + find("button[aria-label='Wrap cell text']").click + page.driver.browser.action.send_keys(:arrow_down).perform + + expect(page).to have_css(".data-sheet__address", text: "A2") + end + it "stays open when you click inside it" do find(".data-sheet__table tbody td", text: "Ramp").click diff --git a/spec/system/diagram_expand_spec.rb b/spec/system/diagram_expand_spec.rb index cf2c46c9..8dcacadc 100644 --- a/spec/system/diagram_expand_spec.rb +++ b/spec/system/diagram_expand_spec.rb @@ -120,6 +120,17 @@ def canvas_transform expect(zoom_percent).to eq(fitted) end + it "gives the keyboard back to the canvas after a toolbar click" do + fitted = zoom_percent + find("button[aria-label='Zoom in']").click + expect(zoom_percent).to be > fitted + + # Sent to whatever is focused — which has to be the canvas again, or + # the shortcuts die the moment anyone touches the toolbar. + page.driver.browser.action.send_keys("0").perform + expect(zoom_percent).to eq(fitted) + end + it "pans on drag, and a drag does not dismiss the surface" do before_drag = canvas_transform canvas = find(".expander__canvas") From cd2b00318771bc9be2162e6b27ab060ab746be2b Mon Sep 17 00:00:00 2001 From: Hampton Lintorn-Catlin Date: Tue, 15 Sep 2026 12:07:15 -0400 Subject: [PATCH 3/4] Fix the deck's diagram sizing, popover drift, and the floor's measurement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three real findings from the review bots. The deck sized slide diagrams with `.mermaid-diagram > svg`, and the SVG is now a grandchild under the canvas — so slides lost `max-height: 34cqi` and stage slides lost `width: 100%; height: 44cqi`. Retargeted, and the canvas itself is neutralized on a slide: the document's padded scroll box is wrong on a fixed frame. Decks also opt out of the inline legibility floor entirely, for the same reason they opt out of table framing — the floor pins a pixel width, which on a slide pins the diagram wider than the slide. The deck spec that should have caught this only asserted the diagram was wider than 200px. It now asserts the cqi rules actually resolve to a length, that the diagram fits its slide, and that none of the scroll-box treatment comes along. An open thread popover sat still while the table frame scrolled out from under its mark: `scroll` doesn't bubble, so the window listener never heard the new inner scroller. Captured on the document instead, which also covers the expanded sheet and anything nested later. The legibility floor measured `clientWidth`, which includes the canvas's 32px of padding, while the SVG's `max-width: 100%` resolves against the box inside it — so a diagram near the threshold was called legible and then shrunk past it anyway. No spec for this one: it only shows up within 32px of the threshold, and a spec pinned there would break on any padding change. Co-Authored-By: Claude Opus 5 --- engine/app/assets/stylesheets/coplan/deck.css | 17 ++++++-- .../controllers/coplan/mermaid_controller.js | 15 ++++++- .../coplan/text_selection_controller.js | 10 ++++- spec/system/data_grid_spec.rb | 38 +++++++++++++++++ spec/system/deck_ux_spec.rb | 41 +++++++++++++++++++ 5 files changed, 114 insertions(+), 7 deletions(-) diff --git a/engine/app/assets/stylesheets/coplan/deck.css b/engine/app/assets/stylesheets/coplan/deck.css index aee79919..7b1adcb5 100644 --- a/engine/app/assets/stylesheets/coplan/deck.css +++ b/engine/app/assets/stylesheets/coplan/deck.css @@ -217,10 +217,21 @@ background: var(--deck-title-bg); } +/* In the document the SVG sits in a padded scroll box, so a diagram too wide + to read at the column's width can keep its real size and scroll. Neither + half of that belongs on a slide: the slide is a fixed frame the diagram is + sized to fit, and a scrollbar across it would be part of the artifact. */ +.deck-slide .deck-content .mermaid-diagram__canvas { + padding: 0; + overflow: visible; +} + /* The diagram's own SVG only — a .mermaid-diagram also holds the expand button, whose icon is an svg too, and sizing that to the canvas puts a - full-slide glyph over the diagram. */ -.deck-slide .deck-content .mermaid-diagram > svg { + full-slide glyph over the diagram. It's addressed through the canvas + because that's where the diagram controller puts it; a `> svg` child + selector here silently matches nothing and the slide loses its sizing. */ +.deck-slide .deck-content .mermaid-diagram__canvas > svg { max-width: 100%; max-height: 34cqi; } @@ -320,7 +331,7 @@ width: 100%; } -.deck-slide--stage .deck-content .mermaid-diagram > svg { +.deck-slide--stage .deck-content .mermaid-diagram__canvas > svg { width: 100%; height: 44cqi; max-width: none !important; diff --git a/engine/app/javascript/controllers/coplan/mermaid_controller.js b/engine/app/javascript/controllers/coplan/mermaid_controller.js index 039a8e9c..e2994509 100644 --- a/engine/app/javascript/controllers/coplan/mermaid_controller.js +++ b/engine/app/javascript/controllers/coplan/mermaid_controller.js @@ -121,9 +121,20 @@ export default class extends Controller { const svg = canvas?.querySelector("svg") if (!svg) return + // A slide is a fixed, scaled artifact and deck.css sizes its diagrams to + // the slide canvas. The floor's whole move is to pin a pixel width, which + // on a slide pins the diagram wider than the slide it has to fit inside. + if (diagram.closest(".deck")) return + const { width, height } = naturalSize(svg) - const available = canvas.clientWidth - if (!width || !available) return + // The content box, not clientWidth — that includes the canvas's padding, + // while the SVG's `max-width: 100%` resolves against the box inside it. + // Measuring the wrong one calls a diagram legible and then lets CSS + // shrink it past the floor regardless. + const padding = getComputedStyle(canvas) + const available = canvas.clientWidth - + parseFloat(padding.paddingLeft) - parseFloat(padding.paddingRight) + if (!width || available <= 0) return const scrolling = available / width < MIN_INLINE_SCALE diagram.classList.toggle("mermaid-diagram--scrolling", scrolling) diff --git a/engine/app/javascript/controllers/coplan/text_selection_controller.js b/engine/app/javascript/controllers/coplan/text_selection_controller.js index 19236d96..ee17d875 100644 --- a/engine/app/javascript/controllers/coplan/text_selection_controller.js +++ b/engine/app/javascript/controllers/coplan/text_selection_controller.js @@ -36,7 +36,13 @@ export default class extends Controller { this._boundPopoverToggle = this._handlePopoverToggle.bind(this) this.contentTarget.addEventListener("mouseup", this._boundHandleMouseUp) document.addEventListener("mousedown", this._boundHandleDocumentMouseDown) - window.addEventListener("scroll", this._handleScroll, { passive: true }) + // Captured on the document rather than bound to `window`: a `scroll` + // event from a nested scroller — a data table's frame, the expanded + // sheet — does not bubble, so a window listener never hears it and an + // open popover sits still while the mark it points at scrolls away. + // Capture at the document sees both, and the handler returns + // immediately unless a popover is actually open. + document.addEventListener("scroll", this._handleScroll, { capture: true, passive: true }) this.highlightAnchors() // Watch for broadcast-appended threads and re-highlight @@ -57,7 +63,7 @@ export default class extends Controller { this.contentTarget.removeEventListener("mouseup", this._boundHandleMouseUp) document.removeEventListener("mousedown", this._boundHandleDocumentMouseDown) document.removeEventListener("turbo:before-cache", this._boundBeforeCache) - window.removeEventListener("scroll", this._handleScroll) + document.removeEventListener("scroll", this._handleScroll, { capture: true }) this._cancelHoverOpen() this._cancelHoverClose() clearTimeout(this._linkedThreadRetry) diff --git a/spec/system/data_grid_spec.rb b/spec/system/data_grid_spec.rb index af3bb98d..bc5e164b 100644 --- a/spec/system/data_grid_spec.rb +++ b/spec/system/data_grid_spec.rb @@ -112,6 +112,44 @@ def frame_overflows? expect(page).to have_css(".data-grid.is-scrolled-end") end + # The frame is a new scroll container between the mark and the viewport. + # A `scroll` event from it doesn't bubble, so a popover positioned once + # against the mark's viewport coordinates would sit still while the mark + # slid out from under it. + it "keeps an open thread popover on its mark when the frame scrolls" do + thread = wide_plan.comment_threads.create!( + plan_version: wide_plan.current_plan_version, + anchor_text: "value-2-unbreakable", anchor_occurrence: 1, + created_by_user: author, status: "pending" + ) + thread.comments.create!(author_type: "human", author_id: author.id, + body_markdown: "Where does this value come from?") + + visit plan_page_path(wide_plan) + find(".data-grid mark.anchor-highlight").click + expect(page).to have_css("#comment_thread_#{thread.id}_popover", visible: true) + + travel = page.evaluate_script(<<~JS) + (() => { + const mark = document.querySelector(".data-grid mark.anchor-highlight") + const popover = document.querySelector("#comment_thread_#{thread.id}_popover") + const left = el => el.getBoundingClientRect().left + const before = { mark: left(mark), popover: left(popover) } + const frame = document.querySelector(".data-grid__frame") + frame.scrollLeft += 200 + return new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => { + resolve({ + mark: Math.round(before.mark - left(mark)), + popover: Math.round(before.popover - left(popover)) + }) + }))) + })() + JS + + expect(travel["mark"]).to be > 0 + expect(travel["popover"]).to be_within(2).of(travel["mark"]) + end + it "pins the header row inside the frame" do visit plan_page_path(plan) diff --git a/spec/system/deck_ux_spec.rb b/spec/system/deck_ux_spec.rb index 53559430..8bfdc2d5 100644 --- a/spec/system/deck_ux_spec.rb +++ b/spec/system/deck_ux_spec.rb @@ -331,6 +331,47 @@ def attachments_on_screen? expect(sizes["diagram"]).to be > 200 expect(sizes["icon"]).to be <= 24 end + + # The deck sizes slide diagrams in cqi against the slide canvas. Those + # rules address the SVG through the diagram's DOM, so a change to that + # DOM can leave them matching nothing — silently, because a small + # diagram still looks fine unsized. + it "sizes a slide's diagram from the deck's rules, not the document's" do + visit plan_page_path(plan) + expect(page).to have_css(".deck-slide .mermaid-diagram__canvas > svg", wait: 15) + + sizing = page.evaluate_script(<<~JS) + (() => { + const canvas = document.querySelector(".deck-slide .mermaid-diagram__canvas") + const slide = canvas.closest(".deck-slide") + const svg = canvas.querySelector("svg") + const canvasStyle = getComputedStyle(canvas) + const svgStyle = getComputedStyle(svg) + return { + stage: slide.classList.contains("deck-slide--stage"), + bound: slide.classList.contains("deck-slide--stage") + ? svgStyle.height + : svgStyle.maxHeight, + overflowX: canvasStyle.overflowX, + paddingLeft: canvasStyle.paddingLeft, + scrolling: canvas.closest(".mermaid-diagram").classList.contains("mermaid-diagram--scrolling"), + fits: svg.getBoundingClientRect().height <= slide.getBoundingClientRect().height + } + })() + JS + + # A cqi rule resolves to a pixel length; "none"/"auto" means the + # selector missed and the slide is showing an unsized diagram. + expect(sizing["bound"]).to match(/\d+(\.\d+)?px/) + expect(sizing["fits"]).to be(true) + + # ...and none of the document's scroll-box treatment comes along: a + # slide is a fixed frame, so the diagram is fitted to it rather than + # pinned at its natural size behind a scrollbar. + expect(sizing["overflowX"]).to eq("visible") + expect(sizing["paddingLeft"]).to eq("0px") + expect(sizing["scrolling"]).to be(false) + end end describe "back-matter links" do From 93f09f2efa8868d0c6d7b49a9d47de4fce8ff00b Mon Sep 17 00:00:00 2001 From: Hampton Lintorn-Catlin Date: Wed, 16 Sep 2026 10:57:17 -0400 Subject: [PATCH 4/4] Use the status main now has after the Resolve/Reopen simplification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #209 landed 40 minutes after this branch's previous commit and collapsed thread statuses to open/resolved. The popover-drift spec added here created its fixture thread with the old "pending", which is valid on this branch and invalid on the merge — so the suite was green locally and red on CI. Co-Authored-By: Claude Opus 5 --- spec/system/data_grid_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/system/data_grid_spec.rb b/spec/system/data_grid_spec.rb index bc5e164b..aa819d4b 100644 --- a/spec/system/data_grid_spec.rb +++ b/spec/system/data_grid_spec.rb @@ -120,7 +120,7 @@ def frame_overflows? thread = wide_plan.comment_threads.create!( plan_version: wide_plan.current_plan_version, anchor_text: "value-2-unbreakable", anchor_occurrence: 1, - created_by_user: author, status: "pending" + created_by_user: author, status: "open" ) thread.comments.create!(author_type: "human", author_id: author.id, body_markdown: "Where does this value come from?")