fix(ai): nav inject hops single-child wrappers + button icon matches text
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.
This commit is contained in:
Fini
parent
c6d47dd689
commit
6b2e4bf18e
|
|
@ -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<string, unknown>;
|
||||
|
||||
|
|
@ -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<string, unknown>).fill = fgFill;
|
||||
(child.stroke as unknown as Record<string, unknown>).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'];
|
||||
|
||||
|
|
|
|||
|
|
@ -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' },
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in a new issue