Skip to content

fix(1448): require base btn token alongside every btn--* modifier - #1453

Merged
rumblefrog merged 1 commit into
mainfrom
fix-1448-burger-menu-grey-bg
May 25, 2026
Merged

rumblefrog merged 1 commit into
mainfrom
fix-1448-burger-menu-grey-bg

Conversation

@rumblefrog

Copy link
Copy Markdown
Member

Summary

Fixes #1448 — the mobile-viewport burger button rendered with the user-agent default grey 1px-border chrome instead of the panel's ghost-button styling.

The button in web/themes/default/core/title.tpl shipped class="btn--ghost btn--icon" — modifier-only, no base btn. In theme.css:

  • .btn is the load-bearing rule: it declares the --btn-bg / --btn-color / --btn-border defaults AND applies them as background / color / border / display: inline-flex / padding / height.
  • .btn--ghost / .btn--primary / .btn--secondary / .btn--danger are pure custom-property overrides — they SET the variables but don't apply them.
  • .btn--sm / .btn--icon / .btn--xs layer geometry on top but still don't carry the chrome declarations.

Without .btn in the chain, the variables get set but nothing reads them — <button> falls back to UA-default chrome (visible grey pill that fights the topbar's dark background on mobile, where [data-mobile-menu] flips from display: none to display: inline-flex at <=1024px).

Sister sites swept in the same PR

  • core/footer.tpl — the palette Esc close button shipped the same modifier-only chain. Same live wrong-chrome shape behind the <kbd>Esc</kbd> child.
  • partials/player-drawer.tpl — drawer-close X button. Reference template, not live render path — the live drawer chrome is rendered by web/themes/default/js/theme.js's renderDrawerBody() / renderDrawerLoading() (which always shipped the correct three-class chain). Documentation sync, not a behaviour fix.

Regression guard

New test web/tests/integration/ButtonClassChainTest.php — single-method parser-style sweep across:

  • web/themes/ (every Smarty template + the install wizard chrome)
  • web/themes/default/js/ (the runtime HTML emitter)
  • web/scripts/ (panel-side JS)
  • web/pages/ (page handlers)
  • web/includes/View/ (view DTOs)
  • web/install/, web/updater/, web/api/handlers/ (preventive coverage for any future inline-HTML emission)

For *.tpl, *.php, AND *.js. Every class="…" / class='…' attribute body carrying a btn--* modifier MUST also carry the base btn token. The JS-side coverage is what makes the gate's "panel chrome" protection structural — theme.js ships seven runtime <button class="btn btn--ghost btn--icon[ btn--xs]"> strings via template concatenation; without scanning them the gate would paper over the live bug surface.

Implementation details:

  • Strips Smarty {* *} comments (default + -{* *}- non-default delimiters used by page_login.tpl / page_blockit.tpl / page_kickit.tpl / page_admin_servers_rcon.tpl), PHP comments via php_strip_whitespace(), and JS /* */ + // comments so AGENTS.md-style explanatory blocks don't false-match.
  • Negative lookbehind (?<![-_\w]) on the class keyword skips data-class= / aria-class= siblings.
  • Sanity-check assertions (>100 files scanned, >200 attribute bodies extracted) guard against silent zero-scan if ROOT / scanRoots() ever drift.
  • Smarty-conditional class chains (anything containing {) are skipped; false negatives bounded to chains with NO base in any branch (none in the codebase today).

I deliberately introduced a regression in theme.js during development to verify the gate fires; it caught the bug at the right line with the right message.

AGENTS.md changes

  • New "Anti-patterns" entry documenting the bug class with the user-reported regression details, sister-site rationale, and regression-guard scope.
  • New "Button class chains" Conventions block (positive-shape contract paired with the anti-pattern).
  • New "Where to find what" row pointing at the regression guard.

Adversarial review trail

Spawned an adversarial reviewer subagent on the initial diff. It surfaced:

  • theme.js runtime-emitted button strings were outside the scan scope (MAJOR — fixed by extending scanRoots + adding JS comment stripping).
  • The "PURE CSS-variable overrides" claim was technically wrong for sizing modifiers (MAJOR — tightened wording across all five sites).
  • Missing "Where to find what" row (MAJOR — added).
  • The drawer-template framing as a "live sister bug" was misleading (MAJOR — clarified in the AGENTS.md entry; the JS scan extension also makes the framing structurally honest now since the gate covers both halves).
  • Several MINOR / NIT findings around docstring honesty, scan scope completeness, regex edge cases, and template-comment bloat — all addressed in this commit.

Test plan

  • ./sbpp.sh phpstan passes (level 5, baseline unchanged)
  • ./sbpp.sh ts-check passes
  • ./sbpp.sh composer api-contract is in sync (no diff)
  • ./sbpp.sh test — full suite passes (891 tests, 3338 assertions, including the 3-assertion regression guard)
  • Test catches a fresh regression — verified by intentionally breaking theme.js's renderDrawerLoading() button, watching the gate fire, then reverting
  • Visual verification via Playwright on mobile viewport (iPhone 13-like 390×844, light + dark themes, before-and-after screenshots) — burger button now renders with the panel ghost-button chrome instead of UA-default grey

CLA: I have read the CLA Document and I hereby sign the CLA

The mobile burger menu in `core/title.tpl` shipped
`class="btn--ghost btn--icon"` (modifier-only, no base `btn`).
`.btn--ghost` and `.btn--icon` are CSS-custom-property overrides;
only `.btn` itself reads those properties and applies them as
`background` / `color` / `border` / `display: inline-flex` /
`padding` / `height`. Without `.btn` in the chain the variables
get set but nothing reads them, so `<button>` falls back to the
user-agent default chrome — a glaring grey 1px-border pill that
fights the topbar's dark background on mobile.

Sister sites swept in the same PR: `core/footer.tpl`'s palette
`Esc` close button (live bug — same wrong-chrome shape) and
`partials/player-drawer.tpl`'s drawer-close `X` (reference
template — the live drawer chrome is rendered by `theme.js`'s
`renderDrawerBody()` / `renderDrawerLoading()` family, which
always shipped the correct three-class chain).

Regression guard: `web/tests/integration/ButtonClassChainTest.php`.
Parser-style sweep across `web/themes/`, `web/pages/`,
`web/includes/View/`, `web/install/`, `web/updater/`,
`web/api/handlers/`, AND `web/scripts/` + `web/themes/default/js/`
for `*.tpl` / `*.php` / `*.js` — every `class="…"` attribute body
carrying a `btn--*` modifier MUST also carry `btn`. Strips Smarty
`{* *}` comments (default + `-{* *}-` non-default delimiters),
PHP comments via `php_strip_whitespace()`, and JS `/* */` + `//`
comments so AGENTS.md-style explanatory blocks don't false-match.
Negative-lookbehind on the `class` keyword skips `data-class=` /
`aria-class=` siblings. Sanity-check assertions (>100 files
scanned, >200 attribute bodies extracted) guard against silent
zero-scan if `ROOT` or `scanRoots()` ever drift.

The JS-side scan coverage is what makes the gate's "panel chrome"
protection actually structural — `theme.js` ships seven runtime
`<button class="btn btn--ghost btn--icon[ btn--xs]">` strings via
template concatenation; without scanning them the gate would
paper over the live bug surface.

AGENTS.md additions:
- "Anti-patterns" entry with the user-reported regression details,
 the sister-site rationale, and the regression-guard scope.
- Paired Conventions block ("Button class chains") with the
 positive-shape contract.
- "Where to find what" row pointing at the regression guard.
@rumblefrog
rumblefrog added this pull request to the merge queue May 25, 2026
Merged via the queue into main with commit def9737 May 25, 2026
5 checks passed
@rumblefrog
rumblefrog deleted the fix-1448-burger-menu-grey-bg branch May 25, 2026 18:42
@github-actions github-actions Bot locked and limited conversation to collaborators May 25, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mobile view - Burger menu background is grey

1 participant