shared terminal: align row icons via a shared grid, not offsets
Per mara's screenshot report (agent-terminal icons not aligned in the
first column) — and her explicit follow-up steer on the first version of
this fix: 'dont do it by offsets at all, it should be part of the layout
that they align.'
Root cause traced first, not guessed: .row-glyph relied on inheriting
text-indent: -1.4em from .live .row to paint its glyph in the reserved
prefix slot; details.row > summary separately reset text-indent: 0 for
its own hanging-caret needs, which also zeroed the inherited value for
the icon nested inside it (indent inherits by computed value, not by
rule). Two independently-computed offsets that had to agree by hand,
and silently didn't.
Replaced the whole offset scheme with a real structural fix: every row
is a 2-column CSS grid (icon column, width from one shared
--row-icon-col custom property, then content column), and a details
row grids its own <summary> with the exact same grid-template-columns
value instead of griding itself (so its <pre> body still stacks full
width below, not squeezed into column 2). Icon and content are placed
by explicit grid-column, not auto-placement inference, so an icon-less
row's lone child still lands in the content column. .row-content is a
new wrapper class (Row.tsx, terminal.js's row()/mutableRow()/
placeholder()) giving that content an element the grid can target by
class - a DocumentFragment (what linkify() returns) doesn't persist as
a node once appended, so without an explicit wrapper there was nothing
for the grid to place.
Caught and fixed a second real bug while building this for real (not
just reasoning about the CSS): the first draft's details.row { display:
block } had lower specificity than .live .row's display: grid and never
actually applied, squeezing <summary> into the row's own 1.4em icon
column and wrapping its text one character per line. Needed
.live details.row to out-specify it.
Verified with a headless-chromium render of all 6 row shapes (flat
icon / flat icon-less / flat markdown-body / details icon / details
icon-less / a long-wrapping flat row) - all align and wrap correctly.
Also verified via the earlier Range.getBoundingClientRect() measurement
(glyph paint position, not just the element's own box): diff 0.00 for
both this and the previous fix, but only this one is structural rather
than two numbers that happen to still agree today.
This commit is contained in:
iris
mara
parent
915c6c6f92
commit
adfb0f9e02
4 changed files with 123 additions and 81 deletions
|
|
@ -66,7 +66,11 @@ export function Row({ row }: { row: TermRow }) {
|
|||
return (
|
||||
<div className={`row ${cssClass}`}>
|
||||
{icon}
|
||||
{linkifyToNodes(row.summary)}
|
||||
{/* Explicit wrapper, not bare children — the row's grid places
|
||||
`.row-content` in its content column by class, not by
|
||||
auto-placement inference (an icon-less row's lone child would
|
||||
otherwise auto-place into the icon column instead). */}
|
||||
<span className="row-content">{linkifyToNodes(row.summary)}</span>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -42,6 +42,14 @@
|
|||
anchoring would double-compensate scrollTop during loadMore()
|
||||
prepends, causing an erratic jump. */
|
||||
overflow-anchor: none;
|
||||
/* Single source of truth for the icon column's width, shared by every
|
||||
row kind's own grid below (`.live .row` and `details.row > summary`).
|
||||
Structural alignment: both contexts read the *same* custom property
|
||||
rather than each hand-computing an offset that has to independently
|
||||
agree with the other (mara: "dont do it by offsets at all, it should
|
||||
be part of the layout that they align" — see the incident this
|
||||
replaced, two paragraphs down). */
|
||||
--row-icon-col: 1.4em;
|
||||
}
|
||||
.live.terminal {
|
||||
background: transparent;
|
||||
|
|
@ -74,37 +82,68 @@
|
|||
transform: translateY(0);
|
||||
}
|
||||
}
|
||||
/* Unified prefix column for every row kind. The glyph (`→ ← · ◆ ✓ ✗ ⌁ !`)
|
||||
is the first character of the row's text content; `padding-left` reserves
|
||||
the column and `text-indent: -1.4em` pulls the glyph back into it. Wrapped
|
||||
continuation lines then start under the body, not under the glyph, so
|
||||
wraps don't blur into the next row. `details.row` summaries reuse the
|
||||
same metrics below. */
|
||||
/* Unified prefix column for every row kind, via CSS grid rather than
|
||||
hand-computed offsets (see the incident this replaced, below). A flat
|
||||
row is its own 2-column grid (`--row-icon-col` | content); a `details`
|
||||
row instead grids its `<summary>` (see further down) so its `<pre>`
|
||||
body can stack under the *whole* row, full width, not just the content
|
||||
column. `.row-glyph` and the row's actual content are both placed by
|
||||
explicit `grid-column` (below), never by auto-placement inference —
|
||||
an icon-less row's content still lands in column 2, not wherever the
|
||||
grid would otherwise auto-place a lone child.
|
||||
|
||||
Previously: `padding-left` + a negative `text-indent` pulling the first
|
||||
inline box (or bare first character) back into the reserved slot.
|
||||
Replaced after a real bug, found the same session: `.row-glyph` needed
|
||||
its own copy of that indent to actually paint there (being
|
||||
`inline-block`, it's a block container for its own text, so it doesn't
|
||||
inherit the *effect* just by inheriting the *value*) — and
|
||||
`details.row > summary` separately reset `text-indent: 0` for its own
|
||||
hanging-caret needs, which silently zeroed the *inherited* indent for
|
||||
the icon nested inside it too, since indent inherits by computed
|
||||
value, not by rule. A details-row icon ended up painted 1.4em right of
|
||||
a flat row's despite both icons' own boxes already sharing a left edge
|
||||
— two independent numbers (`padding-left`/`text-indent` here,
|
||||
`margin-left`/`padding-left` on `details.row > summary`) that had to
|
||||
agree by hand, and silently didn't. mara: "dont do it by offsets at
|
||||
all, it should be part of the layout that they align" — a shared grid
|
||||
column, read from one custom property, replaces both. */
|
||||
.live .row {
|
||||
display: grid;
|
||||
grid-template-columns: var(--row-icon-col) 1fr;
|
||||
column-gap: 0.5em;
|
||||
white-space: pre-wrap;
|
||||
word-break: break-word;
|
||||
padding: 0.05em 0;
|
||||
line-height: 1.45;
|
||||
border-left: 2px solid transparent;
|
||||
padding-left: 1.9em;
|
||||
text-indent: -1.4em;
|
||||
margin: 0.1em 0;
|
||||
}
|
||||
.live .row + .row {
|
||||
border-top: 0;
|
||||
}
|
||||
/* Fixed-width icon column. Rows built with an `icon` (see terminal.js
|
||||
`row()` / `details()`) put it in a `.row-glyph` element instead of as a
|
||||
bare first character. `inline-block` with a fixed `width` means the icon
|
||||
occupies one constant-width cell regardless of the glyph's rendered width
|
||||
(emoji differ; some carry a variation selector), so every icon's left edge
|
||||
lines up — a flat-row `🧠` and a `details` summary's `🖥️` share the column.
|
||||
It's the first inline box, so the row's `text-indent: -1.4em` pulls it into
|
||||
the reserved prefix slot exactly like a bare glyph; the following text then
|
||||
starts at the `padding-left` (1.9em) where wrapped lines also hang. */
|
||||
/* Icon column. Every row kind's icon lands in `grid-column: 1` of
|
||||
whichever grid is active in that context (this rule for a flat row,
|
||||
`details.row > summary`'s own grid further down for a collapsible
|
||||
one) — same column definition (`--row-icon-col`), so every icon's
|
||||
left edge lines up regardless of the glyph's own rendered width
|
||||
(emoji differ; some carry a variation selector). */
|
||||
.live .row-glyph {
|
||||
display: inline-block;
|
||||
width: 1.4em;
|
||||
grid-column: 1;
|
||||
}
|
||||
/* Content column — explicit, not left to grid auto-placement. Rows
|
||||
built with an icon (see terminal.js `row()` / Row.tsx) wrap their
|
||||
actual content in `.row-content` (a markdown body just uses its own
|
||||
`.md` class, styled the same way below) specifically so an icon-less
|
||||
row's lone remaining child still lands in column 2: without an
|
||||
explicit `grid-column`, auto-placement would put a row's *only* grid
|
||||
item in column 1 (the first available cell), landing text in the
|
||||
narrow icon column instead of the content one. */
|
||||
.live .row-content,
|
||||
.live .row > .md,
|
||||
.live .row > details {
|
||||
grid-column: 2;
|
||||
min-width: 0;
|
||||
}
|
||||
/* Row colours, keyed by severity `level` (hive-agent's `term_msg.rs`),
|
||||
not by row kind any more — mara's terminal-message redesign dropped
|
||||
|
|
@ -147,13 +186,6 @@
|
|||
text-shadow: 0 0 14px color-mix(in srgb, var(--amber) 95%, transparent);
|
||||
}
|
||||
}
|
||||
/* Any child block (markdown body, nested details) resets the parent
|
||||
row's hanging indent so the content lays out from column 0 of the
|
||||
body area. */
|
||||
.live .row .md,
|
||||
.live .row > details {
|
||||
text-indent: 0;
|
||||
}
|
||||
/* "↓ N new" pill: shown when new rows arrive while the operator is
|
||||
scrolled up; click to jump to bottom. Positioned by the wrapper's
|
||||
`position: relative` (terminal-wrap supplies it; pages that skip the
|
||||
|
|
@ -217,58 +249,49 @@
|
|||
cursor: default;
|
||||
opacity: 0.6;
|
||||
}
|
||||
/* Expandable rows reuse the flat-row prefix metrics (padding-left +
|
||||
negative text-indent). The summary's icon (when present) sits in the
|
||||
shared `.row-glyph` column — same cell as a flat row's icon — so a
|
||||
`details` summary's `🖥️` lines up under a flat row's `🧠`. The
|
||||
disclosure caret (`▸ / ▾`) leads the `.summary-text` (not the icon) via
|
||||
`::before`, so it sits where the summary text starts rather than shoving
|
||||
the icon out of the prefix column. Icon-less summaries have no `.row-glyph`,
|
||||
so the caret falls back into the prefix column like the old directional
|
||||
glyph. The summary text carries no `→ / ←`; the row colour (cyan =
|
||||
outbound tool, muted = inbound result) carries the direction. */
|
||||
details.row {
|
||||
/* Expandable rows don't grid themselves the way a flat row does — a
|
||||
`<details>` needs its `<summary>` and `<pre>` body to stack normally
|
||||
(full row width each), not sit side by side in the row's own 2
|
||||
columns. `.live details.row` (not just `details.row`) overrides
|
||||
`.live .row`'s `display: grid` back to block for the `<details>`
|
||||
element itself — needs the extra `.live` to out-specify it (two
|
||||
classes beats one class + one type selector), or the override never
|
||||
applies and `<summary>`/`<pre>` get squeezed into the *row's* 2-column
|
||||
grid instead of stacking (caught rendering this for real: a
|
||||
`<summary>` auto-placed into the 1.4em icon column wraps its text one
|
||||
character per line). The icon+text split happens one level down, on
|
||||
`<summary>` alone, which grids itself with the exact same
|
||||
`grid-template-columns: var(--row-icon-col) 1fr` as `.live .row` —
|
||||
same column, same custom property, so a details summary's icon and a
|
||||
flat row's icon share a left edge *because they're the same declared
|
||||
column*, not because two independently-computed offsets happen to
|
||||
agree. The disclosure caret (`▸ / ▾`) leads `.summary-text` (not the
|
||||
icon) via `::before`. The summary text carries no `→ / ←`; the row
|
||||
colour (cyan = outbound tool, muted = inbound result) carries the
|
||||
direction. */
|
||||
.live details.row {
|
||||
display: block;
|
||||
white-space: normal;
|
||||
}
|
||||
/* Two-column layout for expandable summary rows.
|
||||
Left col: fixed-width icon cell. Right col: disclosure chevron +
|
||||
text, wrapping within itself so no continuation line bleeds under
|
||||
the icon. Overrides the flat-row hanging-indent metrics (.live .row
|
||||
sets padding-left: 1.9em; text-indent: -1.4em) with explicit flex
|
||||
geometry. The negative margin-left on summary cancels the details
|
||||
container's inherited padding-left so the icon lands at the same
|
||||
horizontal position as flat-row icons. */
|
||||
details.row > summary {
|
||||
display: grid;
|
||||
grid-template-columns: var(--row-icon-col) 1fr;
|
||||
column-gap: 0.5em;
|
||||
align-items: baseline;
|
||||
cursor: pointer;
|
||||
list-style: none;
|
||||
white-space: pre-wrap;
|
||||
word-break: break-word;
|
||||
display: flex;
|
||||
align-items: baseline;
|
||||
margin-left: -1.9em;
|
||||
padding-left: 0.5em;
|
||||
text-indent: 0;
|
||||
}
|
||||
/* Icon column: 2em accommodates wide emoji without ink bleeding into
|
||||
the chevron column. Overrides the inline-block + width: 1.4em set
|
||||
on .live .row-glyph for the shared flat-row context. */
|
||||
details.row > summary > .row-glyph {
|
||||
flex: 0 0 2em;
|
||||
width: auto;
|
||||
text-align: center;
|
||||
}
|
||||
/* Content column: chevron (::before) + text, wraps within the cell. */
|
||||
/* `.row-glyph`'s own `grid-column: 1` (set once, above) already applies
|
||||
here too — nothing details-specific to repeat. `.summary-text` needs
|
||||
its own explicit `grid-column: 2` for the same reason `.row-content`
|
||||
does on a flat row: an icon-less summary's only child must not fall
|
||||
back to column 1 via auto-placement. */
|
||||
details.row > summary > .summary-text {
|
||||
flex: 1;
|
||||
grid-column: 2;
|
||||
min-width: 0;
|
||||
}
|
||||
/* Icon-less summaries: the CSS-generated ▸/▾ acts as the hanging
|
||||
marker. Hanging indent keeps wrapped lines under the text body,
|
||||
not under the chevron. Mirrors the flat-row text-indent metric. */
|
||||
details.row > summary > .summary-text:only-child {
|
||||
padding-left: 1.4em;
|
||||
text-indent: -1.4em;
|
||||
}
|
||||
details.row > summary > .summary-text::before {
|
||||
content: "▸ ";
|
||||
color: inherit;
|
||||
|
|
@ -280,7 +303,6 @@ details.row > pre.diff-body,
|
|||
details.row > pre.tool-body {
|
||||
margin: 0.3em 0 0.4em 0;
|
||||
padding: 0.4em 0.6em;
|
||||
text-indent: 0;
|
||||
background: rgba(255, 255, 255, 0.02);
|
||||
border-left: 2px solid var(--purple-dim);
|
||||
white-space: pre-wrap;
|
||||
|
|
@ -301,8 +323,7 @@ details.row > pre.diff-body .diff-ctx {
|
|||
color: var(--fg);
|
||||
}
|
||||
/* Markdown body inside a row (assistant text, send/recv message
|
||||
bodies). Inline elements get muted accents; block elements
|
||||
reset the parent row's hanging indent so content lays out cleanly. */
|
||||
bodies). Inline elements get muted accents. */
|
||||
.live .row .md p {
|
||||
margin: 0.2em 0;
|
||||
}
|
||||
|
|
@ -323,7 +344,6 @@ details.row > pre.diff-body .diff-ctx {
|
|||
padding: 0.4em 0.6em;
|
||||
background: rgba(255, 255, 255, 0.04);
|
||||
border-left: 2px solid var(--purple-dim);
|
||||
text-indent: 0;
|
||||
white-space: pre-wrap;
|
||||
word-break: break-word;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -185,7 +185,7 @@ export function create(opts) {
|
|||
clearPlaceholder();
|
||||
const e = document.createElement("div");
|
||||
e.className = "row note";
|
||||
e.textContent = text;
|
||||
e.appendChild(contentSpan(document.createTextNode(text)));
|
||||
log.appendChild(e);
|
||||
placeholderEl = e;
|
||||
}
|
||||
|
|
@ -200,6 +200,19 @@ export function create(opts) {
|
|||
g.textContent = icon;
|
||||
return g;
|
||||
}
|
||||
// Wraps a flat row's actual content (text/links, or the mutable-row
|
||||
// text node) in a real element the row's own grid can place by class
|
||||
// (`grid-column: 2` in terminal.css) — `linkify()` returns a
|
||||
// `DocumentFragment`, which doesn't persist as a node once appended,
|
||||
// so without this wrapper an icon-less row's content has no element of
|
||||
// its own for the grid to target and would auto-place into the icon
|
||||
// column instead (see terminal.css's own comment on why that matters).
|
||||
function contentSpan(node) {
|
||||
const c = document.createElement("span");
|
||||
c.className = "row-content";
|
||||
c.appendChild(node);
|
||||
return c;
|
||||
}
|
||||
// Build a <summary> whose icon (if any) sits in the shared `.row-glyph`
|
||||
// column and whose text lives in a `.summary-text` span — the disclosure
|
||||
// caret (CSS `.summary-text::before`) then leads the text, not the icon,
|
||||
|
|
@ -219,7 +232,7 @@ export function create(opts) {
|
|||
const e = document.createElement("div");
|
||||
e.className = "row " + (cls || "") + (currentNoAnim ? " no-anim" : "");
|
||||
if (icon != null && icon !== "") e.appendChild(glyphSpan(icon));
|
||||
e.appendChild(linkify(text));
|
||||
e.appendChild(contentSpan(linkify(text)));
|
||||
log.appendChild(e);
|
||||
afterAppend(wasNearBottom);
|
||||
return e;
|
||||
|
|
@ -235,7 +248,7 @@ export function create(opts) {
|
|||
e.className = "row " + (cls || "") + (currentNoAnim ? " no-anim" : "");
|
||||
if (icon != null && icon !== "") e.appendChild(glyphSpan(icon));
|
||||
const tn = document.createTextNode(text == null ? "" : String(text));
|
||||
e.appendChild(tn);
|
||||
e.appendChild(contentSpan(tn));
|
||||
log.appendChild(e);
|
||||
afterAppend(wasNearBottom);
|
||||
return [e, tn];
|
||||
|
|
|
|||
Loading…
Reference in a new issue