diff --git a/packages/overlay/src/change-set.test.ts b/packages/overlay/src/change-set.test.ts index e591725..f5f7857 100644 --- a/packages/overlay/src/change-set.test.ts +++ b/packages/overlay/src/change-set.test.ts @@ -336,3 +336,72 @@ describe("ChangeSet previews and payload filtering", () => { expect(set.targets()).toHaveLength(1); }); }); + +/* + * The no-op test, which `===` was not. + * + * `from` and `to` arrive in different serialisations and always have. `recordOn` + * takes `from` from computed style, which every engine hands back in the legacy + * comma form — `rgb(59, 130, 246)` — while `to` comes from the colour picker's + * `formatColor`, which writes the modern space form, `rgb(59 130 246)`. One + * colour, two strings. + * + * So a colour set back to its original was never recognised as unchanged: the + * chip stayed in the composer and the agent was sent an instruction to make an + * edit that changes nothing — + * + * background-color: rgb(59, 130, 246) → rgb(59 130 246) + */ +describe("a colour edited back to where it started", () => { + let set: ChangeSet; + let button: Element; + + beforeEach(() => { + set = new ChangeSet(); + button = node("button"); + }); + + const record = (property: string, from: string, to: string): void => { + set.record({ element, from, node: button, property, source: null, to }); + }; + + it("is dropped across the comma/space spellings of one colour", () => { + record("background-color", "rgb(59, 130, 246)", "rgb(59 130 246)"); + expect(set.count()).toBe(0); + }); + + it("is dropped when the source authored hex and the DOM computed rgb", () => { + record("color", "rgb(0, 170, 255)", "#0af"); + record("border-top-color", "#0af", "rgb(0 170 255)"); + expect(set.count()).toBe(0); + }); + + it("is dropped for a colour carrying an alpha", () => { + record("background-color", "rgba(0, 0, 0, 0.5)", "rgb(0 0 0 / 0.5)"); + expect(set.count()).toBe(0); + }); + + it("still records a colour that genuinely changed", () => { + // The guard must not swallow real edits, including near-misses. + record("background-color", "rgb(59, 130, 246)", "rgb(59 130 247)"); + expect(set.count()).toBe(1); + }); + + it("leaves non-colour properties on exact string equality", () => { + /* + * Deliberately scoped to colours. Lengths already round-trip as the same + * string because `keepAuthoredUnit` preserves the unit the source wrote, and + * teaching this to equate `16px` with `1rem` would drop edits that a + * stylesheet author means to keep. + */ + record("padding-top", "16px", "1rem"); + expect(set.count()).toBe(1); + }); + + it("does not treat two unreadable values as equal", () => { + // `sameColor` refuses rather than guessing, so a pair it cannot parse falls + // back to the string test — and two different strings are a real change. + record("background-color", "var(--a)", "var(--b)"); + expect(set.count()).toBe(1); + }); +}); diff --git a/packages/overlay/src/change-set.ts b/packages/overlay/src/change-set.ts index 8765f66..5f2d118 100644 --- a/packages/overlay/src/change-set.ts +++ b/packages/overlay/src/change-set.ts @@ -6,6 +6,8 @@ import type { TokenRef, VisualEditTarget, } from "@airship/protocol"; +import { categoryForProperty } from "@airship/protocol/tokens"; +import { sameColor } from "./inspector/css-value"; import { applyPreview, clearPreview } from "./inspector/style-model"; /** @@ -83,6 +85,39 @@ function addresses( ); } +/** + * Did this edit actually change anything? The no-op test, which `===` fails. + * + * `from` and `to` arrive in different serialisations and always have. + * `recordOn` takes `from` from computed style, which every engine hands back in + * the legacy comma form — `rgb(59, 130, 246)` — while `to` comes from the colour + * picker's `formatColor`, which writes the modern space form, `rgb(59 130 246)`. + * One colour, two strings, so a colour set back to its original compared unequal + * and the change survived: a chip stayed in the composer, and the agent was sent + * + * background-color: rgb(59, 130, 246) → rgb(59 130 246) + * + * — an instruction to make an edit that changes nothing. + * + * Only colours need this. Lengths already round-trip as the same string, because + * `keepAuthoredUnit` preserves the unit the source wrote; it is specifically the + * colour boundary that has two normal forms. Keyed on `categoryForProperty` so + * the set of colour-valued properties stays the one table everything else reads. + */ +function unchanged( + property: string, + from: string, + to: string, + node: Element +): boolean { + if (to === from) { + return true; + } + return ( + categoryForProperty(property) === "colors" && sameColor(from, to, node) + ); +} + /** Grouping key for splitting one node's declarations back into wire targets. */ function slotOf(change: Change): string { return `${change.scope ?? ""}\u0000${change.state ?? ""}`; @@ -157,7 +192,7 @@ export class ChangeSet { const { node, property, from, to, target } = input; // A binding survives an unchanged value; a plain edit does not. See // `RecordInput.binding`. - if (to === from && !input.binding) { + if (unchanged(property, from, to, node) && !input.binding) { this.remove(node, property, target); return; } diff --git a/packages/overlay/src/inspector/controls/color-picker.test.ts b/packages/overlay/src/inspector/controls/color-picker.test.ts index be2f0ed..bf6d5cb 100644 --- a/packages/overlay/src/inspector/controls/color-picker.test.ts +++ b/packages/overlay/src/inspector/controls/color-picker.test.ts @@ -1,6 +1,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { cls } from "../../dom"; import { closeOpenPopover } from "../../popover-host"; +import { recentColors } from "../recent-colors"; import { createColorRow } from "./color-picker"; /* @@ -192,3 +193,201 @@ describe("gestures", () => { expect(end).toHaveBeenCalledTimes(1); }); }); + +/* + * The two hex fields, which used to disagree with each other. + * + * The row's field gated on a private `/^[0-9a-f]{3}$|^[0-9a-f]{6}$/i` while the + * *same file's* popover field gated on nothing and went straight to + * `parseColor` — so `FF0000AA` was accepted in one and silently reverted in the + * other. Both now go through `isHexColor`, the grammar `parseHex` implements. + * + * And the row committed `formatColor(…, "rgb")` unconditionally, which made it + * the one control performing the exact conversion the picker was fixed to stop + * doing: "editing any Tailwind 4 palette colour silently rewrote it as `rgb()` + * — a gamut and readability downgrade". + */ +describe("the row's hex field", () => { + /** `bindField` commits on Enter and on blur, never on `change`. */ + function type(input: HTMLInputElement, text: string): void { + input.value = text; + press(input, "Enter"); + } + + it("accepts the four- and eight-digit hex parseColor reads", () => { + const r = row("rgb(255, 0, 0)"); + type(r.hex, "00FF00AA"); + expect(r.commits).toHaveLength(1); + // Eight digits carry their own alpha: 0xAA / 255 = 0.667. + expect(r.commits[0]).toBe("rgb(0 255 0 / 0.667)"); + }); + + it("accepts four-digit hex, which the old gate also refused", () => { + const r = row("rgb(255, 0, 0)"); + type(r.hex, "0F08"); + expect(r.commits).toHaveLength(1); + // `0F08` expands to `#00FF0088`; 0x88 / 255 = 0.533. + expect(r.commits[0]).toBe("rgb(0 255 0 / 0.533)"); + }); + + it("shows a typed alpha in the % field beside it", () => { + const r = row("rgb(255, 0, 0)"); + type(r.hex, "00FF00AA"); + // The row splits colour and alpha across two affordances, so an eight-digit + // hex has to land in both rather than only in what gets committed. + expect(r.hex.value).toBe("00FF00"); + expect(r.alpha.value).toBe("67"); + }); + + it("leaves the alpha alone for a hex that does not carry one", () => { + // Three and six digits say nothing about alpha, so the % field keeps what + // the user last set there. + const r = row("rgba(255, 0, 0, 0.5)"); + type(r.hex, "00FF00"); + expect(r.commits[0]).toBe("rgb(0 255 0 / 0.5)"); + expect(r.alpha.value).toBe("50"); + }); + + it("commits hex as hex rather than downgrading it to rgb()", () => { + const r = row("rgb(255, 0, 0)"); + type(r.hex, "00FF00"); + expect(r.commits[0]).toBe("#00ff00"); + }); + + it("falls back to rgb() only when an alpha has to be carried", () => { + // The `%` field is holding a partial alpha that hex cannot express without + // appending two digits the user did not type. + const r = row("rgba(255, 0, 0, 0.5)"); + type(r.hex, "00FF00"); + expect(r.commits[0]).toBe("rgb(0 255 0 / 0.5)"); + }); + + it("reverts rather than committing something that is not hex", () => { + const r = row("rgb(255, 0, 0)"); + type(r.hex, "nonsense"); + expect(r.commits).toHaveLength(0); + expect(r.hex.value).toBe("FF0000"); + }); + + it("reverts on a hex of a length that is not a colour", () => { + const r = row("rgb(255, 0, 0)"); + type(r.hex, "ABCDE"); + expect(r.commits).toHaveLength(0); + expect(r.hex.value).toBe("FF0000"); + }); +}); + +/* + * The popover's hex field read an eight-digit hex's alpha and threw it away: + * `state.a` was never assigned, so `FF000080` produced fully opaque red and the + * alpha slider did not move. `setValue` and the recents row both assign it. + */ +describe("the popover's hex field", () => { + it("keeps the alpha of an eight-digit hex", () => { + const r = row("rgb(255, 0, 0)"); + r.swatch.click(); + const fields = [ + ...document.querySelectorAll(`.${cls("ctl-input")}`), + ]; + // The popover's own hex field, not the row's — the row's is the first in + // the document, the popover's is inside the `.ap-pop` subtree. + const pop = document.querySelector(`.${cls("pop")}`) as HTMLElement; + const hex = pop.querySelector(`.${cls("ctl-input")}`); + expect(hex).not.toBeNull(); + expect(fields.length).toBeGreaterThan(1); + if (!hex) { + return; + } + hex.value = "00FF0080"; + hex.dispatchEvent( + new KeyboardEvent("keydown", { + bubbles: true, + cancelable: true, + key: "Enter", + }) + ); + // 0x80 / 255 = 0.502. Before the fix this committed a fully opaque colour. + expect(r.commits.at(-1)).toBe("#00ff0080"); + }); +}); + +/* + * The recents row is keyed on the raw string — `pushRecentColor` dedupes with + * `c !== value` — so every caller has to push one colour in one notation. That + * held only by accident until the row's hex field started committing hex: the + * same colour typed here and picked in the popover produced two swatches. + * + * What notation reaches the user's source and what keys an in-memory palette + * are separate questions, and this is where they were conflated. + */ +describe("the recents row", () => { + it("records one swatch however the colour was chosen", () => { + const r = row("rgb(255, 0, 0)"); + r.hex.value = "00FF00"; + press(r.hex, "Enter"); + // The source gets hex; the palette gets the canonical form every other + // caller pushes. + expect(r.commits.at(-1)).toBe("#00ff00"); + expect(recentColors()).toContain("rgb(0 255 0)"); + /* + * The invariant, asserted over the whole list rather than by counting: the + * module keeps its recents for the life of the page, so this file's earlier + * cases have already pushed to it and there is no reset to call. "No entry + * is in hex" is the property that actually stops a colour appearing twice, + * and it does not care what else is in the list. + */ + expect(recentColors().some((c) => c.startsWith("#"))).toBe(false); + }); +}); + +/* + * The `%` field, now routed through `withAlpha`. + * + * `commitPct` was `withAlpha`'s body written out again, which made it the + * second definition of a rule that already had a name — and left the exported + * one with no callers at all, free to drift from the copy that was actually + * running. The behaviour below is what both spellings owed. + */ +describe("the alpha field", () => { + /** `createNumField` commits on blur, unlike `bindField`'s Enter-or-blur. */ + function type(input: HTMLInputElement, text: string): void { + input.value = text; + input.dispatchEvent(new Event("blur")); + } + + it("folds the alpha into the colour rather than writing opacity", () => { + // `opacity` composites the element and all its children, so a fill at 50% + // would fade the text inside it. This is the whole reason the rule exists. + const r = row("rgb(255, 0, 0)"); + type(r.alpha, "40"); + expect(r.commits.at(-1)).toBe("rgb(255 0 0 / 0.4)"); + }); + + it("keeps the colour and changes only the channel it owns", () => { + const r = row("rgba(0, 128, 255, 0.25)"); + type(r.alpha, "75"); + expect(r.commits.at(-1)).toBe("rgb(0 128 255 / 0.75)"); + }); + + it("commits nothing on a row whose colour it cannot read", () => { + // `withAlpha` returns its input unchanged when unparseable, which is how a + // `Mixed` row falls through to `reflect` without a second parse. + const r = row("Mixed"); + type(r.alpha, "40"); + expect(r.commits).toEqual([]); + }); + + it("commits nothing when the alpha did not actually change", () => { + // The other half of the same `next === current` test. + const r = row("rgb(255 0 0 / 0.4)"); + type(r.alpha, "40"); + expect(r.commits).toEqual([]); + }); + + it("reverts a value that is not a number", () => { + const r = row("rgb(255, 0, 0)"); + type(r.alpha, "banana"); + expect(r.commits).toEqual([]); + expect(r.alpha.value).toBe("100"); + }); +}); diff --git a/packages/overlay/src/inspector/controls/color-picker.ts b/packages/overlay/src/inspector/controls/color-picker.ts index a395378..9b0d98f 100644 --- a/packages/overlay/src/inspector/controls/color-picker.ts +++ b/packages/overlay/src/inspector/controls/color-picker.ts @@ -18,14 +18,17 @@ import { alphaOf, formatColor, hsvToRgb, + isHexColor, isParseableColor, opaque, parseColor, type RGBA, rgbToHsl, rgbToHsv, + withAlpha, } from "../css-value"; import type { Descriptor } from "../descriptors"; +import { MIXED } from "../mixed"; import { onRecentColorsChange, pushRecentColor, @@ -34,11 +37,8 @@ import { import { bindField, createNumField, createTextField } from "./num-field"; import type { ControlHandle, Gestures, OnChange } from "./types"; -/** Multi-select disagreement. Mirrors `panel.ts`'s own sentinel. */ -const MIXED = "Mixed"; /** The hex field shows and accepts bare digits; the `#` is chrome. */ const LEADING_HASH = /^#/; -const SHORT_OR_LONG_HEX = /^[0-9a-f]{3}$|^[0-9a-f]{6}$/i; type Mode = "hex" | "rgb" | "hsl"; @@ -63,6 +63,17 @@ interface HSVA { export interface ColorRowOptions { /** Brackets a slider drag into one undo step. See `Gestures`. */ gestures?: Gestures; + /** + * The element the colour belongs to, naming the realm it resolves against. + * + * A `var()` or a named colour is read by `parseColor`'s engine probe, and a + * probe in the wrong document answers about the wrong `--brand`: a canvas + * frame's colour resolved against the overlay shell, where the custom property + * is undefined, so the swatch painted the shell's inherited colour and the + * next edit wrote that back into the frame. Optional because a row can edit a + * colour that belongs to no element yet — a gradient stop being composed. + */ + node?: Element | null; onChange: (next: string) => void; tip?: string; value: string; @@ -132,7 +143,7 @@ export function createColorRow(opts: ColorRowOptions): ColorRowHandle { alpha.element.classList.add(cls("paint-pct")); function paintSwatch(css: string): void { - const mixed = !isParseableColor(css); + const mixed = !isParseableColor(css, opts.node); swatch.toggleAttribute("data-mixed", mixed); // Two background layers: the colour over a checkerboard, so alpha is // legible without a wrapper element. A 40% fill used to read as an @@ -155,9 +166,11 @@ export function createColorRow(opts: ColorRowOptions): ColorRowHandle { if (boundToken !== null) { return; } - if (isParseableColor(css)) { - hex.value = opaque(css).replace(LEADING_HASH, "").toUpperCase(); - alpha.setValue(String(Math.round(alphaOf(css) * 100))); + if (isParseableColor(css, opts.node)) { + hex.value = opaque(css, opts.node) + .replace(LEADING_HASH, "") + .toUpperCase(); + alpha.setValue(String(Math.round(alphaOf(css, opts.node) * 100))); } else { hex.value = ""; hex.placeholder = MIXED; @@ -170,30 +183,82 @@ export function createColorRow(opts: ColorRowOptions): ColorRowHandle { opts.onChange(css); } + /* + * The typed value is committed as hex when the alpha allows it. + * + * This used to be a hardcoded `formatColor(…, "rgb")`, which made the row the + * one control that performed the exact conversion the picker was fixed to stop + * doing — see `emit`, which honours the mode button because "editing any + * Tailwind 4 palette colour silently rewrote it as `rgb()` — a gamut and + * readability downgrade". Someone typing six hex digits has said which + * notation they want; writing back `rgb(…)` overrules them for no reason. + * + * `rgb()` is still the form when a partial alpha has to be carried, because + * hex cannot express it without appending two digits the user did not type + * into their source. + * + * Where that alpha comes from depends on what was typed. Four and eight digits + * carry one; three and six do not, and for those the alpha stays where the + * user last put it, in the `%` field beside this one. Reading `alphaOf(current)` + * unconditionally — which is what this did — meant an eight-digit hex was + * accepted and its alpha silently dropped, the same defect the popover's hex + * field had. + */ function commitHex(): void { const raw = hex.value.trim().replace(LEADING_HASH, ""); - if (SHORT_OR_LONG_HEX.test(raw)) { + if (isHexColor(`#${raw}`)) { const rgb = parseColor(`#${raw}`); if (rgb) { + const carriesAlpha = raw.length === 4 || raw.length === 8; + const a = carriesAlpha ? rgb[3] : alphaOf(current, opts.node); const next = formatColor( - [rgb[0], rgb[1], rgb[2], alphaOf(current)], - "rgb" + [rgb[0], rgb[1], rgb[2], a], + a >= 1 ? "hex" : "rgb" ); emit(next); - pushRecentColor(next); + /* + * The recents row gets the `rgb()` form even when the source gets hex. + * + * `pushRecentColor` dedupes on the raw string, and every other caller + * pushes `formatColor(…, "rgb")` — so pushing `next` here would give the + * same colour two swatches depending on whether it was typed into this + * field or picked in the popover. What notation reaches the user's + * source and what keys an in-memory palette are separate questions. + */ + pushRecentColor(formatColor([rgb[0], rgb[1], rgb[2], a], "rgb")); return; } } reflect(current); } + /* + * The `%` field, through the one function that folds an alpha into a paint. + * + * This was `withAlpha`'s body written out again — parse, replace the fourth + * channel, re-serialise — which made it the second definition of a rule that + * already had a name and a docstring explaining why it exists (`opacity` + * composites the element *and its children*, so a fill at 50% would fade the + * text inside it; the alpha of that one paint is the exact equivalent). + * + * The `next === current` test is doing two jobs, deliberately. `withAlpha` + * returns its input unchanged when the colour is unparseable, so a `Mixed` row + * lands here — and so does an alpha that did not actually change. Both want + * the same thing: put the field back to what the row is showing and record + * nothing. Splitting them would need a second parse to tell them apart, for a + * distinction with no different outcome. + */ function commitPct(n: number): void { - const rgb = parseColor(current); - if (Number.isNaN(n) || !rgb) { + if (Number.isNaN(n)) { reflect(current); return; } - emit(formatColor([rgb[0], rgb[1], rgb[2], n / 100], "rgb")); + const next = withAlpha(current, n / 100, opts.node); + if (next === current) { + reflect(current); + return; + } + emit(next); } bindField(hex, commitHex, () => reflect(current)); @@ -224,13 +289,14 @@ export function createColorRow(opts: ColorRowOptions): ColorRowHandle { picker = openColorPicker({ anchor: swatch, gestures: opts.gestures, + node: opts.node, onChange: emit, onClose: () => { picker = null; }, // A `Mixed` selection is "several values, one of which you are about to // impose", not a read-only state — so the picker opens, seeded white. - value: isParseableColor(current) ? current : "#FFFFFF", + value: isParseableColor(current, opts.node) ? current : "#FFFFFF", }); }); @@ -302,10 +368,12 @@ export function createColorControl( descriptor: Descriptor, initial: string, onChange: OnChange, - gestures?: Gestures + gestures?: Gestures, + node?: Element | null ): ControlHandle { const row = createColorRow({ gestures, + node, onChange: (next) => onChange(descriptor.cssProperty, next), tip: descriptor.label, value: initial || descriptor.defaultValue, @@ -329,6 +397,8 @@ export function createColorControl( export interface PickerOptions { anchor: HTMLElement; gestures?: Gestures; + /** The realm the seed colour resolves against. See `ColorRowOptions.node`. */ + node?: Element | null; onChange: (next: string) => void; onClose?: () => void; value: string; @@ -348,7 +418,7 @@ export interface PickerHandle { * the hue you were working in. Holding H separately is the only fix. */ export function openColorPicker(opts: PickerOptions): PickerHandle { - const start = parseColor(opts.value) ?? [255, 255, 255, 1]; + const start = parseColor(opts.value, opts.node) ?? [255, 255, 255, 1]; const [h0, s0, v0] = rgbToHsv(start[0], start[1], start[2]); const state: HSVA = { a: start[3], h: h0, s: s0, v: v0 }; let mode: Mode = "hex"; @@ -545,7 +615,7 @@ export function openColorPicker(opts: PickerOptions): PickerHandle { return { close: () => handle.close(), setValue(css) { - const rgb = parseColor(css); + const rgb = parseColor(css, opts.node); if (!rgb) { return; } @@ -625,6 +695,14 @@ export function openColorPicker(opts: PickerOptions): PickerHandle { state.h = s === 0 ? state.h : h; state.s = s; state.v = v; + /* + * The alpha too. `parseColor` reads `#rrggbbaa` and this was the one + * of the three places holding an `RGBA` that dropped the fourth + * channel on the floor — `setValue` and the recents row both assign it + * — so typing `FF000080` produced fully opaque red and the alpha + * slider did not move. + */ + state.a = rgb[3]; emit(); pushRecentColor(formatColor(rgba(), "rgb")); return true; @@ -700,7 +778,7 @@ export function openColorPicker(opts: PickerOptions): PickerHandle { class: cls("pop-recent"), "data-tip": color, onClick: () => { - const rgb = parseColor(color); + const rgb = parseColor(color, opts.node); if (!rgb) { return; } diff --git a/packages/overlay/src/inspector/controls/gradient-editor.ts b/packages/overlay/src/inspector/controls/gradient-editor.ts index 0ba6785..573e7e7 100644 --- a/packages/overlay/src/inspector/controls/gradient-editor.ts +++ b/packages/overlay/src/inspector/controls/gradient-editor.ts @@ -39,6 +39,8 @@ const ANGLE_COARSE_STEP = 15; export interface GradientEditorOptions { gestures?: Gestures; + /** The realm a stop colour resolves against. See `ColorRowOptions.node`. */ + node?: Element | null; onChange: (css: string) => void; /** The gradient's current CSS text. */ value: string; @@ -114,7 +116,7 @@ export function openGradientEditor( stopBar(gradient, selected, { onAdd: (fraction) => { const stop: GradientStop = { - color: interpolate(gradient, fraction), + color: interpolate(gradient, fraction, opts.node), position: `${Math.round(fraction * 100)}%`, }; const stops = [...gradient.stops, stop]; @@ -141,24 +143,33 @@ export function openGradientEditor( // `opts.gestures` so an angle drag is one undo step, as `stopList`'s fields // already are — without it a 40-degree drag left ~40 entries in the history. angleRow(gradient, commit, track, opts.gestures), - stopList(gradient, selected, opts.gestures, track, { - onEdit: (index, stop) => { - const stops = gradient.stops.map((s, i) => (i === index ? stop : s)); - commit({ ...gradient, stops }); + stopList( + gradient, + selected, + opts.gestures, + track, + { + onEdit: (index, stop) => { + const stops = gradient.stops.map((s, i) => + i === index ? stop : s + ); + commit({ ...gradient, stops }); + }, + onRemove: (index) => { + if (gradient.stops.length <= MIN_STOPS) { + return; + } + const stops = gradient.stops.filter((_, i) => i !== index); + selected = Math.min(selected, stops.length - 1); + commit({ ...gradient, stops }); + }, + onSelect: (index) => { + selected = index; + paint(); + }, }, - onRemove: (index) => { - if (gradient.stops.length <= MIN_STOPS) { - return; - } - const stops = gradient.stops.filter((_, i) => i !== index); - selected = Math.min(selected, stops.length - 1); - commit({ ...gradient, stops }); - }, - onSelect: (index) => { - selected = index; - paint(); - }, - }) + opts.node + ) ); handle.reposition(); }; @@ -416,7 +427,8 @@ function stopList( selected: number, gestures: Gestures | undefined, track: (dispose: () => void) => void, - handlers: StopHandlers + handlers: StopHandlers, + node?: Element | null ): HTMLElement { const list = el("div", { class: cls("grad-stops") }); const order = sortedStops(gradient); @@ -432,6 +444,7 @@ function stopList( const color = createColorRow({ gestures, + node, onChange: (next) => handlers.onEdit(index, { ...stop, color: next }), tip: "Stop color", value: stop.color, diff --git a/packages/overlay/src/inspector/controls/token-field.test.ts b/packages/overlay/src/inspector/controls/token-field.test.ts index f731a82..b43f7e4 100644 --- a/packages/overlay/src/inspector/controls/token-field.test.ts +++ b/packages/overlay/src/inspector/controls/token-field.test.ts @@ -114,7 +114,7 @@ describe("the token picker", () => { // which is exactly the case that must not disappear. const marked = all.every((r) => r.node.dataset.outOfScope !== undefined); expect(marked).toBe(true); - expect(all[0].node.dataset.tip).toContain("Not defined on this element"); + expect(all[0].node.dataset.tip).toContain("Not defined here"); }); it("still applies an out-of-scope token", () => { diff --git a/packages/overlay/src/inspector/controls/token-field.ts b/packages/overlay/src/inspector/controls/token-field.ts index ed13b44..77c17e3 100644 --- a/packages/overlay/src/inspector/controls/token-field.ts +++ b/packages/overlay/src/inspector/controls/token-field.ts @@ -8,7 +8,7 @@ * of dead icons. */ import type { TokenRef } from "@airship/protocol"; -import type { DesignToken } from "@airship/protocol/tokens"; +import { type DesignToken, looksLikeColor } from "@airship/protocol/tokens"; import { cls, el } from "../../dom"; import { icon } from "../../icons"; import { openPopover } from "../../popover-host"; @@ -123,11 +123,11 @@ function describe(current: TokenRef | undefined, property: string): string { return `Use a design token for ${property}`; } if (!current.exact) { - return `Closest token: ${current.name} (${current.actual}) — click to apply`; + return `Closest: ${current.name} (${current.actual})`; } return current.via === "reference" - ? `Using ${current.name} — click to change or detach` - : `Matches ${current.name} — click to use it by name`; + ? `Using ${current.name}. Click to change` + : `Matches ${current.name}. Click to bind`; } /** @@ -309,7 +309,7 @@ function row( // Name on the left, value right-aligned in the dimmed mono the menus already // use for hints — a token's value is exactly that kind of trailing detail. const main = el("span", { class: cls("pop-item-main") }); - if (isColorish(value)) { + if (looksLikeColor(value)) { const swatch = el("span", { class: cls("token-swatch") }); swatch.style.background = value; main.append(swatch); @@ -348,9 +348,7 @@ function row( const where = `${token.file ?? ""}${token.file && token.line ? `:${token.line}` : ""}`; const tip = bound ? `Stop using ${token.name} here and write a literal value` - : [inScope ? "" : "Not defined on this element", where] - .filter(Boolean) - .join(" — "); + : [inScope ? "" : "Not defined here", where].filter(Boolean).join(" · "); if (tip) { item.dataset.tip = tip; } @@ -366,9 +364,3 @@ function row( }); return item; } - -const COLORISH = /^(#|rgba?\(|hsla?\(|oklch\(|oklab\(|lab\(|lch\(|color\()/i; - -function isColorish(value: string): boolean { - return COLORISH.test(value.trim()); -} diff --git a/packages/overlay/src/inspector/css-value.test.ts b/packages/overlay/src/inspector/css-value.test.ts index 475f0dd..20ab35e 100644 --- a/packages/overlay/src/inspector/css-value.test.ts +++ b/packages/overlay/src/inspector/css-value.test.ts @@ -1,11 +1,14 @@ +import { normalizeTokenValue } from "@airship/protocol/tokens"; import { describe, expect, it } from "vitest"; import { alphaOf, formatColor, + isHexColor, isParseableColor, opaque, parseColor, type RGBA, + sameColor, splitTop, splitWords, withAlpha, @@ -201,3 +204,217 @@ describe("isParseableColor", () => { expect(isParseableColor("#abcdef")).toBe(true); }); }); + +/* + * `sameColor` — the comparison, and the reason `===` is not it. + * + * Three places asked "are these the same colour?" and all three answered with + * string equality, which is wrong because every boundary here hands back a + * different spelling: computed style gives the legacy comma form, `formatColor` + * writes the modern space form, and stylesheets author hex. + */ +describe("sameColor", () => { + it("sees through the spelling", () => { + const same = [ + ["#0af", "rgb(0, 170, 255)"], + ["#0af", "rgb(0 170 255)"], + ["#00AAFF", "#0af"], + ["rgb(59, 130, 246)", "rgb(59 130 246)"], + ["rgba(0, 0, 0, 0.5)", "rgb(0 0 0 / 50%)"], + ["transparent", "rgba(0, 0, 0, 0)"], + ]; + for (const [a, b] of same) { + expect(sameColor(a, b), `${a} vs ${b}`).toBe(true); + } + }); + + it("survives a round trip through formatColor", () => { + /* + * The case that forces alpha to be compared as an 8-bit channel rather than + * a float. `#00000080` is 128/255 = 0.50196…, and `formatColor` rounds alpha + * to three decimals — so the value it writes back is `0.502`, and exact + * float equality would report a colour as different from itself. + */ + const parsed = parseColor("#00000080") as RGBA; + expect(sameColor("#00000080", formatColor(parsed, "rgb"))).toBe(true); + expect(sameColor("#00000080", formatColor(parsed, "hex"))).toBe(true); + }); + + it("still calls a different colour different", () => { + // No tolerance: `findToken` is right that a slightly different hex is a + // different colour, not an approximation of one. + expect(sameColor("#000000", "#010000")).toBe(false); + expect(sameColor("rgb(0 0 0 / 0.5)", "rgb(0 0 0 / 0.6)")).toBe(false); + expect(sameColor("#000000", "rgba(0, 0, 0, 0.99)")).toBe(false); + }); + + it("refuses rather than guessing when either side is unreadable", () => { + // `Mixed` is the sentinel the panel uses for a multi-select disagreement; + // it must never compare equal to anything, including itself. + expect(sameColor("Mixed", "Mixed")).toBe(false); + expect(sameColor("#000000", "Mixed")).toBe(false); + }); +}); + +/* + * The one hex grammar. There were three, and the two in `color-picker.ts` and + * `gradient.ts` both refused four- and eight-digit hex that `parseHex` reads. + */ +describe("isHexColor", () => { + it("accepts every length parseHex implements, in either case", () => { + for (const value of ["#abc", "#abcd", "#aabbcc", "#aabbccdd", "#AABBCC"]) { + expect(isHexColor(value), value).toBe(true); + } + }); + + it("rejects the lengths that are not hex colours", () => { + for (const value of ["#ab", "#abcde", "#abcdefg", "#", "abc", "#zzz"]) { + expect(isHexColor(value), value).toBe(false); + } + }); + + it("never refuses something parseColor would have read", () => { + // The property that makes it safe as a field gate. + for (const value of ["#abc", "#abcd", "#aabbcc", "#aabbccdd"]) { + expect(parseColor(value), value).not.toBeNull(); + } + }); +}); + +/* + * The realm the engine probe runs in. + * + * `parseColor` has always taken a `node` for this, and for a long time nothing + * passed one: `withAlpha`, `alphaOf`, `opaque` and `isParseableColor` are how + * the swatch, the hex field and `gates.ts` actually ask, and all four dropped + * it. So a `var(--brand)` inside a canvas frame went on resolving against the + * overlay shell — where `--brand` is undefined — and the swatch showed the + * shell's inherited colour while `withAlpha` wrote that wrong colour back. + * + * A parameter nothing passes is not a fix, so what is asserted here is the + * threading, not just the signature. + */ +describe("the realm a colour resolves in", () => { + /** A stand-in frame: a real probe element, but its own window for the read. */ + function frame(color: string) { + const host = document.createElement("div"); + document.body.append(host); + const seen: string[] = []; + const doc = { + body: host, + createElement: (tag: string) => document.createElement(tag), + defaultView: { + getComputedStyle: () => { + seen.push(color); + return { color } as CSSStyleDeclaration; + }, + }, + } as unknown as Document; + return { + node: { nodeType: 1, ownerDocument: doc } as unknown as Element, + seen, + teardown: () => host.remove(), + }; + } + + /** The shell answers a colour nothing in the frame would ever produce. */ + function withShellSaying(color: string, run: () => T): T { + const real = globalThis.getComputedStyle; + globalThis.getComputedStyle = (() => + ({ color }) as CSSStyleDeclaration) as typeof real; + try { + return run(); + } finally { + globalThis.getComputedStyle = real; + } + } + + it("asks the node's own window, not the shell's", () => { + const f = frame("rgb(0, 170, 255)"); + try { + withShellSaying("rgb(1, 1, 1)", () => { + expect(parseColor("var(--brand)", f.node)).toEqual([ + 0, 170, 255, 1, + ]); + }); + expect(f.seen).toHaveLength(1); + } finally { + f.teardown(); + } + }); + + it("threads that realm through every wrapper", () => { + const f = frame("rgba(0, 170, 255, 0.5)"); + try { + withShellSaying("rgb(1, 1, 1)", () => { + // Each of these is a route the panel actually takes. Without the node + // every one of them reports the shell's opaque `rgb(1, 1, 1)`. + expect(opaque("var(--brand)", f.node)).toBe("#00aaff"); + expect(alphaOf("var(--brand)", f.node)).toBe(0.5); + expect(isParseableColor("var(--brand)", f.node)).toBe(true); + expect(withAlpha("var(--brand)", 1, f.node)).toBe("rgb(0 170 255)"); + expect(sameColor("var(--brand)", "rgb(0 170 255 / 0.5)", f.node)).toBe( + true + ); + }); + } finally { + f.teardown(); + } + }); + + it("still falls back to the shell when given no node", () => { + // The old behaviour, which is correct for a colour that belongs to no frame. + withShellSaying("rgb(1, 1, 1)", () => { + expect(parseColor("var(--brand)")).toEqual([1, 1, 1, 1]); + }); + }); +}); + +/* + * The two hex grammars that are left, held together. + * + * There is no way to get to one. `@airship/protocol` is a leaf and must stay + * DOM-free, so it cannot import `parseHex`; the overlay's grammar has to live + * beside the expansion logic it describes. Two definitions in two packages is + * structural — three in one package was the bug. + * + * What can be guaranteed is that they never disagree, which is the failure that + * would matter: `normalizeTokenValue` keys the registry and `parseColor` reads + * the control, so a hex one accepts and the other refuses is a token that + * silently stops binding. This is the seam where that would show up. + */ +describe("the overlay and protocol agree about hex", () => { + it("accepts the same set of hex colours", () => { + for (const value of [ + "#abc", + "#abcd", + "#aabbcc", + "#aabbccdd", + "#ABC", + "#ab", + "#abcde", + "#abcdefg", + "#zzz", + ]) { + const overlay = parseColor(value) !== null; + // Protocol converts what it recognises and passes the rest through, so a + // changed value is exactly "this was hex to me". + const protocolSaw = normalizeTokenValue(value) !== value.toLowerCase(); + expect(protocolSaw, value).toBe(overlay); + } + }); + + it("resolves them to the same colour", () => { + for (const value of ["#0af", "#00aaff", "#0af8", "#00aaff88"]) { + const viaOverlay = parseColor(value) as RGBA; + // `normalizeTokenValue`'s output is `rgb()`, which the overlay parses on a + // fast path — so a disagreement anywhere in either expansion shows here. + const viaProtocol = parseColor(normalizeTokenValue(value)) as RGBA; + expect(viaProtocol[0], value).toBe(viaOverlay[0]); + expect(viaProtocol[1], value).toBe(viaOverlay[1]); + expect(viaProtocol[2], value).toBe(viaOverlay[2]); + // Alpha survives protocol's three-decimal rounding to the same 8-bit step. + expect(sameColor(value, normalizeTokenValue(value)), value).toBe(true); + } + }); +}); diff --git a/packages/overlay/src/inspector/css-value.ts b/packages/overlay/src/inspector/css-value.ts index 8f688de..01e8e6a 100644 --- a/packages/overlay/src/inspector/css-value.ts +++ b/packages/overlay/src/inspector/css-value.ts @@ -230,6 +230,15 @@ export type RGBA = [number, number, number, number]; * Exported because it is the whole conversion boundary for the colour picker: * this module owns `string ↔ RGBA` and the picker owns `RGBA ↔ HSVA`, so there * is exactly one CSS colour parser in the overlay. + * + * `node` names the realm `viaEngine` resolves against, and every wrapper below + * takes it for one reason: for a long time none of them did, so the realm fix + * `viaEngine` documents was unreachable. `withAlpha`, `alphaOf`, `opaque` and + * `isParseableColor` each called `parseColor(color)` with no node, and they are + * how the swatch, the hex field and `gates.ts` actually ask — so a `var()` + * colour inside a canvas frame went on resolving against the overlay shell + * exactly as before, and `hasFill` could answer `true` off the shell's inherited + * colour. A parameter nothing passes is not a fix; the callers have to thread it. */ export function parseColor(color: string, node?: Element | null): RGBA | null { const c = color.trim().toLowerCase(); @@ -489,8 +498,12 @@ function viaEngine(value: string, realm?: Document | null): RGBA | null { * fill at 50% would fade the text inside it. A design tool's fill opacity is the alpha * of that one paint, and `rgb(r g b / a)` is its exact CSS equivalent. */ -export function withAlpha(color: string, alpha: number): string { - const parsed = parseColor(color); +export function withAlpha( + color: string, + alpha: number, + node?: Element | null +): string { + const parsed = parseColor(color, node); if (!parsed) { return color; } @@ -499,11 +512,11 @@ export function withAlpha(color: string, alpha: number): string { } /** The alpha already carried by a colour, as 0–1. */ -export function alphaOf(color: string): number { +export function alphaOf(color: string, node?: Element | null): number { if (color.trim() === "transparent") { return 0; } - return parseColor(color)?.[3] ?? 1; + return parseColor(color, node)?.[3] ?? 1; } /** @@ -513,8 +526,8 @@ export function alphaOf(color: string): number { * field is six characters wide, and putting `color(srgb 0.97 …)` in it is worse * than being slightly wrong about a colour nothing can edit anyway. */ -export function opaque(color: string): string { - const parsed = parseColor(color); +export function opaque(color: string, node?: Element | null): string { + const parsed = parseColor(color, node); if (!parsed) { return "#000000"; } @@ -522,9 +535,96 @@ export function opaque(color: string): string { return `#${[r, g, b].map((n) => Math.max(0, Math.min(255, n)).toString(16).padStart(2, "0")).join("")}`; } -/** Can this value be shown in the picker at all? Guards `Mixed` and keywords. */ -export function isParseableColor(color: string): boolean { - return parseColor(color) !== null; +/** + * Is this a hex colour? The one hex grammar, for every field that takes one. + * + * There were three of these. `color-picker.ts` gated its row's hex field on + * `/^[0-9a-f]{3}$|^[0-9a-f]{6}$/i` while the *same file's* popover field gated on + * nothing and went straight to `parseColor` — so `FF0000AA` was accepted in one + * field and silently reverted in the other. `gradient.ts` had a third, + * `/^#([\da-f]{3}|[\da-f]{6})$/i`, behind its own hex parser. + * + * `HEX_COLOR` is the grammar `parseHex` actually implements, so a field that + * gates on this can never refuse a value the parser would have read. + */ +export function isHexColor(value: string): boolean { + return HEX_COLOR.test(value.trim().toLowerCase()); +} + +/** + * Can this value be shown in the picker at all? Guards `Mixed` and keywords. + * + * One of four colour predicates, which look interchangeable and are not. They + * answer different questions, and picking the wrong one is how `none` came to + * render as the `Mixed` hairline on every unstroked shape: + * + * - **this** — *can the picker render it?* Asks the engine, so it is the only + * one that knows about `oklch()` and named colours, and the only one that + * costs a style recalc. + * - `looksLikeColor` (`@airship/protocol/tokens`) — *does it look like one?* A + * prefix test, no engine. For the places a wrong answer is cheap and a + * hundred style recalcs would not be. + * - `isEditablePaint` (`svg-paint.ts`) — *is it a colour rather than a paint + * server?* SVG `fill`/`stroke` also take `url(#grad)` and `context-fill`, + * which are valid paints this module cannot read. + * - `hasFill` / `hasStroke` (`gates.ts`) — *does the element have one at all?* + * About the element, not the string. + * + * And `sameColor`, above, is the one comparison. Nothing should be asking + * whether two colours are equal with `===`. + */ +export function isParseableColor( + color: string, + node?: Element | null +): boolean { + return parseColor(color, node) !== null; +} + +/** + * Are these the same colour? The comparison `===` cannot do. + * + * Three places in the overlay ask this question, and all three used to answer it + * with string equality: the change set's no-op check, token matching, and the + * gates. String equality is wrong here because every boundary in this codebase + * hands back a *different spelling of the same colour*. `getComputedStyle` + * returns the legacy comma form, `formatColor` writes the modern space form, and + * a design system authors hex — so `#0af`, `rgb(0 170 255)` and + * `rgb(0, 170, 255)` are one colour that compared unequal three ways. + * + * What that cost: a colour edited and then set back to its original was not + * recognised as a no-op, so a change reading `background-color: rgb(59, 130, + * 246) → rgb(59 130 246)` reached the agent asking it to do nothing. + * + * Exact, not approximate. `findToken` is right that colours have no meaningful + * "near" — a slightly different hex is a different colour, not an approximation + * of one — so this is equality with the spellings normalised away, never a + * tolerance. + * + * The one place that reads as a tolerance and is not: alpha is compared as the + * 8-bit channel it composites to. It has to be, because `formatColor` rounds + * alpha to three decimals, so `#00000080` (128/255 = 0.50196…) serialises as + * `rgb(0 0 0 / 0.502)` and would not equal itself across a round trip. Both + * round to 128, which is the only sense in which they differ or don't. + * + * `node` picks the realm the engine probe resolves against — see `parseColor`. + */ +export function sameColor( + a: string, + b: string, + node?: Element | null +): boolean { + const ca = parseColor(a, node); + const cb = parseColor(b, node); + if (!(ca && cb)) { + return false; + } + const alpha8 = (rgba: RGBA) => Math.round(clamp01(rgba[3]) * 255); + return ( + ca[0] === cb[0] && + ca[1] === cb[1] && + ca[2] === cb[2] && + alpha8(ca) === alpha8(cb) + ); } // -- HSV --------------------------------------------------------------------- diff --git a/packages/overlay/src/inspector/gates.ts b/packages/overlay/src/inspector/gates.ts index af5a791..627ad0b 100644 --- a/packages/overlay/src/inspector/gates.ts +++ b/packages/overlay/src/inspector/gates.ts @@ -24,14 +24,28 @@ * * Hence `Reader`: the caller composes "pending, then computed" once and every * gate asks the same question the same way. + * + * These are the *element* half of the four colour predicates — "does this node + * have a fill at all", not "is this string a colour". `isParseableColor` in + * `css-value.ts` lists all four and says which question each answers. */ import { alphaOf } from "./css-value"; /** How a gate reads a property. Pending value first, computed style behind it. */ export type Reader = (property: string) => string; -/** Does this element have a background worth showing a Fill row for? */ -export function hasFill(read: Reader): boolean { +/** + * Does this element have a background worth showing a Fill row for? + * + * `node` is the element the value was read from, and it is here because the + * answer can depend on which document resolves it. A pending edit can be + * `var(--brand)`, and `alphaOf` reaches `parseColor`'s engine probe to read it — + * against the overlay shell, where `--brand` is undefined, unless it is told + * otherwise. That returned the shell's inherited colour at alpha 1, so an + * unresolvable token read as a fill and the row stayed. Optional because the + * gates are pure functions of a `Reader` and the tests exercise them that way. + */ +export function hasFill(read: Reader, node?: Element): boolean { const color = read("background-color"); if (!color) { return false; @@ -39,7 +53,7 @@ export function hasFill(read: Reader): boolean { // `transparent` never comes back from computed style — it resolves to // `rgba(0, 0, 0, 0)` — but it is exactly what a pending "remove fill" writes, // so both spellings have to be understood here. - return color.trim() !== "transparent" && alphaOf(color) > 0; + return color.trim() !== "transparent" && alphaOf(color, node) > 0; } /** The four edges, so a gate never speaks for the box from one of them. */ diff --git a/packages/overlay/src/inspector/gradient.test.ts b/packages/overlay/src/inspector/gradient.test.ts index ff9a6a2..f80849a 100644 --- a/packages/overlay/src/inspector/gradient.test.ts +++ b/packages/overlay/src/inspector/gradient.test.ts @@ -3,6 +3,7 @@ import { angleOf, formatGradient, type Gradient, + interpolate, isGradient, parseGradient, reverse, @@ -161,3 +162,87 @@ describe("stop ordering", () => { ]); }); }); + +/* + * `interpolate` — the colour of a stop you get by clicking the gradient bar. + * + * This whole function was untested, which is how it shipped carrying a second, + * weaker hex parser: a private `HEX` regex and `hexToRgb` that read three- and + * six-digit hex and nothing else. Everything it could not read fell into the + * "nearer one wins" branch, silently — so clicking the bar inserted an exact + * copy of one end instead of the blend at the point clicked. + * + * What it could not read was most things. `oklch()` is Tailwind 4's entire + * default palette, so no Tailwind project's gradients interpolated at all; and + * because the old serialiser emitted six-digit hex, mixing two `#rrggbbaa` stops + * threw away both alphas. + */ +describe("interpolate", () => { + const ramp = (a: string, b: string): Gradient => + parseGradient(`linear-gradient(${a}, ${b})`) as Gradient; + + it("blends hex, which is all it could ever do before", () => { + expect(interpolate(ramp("#000000", "#ffffff"), 0.5)).toBe( + "rgb(128 128 128)" + ); + // Short hex expands rather than being read as three channels. + expect(interpolate(ramp("#000", "#fff"), 0.5)).toBe("rgb(128 128 128)"); + }); + + it("blends the notations the old parser fell through on", () => { + // Each of these used to return one endpoint verbatim. + expect(interpolate(ramp("rgb(0, 0, 0)", "rgb(255, 255, 255)"), 0.5)).toBe( + "rgb(128 128 128)" + ); + expect(interpolate(ramp("rgb(0 0 0)", "rgb(255 255 255)"), 0.5)).toBe( + "rgb(128 128 128)" + ); + expect(interpolate(ramp("hsl(0 0% 0%)", "hsl(0 0% 100%)"), 0.5)).toBe( + "rgb(128 128 128)" + ); + }); + + it("interpolates the alpha instead of discarding it", () => { + // The old serialiser emitted six-digit hex, so both alphas were lost and a + // fade-to-transparent ramp produced an opaque stop in the middle. + expect(interpolate(ramp("#00000000", "#000000ff"), 0.5)).toBe( + "rgb(0 0 0 / 0.5)" + ); + // A quarter of the way, so the alpha is a value only interpolation produces + // — not either endpoint, which is what the old "nearer one wins" returned. + expect(interpolate(ramp("#00000000", "#000000ff"), 0.25)).toBe( + "rgb(0 0 0 / 0.25)" + ); + expect(interpolate(ramp("rgb(0 0 0 / 0)", "rgb(0 0 0 / 1)"), 0.5)).toBe( + "rgb(0 0 0 / 0.5)" + ); + }); + + it("respects where along the ramp the click landed", () => { + const g = ramp("#000000", "#ffffff"); + expect(interpolate(g, 0)).toBe("rgb(0 0 0)"); + expect(interpolate(g, 1)).toBe("rgb(255 255 255)"); + expect(interpolate(g, 0.25)).toBe("rgb(64 64 64)"); + }); + + it("honours authored stop positions rather than assuming an even spread", () => { + // Both stops sit in the first half, so the midpoint of the *bar* is past + // the last one and the blend has to clamp to it. + const g = parseGradient( + "linear-gradient(#000000 0%, #ffffff 50%)" + ) as Gradient; + expect(interpolate(g, 0.25)).toBe("rgb(128 128 128)"); + expect(interpolate(g, 0.5)).toBe("rgb(255 255 255)"); + }); + + it("still lets the nearer stop win when one is genuinely unreadable", () => { + /* + * The fallback is kept — but it now means what it says. A `var()` that does + * not resolve is a real "no idea", unlike the `rgb()` and `oklch()` values + * that used to land here. + */ + const g = ramp("var(--nope)", "#ffffff"); + expect(interpolate(g, 0.1)).toBe("var(--nope)"); + expect(interpolate(g, 0.9)).toBe("#ffffff"); + }); +}); diff --git a/packages/overlay/src/inspector/gradient.ts b/packages/overlay/src/inspector/gradient.ts index da9ea1b..43f6540 100644 --- a/packages/overlay/src/inspector/gradient.ts +++ b/packages/overlay/src/inspector/gradient.ts @@ -17,7 +17,7 @@ * rounds every stop to a whole percent, and mishandles `turn`, `rad` and * negative angles — all of which are silent data loss on save. */ -import { splitTop, splitWords } from "./css-value"; +import { formatColor, parseColor, splitTop, splitWords } from "./css-value"; export type GradientKind = "linear" | "radial" | "conic"; @@ -278,8 +278,18 @@ export function sortedStops(gradient: Gradient): GradientStop[] { .map((entry) => entry.stop); } -/** Linear RGB interpolation, for the colour of a stop added by clicking. */ -export function interpolate(gradient: Gradient, fraction: number): string { +/** + * Linear RGB interpolation, for the colour of a stop added by clicking. + * + * `node` names the realm the stop colours resolve against — a stop can be + * `var(--brand)`, and a probe run in the wrong document answers about the wrong + * custom property. See `parseColor`. + */ +export function interpolate( + gradient: Gradient, + fraction: number, + node?: Element | null +): string { const { stops } = gradient; let [before] = stops; let after = stops.at(-1) as GradientStop; @@ -298,40 +308,38 @@ export function interpolate(gradient: Gradient, fraction: number): string { }); const span = afterAt - beforeAt; const t = span > 0 ? (fraction - beforeAt) / span : 0; - return mix(before.color, after.color, t); + return mix(before.color, after.color, t, node); } -const HEX = /^#([\da-f]{3}|[\da-f]{6})$/i; - -/** Mix two colours, when both are hex. Otherwise the nearer one wins. */ -function mix(a: string, b: string, t: number): string { - const ca = hexToRgb(a); - const cb = hexToRgb(b); +/** + * Mix two colours. The nearer one wins only when a stop is genuinely unreadable. + * + * This used to carry its own `HEX` regex and `hexToRgb`, accepting three- and + * six-digit hex and nothing else — a second, weaker colour parser living one + * directory away from the real one. Everything it could not read fell into the + * "nearer one wins" branch, and that branch is silent: clicking the gradient bar + * inserted a stop that was an exact copy of one end instead of the blend at the + * point clicked, with no indication anything had been approximated. + * + * What it could not read turned out to be most things. `oklch()` is Tailwind 4's + * entire default palette, so no Tailwind project's gradients interpolated at + * all; `rgb()`, `hsl()` and named colours were equally invisible. And because + * the old serialiser emitted six-digit hex, mixing two `#rrggbbaa` stops threw + * away both alphas. + * + * `parseColor` reads all of those, so the fallback now means what it says: a + * `var()` that does not resolve, or a `currentColor` with no element to resolve + * against. Alpha is the fourth channel and interpolates with the rest; + * `formatColor` rounds and clamps, so the fractional results go straight to it. + */ +function mix(a: string, b: string, t: number, node?: Element | null): string { + const ca = parseColor(a, node); + const cb = parseColor(b, node); if (!(ca && cb)) { return t < 0.5 ? a : b; } - const channel = (i: number): number => - Math.round(ca[i] + (cb[i] - ca[i]) * t); - return `#${[0, 1, 2].map((i) => channel(i).toString(16).padStart(2, "0")).join("")}`; -} - -function hexToRgb(color: string): [number, number, number] | null { - const match = HEX.exec(color.trim()); - if (!match) { - return null; - } - const hex = - match[1].length === 3 - ? match[1] - .split("") - .map((c) => c + c) - .join("") - : match[1]; - return [ - Number.parseInt(hex.slice(0, 2), 16), - Number.parseInt(hex.slice(2, 4), 16), - Number.parseInt(hex.slice(4, 6), 16), - ]; + const channel = (i: number): number => ca[i] + (cb[i] - ca[i]) * t; + return formatColor([channel(0), channel(1), channel(2), channel(3)], "rgb"); } /** A left-to-right preview of the stops, for the editor's bar. */ diff --git a/packages/overlay/src/inspector/mixed.test.ts b/packages/overlay/src/inspector/mixed.test.ts new file mode 100644 index 0000000..bde8d12 --- /dev/null +++ b/packages/overlay/src/inspector/mixed.test.ts @@ -0,0 +1,63 @@ +/** + * The disagreement sentinel, and the fold that produces it. + * + * `MIXED` was declared twice — `panel.ts` and `controls/color-picker.ts` each + * had a private `const MIXED = "Mixed"`, the second commented "mirrors + * `panel.ts`'s own sentinel" — and `sections/stroke.ts` was about to add a + * third. `agreed` exists because that section needed the same fold four times + * over: what do these edges say, and do they say it together. + * + * The comparator parameter is the load-bearing part. `panel.seed` folds with + * `===`, which is right for the keywords and lengths it handles and wrong for + * colours, because the values being compared do not all come from the same + * place once an edit is pending. + */ +import { describe, expect, it } from "vitest"; +import { sameColor } from "./css-value"; +import { agreed, MIXED } from "./mixed"; + +describe("agreed", () => { + it("gives the shared value when everything agrees", () => { + expect(agreed(["8px", "8px", "8px"], "0px")).toBe("8px"); + }); + + it("gives the sentinel when anything disagrees", () => { + expect(agreed(["8px", "8px", "4px"], "0px")).toBe(MIXED); + }); + + it("treats an empty value as the fallback on both sides of the compare", () => { + // Four longhands read off an element that declares none come back as empty + // strings; they agree, and what they agree on is the fallback. + expect(agreed(["", "", ""], "0px")).toBe("0px"); + expect(agreed(["0px", "", "0px"], "0px")).toBe("0px"); + }); + + it("returns the fallback for nothing to compare", () => { + expect(agreed([], "0px")).toBe("0px"); + }); + + it("does not call one colour Mixed for being spelled two ways", () => { + /* + * The reason the comparator is a parameter. `getComputedStyle` hands back + * the legacy comma form while `formatColor` writes the modern space one, so + * a box the user just made uniform reads as four values under `===`. + */ + const spellings = ["rgb(0, 170, 255)", "rgb(0 170 255)", "#0af"]; + expect(agreed(spellings, "#000000")).toBe(MIXED); + expect(agreed(spellings, "#000000", sameColor)).toBe("rgb(0, 170, 255)"); + }); + + it("still separates colours that genuinely differ", () => { + expect( + agreed(["rgb(0, 170, 255)", "rgb(0, 170, 254)"], "#000000", sameColor) + ).toBe(MIXED); + }); + + it("reports the first value, not a normalised one", () => { + // Whatever comes back is handed to a control as its seed, so it has to be a + // value that control can render — not a canonical form invented here. + expect(agreed(["#0af", "rgb(0 170 255)"], "#000000", sameColor)).toBe( + "#0af" + ); + }); +}); diff --git a/packages/overlay/src/inspector/mixed.ts b/packages/overlay/src/inspector/mixed.ts new file mode 100644 index 0000000..bedfb70 --- /dev/null +++ b/packages/overlay/src/inspector/mixed.ts @@ -0,0 +1,51 @@ +/* + * The sentinel a control shows when the values behind it disagree. + * + * There were two of these — `panel.ts` and `controls/color-picker.ts` each + * declared their own `const MIXED = "Mixed"`, the second commented "mirrors + * `panel.ts`'s own sentinel" — and `sections/stroke.ts` was about to be the + * third. A sentinel that two modules spell independently is one rename away + * from a control that renders the literal word for a value the panel never + * produced, which is the same failure `num.ts` was written to stop. + * + * The reason it is a plain string rather than a symbol or a wrapper type is + * `panel.seed`'s, and it is worth keeping stated: "`MIXED` is a plain string, + * which is what lets it flow through the existing controls untouched — a number + * field renders it as text, a segmented group matches no option and shows + * nothing active. Both are exactly right, and neither needed a new code path." + * + * Note what this is *not* used for. A colour row does not test for this string; + * it asks `isParseableColor`, and treats anything it cannot render as mixed — + * so a keyword or an unresolved `var()` gets the same hairline. This constant is + * what a *producer* writes; consumers should keep asking the question they + * actually care about. + */ + +/** What a control shows when the values behind it disagree. */ +export const MIXED = "Mixed"; + +/** + * Do these all agree? The shared value if so, `MIXED` if not. + * + * `same` is a parameter because the right comparison depends on the property. + * Colours need `sameColor` — `getComputedStyle` hands back the legacy comma + * form while `formatColor` writes the modern space one, so `===` reports one + * colour as several. Keywords and lengths are compared as strings. + * + * An empty list has nothing to disagree about and returns the fallback, which + * is what a caller reading four longhands off an element with none wants. + */ +export function agreed( + values: readonly string[], + fallback: string, + same: (a: string, b: string) => boolean = (a, b) => a === b +): string { + const [first] = values; + if (first === undefined) { + return fallback; + } + const value = first || fallback; + return values.every((other) => same(other || fallback, value)) + ? value + : MIXED; +} diff --git a/packages/overlay/src/inspector/paint.ts b/packages/overlay/src/inspector/paint.ts index 3335bad..8c332a5 100644 --- a/packages/overlay/src/inspector/paint.ts +++ b/packages/overlay/src/inspector/paint.ts @@ -55,7 +55,8 @@ export function shadowRow( shadow: Shadow, onEdit: (next: Shadow) => void, gestures?: Gestures, - onDispose?: (fn: () => void) => void + onDispose?: (fn: () => void) => void, + node?: Element | null ): HTMLElement { let kind: EffectKind = shadow.inset ? "inner-shadow" : "drop-shadow"; @@ -208,6 +209,7 @@ export function shadowRow( ]), createColorRow({ gestures, + node, onChange: (color) => onEdit({ ...shadow, color }), tip: "Shadow colour", value: shadow.color, @@ -228,7 +230,8 @@ export function shadowRow( export function createShadowList( css: string, onChange: OnChange, - gestures?: Gestures + gestures?: Gestures, + node?: Element | null ): RowListHandle { return createRowList( { @@ -239,7 +242,7 @@ export function createShadowList( enabled: (r) => r.enabled, parse: parseShadowList, render: (row, onEdit, onDispose) => - shadowRow(row, onEdit, gestures, onDispose), + shadowRow(row, onEdit, gestures, onDispose, node), serialize: formatShadowList, setEnabled: (r, on) => ({ ...r, enabled: on }), }, @@ -262,7 +265,8 @@ export function createShadowList( export function fillLayerRow( row: Fill, onEdit: (next: Fill) => void, - gestures?: Gestures + gestures?: Gestures, + node?: Element | null ): HTMLElement { const input = el("input", { "aria-label": "Fill layer", @@ -286,6 +290,7 @@ export function fillLayerRow( } openGradientEditor(glyph, { gestures, + node, onChange: (css) => { // Keep the text field in step: it is the same value, and leaving it // stale would make the two disagree about what the fill is. @@ -324,11 +329,11 @@ export function fillLayerRow( } glyph = next; if (image) { - wrap.dataset.tip = "Image fill — CSS url()"; + wrap.dataset.tip = "Image fill, from a CSS url()"; } else { wrap.dataset.tip = editable - ? "Gradient fill — click the swatch to edit, or type CSS" - : "Gradient fill — CSS"; + ? "Gradient fill. Click the swatch to edit" + : "Gradient fill, shown as authored"; } }; setKind(row.kind); diff --git a/packages/overlay/src/inspector/sections/effects.ts b/packages/overlay/src/inspector/sections/effects.ts index e7f37de..977b560 100644 --- a/packages/overlay/src/inspector/sections/effects.ts +++ b/packages/overlay/src/inspector/sections/effects.ts @@ -23,7 +23,8 @@ export function renderEffects(ctx: SectionContext, node: Element): HTMLElement { const shadows = createShadowList( readValue(node, "box-shadow"), ctx.onChange, - ctx.gestures + ctx.gestures, + node ); /* * A shadow scale is a real token category and had no picker at all. diff --git a/packages/overlay/src/inspector/sections/fill.ts b/packages/overlay/src/inspector/sections/fill.ts index e4b0362..8f2229e 100644 --- a/packages/overlay/src/inspector/sections/fill.ts +++ b/packages/overlay/src/inspector/sections/fill.ts @@ -118,7 +118,7 @@ export function renderFill(ctx: SectionContext, node: Element): HTMLElement { // the DOM refused it. Binding a colour token whose `var()` did not resolve // blanked the computed value, and this row — the one carrying the badge that // had just been used — deleted itself. - const filled = hasFill(ctx.gate(node)); + const filled = hasFill(ctx.gate(node), node); // Held so "remove" can take just this row out, and the `+` can put one // back, without either rebuilding the panel. @@ -143,7 +143,7 @@ export function renderFill(ctx: SectionContext, node: Element): HTMLElement { parse: parseFillLayers, render: (row, onEdit, _onDispose, index) => el("div", { class: cls("fill-layer") }, [ - fillLayerRow(row, onEdit, ctx.gestures), + fillLayerRow(row, onEdit, ctx.gestures, node), // Per-layer geometry, for the layers that have any. A solid colour is // painted by `background-color` and has no size, position or repeat. ...(row.kind === "solid" ? [] : [layerGeometry(ctx, node, index)]), diff --git a/packages/overlay/src/inspector/sections/filters.ts b/packages/overlay/src/inspector/sections/filters.ts index 9ffb472..53756a3 100644 --- a/packages/overlay/src/inspector/sections/filters.ts +++ b/packages/overlay/src/inspector/sections/filters.ts @@ -235,7 +235,7 @@ function filterRow( class: cls("filter-raw"), text: row.value, }); - text.dataset.tip = "Airship does not model this filter — shown as authored"; + text.dataset.tip = "Shown as authored, not editable"; return text; } @@ -279,7 +279,7 @@ function dropShadowRow( // Unlike `box-shadow`, this one follows the element's alpha channel — it // shadows the *shape*, not the box. Worth saying, because it is the reason // to reach for it over the Effects section. - wrap.dataset.tip = "Follows the element's alpha channel, unlike a box shadow"; + wrap.dataset.tip = "Follows the shape, not the box"; /* * A header line, which this row is the only filter to have lost. @@ -329,13 +329,14 @@ function dropShadowRow( */ const color = createColorRow({ gestures, + node, onChange: (next) => { shadow.color = next; commit(); }, tip: shadow.color - ? "Shadow color" - : "Shadow color — currently the element's own `color`", + ? "Shadow colour" + : "Shadow colour, taken from the element", value: shadow.color || readValue(node, "color"), }); onDispose(color.destroy); diff --git a/packages/overlay/src/tokens/match.test.ts b/packages/overlay/src/tokens/match.test.ts index 1424d8f..ed1cebd 100644 --- a/packages/overlay/src/tokens/match.test.ts +++ b/packages/overlay/src/tokens/match.test.ts @@ -118,3 +118,78 @@ describe("hasTokensFor", () => { expect(hasTokensFor("cursor")).toBe(false); }); }); + +/* + * Colour tokens, against the value a control actually holds. + * + * The suite above only ever compared a hex token to a hex value, which is the + * one colour case that always worked — and it hid the fact that the feature did + * not. `readValue` returns *computed* style, so a control's value is + * `rgb(0, 170, 255)` while the registry is keyed on the authored `#0af`. Those + * hashed to different keys and `sameValue` compared them as strings, so no + * hex-authored colour token ever bound to a control. The badge never appeared. + * + * Two layers fix it, and both are exercised here: `normalizeTokenValue` collapses + * hex in `@airship/protocol` (pure string math, so it works on the server scan + * too), and `sameColor` catches everything needing an engine. + */ +describe("colour tokens against a computed value", () => { + beforeEach(() => { + load([token("--brand", "#0af", "colors")]); + }); + + it("matches the computed rgb() a control really holds", () => { + // The regression. Every spelling a browser or a stylesheet can produce. + for (const value of [ + "rgb(0, 170, 255)", + "rgb(0 170 255)", + "#0af", + "#00AAFF", + ]) { + expect(findToken("background-color", value)?.name, value).toBe("--brand"); + } + }); + + it("reports the match as exact, not as a near miss", () => { + // `exact` is what lets the prompt phrase it as a fact rather than a + // suggestion, and a spelling difference is not an approximation. + expect(findToken("color", "rgb(0, 170, 255)")?.exact).toBe(true); + }); + + it("matches a token an engine has to resolve", () => { + /* + * The half that has to live in the overlay. `oklch()` — Tailwind 4's entire + * default palette — and named colours are both past what + * `normalizeTokenValue` can do without a DOM, so only `sameColor` can settle + * them, and this asserts `sameValue` actually consults it. + * + * A *named* colour rather than `oklch()`, because happy-dom's CSSOM rejects + * `oklch()` at the setter, so the probe returns before it reads anything and + * the case cannot be expressed here at all. `red` takes the identical path — + * fast paths decline, `viaEngine` asks the engine — and is the same + * assertion about our side of the boundary. `oklch()` is covered for real in + * the Storybook browser tier. + * + * The engine is stubbed because no test environment normalises colours. + */ + load([token("--brand", "red", "colors")]); + const real = globalThis.getComputedStyle; + globalThis.getComputedStyle = (() => + ({ color: "rgb(255, 0, 0)" }) as CSSStyleDeclaration) as typeof real; + try { + expect(findToken("background-color", "rgb(255, 0, 0)")?.name).toBe( + "--brand" + ); + } finally { + globalThis.getComputedStyle = real; + } + }); + + it("still refuses a colour that is merely close", () => { + // The tolerance that applies to lengths must never reach colours. + expect(findToken("background-color", "rgb(0, 170, 254)")).toBeUndefined(); + expect(findToken("background-color", "rgba(0, 170, 255, 0.5)")).toBe( + undefined + ); + }); +}); diff --git a/packages/overlay/src/tokens/match.ts b/packages/overlay/src/tokens/match.ts index 0a5fac4..f44fc97 100644 --- a/packages/overlay/src/tokens/match.ts +++ b/packages/overlay/src/tokens/match.ts @@ -15,6 +15,7 @@ import { } from "@airship/protocol/tokens"; import { toPx } from "../inspector/css-length"; import { type MatchedResult, matchedRules } from "../inspector/css-rules"; +import { sameColor } from "../inspector/css-value"; import { computedStyle } from "../realm"; import { tokens, tokenValue } from "./registry"; @@ -32,11 +33,37 @@ const LENGTH_UNIT = /[a-z%]$/i; const NEAR_ABSOLUTE_PX = 2; const NEAR_RELATIVE = 0.1; -/** Are two CSS values the same, allowing for units and formatting? */ -function sameValue(a: string, b: string, node?: Element): boolean { +/** + * Are two CSS values the same, allowing for units and formatting? + * + * The colour branch is the second half of a two-layer fix, and it is here rather + * than in `normalizeTokenValue` because of what each layer can reach. + * `normalizeTokenValue` lives in `@airship/protocol`, which must not touch the + * DOM, so it can convert hex — exact string math — and nothing further. But + * `oklch()` is Tailwind 4's entire default palette, and `red` and `hsl()` are + * ordinary things to author; resolving any of those needs an engine probe, which + * only exists in the overlay. + * + * So: `normalizeTokenValue` collapses the spellings it can on both sides of the + * wire, and `sameColor` catches the rest here, where a document is available. + * Without this branch a Tailwind project's colour tokens never bind to a control. + * + * Deliberately still no *tolerance* for colours — `findToken` is right that a + * slightly different hex is a different colour, not an approximation of one. + * `sameColor` is exact; only the spelling is normalised away. + */ +function sameValue( + property: string, + a: string, + b: string, + node?: Element +): boolean { if (normalizeTokenValue(a) === normalizeTokenValue(b)) { return true; } + if (categoryForProperty(property) === "colors") { + return sameColor(a, b, node); + } const pa = toPx(a, node); const pb = toPx(b, node); return pa !== null && pb !== null && Math.abs(pa - pb) < 0.5; @@ -174,7 +201,7 @@ function findByUnitAwareEquality( node?: Element ): TokenRef | undefined { for (const token of tokensFor(property)) { - if (sameValue(tokenValue(token, property), value, node)) { + if (sameValue(property, tokenValue(token, property), value, node)) { return toRef(token, property, true); } } diff --git a/packages/protocol/src/tokens.test.ts b/packages/protocol/src/tokens.test.ts index ef0bc11..c514169 100644 --- a/packages/protocol/src/tokens.test.ts +++ b/packages/protocol/src/tokens.test.ts @@ -1,5 +1,10 @@ import { describe, expect, it } from "vitest"; -import { categorizeToken, categoryForProperty } from "./tokens"; +import { + categorizeToken, + categoryForProperty, + looksLikeColor, + normalizeTokenValue, +} from "./tokens"; /* * Which scale a custom property belongs to. @@ -148,3 +153,91 @@ describe("the values that used to become fonts", () => { expect(at("--pk-layout-breakpoint-nav", "1240px")).toBe("sizing"); }); }); + +/* + * `normalizeTokenValue` is the key both scanners hash a token's value into, and + * it did not touch hex — so the registry could not match its own tokens. A + * design system authors `--brand: #0af`; every control reads back the *computed* + * `rgb(0, 170, 255)`; the two hashed to different keys, and no hex-authored + * colour token ever bound to a control. The badge simply never appeared. + * + * Hex is the one colour conversion that belongs in this module — it is exact + * string math on 8-bit sRGB and needs no engine. Everything past it (`oklch()`, + * named colours) is `sameColor`'s job in the overlay. + */ +describe("normalizeTokenValue", () => { + it("collapses every spelling of one colour into one key", () => { + const key = "rgb(0, 170, 255)"; + for (const spelling of [ + "#0af", + "#00AAFF", + "#00aaff", + "rgb(0, 170, 255)", + "rgb(0 170 255)", + " #0AF ", + ]) { + expect(normalizeTokenValue(spelling)).toBe(key); + } + }); + + it("carries the alpha of a four- or eight-digit hex", () => { + // 0x80 / 255 = 0.502 — three decimals, matching the overlay's `formatColor`. + expect(normalizeTokenValue("#00000080")).toBe("rgb(0, 0, 0, 0.502)"); + // `#0008` expands to `#00000088`, not to `#000` with a stray alpha. + expect(normalizeTokenValue("#0008")).toBe("rgb(0, 0, 0, 0.533)"); + expect(normalizeTokenValue("#ffffffff")).toBe("rgb(255, 255, 255, 1)"); + }); + + it("agrees with the four-argument rgb() the legacy branch already emits", () => { + // The whole point is one colour, one key — so hex with alpha has to land on + // the same string as the two spellings a browser and a stylesheet produce. + expect(normalizeTokenValue("rgba(0, 0, 0, 0.5)")).toBe("rgb(0, 0, 0, 0.5)"); + expect(normalizeTokenValue("rgb(0 0 0 / 0.5)")).toBe("rgb(0, 0, 0, 0.5)"); + }); + + it("leaves alone what it cannot convert exactly", () => { + // No engine here, so these pass through for `sameColor` to settle. + expect(normalizeTokenValue("oklch(0.7 0.1 200)")).toBe( + "oklch(0.7 0.1 200)" + ); + expect(normalizeTokenValue("REBECCAPURPLE")).toBe("rebeccapurple"); + expect(normalizeTokenValue("var(--brand)")).toBe("var(--brand)"); + }); + + it("does not mistake a malformed hex for a colour", () => { + for (const bad of ["#zzz", "#12345", "#1234567", "#", "#ff"]) { + expect(normalizeTokenValue(bad)).toBe(bad.toLowerCase()); + } + }); + + it("still normalizes the non-hex forms it always did", () => { + expect(normalizeTokenValue("255 229 202")).toBe("rgb(255, 229, 202)"); + expect(normalizeTokenValue(" 8PX ")).toBe("8px"); + }); +}); + +/* + * The prefix test the overlay used to keep a byte-identical copy of, under the + * name `COLORISH`. Deliberately weak — it is a swatch gate, not a parser. + */ +describe("looksLikeColor", () => { + it("accepts hex and every colour function", () => { + for (const value of [ + "#0af", + "rgb(0 0 0)", + "rgba(0,0,0,.5)", + "hsl(0 100% 50%)", + "oklch(0.7 0.1 200)", + "lab(50% 40 59)", + "color(srgb 1 0 0)", + ]) { + expect(looksLikeColor(value)).toBe(true); + } + }); + + it("rejects what does not start like one", () => { + for (const value of ["8px", "bold", "var(--brand)", "255 229 202"]) { + expect(looksLikeColor(value)).toBe(false); + } + }); +}); diff --git a/packages/protocol/src/tokens.ts b/packages/protocol/src/tokens.ts index 5b977db..b3628e5 100644 --- a/packages/protocol/src/tokens.ts +++ b/packages/protocol/src/tokens.ts @@ -230,6 +230,29 @@ const NAME_PATTERNS: [RegExp, TokenCategory][] = [ const HEX_OR_FUNC_COLOR = /^(#|rgba?\(|hsla?\(|oklch\(|oklab\(|lab\(|lch\(|color\()/i; + +/** + * Does this value *look* like a colour? A shape test, not a parse. + * + * Deliberately weak, and only used where being wrong is cheap. It answers on the + * prefix alone — `#`, and the names of the colour functions — so `#zzz` and a + * bare `rgb(` both pass. Its two callers can afford that: `categoryFromValue`, + * picking a scale for a custom property when usage and name have both declined + * to say, and the token picker, deciding whether a menu row gets a swatch. + * + * The overlay *has* a real answer — `isParseableColor` in `css-value.ts`, which + * asks the engine — and the picker deliberately does not call it. That path + * inserts a probe element and forces a style recalc per call, and a colour scale + * routinely runs to a hundred rows; a hundred recalcs to open a menu is a worse + * bug than the occasional swatch that paints nothing. + * + * Exported because the overlay held a byte-identical copy of this regex under + * the name `COLORISH`. Two spellings of one rule in two packages is how they + * drift apart. + */ +export function looksLikeColor(value: string): boolean { + return HEX_OR_FUNC_COLOR.test(value.trim()); +} const NUMERIC_LENGTH = /^-?[\d.]+(px|r?em|%|v[hw]|ch|ex)$/; const THREE_DIGITS = /^\d{3}$/; /** Whole-value now, not a prefix — so `ui-` has to spell out what it heads. */ @@ -395,7 +418,7 @@ function categoryFromName(name: string): TokenCategory | null { function categoryFromValue(value: string): TokenCategory | null { const v = value.trim(); - if (HEX_OR_FUNC_COLOR.test(v) || isChannelTriple(v)) { + if (looksLikeColor(v) || isChannelTriple(v)) { return "colors"; } if (THREE_DIGITS.test(v)) { @@ -455,6 +478,52 @@ const LEGACY_RGB = /^rgba?\(([^)]+)\)$/; const SPACE_RGB = /^(\d{1,3})\s+(\d{1,3})\s+(\d{1,3})$/; /** Commas, slashes and whitespace all separate colour channels. */ const CHANNEL_SEPARATOR = /[,/\s]+/; +/** `#rgb`, `#rgba`, `#rrggbb`, `#rrggbbaa` — already lowercased by the caller. */ +const HEX = /^#([\da-f]{3}|[\da-f]{4}|[\da-f]{6}|[\da-f]{8})$/; + +/** + * A hex colour as `rgb()` — pure string math, no DOM, no colour science. + * + * This is the one colour conversion that belongs in this module. It is exact + * (hex *is* 8-bit sRGB, so nothing is approximated), it needs no engine, and + * without it the registry could not match its own tokens: `byValue` is keyed on + * the value as **authored**, and a design system authors `--brand: #0af` while + * every control reads back the *computed* `rgb(0, 170, 255)`. Those are the same + * colour and they hashed to two different keys, so no hex-authored colour token + * ever bound to a control. + * + * `oklch()`, named colours and `hsl()` need a real parser and cannot be done + * here — `css-value.ts` in the overlay handles those, which is why `match.ts` + * has a second, engine-backed comparison behind this one. + */ +function hexToRgb(value: string): string | null { + const matched = HEX.exec(value); + if (!matched) { + return null; + } + const [, digits] = matched; + const full = + digits.length <= 4 + ? digits + .split("") + .map((d) => d + d) + .join("") + : digits; + const channel = (i: number) => Number.parseInt(full.slice(i, i + 2), 16); + const rgb = `${channel(0)}, ${channel(2)}, ${channel(4)}`; + if (full.length === 8) { + /* + * `rgb(r, g, b, a)`, not `rgba(…)` — the four-argument `rgb(` is what the + * legacy branch below already produces for both `rgba(0, 0, 0, .5)` and the + * modern `rgb(0 0 0 / .5)`, and the whole point of this function is that one + * colour gets one key. Three decimals, matching the overlay's `formatColor`, + * so a round trip through either side lands on the same string. + */ + const alpha = Math.round((channel(6) / 255) * 1000) / 1000; + return `rgb(${rgb}, ${alpha})`; + } + return `rgb(${rgb})`; +} /** True when a bare value is three space-separated 0–255 channels. */ export function isChannelTriple(value: string): boolean { @@ -469,14 +538,27 @@ export function isChannelTriple(value: string): boolean { * registry key `byValue` through this, so a token found statically and the same * token found at runtime collapse to one entry instead of two. * - * Deliberately *not* a colour parser — it only has to be consistent, and - * anything needing real colour maths goes through `css-value.ts` in the overlay. + * Still not a colour parser, with one exception that earns its place. Hex is + * converted, because hex *is* 8-bit sRGB — the conversion is exact string math, + * needs no engine, and without it this function could not do the job named in + * the paragraph above: a design system authors `--brand: #0af`, every control + * reads back the computed `rgb(0, 170, 255)`, and the two hashed to different + * keys, so no hex-authored colour token ever matched anything. + * + * Everything past hex — `oklch()`, named colours, `hsl()`, `color-mix()` — needs + * a real parser and an engine probe, which this module cannot have and must not + * grow. Those go through `css-value.ts`'s `sameColor` in the overlay, which + * `tokens/match.ts` consults as a second pass behind this one. */ export function normalizeTokenValue(value: string): string { const trimmed = value.trim().toLowerCase().replace(SPACE_RUN, " "); if (isChannelTriple(trimmed)) { return `rgb(${trimmed.replace(SPACE_RUN, ", ")})`; } + const hex = hexToRgb(trimmed); + if (hex) { + return hex; + } const legacy = LEGACY_RGB.exec(trimmed); if (legacy) { // `rgb(0,0,0)` and `rgb(0 0 0)` are the same colour; make them one key.