Skip to content

fix(core): remove border when borderStyle set to null/undefined#1214

Open
posva wants to merge 4 commits into
anomalyco:mainfrom
posva:fix/border-style-null
Open

fix(core): remove border when borderStyle set to null/undefined#1214
posva wants to merge 4 commits into
anomalyco:mainfrom
posva:fix/border-style-null

Conversation

@posva

@posva posva commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Setting borderStyle to null/undefined now removes the border instead of falling back to the default style and force-enabling it. Adds regression tests.

@posva posva force-pushed the fix/border-style-null branch from a868612 to 6e1ff71 Compare July 1, 2026 12:03
posva and others added 2 commits July 1, 2026 14:19
Clearing borderStyle should not disable a border that callers enabled
explicitly. Keep explicit borders active with the default style, and only
remove borders that borderStyle enabled implicitly.
@kommander

Copy link
Copy Markdown
Collaborator

Verified this against both branch and main: the bug is real, all 8 new tests fail on main and pass here, and the framework paths that motivate the fix are confirmed — React sets removed props to null (packages/react/src/utils/index.ts:119) and passes undefined on value transitions (line 127); Solid assigns undefined directly and never passes props to the constructor (packages/solid/src/reconciler.ts:191). On main, all three setters funnel into initializeBorder(), so a border could never be removed by un-setting the style prop. The color-path fix is correct: borderColor = null on a bordered box resets to the default color without touching the border.

One semantic flaw should be fixed before merge, though.

borderStyle = null destroys explicit border state

The setter unconditionally does this.border = false, but it can't distinguish an implied border (enabled because a style was set) from an explicit one. Verified on this branch:

  • new BoxRenderable(ctx, { border: true, borderStyle: "double" })box.borderStyle = undefined → border silently removed. In React, <box border borderStyle={cond ? "double" : undefined}> loses its border permanently when cond flips: the reconciler sends borderStyle: undefined but never re-applies border because that prop didn't change in the diff.
  • border: ["left"]borderStyle = null → the sides config is stomped to false and lost (a later border = true gives all four sides).

It's also internally inconsistent: borderColor = null means "reset to default, keep border" while borderStyle = null means "remove border".

Suggested fix: track explicitness — set a _borderExplicit flag in the constructor (options.border !== undefined) and in the border setter; on borderStyle = null, disable the border only when it was implied, otherwise keep it and reset _borderStyle to the default. That makes both clear-semantics consistent and handles both framework patterns (<box borderStyle={maybe}> and <box border borderStyle={maybe}>).

Smaller issues

  • Getter asymmetry: after box.borderStyle = null the getter still returns the old style (e.g. "double"), and border = true later restores it. This "style memory" is defensible but currently an undocumented, untested decision.
  • The null path skips this._customBorderChars = undefined, which the non-null path clears (Box.ts:165) — stale custom chars revive on re-enable.
  • Missing test coverage: explicit border: true + clear, border: [sides] + clear, getter after clear, re-enable after clear, and borderColor = null on a bordered box (the actual reset path — the new tests only cover borderless boxes).
  • Adjacent, pre-existing: border itself receives null from React prop removal; it renders as no-border only because getBorderSides(null) falls through (border.ts:144), while _border stores null against its boolean | BorderSides[] type and flows into drawBox. Worth normalizing nullfalse in the border setter in the same effort.
  • ScrollBox's rootOptions/wrapperOptions/viewportOptions/contentOptions Object.assign straight into boxes (ScrollBox.ts:831-865), so option objects carrying explicit borderStyle: undefined keys now remove borders there too — right direction, but silent and untested.

Docs

components/box.mdx documents border/borderStyle/borderColor but never the implied-border behavior (setting a style/color enables the border), and this PR adds its inverse — "set borderStyle to null/undefined to remove the border" — as public API (the setter signature is now BorderStyle | null | undefined). Both behaviors belong on that page; focusedBorderColor is missing from the property table entirely.

@posva

posva commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

I can't help but notice you generated that message with AI. Is it for me or is it just a note for yourself and other maintainers?

@simonklee

Copy link
Copy Markdown
Member

I can't help but notice you generated that message with AI. Is it for me or is it just a note for yourself and other maintainers?

I think he's just testing using fable to review some of these prs. I had already made a fix for the semantic problems a few days ago, but i pushed to origin instead of your branch by mistake. I did just fixed that now. That commit mostly addresses the fable comments. I'll follow up with a few minor changes then i think this PR is good to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants