diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ac304f2..dd2c14ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -206,6 +206,17 @@ auto-generated per-PR notes; this file is the curated, human-readable history. renders a multi-select. The marker survives for every other container, and for an `Array(scalar T)` nobody has configured.) +- **The no-inferred-control diagnostic no longer renders a duplicate second row** + (#470). The marker above used to be a sibling element (icon + a repeat of the + declared type, e.g. `Array(Int32)`) appended beside the input; since it and the + input's own wrapper were both pinned to the same CSS grid column, the layout + pushed it into a visible second row that read as a duplicate control rather + than a diagnostic. It now adorns the SAME input in place — a yellow/dashed + class (mirroring the existing invalid/conflict affordances), a decorative + icon absolutely positioned over the input, and the full message reachable on + hover (`title`) and keyboard focus (`aria-describedby`) — with no change to + the input's rendered width or the one-row filter layout. + ### Changed - **A Dashboard tile opens its own query in the Workbench; the Dashboard-level `< Query` button is gone** (#471). Every query-backed tile carries an diff --git a/src/styles.css b/src/styles.css index 01b3be7d..0091392d 100644 --- a/src/styles.css +++ b/src/styles.css @@ -2033,17 +2033,24 @@ body.detached-tab .graph-overlay-panel { } .var-combo-clear-inline:hover { background: var(--bg-hover); color: var(--fg); } .var-combo-clear-inline:disabled { cursor: not-allowed; opacity: .5; } -/* #447 phase 2: a variable whose declared type is a container (Array/Tuple/Map/ - Nested) gets no INFERRED control, so this marker sits beside its plain input - rather than replacing it — a literal value typed there still binds. Warning, not - error: nothing is wrong, the Dashboard just cannot infer a control. */ -.var-unsupported { - grid-column: 2; - margin-top: 2px; - height: 24px; display: inline-flex; align-items: center; gap: 6px; - padding: 0 8px; border-radius: var(--r-sm); - border: 1px dashed var(--warn-bd); background: var(--warn-bg); color: var(--warn-fg); - font-size: var(--text-label); white-space: nowrap; +/* #447 phase 2 (relaid out #470): a variable whose declared type is a container + (Array/Tuple/Map/Nested) gets no INFERRED control, so this state adorns its + plain input rather than replacing it — a literal value typed there still + binds. Warning, not error: nothing is wrong, the Dashboard just cannot infer + a control. It used to be a separate element beside the input; both were + pinned to this same grid column, so auto-placement pushed it into a second + row that read as a duplicate control. Now the warning styling lands on the + input itself (like .is-conflict above) and only a decorative icon is added, + absolutely positioned over the input (like .var-combo-clear-inline) so the + `.var-combo` column never gains a second row. */ +.var-input.is-unsupported { + border-style: dashed; border-color: var(--warn-bd); background: var(--warn-bg); + padding-right: 26px; +} +.var-unsupported-icon { + position: absolute; top: 0; right: 6px; height: 24px; + display: inline-flex; align-items: center; color: var(--warn-fg); + pointer-events: none; } .var-combo-list { position: fixed; z-index: 70; max-height: 260px; overflow-y: auto; diff --git a/src/ui/variable-bar.ts b/src/ui/variable-bar.ts index 6285554c..9e7ac82d 100644 --- a/src/ui/variable-bar.ts +++ b/src/ui/variable-bar.ts @@ -35,7 +35,7 @@ import { } from './relative-time-field.js'; import { buildRecentField as _buildRecentField } from './recent-field.js'; import { buildEnumField } from './enum-field.js'; -import { wireComboInput } from './combobox.js'; +import { idSafe, wireComboInput } from './combobox.js'; import type { ComboField } from './combobox.js'; import { buildTimeRangeField } from './time-range-field.js'; import { buildVariableOptionField } from './variable-option-field.js'; @@ -363,7 +363,13 @@ export function buildVariableBar( const specOf = (name: string): VariableFieldSpec | undefined => (variables ? variables[name] : undefined); - /** The marker shown beside a variable the Dashboard cannot infer a control for. + /** The "cannot infer a control" diagnostic's two texts — a short summary (for + * the accessible description) and a longer, actionable detail (folded into + * the field's `title` by its caller, `buildParamField`, since `title` is + * reset from `baseTitle` on every keystroke/commit via `applyFieldState`; + * computing the detail as part of `baseTitle` itself, rather than assigning + * `input.title` once here, is what makes it SURVIVE interaction instead of + * reverting to the plain type tooltip after the first character typed). * * Two reasons reach it, and they say different things: * - a CONTAINER with no flat element list (`Tuple`/`Map`/`Nested`, or a @@ -372,28 +378,49 @@ export function buildVariableBar( * - an `Array(scalar T)` with no option SQL has no LIST to pick from yet. * Its type is perfectly controllable — configuring option SQL turns it * into the multiselect — so saying "container type" would be misleading - * advice. It names the fix instead. + * advice. It names the fix instead. */ + const unsupportedNote = (p: FieldControl, type: string, listable: boolean): { summary: string; detail: string } => ({ + summary: listable + ? `${p.name} has no option list: add option SQL to pick from a list, or type a literal value` + : `${p.name} has no inferred control: ${type} is a container type — type a literal value`, + detail: listable + ? `A Dashboard has no option list for ${type}. Add option SQL to this variable to pick ` + + 'values from a list, or type a literal value directly.' + : `A Dashboard cannot infer a control for ${type}, which is a container type. ` + + 'Type a literal value directly.', + }); + + /** Applies the diagnostic to the variable's EXISTING input in place (#470) — + * never a second element. It used to be a sibling `` (icon + a repeat + * of the declared type) appended into `.var-field`'s two-column grid; since + * that span and the real control's wrapper were both pinned to the same grid + * column, auto-placement pushed it into its own row, which read as a + * duplicate control rather than a diagnostic. Now it mirrors `var-field.ts`'s + * `applyFieldState` (`is-invalid`/`is-conflict`): a class on the SAME + * `.var-input` carries the yellow/dashed styling, and an `aria-describedby` + * sibling — a visually-hidden `.sr-only` span, never a visible row — carries + * the summary to keyboard focus/screen readers. (The longer detail text + * reaches pointer hover through `title`/`baseTitle`, set by the caller — see + * `unsupportedNote`'s header comment for why it can't be assigned here.) The + * decorative icon is appended into `comboEl` (`.var-combo`, + * `position: relative`) and positioned absolutely, the same technique + * `variable-option-field.ts`'s inline clear (×) already uses to sit over the + * input without adding to the combo's flex-column flow. * - * Either way it ADORNS the plain field rather than replacing it. Removing the - * input outright would make an existing Dashboard strictly less capable — a + * This ADORNS the plain field rather than replacing it. Removing the input + * outright would make an existing Dashboard strictly less capable — a * container-typed variable already rendered a free-text field, and * `param-serialize.ts` binds an array literal typed into it perfectly well, so - * taking it away would leave those panels permanently `unfilled` with no way to - * fill them. The marker says the Dashboard cannot infer a control; it does not - * claim the value is unusable. */ - const unsupportedMarker = (p: FieldControl, type: string, listable = false): HTMLElement => - h('span', { - class: 'var-unsupported', - role: 'img', - 'aria-label': listable - ? `${p.name} has no option list: add option SQL to pick from a list, or type a literal value` - : `${p.name} has no inferred control: ${type} is a container type — type a literal value`, - title: listable - ? `A Dashboard has no option list for ${type}. Add option SQL to this variable to pick ` - + 'values from a list, or type a literal value directly.' - : `A Dashboard cannot infer a control for ${type}, which is a container type. ` - + 'Type a literal value directly.', - }, Icon.eyeOff(), type); + * taking it away would leave those panels permanently `unfilled` with no way + * to fill them. The marker says the Dashboard cannot infer a control; it does + * not claim the value is unusable. */ + const markUnsupported = (input: HTMLInputElement, comboEl: HTMLElement, p: FieldControl, summary: string): void => { + input.classList.add('is-unsupported'); + const descId = 'var-unsupported-desc-' + idSafe(p.name); + input.setAttribute('aria-describedby', descId); + comboEl.appendChild(h('span', { class: 'sr-only', id: descId }, summary)); + comboEl.appendChild(h('span', { class: 'var-unsupported-icon', 'aria-hidden': 'true' }, Icon.eyeOff())); + }; /** The searchable multiselect over one `Array(scalar T)` variable's batched * option rows (#189, restored). Its committed value is a real `string[]`, so @@ -469,7 +496,9 @@ export function buildVariableBar( h('span', { class: 'var-name' }, p.name), field.el); }; - const buildParamField = (p: FieldControl): HTMLElement => { + const buildParamField = ( + p: FieldControl, unsupported?: { type: string; listable: boolean }, + ): HTMLElement => { let timer: ReturnType | null = null; timerClears.push(() => { if (timer != null) clearTimeout(timer); timer = null; }); // #173 acceptance (review F1): a type-conflicted param (declared with @@ -479,9 +508,16 @@ export function buildVariableBar( // a tooltip listing them. const conflictNote = p.conflict ? 'Conflicting type declarations: ' + p.conflict.join(' vs ') : null; + const unsupported_ = unsupported ? unsupportedNote(p, unsupported.type, unsupported.listable) : null; + // #470: the unsupported detail is folded into `baseTitle` itself — not + // assigned to `input.title` once after build — so it survives every later + // `applyFieldState` call (onValueInput/onCommitHard/onPick all reset + // `title` from this SAME closed-over `baseTitle`), rather than reverting to + // the plain type tooltip the moment the user types or commits. const baseTitle = p.name + ': ' + p.type + (p.optional ? ' — optional: blank leaves its filter block out' : '') - + (conflictNote ? ' — ' + conflictNote : ''); + + (conflictNote ? ' — ' + conflictNote : '') + + (unsupported_ ? ' — ' + unsupported_.detail : ''); const commitNow = (): void => { if (timer == null) return; clearTimeout(timer); @@ -550,6 +586,7 @@ export function buildVariableBar( wireComboInput(combo, { onValueInput, onCommit: onCommitHard }); if (conflictNote) input.classList.add('is-conflict'); applyFieldState(input, getField(p.name, 'execute'), baseTitle, combo?.previewEl); + if (unsupported_) markUnsupported(input, combo.el, p, unsupported_.summary); return h('label', { class: 'var-field' + (p.optional ? ' is-optional' : '') }, h('span', { class: 'var-name' }, p.name), combo.el); }; @@ -588,19 +625,13 @@ export function buildVariableBar( // so a type that gets a select can never be one whose option SQL was skipped. const kind = fieldControlKind(p, null, { scalarControls: !!variables }).kind; // A container with no flat element list: no control is inferable at all. - if (kind === 'unsupported') { - const field = buildParamField(p); - field.appendChild(unsupportedMarker(p, p.type)); - return field; - } + if (kind === 'unsupported') return buildParamField(p, { type: p.type, listable: false }); const spec = specOf(p.name); if (kind === 'multi') { // The type CAN be multi-selected; whether there is anything to select from // is the spec's answer, which only this layer can see. if (spec && spec.options !== null) return buildMultiField(p, spec); - const field = buildParamField(p); - field.appendChild(unsupportedMarker(p, p.type, true)); - return field; + return buildParamField(p, { type: p.type, listable: true }); } if (spec && spec.options !== null) return buildOptionField(p, spec); return buildParamField(p); diff --git a/tests/unit/dashboard.test.ts b/tests/unit/dashboard.test.ts index 82eca739..a22c042e 100644 --- a/tests/unit/dashboard.test.ts +++ b/tests/unit/dashboard.test.ts @@ -3530,7 +3530,7 @@ describe('renderDashboard — option-source runtime rebuild + diagnostics (#359) expect(app.root!.querySelectorAll('.dash-config-diagnostic')).toHaveLength(0); }); - it('renders an unsupported-type note instead of a control for a container variable', async () => { + it('renders an unsupported-type diagnostic on the control itself, not a second row, for a container variable', async () => { const { app } = dashApp({ responder: () => ({ columns: [{ name: 'n', type: 'UInt8' }], rows: [[1]] }), workspace: wsWith({ @@ -3539,8 +3539,13 @@ describe('renderDashboard — option-source runtime rebuild + diagnostics (#359) }), }); await render(app); - const note = qs(app.root, '.var-unsupported'); - expect(note.textContent).toContain('Array(String)'); + // #470: no second element repeating the type text — the SAME input carries + // the warning styling, and the icon (the only sibling the diagnostic adds) + // is textless. + const input = qs(app.root, '.var-input.is-unsupported'); + expect(input.placeholder).toBe('Array(String)'); + const icon = qs(app.root, '.var-unsupported-icon'); + expect(icon.textContent).toBe(''); }); it('renders an Array(String) variable WITH option SQL as the multi-select, and binds its selection', async () => { @@ -3566,7 +3571,7 @@ describe('renderDashboard — option-source runtime rebuild + diagnostics (#359) // Unset, so the panel waits — and the control is the multiselect, not a text // box with the no-inferred-control marker. expect(panelRuns()).toHaveLength(0); - expect(app.root!.querySelector('.var-unsupported')).toBeNull(); + expect(app.root!.querySelector('.var-unsupported-icon')).toBeNull(); const trigger = qs(app.root, '.ms-trigger'); expect(trigger.textContent).toBe('Not set'); diff --git a/tests/unit/variable-bar.test.ts b/tests/unit/variable-bar.test.ts index 46fb48f9..78ba1511 100644 --- a/tests/unit/variable-bar.test.ts +++ b/tests/unit/variable-bar.test.ts @@ -578,7 +578,24 @@ describe('buildVariableBar — Dashboard variable controls (#447 phase 2)', () = expect(bar.openPopoverKey()).toBeNull(); }); - it('marks a container type as having no inferred control, but KEEPS its input', () => { + // #470: the diagnostic used to be a second element (icon + a repeat of the + // declared type) appended beside the input; both it and the input's wrapper + // were pinned to the same CSS grid column, so it landed in a second row that + // read as a duplicate control. It must now adorn the SAME `.var-input` in + // place — one row, one occurrence of the type text (the input's own + // `placeholder`, unchanged) — with the full diagnostic reachable on hover + // (`title`) and keyboard focus (`aria-describedby`). + const unsupportedDesc = (field: HTMLElement, input: HTMLInputElement): Element => { + const id = input.getAttribute('aria-describedby'); + expect(id).not.toBeNull(); + // `field` isn't attached to `document` in these tests, so `getElementById` + // wouldn't find it — a scoped `querySelector` does. + const desc = field.querySelector('#' + id); + expect(desc).not.toBeNull(); + return desc!; + }; + + it('marks a container type as having no inferred control, but KEEPS its input, in ONE row', () => { // Removing the input would make an existing Dashboard strictly less capable: a // container-typed variable already had a free-text field, and param-serialize // binds an array literal typed into it — taking it away leaves those panels @@ -587,11 +604,30 @@ describe('buildVariableBar — Dashboard variable controls (#447 phase 2)', () = variables: { tags: { options: null } }, }); const field = fieldFor(bar, 'tags'); - expect(field.querySelector('.var-input')).not.toBeNull(); - const note = field.querySelector('.var-unsupported')!; - expect(note.textContent).toContain('Map(String, String)'); - expect(note.getAttribute('aria-label')).toContain('no inferred control'); - expect(note.getAttribute('title')).toContain('container type'); + const input = field.querySelector('.var-input')!; + expect(input).not.toBeNull(); + expect(input.classList.contains('is-unsupported')).toBe(true); + expect(input.placeholder).toBe('Map(String, String)'); + expect(input.title).toContain('container type'); + // The icon is the ONLY other element the diagnostic adds, and it carries + // no text of its own — so the type label is never rendered twice. + const icon = field.querySelector('.var-unsupported-icon')!; + expect(icon).not.toBeNull(); + expect(icon.textContent).toBe(''); + expect(icon.getAttribute('aria-hidden')).toBe('true'); + expect(unsupportedDesc(field, input).textContent).toContain('no inferred control'); + // Exactly the name label + the combo wrapper — never a third top-level + // child that would push the diagnostic into its own grid row. + expect(field.children.length).toBe(2); + // #470 regression: `applyFieldState` resets `title` from `baseTitle` on + // every keystroke/commit (`onValueInput`/`onCommitHard`) — the diagnostic + // must survive that, not revert to the plain name/type tooltip the moment + // the user types. + input.value = '{}'; + input.dispatchEvent(new Event('input', { bubbles: true })); + expect(input.title).toContain('container type'); + input.dispatchEvent(new Event('blur', { bubbles: true })); + expect(input.title).toContain('container type'); }); it('the unsupported verdict wins over a configured option list', () => { @@ -602,48 +638,58 @@ describe('buildVariableBar — Dashboard variable controls (#447 phase 2)', () = const { bar } = build(`SELECT * FROM t WHERE x IN {tags:${type}}`, { variables: { tags: { options: OPTIONS } }, }); - expect(fieldFor(bar, 'tags').querySelector('.variable-select')).toBeNull(); - expect(fieldFor(bar, 'tags').querySelector('.ms-trigger')).toBeNull(); - expect(fieldFor(bar, 'tags').querySelector('.var-unsupported')).not.toBeNull(); - expect(fieldFor(bar, 'tags').querySelector('.var-input')).not.toBeNull(); + const field = fieldFor(bar, 'tags'); + expect(field.querySelector('.variable-select')).toBeNull(); + expect(field.querySelector('.ms-trigger')).toBeNull(); + expect(field.querySelector('.var-unsupported-icon')).not.toBeNull(); + expect(field.querySelector('.var-input.is-unsupported')).not.toBeNull(); } }); it('reports unsupported for a container even with no entry in the variables map', () => { // The verdict comes from the declared TYPE, not from the map. const { bar } = build('SELECT * FROM t WHERE x IN {tags:Map(String, String)}', { variables: {} }); - expect(fieldFor(bar, 'tags').querySelector('.var-unsupported')).not.toBeNull(); + expect(fieldFor(bar, 'tags').querySelector('.var-unsupported-icon')).not.toBeNull(); }); it('never reports unsupported without the variables map — the workbench keeps its text field', () => { for (const type of ['Array(String)', 'Map(String, String)']) { const { bar } = build(`SELECT * FROM t WHERE x IN {tags:${type}}`); - expect(fieldFor(bar, 'tags').querySelector('.var-unsupported')).toBeNull(); - expect(fieldFor(bar, 'tags').querySelector('.ms-trigger')).toBeNull(); - expect(fieldFor(bar, 'tags').querySelector('.var-input')).not.toBeNull(); + const field = fieldFor(bar, 'tags'); + expect(field.querySelector('.var-unsupported-icon')).toBeNull(); + expect(field.querySelector('.ms-trigger')).toBeNull(); + const input = field.querySelector('.var-input')!; + expect(input).not.toBeNull(); + expect(input.classList.contains('is-unsupported')).toBe(false); } }); - it('marks an Array(scalar) with NO option SQL as having no option list, and keeps its input', () => { + it('marks an Array(scalar) with NO option SQL as having no option list, and keeps its input, for every scalar element type', () => { // Its type IS controllable — configuring option SQL turns it into the // multi-select — so the marker names that fix instead of calling the type // uncontrollable. The free-text input stays either way: a hand-typed - // `['a','b']` still binds. - const { bar } = build('SELECT * FROM t WHERE x IN {tags:Array(String)}', { - variables: { tags: { options: null } }, - }); - const field = fieldFor(bar, 'tags'); - expect(field.querySelector('.var-input')).not.toBeNull(); - expect(field.querySelector('.ms-trigger')).toBeNull(); - const note = field.querySelector('.var-unsupported')!; - expect(note.textContent).toContain('Array(String)'); - expect(note.getAttribute('aria-label')).toContain('no option list'); - expect(note.getAttribute('title')).toContain('Add option SQL'); - // Never the misleading container wording. - expect(note.getAttribute('title')).not.toContain('container type'); + // `['a','b']` still binds. Covers both an integer and a string element type + // (#470 acceptance). + for (const type of ['Array(Int32)', 'Array(String)']) { + const { bar } = build(`SELECT * FROM t WHERE x IN {tags:${type}}`, { + variables: { tags: { options: null } }, + }); + const field = fieldFor(bar, 'tags'); + const input = field.querySelector('.var-input')!; + expect(input).not.toBeNull(); + expect(field.querySelector('.ms-trigger')).toBeNull(); + expect(input.classList.contains('is-unsupported')).toBe(true); + expect(input.placeholder).toBe(type); + expect(input.title).toContain('Add option SQL'); + // Never the misleading container wording. + expect(input.title).not.toContain('container type'); + expect(unsupportedDesc(field, input).textContent).toContain('no option list'); + // One control, one row: the icon is the only sibling, and it's textless. + expect(field.querySelector('.var-unsupported-icon')!.textContent).toBe(''); + } }); - it('renders an Array(scalar) WITH options as the multi-select, not a text field', () => { + it('renders an Array(scalar) WITH options as the multi-select, not a text field — the diagnostic clears with no leftover styling', () => { for (const type of ['Array(String)', 'Array(UInt64)', 'Array(LowCardinality(String))']) { const { bar } = build(`SELECT * FROM t WHERE x IN {tags:${type}}`, { variables: { tags: { options: OPTIONS } }, @@ -653,10 +699,12 @@ describe('buildVariableBar — Dashboard variable controls (#447 phase 2)', () = expect(trigger).not.toBeNull(); expect(trigger.getAttribute('aria-haspopup')).toBe('dialog'); expect(trigger.textContent).toBe('Not set'); - // It REPLACES the plain input and the single-select, rather than adorning. - expect(field.querySelector('.var-unsupported')).toBeNull(); + // It REPLACES the plain input and the single-select, rather than adorning — + // clearing the diagnostic leaves no trace of the warning treatment. + expect(field.querySelector('.var-unsupported-icon')).toBeNull(); expect(field.querySelector('.variable-select')).toBeNull(); expect(field.querySelector('input.var-input')).toBeNull(); + expect(field.querySelector('.is-unsupported')).toBeNull(); } }); @@ -667,7 +715,7 @@ describe('buildVariableBar — Dashboard variable controls (#447 phase 2)', () = variables: { tags: { options: [] } }, }); expect(fieldFor(bar, 'tags').querySelector('.ms-trigger')).not.toBeNull(); - expect(fieldFor(bar, 'tags').querySelector('.var-unsupported')).toBeNull(); + expect(fieldFor(bar, 'tags').querySelector('.var-unsupported-icon')).toBeNull(); }); it('takes the committed selection from the spec, and leaves varValues untouched', () => {