diff --git a/CHANGELOG.md b/CHANGELOG.md index 0881d439..ae7999ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,26 @@ auto-generated per-PR notes; this file is the curated, human-readable history. adapter refactor — no user-visible behavior changes. ### Fixed +- **The Dashboard tree no longer reveals two rows' pencil/trash actions at + once, and its `· N` count now sits inline after the label** (#568). The + hover/focus reveal rule (`.dash-tree-row:focus-within .dash-tree-act`) + matched on any focus, so a row a user had merely clicked to select kept its + actions visible indefinitely, alongside whichever other row the pointer was + hovering — swapped to `:focus-visible`/`:has(:focus-visible)` (the row's own + composite tab stop and its controls, respectively — `:has()`'s implicit + descendant selector alone misses the row itself), which only a real + keyboard Tab/Arrow grants, preserving reveal-on-keyboard-focus without + pinning a clicked row open. A directly-focused action button (`.dash-tree- + act:focus`, no `:focus-visible`) reveals only itself regardless of + modality, so a programmatic focus-return or a test's direct `.focus()` — + the pre-existing contract `tests/e2e/tile-open-workbench.spec.js`'s + "Panels-row plus" test already relied on — still works. Separately, + `Sales revenue · 2` read as a + right-aligned + column rather than immediately after the name (`Library · 66`'s placement, + as `dashboard-tree-model.ts` already documented as the intent): the label + and count are now wrapped in one flex group so only that group grows, + matching the narrow-sidebar test's own claim. - **A detached Data pane's variable dropdown no longer shows a frozen snapshot of recent values** (#478 follow-up). #555's caller-neutral `VariableBarApp` adapter copied `varRecent` into `state` as a plain data diff --git a/src/styles.css b/src/styles.css index 7b507d06..4685b913 100644 --- a/src/styles.css +++ b/src/styles.css @@ -1720,7 +1720,13 @@ body.detached-tab .graph-overlay-panel { .dash-tree-row { position: relative; } /* The group rows (Variables / Panels) are structure, not content. */ .dash-tree-group { color: var(--fg-faint); font-weight: var(--fw-medium); } -.dash-tree-count { margin-left: 2px; font-size: var(--text-label); } +/* `flex-shrink: 0` + `white-space: nowrap`: inside `.dash-tree-label-group` + below, an unbounded `.label` (its flex-basis is its full, untruncated text + width) would otherwise absorb almost none of the shrink budget itself, + leaving this tiny "· N" to take the rest — squeezing it down to a couple of + pixels and wrapping "· 2" onto two lines. The count stays fixed-width; the + label does all the shrinking, same as it always ellipsizes. */ +.dash-tree-count { margin-left: 2px; font-size: var(--text-label); flex-shrink: 0; white-space: nowrap; } /* CURRENT RESOURCE vs KEYBOARD FOCUS must read differently (#426): the current Dashboard/member is a persistent tonal surface with an accent edge, while keyboard focus is the transient ring. A row can legitimately be both. */ @@ -1810,15 +1816,43 @@ body.detached-tab .graph-overlay-panel { opacity: 0; pointer-events: none; } /* Revealed on hover like the Library row actions, but ALWAYS in the keyboard - focus order: `:focus-within` makes the visually concealed cluster visible - while the row or one of its controls owns focus, so no operation is - keyboard-unreachable. + focus order: `:focus-visible`/`:has(:focus-visible)` make the visually + concealed cluster visible while the row itself OR one of its controls holds + KEYBOARD focus, so no operation is keyboard-unreachable. Both are needed — + `:has()`'s argument is an implicit DESCENDANT selector, so `:has(:focus- + visible)` alone matches only when a control inside the row (the chevron or + an action button) is focused, not when the row's own composite tab stop + is (the roving-tabindex owner a real ArrowDown/ArrowUp or the first Tab + into the tree lands on) — that gap left the cluster invisible at exactly + the moment #472 wants a sighted keyboard user to see it waiting. + Deliberately not `:focus-within`, which matches on any focus regardless of + modality — a row a user merely CLICKED to select keeps DOM focus, so + `:focus-within` kept its actions revealed forever, alongside whichever row + the pointer was hovering, showing two pencil/trash pairs at once. Ordinary + pointer focus on the ROW no longer pins it open this way — though a + different row's genuine hover and this row's genuine keyboard focus can + still be visible at once, which is correct: those are two independent, + live states, not the stale-selection bug. Chromium (and other engines) + withhold `:focus-visible` after a pointer interaction but grant it for real + keyboard navigation — Tab/Arrow keys — even when the resulting focus is set + programmatically from a keydown handler (see the Tab-walk and + keyboard-delete tests in `tests/e2e/dashboard-tree.spec.js`). + `.dash-tree-act:focus` (plain, not `:focus-visible`) is separate and + narrower: the button ITSELF holding any focus — Tab, a programmatic + focus-return once its own dialog closes, or a test's direct `.focus()` — + reveals only that one control, never a sibling or the whole row, so it + cannot resurrect the row-level bug above. This preserves the pre-existing + contract `tests/e2e/tile-open-workbench.spec.js`'s "Panels-row plus" test + and its sibling `.dash-tile-actions` ("Focus alone reveals it — a keyboard + user never hovers") already rely on. `[aria-expanded="true"]` holds it visible for its own dialog/confirmation's whole lifetime — both are body-mounted, so by the time one closes the pointer and focus have left the row, and an invisible trigger must not become unreachable before focus can return to it. */ .dash-tree-row:hover .dash-tree-act, -.dash-tree-row:focus-within .dash-tree-act, +.dash-tree-row:focus-visible .dash-tree-act, +.dash-tree-row:has(:focus-visible) .dash-tree-act, +.dash-tree-act:focus, .dash-tree-act[aria-expanded="true"] { opacity: 1; pointer-events: auto; } /* #447's orphan-variable trash stays ALWAYS visible: it is the only way to remove stored SQL nothing references any more, and a hover-only affordance @@ -1914,6 +1948,16 @@ body.detached-tab .graph-overlay-panel { } .tree-row .meta.loading { font-style: italic; opacity: .6; } +/* Dashboard tree count reads immediately after the label — matching Library's + `Library · 66` placement — instead of inheriting the schema tree's + flex:1 label above, which pushes trailing content to the row's far right + edge. Only the GROUP grows; the label inside it shrinks/ellipsizes and the + count stays a fixed-width neighbor. Placed after `.tree-row .label` above + so this wins the `flex` override at equal selector specificity, without + touching the schema (Databases) tree's own `.tree-row .label` behavior. */ +.dash-tree-label-group { display: flex; align-items: center; min-width: 0; flex: 1; } +.dash-tree-label-group .label { flex: 0 1 auto; min-width: 0; } + /* Column name/type independent drag targets (#186). Only these two child spans are drag sources on a column row — the row-level `:active grabbing` above no longer applies to columns (overridden just below, scoped to `.mono` diff --git a/src/ui/dashboard-tree.ts b/src/ui/dashboard-tree.ts index d7b2c04c..451498e2 100644 --- a/src/ui/dashboard-tree.ts +++ b/src/ui/dashboard-tree.ts @@ -576,7 +576,7 @@ function dropProps(app: DashboardTreeApp, row: DashboardTreeRow): Record { - // The row has eight child spans and drag events bubble, so a bare + // The row has several child spans and drag events bubble, so a bare // `dragleave` fires every time the pointer crosses onto one of them. const to = event.relatedTarget; const rowEl = event.currentTarget as HTMLElement; @@ -674,6 +674,11 @@ function buildRow( const count = row.count === null ? null : h('span', { class: 'side-count dash-tree-count' }, '· ' + row.count); + // The label and its count grow as ONE group, so the count reads right after + // the name (`Sales revenue · 2`) like the Library tab's `Library · 66` — + // rather than as a sibling of a flex:1 label, which pushes it into the + // right-aligned cluster with `meta`/actions. + const labelGroup = h('span', { class: 'dash-tree-label-group' }, label, count); // #447: the WORD, not only a colour — an unused (orphaned) variable says so in // text, right after its name. const status = row.invalid === 'variable-unused' @@ -737,7 +742,7 @@ function buildRow( syncRovingTabindex(rowEl.parentElement, row.key); pressRow(app, row, event.shiftKey); }, - }, chevron, h('span', { class: 'icon' }, rowIcon(row)), label, count, status, + }, chevron, h('span', { class: 'icon' }, rowIcon(row)), labelGroup, status, h('span', { class: 'meta' }, row.meta), marker, // #494: the trailing DIRECT controls, in the model's own order — edit // before delete, destructive rightmost. No tree ROW carries a `⋯` any more. diff --git a/tests/e2e/dashboard-tree.spec.js b/tests/e2e/dashboard-tree.spec.js index 55ee127b..a1f3d489 100644 --- a/tests/e2e/dashboard-tree.spec.js +++ b/tests/e2e/dashboard-tree.spec.js @@ -921,6 +921,64 @@ test.describe('direct row actions (#494)', () => { await expect(row.getByRole('button', { name: 'Delete dashboard Ops latency' })).toBeFocused(); }); + // The bug this guards: `:focus-within` reveals a row's actions on ANY + // focus, including the focus a plain CLICK leaves behind on the row it + // selected — so that row's pencil/trash stayed revealed indefinitely, + // alongside whichever OTHER row the pointer moved on to hover next. Only + // real keyboard focus (`:focus-visible`) should pin a row's actions open + // once the pointer has left it. + test('clicking a row to select it does not leave its actions revealed once the pointer hovers another row', async ({ page }) => { + await open(page); + await roleTab(page, 'Dashboards').click(); + const clicked = treeRow(page, 'workspace:ops'); + const hovered = treeRow(page, 'workspace:long'); + await clicked.click(); + await hovered.hover(); + await expect(clicked.locator('.dash-tree-act').first()).toHaveCSS('opacity', '0'); + await expect(hovered.locator('.dash-tree-act').first()).toHaveCSS('opacity', '1'); + }); + + // `:has()`'s argument is an implicit DESCENDANT selector, so a reveal rule + // written as only `.dash-tree-row:has(:focus-visible)` would miss the row's + // OWN composite tab stop — the roving-tabindex owner a real ArrowDown/ + // ArrowUp or the first Tab into the tree lands ON, before a chevron or + // action button has been reached. That left the cluster invisible at + // exactly the moment a sighted keyboard user first arrives at a row. + test('arrowing onto a row reveals its own actions, not just a descendant control\'s', async ({ page }) => { + await open(page); + await roleTab(page, 'Dashboards').click(); + const clicked = treeRow(page, 'workspace:sales'); + const next = treeRow(page, 'workspace:ops'); + // Establish POINTER modality first — a real click, the way a user actually + // lands keyboard focus on a row in practice — so the ArrowDown below is + // the thing that flips modality to keyboard, not a leftover from `open()`'s + // own setup. This isolates the historical cross-engine failure mode: a + // pointer→keyboard transition that lands on a DIFFERENT row entirely. + await clicked.click(); + expect(await clicked.evaluate((el) => el.matches(':focus-visible'))).toBe(false); + await page.keyboard.press('ArrowDown'); + await expect(next).toBeFocused(); + expect(await next.evaluate((el) => el.matches(':focus-visible'))).toBe(true); + await expect(next.locator('.dash-tree-act').first()).toHaveCSS('opacity', '1'); + }); + + // The DESCENDANT half of the reveal rule — `.dash-tree-row:has(:focus- + // visible)` — is exercised separately from the row's own composite tab stop + // above: Tab onto the chevron (a control, not the row, and not an action + // button, so `.dash-tree-act:focus` cannot be why this passes) and confirm + // the cluster stays visible with the pointer elsewhere. + test('tabbing onto the chevron reveals the row\'s actions via the descendant :has() branch', async ({ page }) => { + await open(page); + await roleTab(page, 'Dashboards').click(); + const row = treeRow(page, 'workspace:sales'); + await row.focus(); + await page.keyboard.press('Tab'); + const chevron = row.locator('.dash-tree-chev'); + await expect(chevron).toBeFocused(); + expect(await chevron.evaluate((el) => el.matches(':focus-visible'))).toBe(true); + await expect(row.locator('.dash-tree-act').first()).toHaveCSS('opacity', '1'); + }); + test('the dialog announces itself as a modal named by its heading', async ({ page }) => { await open(page); await roleTab(page, 'Dashboards').click(); @@ -1125,6 +1183,23 @@ test.describe('Dashboard tree counts at the narrow sidebar (#553)', () => { for (const key of ['workspace:sales', 'workspace:sales:group:variables', 'workspace:sales:group:panels']) { await expect(treeRow(page, key).locator('.dash-tree-count')).toBeVisible(); } + // "Inline after the label" is a geometric claim, not just a text/visibility + // one: the count must sit right next to the label's own box, not pushed + // into the row's right-aligned trailing cluster (meta/marker/actions). The + // intended gap is the count's own 2px margin; the OLD (buggy) layout's gap + // was exactly `.tree-row`'s 6px flex `gap` PLUS that 2px margin = 8px, so + // the upper bound sits well clear of 8 rather than grazing it (a boundary + // right on the old value risks a sub-pixel rounding false pass), and a + // lower bound catches the count overlapping the label instead of merely + // failing to be far away from it. + for (const key of ['workspace:sales', 'workspace:sales:group:variables', 'workspace:sales:group:panels']) { + const row = treeRow(page, key); + const labelBox = await row.locator('.label').boundingBox(); + const countBox = await row.locator('.dash-tree-count').boundingBox(); + const gap = countBox.x - (labelBox.x + labelBox.width); + expect(gap).toBeGreaterThanOrEqual(-0.5); + expect(gap).toBeLessThan(5); + } }); test('dragging to <=220px hides every dot/count, but the count stays in the accessible name and actions stay reachable', async ({ page }) => {