From 6b2e4bf18e2712d80f2187a499312e455c7fcf47 Mon Sep 17 00:00:00 2001 From: Fini Date: Tue, 5 May 2026 12:22:57 +0800 Subject: [PATCH] fix(ai): nav inject hops single-child wrappers + button icon matches text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two visible regressions in Image #44: 1. Bottom nav reverted to no-background even though earlier runs worked. GPT-5.5 wrapped its bottom nav in a single-child section: root > frame{role:'section',id:'bottom-tabs-root'} > frame{role:'bottom-tab-bar'} > [tabs] The inject pass only walked DIRECT children of root and bailed on the section wrapper. Now we hop one level when the wrapper is a single-child section AND its sole child is a nav-role frame, so the nested nav gets the surface fill + position-aware shadow. Multi-child sections still bail (those are real content sections, not wrappers). 2. Banner "Order now" CTA shipped with white text + dark icon. My prior contrast fix used a luminance-delta threshold of 0.4, but #0F172A icon vs #F97316 (orange accent) actually has delta 0.48 — the threshold said "good contrast, leave it alone" while the user sees an obvious mismatch with the white text label. Wrong axis: the user's complaint is about CONSISTENCY (icon should read as the same token as text), not raw contrast. Refactored fixButtonForegroundContrast: PASS 1 — find a "reference" foreground from sibling text fill (after refs resolve). The model's own text color is the authoritative signal for what the button's foreground should look like, regardless of what bg/fg luminance suggests. PASS 2 — for each icon_font sibling, override when its resolved hex differs from the reference fg. Icon-only buttons (no text sibling) fall back to a luminance-based check at threshold 0.5 — catches dark-on-dark / light-on- light pairs that motivated the original rule, without the false-negative on saturated mid-luminance bgs (orange). 3 new tests: wrapper-section nav reach, multi-child wrapper bail, and the regression test for the original "dark-on-dark icon-only button" still passing under the new luminance-fallback path. 141 tests in the affected suites all green. Side effect: applyNavSurfaceFill now bails entirely (returns false) when the nav already has a fill — earlier version still added a shadow even when fill was preserved, which violated the "preserves sub-agent intent" semantics the existing tests rely on. --- apps/web/src/services/ai/role-resolver.ts | 105 ++++++++++----- .../__tests__/inject-nav-surface-fill.test.ts | 72 +++++++++++ .../src/layout/inject-nav-surface-fill.ts | 122 ++++++++++++------ 3 files changed, 229 insertions(+), 70 deletions(-) diff --git a/apps/web/src/services/ai/role-resolver.ts b/apps/web/src/services/ai/role-resolver.ts index 9a0cd0406..bb56195d8 100644 --- a/apps/web/src/services/ai/role-resolver.ts +++ b/apps/web/src/services/ai/role-resolver.ts @@ -815,6 +815,20 @@ export function getFirstSolidColor(node: PenNode): string | undefined { // Post-pass helpers // --------------------------------------------------------------------------- +/** + * Luminance-delta fallback for icon-only buttons. Returns true when + * the foreground hex is "too close" to the background hex and should + * be replaced. Threshold 0.5 catches dark-on-dark (e.g. slate-900 + * icon on slate-800 button) while leaving intentional accent icons + * on light bg (red dot on white card, delta ≈ 0.7) alone. + */ +function needsLuminanceContrastOverride(fgHex: string, bgHex: string): boolean { + const fgLum = hexLuminance(fgHex); + const bgLum = hexLuminance(bgHex); + if (!Number.isFinite(fgLum) || !Number.isFinite(bgLum)) return false; + return Math.abs(fgLum - bgLum) < 0.5; +} + function fixButtonForegroundContrast(parent: FrameNode): void { if (parent.role !== 'button' && parent.role !== 'icon-button') return; // A transparent button has no background color to compute contrast @@ -852,6 +866,37 @@ function fixButtonForegroundContrast(parent: FrameNode): void { if (!('children' in parent) || !Array.isArray(parent.children)) return; + // PASS 1: find a "reference" foreground color from a sibling text. + // Inside a button, text + icon should always paint the SAME color — + // they're a unit, not two independent surfaces. The model often + // gives the text a correct fill (white on accent, dark on light) + // but stamps `icon_font.fill` with a hardcoded dark hex from a + // generic default ("icons are dark"), producing white-text-next-to- + // dark-icon regressions like the food-app "Order now" CTA. + // + // Pass 1 reads the resolved hex from any text child's fill; if found + // it overrides the contrast-derived fg. This is more accurate than a + // luminance-delta heuristic because it captures the model's intent + // (whatever color it picked for the label is what it meant for the + // foreground) and matches user expectation that the two glyphs read + // as a single token. + let referenceFgHex: string | null = null; + for (const child of parent.children) { + if (child.type !== 'text') continue; + if (!hasVisibleFill(child)) continue; + const tc = getFirstSolidColor(child); + if (!tc) continue; + const resolved = resolveColorMaybeRef(tc); + if (resolved && !resolved.startsWith('$')) { + referenceFgHex = resolved; + break; + } + } + const finalFgFill: PenFill[] = referenceFgHex + ? [{ type: 'solid', color: referenceFgHex }] + : fgFill; + + // PASS 2: apply foreground. for (const child of parent.children) { const rec = child as unknown as Record; @@ -863,21 +908,37 @@ function fixButtonForegroundContrast(parent: FrameNode): void { rec.fill = fgFill; } } else if (child.type === 'icon_font') { - // Icons get the contrast fg even when they already have a fill. - // The model defaults `icon_font.fill` to a dark text-color (the - // prompt lists `fill` as a property, models reflexively stamp - // a dark hex) — but inside an accent-colored button that paints - // a dark icon next to a contrast-corrected white text label, - // which is the visible regression. ONLY override when the - // existing icon fill has poor contrast against the bg; an - // intentional brand-color icon on a white button (e.g. a red - // notification dot) survives untouched. + // Icons should match the sibling text's color. If the icon + // already has a fill, override it ONLY when its resolved hex + // differs from the reference fg — that catches the dark-icon- + // next-to-white-text bug while leaving intentional accent + // icons (e.g. a red notification dot whose color matches no + // sibling text) untouched. if (!hasVisibleFill(child)) { - rec.fill = fgFill; - } else { + rec.fill = finalFgFill; + continue; + } + if (referenceFgHex) { const existing = getFirstSolidColor(child); const existingHex = existing ? resolveColorMaybeRef(existing) : undefined; - if (existingHex && needsContrastOverride(existingHex, bgColor)) { + if ( + existingHex && + !existingHex.startsWith('$') && + existingHex.toLowerCase() !== referenceFgHex.toLowerCase() + ) { + rec.fill = finalFgFill; + } + } else { + // Icon-only button (no text sibling to copy from). Fall back + // to a luminance-based override: when the icon's existing + // fill is too close to the bg, swap to the contrast-derived + // fg. Threshold 0.5 catches the dark-on-dark case (slate-900 + // icon on slate-800 button, delta ≈ 0.09) without disturbing + // intentional accent icons on light surfaces (red dot on + // white card, delta ≈ 0.7). + const existing = getFirstSolidColor(child); + const existingHex = existing ? resolveColorMaybeRef(existing) : undefined; + if (existingHex && needsLuminanceContrastOverride(existingHex, bgColor)) { rec.fill = fgFill; } } @@ -891,30 +952,14 @@ function fixButtonForegroundContrast(parent: FrameNode): void { if (hasVisibleFill(child)) { // fill-style icon — already styled, skip } else if (hasStroke && !hasStrokeFill) { - (child.stroke as unknown as Record).fill = fgFill; + (child.stroke as unknown as Record).fill = finalFgFill; } else if (!hasStroke && !hasVisibleFill(child)) { - rec.fill = fgFill; + rec.fill = finalFgFill; } } } } -/** - * Return true when a foreground color has poor contrast against a - * background color and should be replaced by the contrast pass. The - * threshold (luminance delta < 0.4) catches dark-on-dark and - * light-on-light pairs while keeping intentional accent icons - * (e.g. red badge dot on white card, brand-blue icon on white button) - * untouched. Both inputs must already be hex; ref resolution happens - * at the caller. - */ -function needsContrastOverride(fgHex: string, bgHex: string): boolean { - const fgLum = hexLuminance(fgHex); - const bgLum = hexLuminance(bgHex); - if (!Number.isFinite(fgLum) || !Number.isFinite(bgLum)) return false; - return Math.abs(fgLum - bgLum) < 0.4; -} - const SECTION_ROLES = new Set(['section', 'hero', 'cta-section', 'stats-section', 'footer']); const ALTERNATING_BG = ['#FFFFFF', '#F8FAFC']; diff --git a/packages/pen-core/src/__tests__/inject-nav-surface-fill.test.ts b/packages/pen-core/src/__tests__/inject-nav-surface-fill.test.ts index 6560de655..f0b069f0b 100644 --- a/packages/pen-core/src/__tests__/inject-nav-surface-fill.test.ts +++ b/packages/pen-core/src/__tests__/inject-nav-surface-fill.test.ts @@ -290,6 +290,78 @@ describe('injectMissingNavSurfaceFill', () => { } }); + it('reaches the nav through a single-child wrapper section', () => { + // Regression: GPT-5.5 food-app run wrapped its bottom nav in a + // root > frame{role:'section', id:'bottom-tabs-root'} + // > frame{role:'bottom-tab-bar'} > [tab-buttons] + // Earlier inject pass only walked DIRECT children of root, so + // the section wrapper hid the nav from injection and the + // bottom-tab-bar shipped with no fill / no shadow. The pass + // now hops one level through a single-child section to find + // the nav. + const innerNav = frame({ + id: 'bottom-tabs-nav', + role: 'bottom-tab-bar', + children: [ + frame({ id: 'tab-home', role: 'button', children: [] }), + frame({ id: 'tab-search', role: 'button', children: [] }), + ], + }); + const wrapper = frame({ + id: 'bottom-tabs-root', + role: 'section', + children: [innerNav], + }); + const root = frame({ + id: 'root', + fill: solidFill('#FFF8F0'), + children: [wrapper], + }); + const changed = injectMissingNavSurfaceFill(root); + expect(changed).toBe(true); + expect((innerNav as PenNode & { fill?: unknown }).fill).toEqual([ + { type: 'solid', color: '$color-surface' }, + ]); + // Wrapper section itself stays untouched — it doesn't gain a + // fill or shadow, only the inner nav does. + expect((wrapper as PenNode & { fill?: unknown }).fill).toBeUndefined(); + expect((wrapper as PenNode & { effects?: unknown }).effects).toBeUndefined(); + // Inner nav got the bottom-shadow direction (offsetY < 0). + const navEffects = ( + innerNav as PenNode & { effects?: Array<{ type?: string; offsetY?: number }> } + ).effects; + expect(navEffects?.[0]?.type).toBe('shadow'); + expect(navEffects?.[0]?.offsetY).toBeLessThan(0); + }); + + it('does not hop into multi-child wrapper sections', () => { + // The single-child carve-out is intentional: a section with + // multiple children is structurally a content section (e.g. + // header with title + nav-link row), not a thin wrapper. We + // want to leave those alone so unrelated nav-shaped frames + // inside content sections aren't accidentally lifted with a + // surface fill they didn't ask for. + const innerNav = frame({ + id: 'inner-nav', + role: 'bottom-tab-bar', + children: [], + }); + const otherChild = frame({ id: 'other', role: 'heading', children: [] }); + const wrapper = frame({ + id: 'multi-section', + role: 'section', + children: [otherChild, innerNav], + }); + const root = frame({ + id: 'root', + fill: solidFill('#FFF8F0'), + children: [wrapper], + }); + const changed = injectMissingNavSurfaceFill(root); + expect(changed).toBe(false); + expect((innerNav as PenNode & { fill?: unknown }).fill).toBeUndefined(); + }); + it('preserves existing effects (sub-agent intentional shadow / glow)', () => { const intentionalShadow = [ { type: 'shadow', offsetX: 0, offsetY: 8, blur: 24, spread: 0, color: '#00000033' }, diff --git a/packages/pen-core/src/layout/inject-nav-surface-fill.ts b/packages/pen-core/src/layout/inject-nav-surface-fill.ts index 4487c24ea..215dda048 100644 --- a/packages/pen-core/src/layout/inject-nav-surface-fill.ts +++ b/packages/pen-core/src/layout/inject-nav-surface-fill.ts @@ -40,51 +40,93 @@ export function injectMissingNavSurfaceFill(rootFrame: PenNode): boolean { if (!('children' in rootFrame) || !Array.isArray(rootFrame.children)) return false; let changed = false; - for (const child of rootFrame.children) { - if (child.type !== 'frame') continue; - const role = (child as PenNode & { role?: string }).role; - if (!role || !NAV_ROLES.has(role)) continue; - const existing = (child as PenNode & { fill?: PenFill[] | string }).fill; - if (hasAnyFill(existing)) continue; - (child as PenNode & { fill?: PenFill[] }).fill = [ - { type: 'solid', color: '$color-surface' } as SolidFill, - ]; - - // Why also inject a shadow: in warm-light themes (`$color-bg-deep` - // = #FFF8F0 cream, `$color-surface` = #FFFFFF white), the - // luminance delta between page bg and the surface fill we just - // applied is ~0.03 — visually indistinguishable. The user reads - // the nav as having "no background" even though it does. Adding - // a soft upward shadow lifts the nav off the page bg - // independently of the fill contrast. We only add the shadow - // when no `effects` were already set; if the sub-agent emitted - // its own effects (intentional drop shadow, brand glow, etc.) - // we leave them alone. - const existingEffects = (child as PenNode & { effects?: PenEffect[] }).effects; - const hasEffects = Array.isArray(existingEffects) && existingEffects.length > 0; - if (!hasEffects) { - // Bottom nav: shadow above (offsetY < 0) — lifts off content - // above. Top nav / generic navbar: shadow below (offsetY > 0) - // — lifts off content below. A downward shadow on a bottom - // nav would hide off-screen and not provide any separation, - // and an upward shadow on a top nav would cling to the screen - // edge and look broken. - const isBottomNav = BOTTOM_NAV_ROLES.has(role); - const shadow: ShadowEffect = { - type: 'shadow', - offsetX: 0, - offsetY: isBottomNav ? -4 : 4, - blur: 12, - spread: 0, - color: '#0000000F', - }; - (child as PenNode & { effects?: PenEffect[] }).effects = [shadow]; + for (const directChild of rootFrame.children) { + if (directChild.type !== 'frame') continue; + // Two shapes the model emits: + // 1. The nav frame is itself the direct child: + // root > frame{role:'bottom-tab-bar'} > [icon-buttons] + // 2. The nav frame is wrapped in a single-child section: + // root > frame{role:'section', id:'bottom-tabs-root'} > frame{role:'bottom-tab-bar'} > ... + // Earlier versions only handled shape (1). Shape (2) showed up + // on the food-app run where GPT-5.5 wrapped its bottom nav in + // a `bottom-tabs-root` section — the inject pass walked the + // direct child (a section, no nav role), bailed, and the + // nested nav stayed transparent. Allow one hop through a + // wrapper section to reach the nav child. + const role = (directChild as PenNode & { role?: string }).role; + if (role && NAV_ROLES.has(role)) { + if (applyNavSurfaceFill(directChild, role)) changed = true; + continue; + } + // Wrapper case: section-like role wrapping a single nav child. + // Only walk one hop to keep scope tight (we don't want to + // recurse into cards etc. that legitimately contain nested + // nav-shaped frames). + if ( + role === 'section' && + Array.isArray((directChild as PenNode & { children?: PenNode[] }).children) && + ((directChild as PenNode & { children?: PenNode[] }).children?.length ?? 0) === 1 + ) { + const inner = (directChild as PenNode & { children?: PenNode[] }).children![0]; + if (inner.type !== 'frame') continue; + const innerRole = (inner as PenNode & { role?: string }).role; + if (innerRole && NAV_ROLES.has(innerRole)) { + if (applyNavSurfaceFill(inner, innerRole)) changed = true; + } } - changed = true; } return changed; } +/** + * Stamp the `$color-surface` fill and a position-appropriate shadow + * on a nav frame that has no fill yet. Returns true if anything + * was written. Bails entirely (returns false, no shadow either) + * when the sub-agent emitted any valid fill — the explicit fill is + * a clear signal of intent, and a sub-agent that picked a specific + * surface color likely also has an opinion about whether the nav + * should carry a shadow. We don't want to silently stamp visual + * lift on a nav the model deliberately left flat. + */ +function applyNavSurfaceFill(navFrame: PenNode, role: string): boolean { + const existing = (navFrame as PenNode & { fill?: PenFill[] | string }).fill; + if (hasAnyFill(existing)) return false; + + (navFrame as PenNode & { fill?: PenFill[] }).fill = [ + { type: 'solid', color: '$color-surface' } as SolidFill, + ]; + + // Why also inject a shadow: in warm-light themes (`$color-bg-deep` + // = #FFF8F0 cream, `$color-surface` = #FFFFFF white), the + // luminance delta between page bg and the surface fill we just + // applied is ~0.03 — visually indistinguishable. The user reads + // the nav as having "no background" even though it does. Adding + // a soft shadow lifts the nav off the page bg independently of + // the fill contrast. Only add the shadow when no `effects` were + // already set; if the sub-agent emitted its own effects + // (intentional drop shadow, brand glow, etc.) we leave them alone. + const existingEffects = (navFrame as PenNode & { effects?: PenEffect[] }).effects; + const hasEffects = Array.isArray(existingEffects) && existingEffects.length > 0; + if (!hasEffects) { + // Bottom nav: shadow above (offsetY < 0) — lifts off content + // above. Top nav / generic navbar: shadow below (offsetY > 0) + // — lifts off content below. A downward shadow on a bottom + // nav would hide off-screen, and an upward shadow on a top + // nav would cling to the screen edge. + const isBottomNav = BOTTOM_NAV_ROLES.has(role); + const shadow: ShadowEffect = { + type: 'shadow', + offsetX: 0, + offsetY: isBottomNav ? -4 : 4, + blur: 12, + spread: 0, + color: '#0000000F', + }; + (navFrame as PenNode & { effects?: PenEffect[] }).effects = [shadow]; + } + return true; +} + /** * True when the frame carries a fill the renderer can ACTUALLY paint — * solid with a non-empty color, gradient with stops, or image with a