edgevoid/vue-tui
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(runtime): margin/padding edge removal falls back to the surviving shorthand (#184)

Browse Source 
* fix(runtime): margin/padding edge removal falls back to the surviving shorthand

Withdrawing a per-edge/axis margin or padding override from a box that still
has a broader shorthand collapsed the edge to 0 instead of falling back. E.g.
`margin={5} marginTop={8}` with marginTop later removed: the setter ran
setMargin(EDGE_TOP, 0), and per yoga edge precedence EDGE_TOP=0 overrides the
surviving EDGE_ALL=5, so the top margin became 0 (the box jumps 5 cells) when
the declarative model (render = f(current props), current = {margin:5}) says 5.
A single per-prop yoga setter can't reconcile an edge that depends on the
specific edge + axis + all-edges shorthand together.

Fix mirrors the existing reconcileBorderEdges pattern: the 14 margin/padding
setters become no-ops, and reconcileMarginEdges/reconcilePaddingEdges recompute
all four physical edges from the box's full el.props with most-specific-wins
precedence (top = marginTop ?? marginY ?? margin ?? 0, ...), zeroing the
composite edges so nothing layers on top. A present-but-non-finite value
(NaN/Infinity) or a withdrawn prop falls THROUGH to the next precedence level,
preserving yoga's prior setMargin(NaN)->fallback behavior; an explicit 0 is
finite and still overrides. margin keeps EDGE_START/END and padding keeps
EDGE_LEFT/RIGHT for left/right, matching the prior setters.

Verified against real yoga-layout@3.2.1 that the SET path produces identical
computed edges as the old per-setter code (no layout regression) across all
combinations and patch orders, and the correct fallback on removal.

This is NOT an Ink-parity item: run against Ink v7.0.4, Ink and pre-fix vue-tui
both collapse to 0 (the identical bug). The fix diverges from Ink by being
declaratively correct under the already-documented G19 reset principle;
recorded in ink-divergences.md alongside the display / flexDirection entries.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(runtime): pin margin/padding spacing to a finite-number contract (Codex review)

The family recompute resolves an edge from a prop only when it is a finite
number (matching the `number` prop type + Ink's number-only spacing); numeric
strings (`margin="5"`) are coerced for Vue static-template ergonomics, but other
non-numeric values (`"50%"`, junk, `""`) are treated as not-set and fall through
to the surviving shorthand instead of being forwarded to yoga.

This makes intentional the behavior change the final review flagged: the OLD
per-setter code incidentally forwarded off-contract strings to yoga (so
`marginTop="50%"` became a percent and `marginTop="foo"` threw). That was
undocumented and non-Ink. Also excludes "" from the present() check so all
non-numeric strings fall through uniformly (Number("")===0 would otherwise
resolve to 0). The numeric/numeric-string SET path is unchanged (re-verified
across all 5040 patch orders).

Tests pin the contract (numeric, numeric-string, "50%"/"foo"/"" fall-through,
NaN/withdrawn fall-through, explicit 0), and ink-divergences.md records it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Yunfei He
2026-06-15 01:18:54 +08:00
committed by GitHub
parent 889e0cdb00
commit 4243cff937
5 changed files with 509 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/runtime-tests/integration/layout/prop-reset.test.tsx
+118
View File
@@ -283,6 +283,124 @@ test("marginX composes with marginLeft (specific edge wins) (Blocker 1)", async
expect(lastFrame({ trimLines: true })).toBe(" X Y");
});
// Withdrawing a more-specific edge override must fall back to the surviving
// shorthand, NOT collapse to 0. With `margin={5} marginTop={8}`, the per-edge
// EDGE_TOP overrides EDGE_ALL; the old per-setter code reset EDGE_TOP to 0 on
// removal, and EDGE_TOP=0 still overrides EDGE_ALL=5, so the top margin wrongly
// collapsed to 0 instead of falling back to the surviving margin={5}. The
// family-recompute (reconcileMarginEdges) resolves each physical edge from the
// full prop set, so the withdrawn marginTop correctly falls back to 5.
test("removing marginTop falls back to surviving margin shorthand (G19 family-recompute)", async () => {
// margin={5} marginTop={8}: top=8 while set. Remove marginTop → top must fall
// back to margin=5 (5 blank lines above 'x'), NOT collapse to 0.
const hasTop = shallowRef(true);
const Dynamic = defineComponent(() => () => (
<Box flexDirection="column" margin={5} {...(hasTop.value ? { marginTop: 8 } : {})}>
<Text>x</Text>
</Box>
));
const { lastFrame } = await render(Dynamic, { columns: 100 });
// margin=5 left + marginTop=8 → 8 blank rows then " x"
expect(lastFrame({ trimLines: true })).toBe("\n\n\n\n\n\n\n\n x\n\n\n\n\n");
hasTop.value = false;
await nextTick();
// marginTop withdrawn → top falls back to margin=5 (5 blank rows), NOT 0
expect(lastFrame({ trimLines: true })).toBe("\n\n\n\n\n x\n\n\n\n\n");
});
test("removing marginX falls back to surviving margin shorthand (G19 family-recompute)", async () => {
// margin={5} marginX={2}: left/right=2 while set. Remove marginX → left/right
// must fall back to margin=5, NOT collapse to 0.
const hasX = shallowRef(true);
const Dynamic = defineComponent(() => () => (
<Box flexDirection="row">
<Box margin={5} {...(hasX.value ? { marginX: 2 } : {})}>
<Text>X</Text>
</Box>
<Text>Y</Text>
</Box>
));
const { lastFrame } = await render(Dynamic, { columns: 100 });
// Inner box has top/bottom margin 5 and left/right margin 2 (marginX). In the
// row, sibling Y has no margin so it sits at row 0 col 5 (after the inner box's
// 2-left + X + 2-right = 5 wide); X sits at row 5 col 2.
expect(lastFrame({ trimLines: true })).toBe(" Y\n\n\n\n\n X\n\n\n\n\n");
hasX.value = false;
await nextTick();
// marginX withdrawn → left/right fall back to margin=5. Inner box is now
// 5 + X + 5 = 11 wide, so Y moves to col 11; X sits at row 5 col 5. NOT col 0.
expect(lastFrame({ trimLines: true })).toBe(" Y\n\n\n\n\n X\n\n\n\n\n");
});
test("removing paddingTop falls back to surviving padding shorthand (G19 family-recompute)", async () => {
// padding={4} paddingTop={8}: top pad=8 while set. Remove paddingTop → top must
// fall back to padding=4, NOT collapse to 0.
const hasTop = shallowRef(true);
const Dynamic = defineComponent(() => () => (
<Box flexDirection="column" padding={4} {...(hasTop.value ? { paddingTop: 8 } : {})}>
<Text>x</Text>
</Box>
));
const { lastFrame } = await render(Dynamic, { columns: 100 });
// top pad 8 → 8 blank rows then " x" (4 left pad)
expect(lastFrame({ trimLines: true })).toBe("\n\n\n\n\n\n\n\n x\n\n\n\n");
hasTop.value = false;
await nextTick();
// paddingTop withdrawn → top pad falls back to padding=4 (4 blank rows), NOT 0
expect(lastFrame({ trimLines: true })).toBe("\n\n\n\n x\n\n\n\n");
});
test("removing paddingX falls back to surviving padding shorthand (G19 family-recompute)", async () => {
// padding={4} paddingX={1}: left/right pad=1 while set. Remove paddingX →
// left/right must fall back to padding=4, NOT collapse to 0.
const hasX = shallowRef(true);
const Dynamic = defineComponent(() => () => (
<Box flexDirection="row">
<Box padding={4} {...(hasX.value ? { paddingX: 1 } : {})}>
<Text>X</Text>
</Box>
<Text>Y</Text>
</Box>
));
const { lastFrame } = await render(Dynamic, { columns: 100 });
// Inner box has top/bottom padding 4 and left/right padding 1 (paddingX). The
// box is 1 + X + 1 = 3 wide, so sibling Y sits at row 0 col 3; X at row 4 col 1.
expect(lastFrame({ trimLines: true })).toBe(" Y\n\n\n\n X\n\n\n\n");
hasX.value = false;
await nextTick();
// paddingX withdrawn → left/right pad fall back to padding=4. Inner box is now
// 4 + X + 4 = 9 wide, so Y moves to col 9; X sits at row 4 col 4. NOT col 0.
expect(lastFrame({ trimLines: true })).toBe(" Y\n\n\n\n X\n\n\n\n");
});
test("keeping marginTop over margin shorthand still wins (no-regression)", async () => {
// Control: with BOTH margin={5} and marginTop={8} present and nothing removed,
// the more-specific marginTop=8 must win for the top edge while the other edges
// stay at margin=5. Guards against the family-recompute over-zeroing.
const Dynamic = defineComponent(() => () => (
<Box flexDirection="column" margin={5} marginTop={8}>
<Text>x</Text>
</Box>
));
const { lastFrame } = await render(Dynamic, { columns: 100 });
// 8 blank rows above, 5 left margin, then trailing 5 below
expect(lastFrame({ trimLines: true })).toBe("\n\n\n\n\n\n\n\n x\n\n\n\n\n");
});
// Removing `display` resets to the DEFAULT (visible / DISPLAY_FLEX), not persist
// and not hide. This is a DELIBERATE divergence from Ink documented in
// .agents/docs/ink-divergences.md ("Removing `display` resets to the default"):
packages/runtime-tests/unit/yoga-prop-reset.test.ts
+182 -9
View File
@@ -5,7 +5,13 @@ import { expect, test } from "vite-plus/test";
// so it cannot be imported here directly. We reference the stable yoga enum
// values numerically (EDGE_LEFT=0, EDGE_TOP=1, DIRECTION_LTR=1) and inspect the
// computed layout via the node's own getComputedMargin/Padding.
import { applyYogaProp, attachYoga, detachYoga } from "../../runtime/src/host/yoga.ts";
import {
applyYogaProp,
attachYoga,
detachYoga,
reconcileMarginEdges,
reconcilePaddingEdges,
} from "../../runtime/src/host/yoga.ts";
import { createBox } from "../../runtime/src/host/nodes.ts";
// yoga-layout YGEnums (generated/YGEnums.ts) — stable values.
@@ -18,8 +24,16 @@ const DISPLAY_NONE = 1;
// Blocker 2: Vue's HOST renderer passes next=null (not undefined) when a key
// disappears from a spread props object (e.g. Static spreads `style` into host
// props). applyYogaProp's reset path must treat null the same as undefined so a
// props). The removal reset path must treat null the same as undefined so a
// removed yoga key resets to its documented default instead of writing NaN/0.
//
// NOTE on layer: margin/padding edges are reconciled from the FULL el.props by
// reconcileMarginEdges / reconcilePaddingEdges (their per-prop yoga setters are
// no-ops — an edge depends on the specific edge + axis + all-edges shorthands
// together). So these tests drive the reconcilers directly with the el.props
// patchProp would have stored (a removed key is null/undefined → treated as
// absent by the reconciler), mirroring the border reconcile pattern. display (a
// single-prop reset) still goes through applyYogaProp below.
function freshBox() {
const box = createBox();
@@ -29,12 +43,13 @@ function freshBox() {
test("null removal of marginTop resets to default (Blocker 2)", () => {
const box = freshBox();
applyYogaProp(box, "marginTop", 4, undefined);
reconcileMarginEdges(box, { marginTop: 4 });
box.yoga.calculateLayout(undefined, undefined, DIRECTION_LTR as never);
expect(box.yoga.getComputedMargin(EDGE_TOP as never)).toBe(4);
// Removal arrives as next=null (key removed from a spread props object).
applyYogaProp(box, "marginTop", null, 4);
// Removal arrives as null in el.props (key removed from a spread props object);
// the reconciler treats null/undefined as absent → edge falls back to 0.
reconcileMarginEdges(box, { marginTop: null });
box.yoga.calculateLayout(undefined, undefined, DIRECTION_LTR as never);
expect(box.yoga.getComputedMargin(EDGE_TOP as never)).toBe(0);
@@ -43,11 +58,11 @@ test("null removal of marginTop resets to default (Blocker 2)", () => {
test("null removal of paddingLeft resets to default (Blocker 2)", () => {
const box = freshBox();
applyYogaProp(box, "paddingLeft", 5, undefined);
reconcilePaddingEdges(box, { paddingLeft: 5 });
box.yoga.calculateLayout(undefined, undefined, DIRECTION_LTR as never);
expect(box.yoga.getComputedPadding(EDGE_LEFT as never)).toBe(5);
applyYogaProp(box, "paddingLeft", null, 5);
reconcilePaddingEdges(box, { paddingLeft: null });
box.yoga.calculateLayout(undefined, undefined, DIRECTION_LTR as never);
expect(box.yoga.getComputedPadding(EDGE_LEFT as never)).toBe(0);
@@ -56,9 +71,9 @@ test("null removal of paddingLeft resets to default (Blocker 2)", () => {
test("raw null does not corrupt a yoga dimension to NaN (Blocker 2)", () => {
const box = freshBox();
applyYogaProp(box, "marginTop", 7, undefined);
reconcileMarginEdges(box, { marginTop: 7 });
// Removal arrives as null; must reset to 0, never NaN.
applyYogaProp(box, "marginTop", null, 7);
reconcileMarginEdges(box, { marginTop: null });
box.yoga.calculateLayout(undefined, undefined, DIRECTION_LTR as never);
const m = box.yoga.getComputedMargin(EDGE_TOP as never);
expect(Number.isNaN(m)).toBe(false);
@@ -66,6 +81,164 @@ test("raw null does not corrupt a yoga dimension to NaN (Blocker 2)", () => {
detachYoga(box);
});
// Family-recompute fallback: a withdrawn more-specific edge must fall back to the
// surviving shorthand, NOT collapse to 0 (the bug). EDGE_TOP overrides EDGE_ALL
// even at 0, so the old per-setter reset to 0 beat a surviving margin={5}. (G19)
test("withdrawn marginTop falls back to surviving margin shorthand, not 0 (G19)", () => {
const box = freshBox();
reconcileMarginEdges(box, { margin: 5, marginTop: 8 });
box.yoga.calculateLayout(undefined, undefined, DIRECTION_LTR as never);
expect(box.yoga.getComputedMargin(EDGE_TOP as never)).toBe(8);
// marginTop removed (null in el.props); top must fall back to margin=5, NOT 0.
reconcileMarginEdges(box, { margin: 5, marginTop: null });
box.yoga.calculateLayout(undefined, undefined, DIRECTION_LTR as never);
expect(box.yoga.getComputedMargin(EDGE_TOP as never)).toBe(5);
detachYoga(box);
});
test("withdrawn paddingLeft falls back to surviving padding shorthand, not 0 (G19)", () => {
const box = freshBox();
reconcilePaddingEdges(box, { padding: 4, paddingLeft: 7 });
box.yoga.calculateLayout(undefined, undefined, DIRECTION_LTR as never);
expect(box.yoga.getComputedPadding(EDGE_LEFT as never)).toBe(7);
reconcilePaddingEdges(box, { padding: 4, paddingLeft: null });
box.yoga.calculateLayout(undefined, undefined, DIRECTION_LTR as never);
expect(box.yoga.getComputedPadding(EDGE_LEFT as never)).toBe(4);
detachYoga(box);
});
// Non-finite numeric edge (NaN/±Infinity, e.g. a user calc like 0/0): the OLD
// per-setter code did setMargin(EDGE_TOP, NaN), which yoga treats as unset so the
// edge fell back to the surviving shorthand → top = margin = 5. The reconcile must
// preserve that by treating a present-but-non-finite value as ABSENT and falling
// THROUGH to the next precedence level (axis → all → 0), not resolving it to 0.
test("non-finite marginTop (NaN) falls through to surviving margin shorthand, not 0 (G19)", () => {
const box = freshBox();
reconcileMarginEdges(box, { margin: 5, marginTop: NaN });
box.yoga.calculateLayout(undefined, undefined, DIRECTION_LTR as never);
expect(box.yoga.getComputedMargin(EDGE_TOP as never)).toBe(5);
detachYoga(box);
});
test("non-finite paddingLeft (NaN) falls through to surviving padding shorthand, not 0 (G19)", () => {
const box = freshBox();
reconcilePaddingEdges(box, { padding: 5, paddingLeft: NaN });
box.yoga.calculateLayout(undefined, undefined, DIRECTION_LTR as never);
expect(box.yoga.getComputedPadding(EDGE_LEFT as never)).toBe(5);
detachYoga(box);
});
// Explicit zero is NOT non-finite — Number(0) is finite — so an explicit edge
// override of 0 must STILL win over the shorthand (resolve to 0), distinct from
// the NaN fall-through above.
test("explicit marginTop=0 overrides the margin shorthand → top is 0, not 5 (G19)", () => {
const box = freshBox();
reconcileMarginEdges(box, { margin: 5, marginTop: 0 });
box.yoga.calculateLayout(undefined, undefined, DIRECTION_LTR as never);
expect(box.yoga.getComputedMargin(EDGE_TOP as never)).toBe(0);
detachYoga(box);
});
// --- spacing value contract (PR #184) ------------------------------------
//
// Spacing props are typed `number` (box-props.ts) and Ink's margin/padding are
// number-only. The family recompute resolves an edge from a value only when it
// coerces to a FINITE number, with one carve-out for template ergonomics: a Vue
// STATIC template attribute (`<Box margin="5">`) arrives as the numeric STRING
// "5", which must still resolve to 5. Any OTHER non-numeric value ("50%", "foo",
// "") is treated as not-set and falls through to the surviving shorthand — the
// reconcile drops the OLD per-setter code's incidental, off-contract string
// forwarding (setMargin(edge, "50%") → yoga percent; setMargin(edge, "foo") →
// throw). These pin that contract so it can't silently drift.
test('numeric string margin="5" resolves to 5 (static template attribute ergonomics)', () => {
const box = freshBox();
// `<Box margin="5">` reaches the host renderer as the string "5"; the family
// recompute coerces it like the numeric prop margin={5}.
reconcileMarginEdges(box, { margin: "5" });
box.yoga.calculateLayout(undefined, undefined, DIRECTION_LTR as never);
expect(box.yoga.getComputedMargin(EDGE_TOP as never)).toBe(5);
expect(box.yoga.getComputedMargin(EDGE_LEFT as never)).toBe(5);
detachYoga(box);
});
test('numeric string marginTop="8" resolves to 8 and overrides the shorthand', () => {
const box = freshBox();
reconcileMarginEdges(box, { margin: 5, marginTop: "8" });
box.yoga.calculateLayout(undefined, undefined, DIRECTION_LTR as never);
expect(box.yoga.getComputedMargin(EDGE_TOP as never)).toBe(8);
detachYoga(box);
});
test('non-numeric string marginTop="50%" falls through to the surviving margin shorthand (PR #184: no longer a yoga percent)', () => {
const box = freshBox();
// OLD per-setter code forwarded "50%" raw → yoga read it as a 50% percent
// margin. The typed contract is number-only, so "50%" is now off-contract /
// not-set and the edge falls back to the surviving margin shorthand.
reconcileMarginEdges(box, { margin: 5, marginTop: "50%" });
box.yoga.calculateLayout(undefined, undefined, DIRECTION_LTR as never);
expect(box.yoga.getComputedMargin(EDGE_TOP as never)).toBe(5);
detachYoga(box);
});
test('non-numeric string paddingLeft="50%" falls through to the surviving padding shorthand (PR #184: no longer a yoga percent)', () => {
const box = freshBox();
reconcilePaddingEdges(box, { padding: 4, paddingLeft: "50%" });
box.yoga.calculateLayout(undefined, undefined, DIRECTION_LTR as never);
expect(box.yoga.getComputedPadding(EDGE_LEFT as never)).toBe(4);
detachYoga(box);
});
test('junk string marginTop="foo" falls through to the surviving margin shorthand (PR #184: no longer throws)', () => {
const box = freshBox();
// OLD per-setter code did setMargin(EDGE_TOP, "foo") which threw; now it is
// not-set and falls back to the shorthand without throwing.
expect(() => {
reconcileMarginEdges(box, { margin: 5, marginTop: "foo" });
}).not.toThrow();
box.yoga.calculateLayout(undefined, undefined, DIRECTION_LTR as never);
expect(box.yoga.getComputedMargin(EDGE_TOP as never)).toBe(5);
detachYoga(box);
});
// Empty-string tweak (PR #184): `Number("") === 0` would otherwise make
// marginTop="" resolve to 0 (overriding the shorthand) while every other
// non-numeric string falls through — `present()` excludes "" so the contract is
// uniform: only numeric strings are coerced, all other strings fall through.
test('empty string marginTop="" falls through to the surviving margin shorthand, not 0 (PR #184 "" tweak)', () => {
const box = freshBox();
reconcileMarginEdges(box, { margin: 5, marginTop: "" });
box.yoga.calculateLayout(undefined, undefined, DIRECTION_LTR as never);
expect(box.yoga.getComputedMargin(EDGE_TOP as never)).toBe(5);
detachYoga(box);
});
test('empty string paddingLeft="" falls through to the surviving padding shorthand, not 0 (PR #184 "" tweak)', () => {
const box = freshBox();
reconcilePaddingEdges(box, { padding: 4, paddingLeft: "" });
box.yoga.calculateLayout(undefined, undefined, DIRECTION_LTR as never);
expect(box.yoga.getComputedPadding(EDGE_LEFT as never)).toBe(4);
detachYoga(box);
});
// display: removing/undefining `display` resets to the DEFAULT (DISPLAY_FLEX =
// visible), a DELIBERATE divergence from Ink (which hides on present-undefined).
// See .agents/docs/ink-divergences.md ("Removing `display` resets to the