diff --git a/docs/architecture/10-interaction-and-feedback.md b/docs/architecture/10-interaction-and-feedback.md index 75be7df..e77e49d 100644 --- a/docs/architecture/10-interaction-and-feedback.md +++ b/docs/architecture/10-interaction-and-feedback.md @@ -368,6 +368,19 @@ bullet is the contract; cite it, not the source.)_ other shows the text visually with no live role. Two live regions would announce the same message twice. +**Resolved — inline _live_ validation feedback is polite, glyphed, and field-linked.** A +validator that re-checks on **every keystroke** (the Chart Builder expression inputs — a +calculated field, a filter in expression mode) is **not** an assertive `alert`: that would +interrupt on each character (APG _Alert_ "avoid frequent interruptions"; WCAG 2.2.4). It is a +**polite `role="status"`** line, and severity reads from a **status glyph** (round error / +triangle warning, arch 09 §5.2) **plus** colour — never colour alone (§3; WCAG 1.4.1), exactly +like the guidance warnings above it. The owning `` carries `aria-invalid` for the state +and `aria-describedby` pointing at the message node so the text is available on focus, not only +when it changes (GOV.UK _error-message_ field association). Contrast the editor's **render** +error, which is a discrete, post-debounce result and stays the single `alert` of the rule +above. _(Consulted via /council → WAI-ARIA APG Alert, GOV.UK error-message, NN/g #9. This +bullet is the contract; cite it, not the source.)_ + **Resolved — feature-modal dismissal & initial focus.** A feature modal (Datasets, Chart Builder) is a **passive** `dialog-modal`: dismissed by the close button, Escape, or a backdrop click (a passive modal carries no in-flight transaction, so diff --git a/docs/chart-builder-enhancement-scope.md b/docs/chart-builder-enhancement-scope.md index 11b13d8..4cae93f 100644 --- a/docs/chart-builder-enhancement-scope.md +++ b/docs/chart-builder-enhancement-scope.md @@ -25,7 +25,34 @@ Newest first. The at-a-glance build-order tracker is §4; per-item detail is §3. This log is the quick "where are we" — read it first. -- **2026-06-11** — **1C + 1D + 1E shipped (the Data section). Up next: Phase 2 (2A value-or-field channels, then 2B field shelf).** +- **2026-06-11 (later)** — **Owed debts on the Data section closed. Up next: Phase 2 (2A value-or-field channels, then 2B field shelf).** + - **Council pass** on the new error/disclosure copy (the previously-deferred auto-fire + surface). Three a11y conformance gaps against `architecture/10` were fixed: the inline + expression feedback now carries a **status glyph** (round error / triangle warning), not + colour alone (§3, WCAG 1.4.1); the parse error is a **polite `role="status"`**, not a + per-keystroke assertive alert (APG Alert / WCAG 2.2.4); and the message is linked to its + input via **`aria-describedby`** (GOV.UK error-message). Unknown-field copy clarified to + "…— not a column in this dataset." Resolution recorded in `architecture/10` §5. + - **Manual/visual pass** run via a headless-Chrome (Playwright) walk-through of the live + builder — filter shelf (predicate + `is between`), field↔expression toggle, expression + error/unknown-field glyphs (filter _and_ calc inputs), calculated field, data-preview + table with type chips, smart-default chart. All surfaces render as intended. + - **Bug found + fixed (mid-edit preview resilience).** `filterTransformObject` / + `calculateTransformObject` emitted **any non-empty** expression — including a half-typed, + unparseable one — so the preview blanked with a raw render error while the user typed. + They now **drop a syntactically-invalid expression** like an empty/incomplete entry + (guarded by core `validateExpression`), matching `buildTransforms`' own "a config + mid-edit still renders" contract; the inline feedback still flags the typo. Pure core, + tested (+2 cases). Visually confirmed: an invalid filter/calc now keeps the last-good + chart instead of breaking the preview. + - **Verified:** `typecheck` + `test` (741 passing) + `eslint` clean. + - **Still open (copy judgment, user's call):** the preview's catch-all + `"Couldn't render this chart: {raw Vega message}"` puts a diagnostic in the headline + (arch 10 says diagnostics go in a disclosure) — correct in the editor, debatable in the + builder; and the builder's terse `"Dataset «X» not found."` drops the next-step the + contract mandates (near-unreachable in the builder). Both noted, not changed. + +- **2026-06-11** — **1C + 1D + 1E shipped (the Data section).** - A new **Data section** at the top of the builder's left pane — "here are your rows; shape them, then encode them" — emits the spec's top-level `transform` array. - **1C · Filters** — a guarded **field + operator + value** predicate shelf. Operators diff --git a/docs/manual-verification.md b/docs/manual-verification.md index 660fb7d..dd80bf0 100644 --- a/docs/manual-verification.md +++ b/docs/manual-verification.md @@ -65,3 +65,15 @@ - [ ] Library search / sort / empty states, the storage monitor, About & Donate modals, and the busy indicator all look deliberate and behave per spec. + +## Driving these checks headlessly + +The eye-checks above can be scripted as a headless walk-through (Playwright with the +already-installed Chromium browser cache) for repeatable screenshots, alongside a live +browser. + +- **Select by accessible name / role / placeholder, never by CSS class.** CSS-module class + names are content-hashed and unstable; the component tests select the same way. +- **Reaching the Chart Builder:** open Datasets from the **header command button** (the + pane-toggle-strip `Datasets` button only exists once the workspace has content) → **New + Dataset** → paste rows inline → **Build Chart**. diff --git a/docs/spec/06-chart-builder.md b/docs/spec/06-chart-builder.md index dd07c06..027c09e 100644 --- a/docs/spec/06-chart-builder.md +++ b/docs/spec/06-chart-builder.md @@ -35,7 +35,7 @@ The section is ordered **input → shaping** so the distinction reads at a glanc - The **field** dropdown offers the dataset's columns plus any calculated fields. - The **operators** offered depend on the field's type: a measure or temporal field offers `is` / `is not` / `<` / `≤` / `>` / `≥` / `is between` (two bounds); a category offers `is` / `is not` / `is one of` (a comma-separated membership list). A quantitative value compares as a number; other types compare as text (ISO dates sort correctly as text). - A filter can be switched to an **expression** power-mode — a raw Vega predicate expression (e.g. `datum.value > 0`) — for what the guarded shelf can't say. The toggle is reversible. -- An **incomplete** filter (no value yet, a blank range bound, an empty expression) is ignored, so the live preview keeps rendering while the user types. +- An **incomplete** filter (no value yet, a blank range bound, an empty or syntactically-invalid expression) is ignored, so the live preview keeps rendering while the user types. ### Calculated fields @@ -47,6 +47,7 @@ The section is ordered **input → shaping** so the distinction reads at a glanc - Both expression inputs (a filter's expression mode, a calculated field) are validated with **Vega's own expression parser** — the same one the chart uses — so a syntax error is reported **inline** the moment it appears, matching exactly what the chart would accept. - A `datum.` reference that does not match a known column raises a soft **"unknown field"** warning (a typo guard) without blocking — the value is genuinely valid Vega, it just won't resolve. +- An expression that does not parse is **left out of the produced spec** (like an incomplete filter or an unnamed calculated field): the inline error is the only feedback, and a half-typed expression never reaches the renderer — the preview keeps showing the last valid chart. ## Inputs and Controls diff --git a/src/app/components/ChartBuilderModal.module.css b/src/app/components/ChartBuilderModal.module.css index 9552301..ffbb2e6 100644 --- a/src/app/components/ChartBuilderModal.module.css +++ b/src/app/components/ChartBuilderModal.module.css @@ -234,11 +234,21 @@ .exprError, .exprWarn { + display: flex; + align-items: flex-start; + gap: var(--space-2); margin: 0; font-size: 11px; line-height: 1.4; } +/* Severity reads from the glyph shape + colour, not colour alone (arch 10 §3); the + icon inherits the line's colour via currentColor (round error / triangle warning). */ +.exprFeedbackIcon { + flex: none; + margin-top: 1px; +} + .exprError { color: var(--support-error); } diff --git a/src/app/components/ChartBuilderModal.test.tsx b/src/app/components/ChartBuilderModal.test.tsx index 1cb7853..6f218ec 100644 --- a/src/app/components/ChartBuilderModal.test.tsx +++ b/src/app/components/ChartBuilderModal.test.tsx @@ -256,8 +256,15 @@ describe('ChartBuilderModal', () => { await Promise.resolve(); }); - const alert = container.querySelector('[role="alert"]'); - expect(alert?.textContent).toMatch(/nexpected|Invalid/); + // Polite, not assertive: live per-keystroke validation uses role="status" with a + // status glyph, never an assertive alert (council: APG Alert / WCAG 2.2.4). + const messages = Array.from(container.querySelectorAll('[role="status"]')); + const errorMsg = messages.find((n) => /nexpected|Invalid/.test(n.textContent ?? '')); + expect(errorMsg).toBeTruthy(); + // The expression input is linked to its message and flagged invalid. + const exprInput = container.querySelector('input[aria-label="Filter expression"]'); + expect(exprInput?.getAttribute('aria-invalid')).toBe('true'); + expect(exprInput?.getAttribute('aria-describedby')).toBe(errorMsg?.id); }); test('the Vega expression reference shows only when an expression is in play (1E)', async () => { diff --git a/src/app/components/ChartBuilderModal.tsx b/src/app/components/ChartBuilderModal.tsx index 562dc64..b97774f 100644 --- a/src/app/components/ChartBuilderModal.tsx +++ b/src/app/components/ChartBuilderModal.tsx @@ -346,11 +346,26 @@ function ChannelBlock({ channel }: { channel: ChannelName }) { /** * Inline feedback for an expression input (filter expression / calculated field): - * a parse error (assertive) takes priority, else a soft warning for `datum.` - * references that don't match a known column — a typo guard before the chart renders - * empty (1E). Nothing renders for a valid, fully-resolved expression. + * a parse error takes priority, else a soft warning for `datum.` references + * that don't match a known column — a typo guard before the chart renders empty (1E). + * Nothing renders for a valid, fully-resolved expression. `messageId` lets the owning + * input point at this node via `aria-describedby`. + * + * Both severities are a **polite** live region carrying a **status glyph** (round + * error / triangle warning), not an assertive alert and never colour alone: the + * expression validates on every keystroke, so an assertive role would interrupt on + * each character (APG Alert / WCAG 2.2.4), and severity must read without colour + * (arch 10 §3; the input also carries `aria-invalid`). */ -function ExprFeedback({ expr, columns }: { expr: string; columns: BuilderColumns }) { +function ExprFeedback({ + expr, + columns, + messageId, +}: { + expr: string; + columns: BuilderColumns; + messageId?: string; +}) { const feedback = useMemo(() => { const validation = validateExpression(expr); if (!validation.valid) { @@ -359,19 +374,23 @@ function ExprFeedback({ expr, columns }: { expr: string; columns: BuilderColumns const unknown = referencedFields(expr).filter((f) => !columns.columns.includes(f)); if (unknown.length > 0) { const plural = unknown.length > 1 ? 's' : ''; - return { kind: 'warn' as const, text: `Unknown field${plural}: ${unknown.join(', ')}` }; + return { + kind: 'warn' as const, + text: `Unknown field${plural}: ${unknown.join(', ')} — not a column in this dataset.`, + }; } return null; }, [expr, columns]); if (!feedback) return null; - return feedback.kind === 'error' ? ( -

- {feedback.text} -

- ) : ( -

- {feedback.text} + const isError = feedback.kind === 'error'; + return ( +

+ + {feedback.text}

); } @@ -412,6 +431,9 @@ function FilterRow({ filter, columns }: { filter: BuilderFilter; columns: Builde const op = filter.op ?? 'equal'; const arity = filterOpArity(op); const hasColumns = columns.columns.length > 0; + // Links the expression input to its feedback line; harmless when no message renders + // (aria-describedby to an absent id is ignored — GOV.UK error-message association). + const exprMsgId = `filter-${filter.id}-expr-msg`; return (
@@ -423,6 +445,7 @@ function FilterRow({ filter, columns }: { filter: BuilderFilter; columns: Builde placeholder={exprPlaceholder(columns, 'filter')} value={filter.expr ?? ''} aria-invalid={!validateExpression(filter.expr ?? '').valid || undefined} + aria-describedby={exprMsgId} onChange={(e) => updateFilter(filter.id, { expr: e.target.value })} /> ) : ( @@ -496,7 +519,9 @@ function FilterRow({ filter, columns }: { filter: BuilderFilter; columns: Builde
)} - {expressionMode && } + {expressionMode && ( + + )} {hasColumns && ( - + ); } @@ -782,9 +809,15 @@ function BuilderPreview() { setTooLarge({ heightPx: e.heightPx, limitPx: e.limitPx }); setError(null); } else if (e instanceof DatasetNotFoundError) { + // TODO: this drops the next-step the error contract wants (arch 10); LivePreview + // gives "Create it from Datasets…". Near-unreachable here (the builder opens from + // an existing dataset), so it's terse — restore the next-step if it can be reached. setError(`Dataset "${e.datasetName}" not found.`); setTooLarge(null); } else { + // TODO: arch 10 routes a raw diagnostic into a disclosure, not the headline. The + // editor surfaces the Vega message inline by design; the builder could fold it + // behind a details disclosure and keep the headline plain. setError(`Couldn't render this chart: ${(e as Error).message}`); setTooLarge(null); } diff --git a/src/core/chart-builder.test.ts b/src/core/chart-builder.test.ts index c860a5a..484f5ad 100644 --- a/src/core/chart-builder.test.ts +++ b/src/core/chart-builder.test.ts @@ -974,6 +974,29 @@ describe('buildTransforms (predicate coercion + shape)', () => { }; expect(buildTransforms(config)).toEqual([]); }); + + it('drops a syntactically-invalid expression filter (mid-edit preview resilience)', () => { + const t = buildTransforms( + withFilters( + filter({ mode: 'expression', expr: 'datum.value *' }), // half-typed → unparseable + filter({ id: 'f2', mode: 'expression', expr: 'datum.value > 0' }), // valid stays + ), + ); + expect(t).toEqual([{ filter: 'datum.value > 0' }]); + }); + + it('drops a calculated field whose expression does not parse', () => { + const config: BuilderConfig = { + datasetName: 'D', + mark: 'bar', + encodings: {}, + calculates: [ + calc({ as: 'bad', expr: 'datum.a +' }), + calc({ id: 'c2', as: 'ok', expr: 'datum.a' }), + ], + }; + expect(buildTransforms(config)).toEqual([{ calculate: 'datum.a', as: 'ok' }]); + }); }); describe('buildChartSpec — transform integration', () => { diff --git a/src/core/chart-builder.ts b/src/core/chart-builder.ts index a3779ca..09a9e06 100644 --- a/src/core/chart-builder.ts +++ b/src/core/chart-builder.ts @@ -26,6 +26,7 @@ import type { ColumnStats } from './profile'; import { DISTINCT_CAP } from './profile'; import type { ColumnType } from './type-inference'; import { VEGA_LITE_SCHEMA_URL } from './snippet'; +import { validateExpression } from './expr-validate'; /** The five mark types the builder offers, in selector order (spec §06). */ export const MARK_TYPES = ['bar', 'line', 'point', 'area', 'circle'] as const; @@ -835,25 +836,31 @@ function predicateObject(filter: BuilderFilter): Record | null function filterTransformObject(filter: BuilderFilter): Record | null { if (filter.mode === 'expression') { const expr = (filter.expr ?? '').trim(); - return expr === '' ? null : { filter: expr }; + // A syntactically-invalid expression is dropped like an empty one: a half-typed + // `datum.x *` must not reach the renderer and blank the preview mid-edit — the + // inline feedback already flags it. (Same mid-edit resilience as a partial predicate.) + return expr === '' || !validateExpression(expr).valid ? null : { filter: expr }; } const predicate = predicateObject(filter); return predicate ? { filter: predicate } : null; } -/** One calculate's `{ calculate, as }` transform entry, or `null` when incomplete. */ +/** One calculate's `{ calculate, as }` transform entry, or `null` when incomplete or unparseable. */ function calculateTransformObject(calc: BuilderCalculate): Record | null { const expr = calc.expr.trim(); const as = calc.as.trim(); - return expr === '' || as === '' ? null : { calculate: expr, as }; + return expr === '' || as === '' || !validateExpression(expr).valid + ? null + : { calculate: expr, as }; } /** * The complete top-level `transform` array for a configuration: every calculated * field first (so filters and encodings can reference the derived columns), then - * every filter, each in the user's list order. Incomplete entries (a half-typed - * filter, an unnamed calculate) are dropped so a configuration mid-edit still - * produces a renderable spec. An empty result means no `transform` key is emitted. + * every filter, each in the user's list order. Incomplete or unparseable entries (a + * half-typed predicate, an unnamed calculate, an expression that doesn't parse) are + * dropped so a configuration mid-edit still produces a renderable spec. An empty + * result means no `transform` key is emitted. */ export function buildTransforms(config: BuilderConfig): Array> { const out: Array> = [];