vndangkhoa/openpencil
1
0
Fork 0
mirror of https://github.com/ZSeven-W/openpencil.git synced 2026-05-31 19:04:29 +07:00
Code Issues Projects Releases Packages Wiki Activity Actions 2

fix(ai): split hasFill into two predicates so transparent fills are respected

Browse source 
Follow-up on b7639da. Widening hasFill to treat transparent fills as
"no fill" fixed the button foreground contrast case but broke the
opposite group of callers — heuristics that rely on hasFill for
overwrite PROTECTION:

  - fixOrphanContainerContrast: wrapped "if hasFill(node) return" around
    the white-fill-and-shadow assignment. With the new hasFill, any
    card whose author chose fill=#00000000 would lose its transparency
    and get painted white with drop shadows.
  - fixSectionAlternation: filtered sections by \!hasFill to decide which
    ones get the alternating background. Explicit transparent sections
    would wrongly enter that pool and get repainted.
  - fixInputSiblingConsistency: filtered "inputs with fill" — a
    transparent input would be silently excluded from sibling
    consistency alignment.

These two use-cases have opposing needs:

  overwrite-protection callers
    "Has the AI declared ANY fill entry, visible or not? If so, don't
    touch it — transparent is a deliberate choice too."
  contrast-paint callers
    "Will this draw a visible color? If not, I need to paint one in."

Split the predicate in two:

  hasFill(node)
    Restored to the original semantics — any non-empty `fill` array
    counts. Used by fixOrphanContainerContrast, fixSectionAlternation,
    fixInputSiblingConsistency, and the `fixButtonForegroundContrast`
    parent guard is UPGRADED to hasVisibleFill below (a transparent
    button has no bg to compute contrast against anyway).

  hasVisibleFill(node)  [new]
    Rejects fully-transparent solid fills (#00000000, any #RRGGBB00,
    "transparent", "none"). Used by fixButtonForegroundContrast to
    decide "does this child need a color painted in?" and on the
    button parent itself to skip transparent buttons.

Rewired fixButtonForegroundContrast to hasVisibleFill at all check
sites (parent + three child branches). All other callers keep the
original hasFill and are unchanged.

Regression coverage:
  - hasFill: added explicit "returns TRUE for transparent hex / CSS
    keyword" tests so a future "helpful" refactor can't re-widen it.
  - hasVisibleFill: 8 new tests covering every transparent form and a
    partially-transparent sanity case (#FF000080 → true).
  - fixOrphanContainerContrast: new case asserting a transparent card
    keeps its fill and gains no shadow.
  - fixSectionAlternation: new case asserting a transparent hero
    section survives alternation unchanged.

apps/web role-resolver suite is now 94 committed tests, all green.
This commit is contained in:
Fini 2026-04-06 20:20:05 +08:00
parent b7639dadde
commit e37fb2e444
2 changed files with 161 additions and 26 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

131
apps/web/src/services/ai/__tests__/role-resolver.test.ts
View file

@ -8,7 +8,7 @@ vi.mock('@/canvas/canvas-text-measure', () => ({
hasCjkText: () => false,
}));
import { hexLuminance, hasFill, resolveNodeRole, resolveTreePostPass } from '../role-resolver';
import { hexLuminance, hasFill, hasVisibleFill, resolveNodeRole, resolveTreePostPass } from '../role-resolver';
import type { RoleContext } from '../role-resolver';
import type { PenNode } from '@zseven-w/pen-types';
@ -48,6 +48,11 @@ describe('hexLuminance', () => {
});
describe('hasFill', () => {
// hasFill answers "has the AI declared any fill entry?" — it's used by
// post-pass heuristics that must NOT overwrite an explicitly-chosen
// fill, even if that fill is transparent. Use `hasVisibleFill` when
// you need "will this draw a visible color?".
it('returns false for node without fill', () => {
const node = { id: 'n1', type: 'frame', x: 0, y: 0, width: 100, height: 100 } as PenNode;
expect(hasFill(node)).toBe(false);
@ -66,17 +71,57 @@ describe('hasFill', () => {
expect(hasFill(node)).toBe(true);
});
// Regression: button foreground contrast relies on hasFill to detect
// "no visible fill here, please supply one". A path icon whose fill
// was normalized to the explicit-transparent hex #00000000 (to
// preserve hollow-shape intent on activity rings / chart lines) must
// still be reported as "no fill" so the contrast pass can paint it.
// hasFill must report transparent fills as "has fill" so the overwrite-
// protection callers (fixOrphanContainerContrast, fixSectionAlternation)
// leave them alone. A frame whose AI author explicitly set
// fill=#00000000 is making a deliberate no-background choice.
it('returns true for explicit-transparent hex (#00000000) — overwrite protection', () => {
const node = {
id: 'n', type: 'frame', x: 0, y: 0, width: 100, height: 100,
fill: [{ type: 'solid', color: '#00000000' }],
} as PenNode;
expect(hasFill(node)).toBe(true);
});
it('returns true for CSS keyword "transparent"', () => {
const node = {
id: 'n', type: 'frame', x: 0, y: 0, width: 100, height: 100,
fill: [{ type: 'solid', color: 'transparent' }],
} as PenNode;
expect(hasFill(node)).toBe(true);
});
});
describe('hasVisibleFill', () => {
// hasVisibleFill answers "will this draw a visible color on screen?".
// Used by the button foreground contrast pass to decide whether a
// child node needs a color supplied. Transparent fills must report
// as false so contrast can paint in a visible foreground color.
it('returns false for node without fill', () => {
const node = { id: 'n', type: 'frame', x: 0, y: 0, width: 100, height: 100 } as PenNode;
expect(hasVisibleFill(node)).toBe(false);
});
it('returns false for empty fill array', () => {
const node = { id: 'n', type: 'frame', x: 0, y: 0, width: 100, height: 100, fill: [] } as PenNode;
expect(hasVisibleFill(node)).toBe(false);
});
it('returns true for node with opaque solid fill', () => {
const node = {
id: 'n', type: 'frame', x: 0, y: 0, width: 100, height: 100,
fill: [{ type: 'solid', color: '#FFFFFF' }],
} as PenNode;
expect(hasVisibleFill(node)).toBe(true);
});
it('returns false for 8-digit transparent hex (#00000000)', () => {
const node = {
id: 'p', type: 'path', x: 0, y: 0, width: 24, height: 24,
fill: [{ type: 'solid', color: '#00000000' }],
} as PenNode;
expect(hasFill(node)).toBe(false);
expect(hasVisibleFill(node)).toBe(false);
});
it('returns false for any 8-digit hex with 00 alpha', () => {
@ -84,7 +129,7 @@ describe('hasFill', () => {
id: 'p', type: 'path', x: 0, y: 0, width: 24, height: 24,
fill: [{ type: 'solid', color: '#FF00FF00' }],
} as PenNode;
expect(hasFill(node)).toBe(false);
expect(hasVisibleFill(node)).toBe(false);
});
it('returns false for CSS keyword "transparent"', () => {
@ -92,7 +137,7 @@ describe('hasFill', () => {
id: 'p', type: 'path', x: 0, y: 0, width: 24, height: 24,
fill: [{ type: 'solid', color: 'transparent' }],
} as PenNode;
expect(hasFill(node)).toBe(false);
expect(hasVisibleFill(node)).toBe(false);
});
it('returns false for CSS keyword "none"', () => {
@ -100,7 +145,7 @@ describe('hasFill', () => {
id: 'p', type: 'path', x: 0, y: 0, width: 24, height: 24,
fill: [{ type: 'solid', color: 'none' }],
} as PenNode;
expect(hasFill(node)).toBe(false);
expect(hasVisibleFill(node)).toBe(false);
});
it('returns true for a partially-transparent 8-digit hex (non-zero alpha)', () => {
@ -108,7 +153,71 @@ describe('hasFill', () => {
id: 'n', type: 'frame', x: 0, y: 0, width: 100, height: 100,
fill: [{ type: 'solid', color: '#FF000080' }],
} as PenNode;
expect(hasFill(node)).toBe(true);
expect(hasVisibleFill(node)).toBe(true);
});
});
describe('resolveTreePostPass — transparent fill overwrite protection', () => {
// Regression: the post-pass heuristics that DON'T want to overwrite an
// explicit fill choice must respect a transparent fill the same way
// they respect any other color. Only the button contrast pass needs
// to treat transparent as "no visible fill".
it('fixOrphanContainerContrast does NOT overwrite a card with explicit transparent fill', () => {
// An author wrote a card with cornerRadius + children but chose a
// transparent background intentionally. The orphan-contrast pass
// must not suddenly paint it white and add shadows.
const card: PenNode = {
id: 'card', type: 'frame', name: 'Hollow Card', x: 0, y: 0, width: 300, height: 200,
cornerRadius: 12,
fill: [{ type: 'solid', color: '#00000000' }],
children: [
{ id: 'txt', type: 'text', name: 'Title', x: 0, y: 0, width: 200, height: 20, content: 'Hello' } as PenNode,
],
} as PenNode;
const root: PenNode = {
id: 'root', type: 'frame', name: 'Root', x: 0, y: 0, width: 1200, height: 800,
children: [card],
} as PenNode;
resolveTreePostPass(root, 1200);
// Fill is exactly what the author set — not #FFFFFF.
expect((card as any).fill).toEqual([{ type: 'solid', color: '#00000000' }]);
// No shadow added.
expect((card as any).effects).toBeUndefined();
});
it('fixSectionAlternation does NOT repaint a section with explicit transparent fill', () => {
const children = [
{
id: 's0', type: 'frame' as const, name: 'Hero',
x: 0, y: 0, width: 1200, height: 400,
role: 'hero', layout: 'vertical' as const, children: [],
fill: [{ type: 'solid', color: '#00000000' }],
},
{
id: 's1', type: 'frame' as const, name: 'Features',
x: 0, y: 0, width: 1200, height: 400,
role: 'section', layout: 'vertical' as const, children: [],
},
{
id: 's2', type: 'frame' as const, name: 'CTA',
x: 0, y: 0, width: 1200, height: 400,
role: 'cta-section', layout: 'vertical' as const, children: [],
},
{
id: 's3', type: 'frame' as const, name: 'Footer',
x: 0, y: 0, width: 1200, height: 400,
role: 'footer', layout: 'vertical' as const, children: [],
},
] as PenNode[];
const root: PenNode = {
id: 'root', type: 'frame', name: 'Root', x: 0, y: 0, width: 1200, height: 1600,
layout: 'vertical', children,
} as PenNode;
resolveTreePostPass(root, 1200);
// The transparent hero stays transparent — alternation does not
// overwrite an explicit fill of any color.
expect((children[0] as any).fill).toEqual([{ type: 'solid', color: '#00000000' }]);
});
});

56
apps/web/src/services/ai/role-resolver.ts
View file

@ -483,20 +483,40 @@ export function hexLuminance(hex: string): number {
* Does NOT distinguish AI-explicit from role-default fills.
*/
/**
* Returns true when a node has a VISIBLE fill.
* Returns true when a node has ANY declared fill entry, visible or not.
*
* The subtle bit is the "visible" qualifier: an explicit-transparent hex
* (`#00000000`), a fully-transparent 8-digit hex (any `#RRGGBB00`), or a
* CSS keyword (`"transparent"` / `"none"`) all satisfy the naive
* `fill.length > 0` test but draw nothing. Downstream heuristics like
* `fixButtonForegroundContrast` ask this function "does this path icon
* already have a color?" expecting a truthful answer. A normalized
* transparent fill (the placeholder the stroke/fill schema normalizer
* leaves on hollow shapes to stop the renderer from falling back to
* the default gray) must be reported as "no visible fill" so the
* contrast pass can paint in a foreground color.
* This is the "overwrite protection" predicate: heuristics like
* `fixOrphanContainerContrast` and `fixSectionAlternation` ask this to
* decide whether the author has already made a deliberate fill choice
* that they should respect. An explicit transparent fill
* (`#00000000`, `"transparent"`, `"none"`) IS a deliberate choice —
* "I want this container to be see-through" — and must be preserved,
* not swapped for a default white background.
*
* When you instead need to know "will this draw a visible color on
* screen?" (e.g. to decide whether the button foreground contrast
* pass needs to paint in a readable color), use `hasVisibleFill`
* instead.
*/
export function hasFill(node: PenNode): boolean {
return 'fill' in node && Array.isArray(node.fill) && node.fill.length > 0;
}
/**
* Returns true when a node has a fill that will actually render a
* visible color. Differs from `hasFill` by rejecting fully-transparent
* solid fills:
* - `#00000000` and any 8-digit hex with `00` alpha
* - CSS keyword `"transparent"`
* - CSS keyword `"none"` (not a legal PenFill color but the
* stroke/fill normalizer may leave it in mixed arrays)
*
* Use this when deciding whether a node needs a color PAINTED ONTO it
* (button foreground contrast, focus ring supply, etc.). Do NOT use
* this to decide whether to overwrite an author's fill choice —
* transparent is a legitimate choice. See `hasFill` for that case.
*/
export function hasVisibleFill(node: PenNode): boolean {
if (!('fill' in node) || !Array.isArray(node.fill) || node.fill.length === 0) return false;
const first = node.fill[0];
if (!first) return false;
@ -531,7 +551,10 @@ export function getFirstSolidColor(node: PenNode): string | undefined {
function fixButtonForegroundContrast(parent: FrameNode): void {
if (parent.role !== 'button' && parent.role !== 'icon-button') return;
if (!hasFill(parent)) return;
// A transparent button has no background color to compute contrast
// against — nothing to do, and we definitely should not paint text
// white on an invisible button.
if (!hasVisibleFill(parent)) return;
const bgColor = getFirstSolidColor(parent);
if (!bgColor) return;
@ -546,7 +569,10 @@ function fixButtonForegroundContrast(parent: FrameNode): void {
const rec = child as unknown as Record<string, unknown>;
if (child.type === 'text' || child.type === 'icon_font') {
if (!hasFill(child)) {
// `hasVisibleFill` treats transparent-hex placeholder fills as
// unfilled, so the normalizer's #00000000 leftover does not
// block contrast from supplying a visible color.
if (!hasVisibleFill(child)) {
rec.fill = fgFill;
}
} else if (child.type === 'path') {
@ -556,11 +582,11 @@ function fixButtonForegroundContrast(parent: FrameNode): void {
Array.isArray((child.stroke as PenStroke)?.fill) &&
(child.stroke as PenStroke).fill!.length > 0;
if (hasFill(child)) {
if (hasVisibleFill(child)) {
// fill-style icon — already styled, skip
} else if (hasStroke && !hasStrokeFill) {
(child.stroke as unknown as Record<string, unknown>).fill = fgFill;
} else if (!hasStroke && !hasFill(child)) {
} else if (!hasStroke && !hasVisibleFill(child)) {
rec.fill = fgFill;
}
}