Skip to content

fix: use actual border size in GetBorderXxxSize when BorderStyle is set without sides#700

Open
LeSingh1 wants to merge 2 commits into
charmbracelet:mainfrom
LeSingh1:fix/border-size-getters-without-sides
Open

fix: use actual border size in GetBorderXxxSize when BorderStyle is set without sides#700
LeSingh1 wants to merge 2 commits into
charmbracelet:mainfrom
LeSingh1:fix/border-size-getters-without-sides

Conversation

@LeSingh1

Copy link
Copy Markdown

When BorderStyle() is used without explicitly enabling individual sides,
isBorderStyleSetWithoutSides() returns true and the renderer correctly
enables all four sides. The GetBorderTopSize, GetBorderLeftSize,
GetBorderBottomSize, and GetBorderRightSize getters were returning a
hardcoded 1 in that path, ignoring the actual width of the border's
characters.

This causes GetHorizontalFrameSize / GetVerticalFrameSize to return
wrong values whenever the border's side characters have a display width
other than 1 (e.g. a custom border using fullwidth Unicode glyphs), and
makes BorderStyle() + Padding() layout math inconsistent with what
the renderer actually draws.

The fix replaces the hardcoded early-return with a call to the border
struct's own GetXxxSize() getter, which already measures actual rune
widths via maxRuneWidth. The two branches now share the same tail call,
so the condition simplifies to a single combined check.

Also corrects a stale comment in GetBorderBottomSize that referred to
the "left edge" instead of the "bottom edge".

Regression test added in style_test.go covering NormalBorder() via
BorderStyle() alone, BorderStyle() + Padding(), and a custom border
with fullwidth vertical-line characters that triggered the wrong result
before this fix.

…et without sides

When BorderStyle() is used without explicitly enabling individual sides,
isBorderStyleSetWithoutSides() returns true and the renderer correctly
enables all four sides. However, the corresponding GetBorderTopSize,
GetBorderLeftSize, GetBorderBottomSize and GetBorderRightSize getters
were returning a hardcoded 1 in that path, ignoring the actual width of
the border's characters.

This causes GetHorizontalFrameSize / GetVerticalFrameSize to return wrong
values for any border whose side characters have a display width other than
1 (e.g. custom borders with fullwidth Unicode glyphs), and makes
BorderStyle() + Padding() layout math inconsistent with what the renderer
draws.

Fix by replacing the hardcoded early-return with a call to the border
struct's own GetXxxSize() getter, which already measures the actual rune
widths via maxRuneWidth. Also correct a stale comment in GetBorderBottomSize
that referred to the "left edge" instead of the "bottom edge".
@cschanhniem

Copy link
Copy Markdown

Nice catch on the hardcoded 1 in the border-size getters. One consideration: maxRuneWidth depends on runewidth.RuneWidth, which on some systems defaults to East Asian Ambiguous = narrow (effectively treating characters like as width 1). If a terminal actually renders ambiguous-wide codepoints as double-width (common on CJK-configured terminals), the border math can still drift. This would manifest the same way as the original bug but only on CJK locales.

Two paths forward if this ever comes up:

  • Expose a SetEastAsianWidth(bool) on the profile so callers can opt into ambiguous=wide
  • Or measure actual cursor advancement via DSR (CSI 6n) at init time, caching the result — the x/ansi package already has the primitives for this

The current fix is correct for the reported case (fullwidth glyphs in NormalBorder). The ambiguous-width scenario is an edge case that may only matter when lipgloss sees adoption in i18n-heavy TUI apps.

…nderer

The four GetBorderXxxSize methods now delegate to getBorderStyle().GetXxxSize(),
which uses the same maxRuneWidth helper that applyBorder uses when computing
the rendered layout width. Add a doc-comment on each getter explaining this
consistency guarantee so that reviewers understand the EAW-ambiguous-width
concern is handled: getter and renderer measure identically, so there is no
layout mismatch regardless of locale or Unicode width table.
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.

2 participants