Skip to content

Fix #1308: copy-SteamID button no longer dead; clipboard delegate honest on plain HTTP - #1316

Merged
rumblefrog merged 4 commits into
mainfrom
fix/issue-1308-copy-steamid
May 10, 2026
Merged

rumblefrog merged 4 commits into
mainfrom
fix/issue-1308-copy-steamid

Conversation

@rumblefrog

Copy link
Copy Markdown
Member

Summary

Fixes #1308 — two bugs in the [data-copy] surface area, both visible identically on every Edge / Chrome / Firefox once the panel is served over plain HTTP behind a TLS-terminating reverse proxy.

Defect A — banlist row's copy button was a dead button

page_bans.tpl carried onclick="event.stopPropagation()" on the <button data-copy=…> row action. The single document-level COPY BUTTONS click delegate in theme.js listens on the bubble phase, so the element-level stop killed every click before the handler ran — no toast, no clipboard write, no console error. Removed the inline stopPropagation; added a stable data-testid="row-action-copy-steam" so the regression spec can target the button without depending on the visible 📋 glyph.

The defensive copy-paste from the sibling Edit/Unban anchors isn't load-bearing for those — they survive on native href navigation regardless. The desktop row's drawer trigger is the player-name anchor (data-drawer-href), not a row-level delegate, so a bubbling click from the copy button has nothing to confuse.

Defect B — drawer / row copy buttons silently lied on plain HTTP

The pre-fix delegate was

if (navigator.clipboard) navigator.clipboard.writeText(value);
showToast({ kind: 'success', title: 'Copied to clipboard' });

On non-secure contexts (the typical self-hoster behind a TLS-terminating reverse proxy where the panel sees plain HTTP) navigator.clipboard is undefined, the writeText is a silent no-op, and the success toast fires anyway — the user sees "Copied" with an empty clipboard. Even on a secure context the Promise can reject and the success toast still fires.

Rewrote the delegate to mirror handlePaletteCopyShortcut (the canonical "do clipboard right" shape already shipped on the palette's Ctrl/Cmd+Enter shortcut):

  1. Feature-detect both navigator.clipboard AND window.isSecureContext; either missing → fallback path.
  2. .then(success, fallback) on the Promise so a rejected writeText() drops to the same fallback.
  3. copyFallback() = hidden-textarea + document.execCommand('copy') — the deprecated-but-shipping API every browser still implements for this exact non-secure-context path. Toast reflects the actual outcome (success vs honest error).

Docs

  • AGENTS.md gains a "Where to find what" row for the canonical single-source clipboard wiring + two anti-pattern entries (no inline stopPropagation on [data-copy]; no unconditional success toast on writeText()).
  • ARCHITECTURE.md's web/scripts/ row corrects the false claim that banlist.js owns the copy-button wiring.
  • banlist.js's own docblock is updated to point readers at the document-level delegate in theme.js.

Test plan

  • New regression spec web/tests/e2e/specs/flows/ui/copy-buttons.spec.ts covers all three paths against the live panel:
    • banlist desktop row click writes the SteamID + toasts (Defect A guard);
    • drawer overview SteamID button writes + toasts (delegate-shared regression for the drawer surface);
    • non-secure-context monkey-patch (navigator.clipboard = undefined) drops to the execCommand fallback and still toasts (Defect B guard).
  • The spec also asserts the absence of the inline onclick attribute on the row's copy button so the Defect A regression resurfaces if a future dev re-adds the defensive copy-paste.

Quality gates run locally (parallel stack on ports :18308 / :13308 / :19308 / :25308 / :21308):

Gate Result
./sbpp.sh phpstan pass (No errors, 228 files)
./sbpp.sh test pass (403 tests, 1765 assertions; 1 pre-existing PHPUnit deprecation, no failures)
./sbpp.sh ts-check pass (clean tsc over web/scripts)
./sbpp.sh composer api-contract clean diff (no API change)
./sbpp.sh e2e --grep "copy buttons" 3 passed, 3 skipped (mobile-chromium gates)
./sbpp.sh e2e --grep "command palette|player drawer|responsive: drawer" 22 passed, 16 skipped — no regression in adjacent specs

…te honest on plain HTTP (#1308)

Two bugs in the same surface area, both visible on Edge / Chrome /
Firefox identically once the panel is served over plain HTTP.

Defect A — banlist row's copy button was a dead button.
  page_bans.tpl carried `onclick="event.stopPropagation()"` on the
  `<button data-copy=…>` row action. The single document-level
  COPY BUTTONS click delegate in theme.js listens on the bubble
  phase, so the element-level stop killed every click before the
  handler ran — no toast, no clipboard write, no console error.
  The defensive copy-paste from the sibling Edit/Unban anchors
  isn't load-bearing for those (they survive on native href
  navigation regardless). The desktop row's drawer trigger is the
  player-name anchor (data-drawer-href), not a row-level
  delegate, so a bubbling click from the copy button has nothing
  to confuse. Removed the inline stopPropagation; added a stable
  `data-testid="row-action-copy-steam"` so the regression spec
  can target the button without depending on the visible glyph.

Defect B — drawer / row copy buttons silently lied on plain HTTP.
  The pre-fix delegate body was
    if (navigator.clipboard) navigator.clipboard.writeText(value);
    showToast({ kind:'success', title:'Copied to clipboard' });
  On non-secure contexts (the typical self-hoster behind a TLS-
  terminating reverse proxy where the panel sees plain HTTP)
  navigator.clipboard is undefined, the writeText is a silent
  no-op, and the success toast fires anyway — the user sees
  "Copied" with an empty clipboard. Even on a secure context the
  Promise can reject and the success toast still fires.

  Rewrite the delegate to mirror handlePaletteCopyShortcut (the
  canonical "do clipboard right" shape already shipped on the
  palette's Ctrl/Cmd+Enter shortcut):
    1. Feature-detect both navigator.clipboard AND
       window.isSecureContext; either missing -> fallback path.
    2. .then(success, fallback) on the Promise so a rejected
       writeText() drops to the same fallback.
    3. copyFallback() = hidden-textarea + document.execCommand
       ('copy') — the deprecated-but-shipping API every browser
       still implements for this exact non-secure-context path.
       Toast reflects the actual outcome (success vs honest
       error).

Regression test: web/tests/e2e/specs/flows/ui/copy-buttons.spec.ts
covers all three paths against the live panel —
  - banlist desktop row click writes the SteamID + toasts,
  - drawer overview SteamID button writes + toasts,
  - non-secure-context monkey-patch (navigator.clipboard =
    undefined) drops to the execCommand fallback and still toasts.
The spec also asserts the absence of the inline onclick attribute
so the Defect A regression resurfaces if a future dev re-adds the
defensive copy-paste.

Docs: AGENTS.md gains a "Where to find what" row for the canonical
single-source clipboard wiring + two anti-pattern entries (no
inline stopPropagation on [data-copy]; no unconditional success
toast). ARCHITECTURE.md's web/scripts/ row corrects the false
claim that banlist.js owns the copy-button wiring; banlist.js's
own docblock is updated to point readers at the document-level
delegate in theme.js.

Fixes #1308
…ims to guard (#1308)

The original Defect B spec only asserted the "Copied to clipboard"
toast appeared after the click, which is exactly the symptom of the
ORIGINAL BUG — the pre-#1308 delegate fired the success toast
unconditionally even when navigator.clipboard was undefined and no
clipboard write happened. So the spec passed against both the buggy
code and the fixed code; reverting theme.js to the unconditional-toast
shape didn't make the test fail.

Replace the toast assertion with a `document.execCommand` spy that
records every command dispatched and assert `'copy'` was among them.
The fallback path is the only thing that calls execCommand in this
flow; the spy stays empty under the regression and toContain('copy')
fails. The toast assertion is kept as belt-and-suspenders.

Verified: with the fix in place all 3 specs pass (chromium); reverting
just the theme.js delegate to the unconditional-toast shape produces
`expect(execCalls).toContain('copy')` failure on the Defect B spec.
@rumblefrog
rumblefrog force-pushed the fix/issue-1308-copy-steamid branch from a8cc0fd to e27305e Compare May 10, 2026 18:51
web-flow added 2 commits May 10, 2026 16:17
Resolve ARCHITECTURE.md conflict by keeping the PR's banlist.js row
update (drops the docblock claim that banlist.js owns the SteamID copy
button wiring; documents that theme.js's document-level [data-copy]
delegate is the single source). The contextMenoo.js row was deleted in
main by #1306/#1319; the PR side hadn't applied that deletion because
it was branched before the merge.
Resolve AGENTS.md conflict by keeping both new anti-pattern entries:
the #1308 stopPropagation-on-data-copy + unconditional-success-toast
entries from this PR, AND the #1301 reason-less-unban + native-required
entries from #1323 (merged to main). Both pairs append to the same
"Anti-patterns" list and don't overlap semantically.
@rumblefrog
rumblefrog added this pull request to the merge queue May 10, 2026
Merged via the queue into main with commit 40bd0dc May 10, 2026
4 checks passed
@rumblefrog
rumblefrog deleted the fix/issue-1308-copy-steamid branch May 10, 2026 20:38
Rushaway pushed a commit to srcdslab/sourcebans-pp that referenced this pull request May 15, 2026
…p#1316) (sbpp#1355)

* fix(theme): scale server cards with screen size on wide displays (sbpp#1316)

The public servers list (`?p=servers`) and admin Server Management
list (`?p=admin&c=servers`) packed cards into a 20rem (320px)
minimum column via an inline `style="grid-template-columns:
repeat(auto-fill, minmax(20rem, 1fr))"` on each grid wrapper. Both
pages cap the page wrapper at 1400px (the public page via the inline
`max-width:1400px` on its outer div, the admin page via
`.page-section`'s `max-width: 1400px`), so on EVERY viewport >=
1400px the available content area was identical at ~1352px. With a
320px min the auto-fill packed 4 columns at ~338px each — same width
as a phone. The "31\" 4K monitor" reporter saw zero benefit from
their wide display: hostname truncated at the 338px column edge
even though there was 3500px of free pixels behind the page-cap.

Pull the rule out of the inline `style=` attributes and into a
single named `.servers-grid` class in `web/themes/default/css/
theme.css` with `repeat(auto-fill, minmax(28rem, 1fr))`. With the
1rem grid-gap factored in:

  - 1280px laptop (~992px content area): 2 cols ~488px each
  - 1400px+ at the page cap (~1352px):   2 cols ~668px each
  - <=768px mobile drops to 1 col via the sibling rule below

That gives the hostname column ~348px (1280 laptop) up to ~528px
(1400+ desktop) after the 36px mod-icon / 80px status pill / 24px
gaps, comfortably fitting a 40-70 char hostname (e.g. "Skial |
2Fort | Vanilla | New York #5" or "Trade Plaza | 24/7 | No Random
Crits | 100 Player Slots | EU FAST") at the shared 14px font size.
Very long hostnames (>70 chars) still need the `truncate` class'
ellipsis + the `title=` tooltip the templates already carry — that
contract is unchanged by the bump.

The mobile rule uses `minmax(0, 1fr)`, NOT bare `1fr`. Bare `1fr`
is shorthand for `minmax(auto, 1fr)`, where the `auto` minimum
resolves to the grid item's min-content size. The server card's
hostname / IP:port descendants both carry `truncate` (`white-space:
nowrap`), so the card's min-content is the rendered width of the
longest single line — typically wider than a phone viewport. Bare
`1fr` would inflate the track to that min-content size and the
card would silently overflow the viewport (~110px right-edge spill
on a 390px iPhone-13 viewport with a `203.0.113.10:27015`-shape
IP:port fallback). `minmax(0, 1fr)` overrides the auto-minimum to
0 so the track resolves to exactly 100% of the grid container, the
`truncate`-CSS handles the overflow per its existing ellipsis
contract, and the page never horizontally scrolls.

The outer 1400px page-section cap is intentionally NOT lifted
here: lifting it would make the cards "fill the screen" on a 31"
4K monitor (~3500px wide → 7 cols at ~500px each), but that's a
broader UX call that affects every Pattern A admin route and the
public banlist / commslist. The reported symptom (truncated
hostname) is fully addressed by the column-min bump alone; the
cap-lift is a follow-up if users want the cards to scale BEYOND
1400px content width.

The change is CSS-only — both grid surfaces continue to share the
hydration contract via `web/scripts/server-tile-hydrate.js`, every
`data-server-hydrate` / `data-testid="server-tile"` / `data-id` /
`data-trunchostname` attribute is preserved verbatim, and the
`.card` generic class is unchanged (still used by every other
`<article class="card">` surface across the panel).

E2E coverage: `web/tests/e2e/specs/responsive/server-cards.spec.ts`
asserts the shared `.servers-grid` class is present on both
surfaces, the desktop column-min holds across 1280-3840px
viewports without horizontal page scroll, the hydration
attributes survive the resize, and the grid collapses to a
single full-width card at iPhone-13-shape (390x844). Because the
e2e suite shares one DB and `page.evaluate` resizes are
process-local, the spec is `test.describe.configure({ mode:
'serial' })` and skipped outside the `chromium` project so
parallel workers don't race on the seeded server row.

AGENTS.md "Where to find what" gets a new row covering
`.servers-grid` and the breakpoint reasoning so a future fork or
theme tweak knows where to find the rule + why the min is 28rem.

* test(server-cards): match comment to shipped `minmax(0, 1fr)` mobile rule

The narrow-viewport spec's docblock said the mobile rule is
`grid-template-columns: 1fr`, but the CSS shipped in the previous
commit is `grid-template-columns: minmax(0, 1fr)`. The discrepancy
matters for future readers because bare `1fr` is shorthand for
`minmax(auto, 1fr)` — the `auto` minimum resolves to the card's
min-content (the `truncate` IP:port descendants force a wider min
than the viewport) and the card overflows. `minmax(0, 1fr)` is the
only shape that lets the track shrink to the grid container at
<=768px. Update the comment so a reader cargo-culting from this
spec doesn't reach for the wrong mobile shape.
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.

Copy-SteamID icon is a no-op (banlist row swallows the click; drawer copy lies on plain HTTP)

2 participants