From bfcc898eaad109f1a5f778619820d8dbda6dcbc1 Mon Sep 17 00:00:00 2001 From: Rushaway Date: Tue, 22 Sep 2026 10:53:25 +0200 Subject: [PATCH 1/4] fix(admin-mods): decode entity-encoded mod metadata and add proper empty state Port of upstream sbpp/sourcebans-pp#1552, continuing our earlier feat/mods-list-compact-actions (#14) port before upstream's own follow-up review passes landed. - Mod name/folder/icon path are stored entity-encoded, but the template rendered them with Smarty's default auto-escape on top, so a mod whose name contains e.g. an ampersand painted the literal `&` in the admin list instead of `&`. Add `|unescape:'html'` at every text and attribute sink (desktop table, mobile cards, edit/delete aria-labels and data-name attributes) to decode once before Smarty's own escape reapplies it safely. - Replace the ad-hoc "No mods configured yet." paragraph with the shared `.empty-state` pattern (icon + title + body + permission- gated CTA) per the "Empty states" convention in AGENTS.md, and add the missing `$permission_addmods` property so the CTA can gate on it. - `$mod_count` counted mid=0 (the reserved "Web" pseudo-mod every install carries) even though `$mod_list` always excludes it, so a fresh install with zero real mods configured showed an empty table instead of the empty state. Scope the COUNT query to `mid > 0` to match. - Mobile cards: give the modfolder line `truncate` + a `title` attribute and move the SU badge to a `flex-shrink:0` sibling instead of letting a long folder name push it off narrow screens. Co-Authored-By: Claude Sonnet 5 --- web/includes/View/AdminModsListView.php | 4 +- web/pages/admin.mods.php | 4 +- web/themes/default/page_admin_mods_list.tpl | 54 ++++++++++++++------- 3 files changed, 43 insertions(+), 19 deletions(-) diff --git a/web/includes/View/AdminModsListView.php b/web/includes/View/AdminModsListView.php index ced82f9fb..29a113903 100644 --- a/web/includes/View/AdminModsListView.php +++ b/web/includes/View/AdminModsListView.php @@ -16,7 +16,8 @@ * Property names keep the historical `permission_*` shape (rather than * the newer `can_*` convention `Sbpp\View\Perms::for()` produces) because * the template references `{$permission_listmods}` / - * `{$permission_editmods}` / `{$permission_deletemods}`. See + * `{$permission_addmods}` / `{$permission_editmods}` / + * `{$permission_deletemods}`. See * `AdminServersListView` / `AdminBansAddView` for the canonical * `can_*` convention on Views that don't have to honour a historical * variable name. @@ -30,6 +31,7 @@ final class AdminModsListView extends View */ public function __construct( public readonly bool $permission_listmods, + public readonly bool $permission_addmods, public readonly bool $permission_editmods, public readonly bool $permission_deletemods, public readonly int $mod_count, diff --git a/web/pages/admin.mods.php b/web/pages/admin.mods.php index d0378f4b9..7f12c7b8e 100644 --- a/web/pages/admin.mods.php +++ b/web/pages/admin.mods.php @@ -37,11 +37,13 @@ return; } +// mid=0 is the reserved Web pseudo-mod, not a configurable game mod. $mod_list = $GLOBALS['PDO']->query("SELECT * FROM `:prefix_mods` WHERE mid > 0 ORDER BY name ASC")->resultset(); -$mod_count = (int) $GLOBALS['PDO']->query("SELECT COUNT(mid) AS cnt FROM `:prefix_mods`")->single()['cnt']; +$mod_count = (int) $GLOBALS['PDO']->query("SELECT COUNT(mid) AS cnt FROM `:prefix_mods` WHERE mid > 0")->single()['cnt']; \Sbpp\View\Renderer::render($theme, new \Sbpp\View\AdminModsListView( permission_listmods: $canList, + permission_addmods: $canAdd, permission_editmods: $userbank->HasAccess(WebPermission::mask(WebPermission::Owner, WebPermission::EditMods)), permission_deletemods: $userbank->HasAccess(WebPermission::mask(WebPermission::Owner, WebPermission::DeleteMods)), mod_count: $mod_count, diff --git a/web/themes/default/page_admin_mods_list.tpl b/web/themes/default/page_admin_mods_list.tpl index 273dc1bbe..41c7bcb37 100644 --- a/web/themes/default/page_admin_mods_list.tpl +++ b/web/themes/default/page_admin_mods_list.tpl @@ -7,6 +7,7 @@ Variable contract (kept in sync by SmartyTemplateRule): - $permission_listmods — gate the whole tab body. + - $permission_addmods — gate the empty-state "Add mod" CTA. - $permission_editmods — gate the per-row "Edit" link. - $permission_deletemods — gate the per-row "Delete" button. - $mod_count — total mods configured. @@ -55,7 +56,7 @@
{if $mod_count > 0} -
+
@@ -69,20 +70,23 @@ + {* Mod metadata is entity-encoded on store. Decode before + Smarty's automatic final escape at every text and + attribute sink so values stay readable without raw HTML. *} {foreach from=$mod_list item=mod} - +
- - {$mod.name} + {$mod.name|unescape:'html'}
{$mod.modfolder}{$mod.modfolder|unescape:'html'} {$mod.steam_universe} {if $mod.enabled} @@ -103,7 +107,7 @@ href="index.php?p=admin&c=mods&o=edit&id={$mod.mid|escape:'url'}" data-testid="editmod-link" data-tooltip="Edit" - aria-label="Edit mod {$mod.name|escape}"> + aria-label="Edit mod {$mod.name|unescape:'html'}"> {/if} @@ -127,11 +131,11 @@ type="button" data-action="mod-delete" data-mid="{$mod.mid}" - data-name="{$mod.name|escape}" + data-name="{$mod.name|unescape:'html'}" data-fallback-href="index.php?p=admin&c=mods" data-testid="deletemod-btn" data-tooltip="Delete" - aria-label="Delete mod {$mod.name|escape}"> + aria-label="Delete mod {$mod.name|unescape:'html'}"> {/if} @@ -151,17 +155,19 @@ {foreach from=$mod_list item=mod}
-
-
{$mod.name}
-
- {$mod.modfolder} - · SU {$mod.steam_universe} +
{$mod.name|unescape:'html'}
+
+ {$mod.modfolder|unescape:'html'} + SU {$mod.steam_universe}
{if $mod.enabled} @@ -179,7 +185,7 @@ href="index.php?p=admin&c=mods&o=edit&id={$mod.mid|escape:'url'}" data-testid="editmod-link-mobile" data-tooltip="Edit" - aria-label="Edit mod {$mod.name|escape}"> + aria-label="Edit mod {$mod.name|unescape:'html'}"> {/if} @@ -188,11 +194,11 @@ type="button" data-action="mod-delete" data-mid="{$mod.mid}" - data-name="{$mod.name|escape}" + data-name="{$mod.name|unescape:'html'}" data-fallback-href="index.php?p=admin&c=mods" data-testid="deletemod-btn-mobile" data-tooltip="Delete" - aria-label="Delete mod {$mod.name|escape}"> + aria-label="Delete mod {$mod.name|unescape:'html'}"> {/if} @@ -202,8 +208,22 @@ {/foreach}
{else} -
-

No mods configured yet.

+
+ +

No mods configured yet

+

Add a game mod before assigning it to bans or servers.

+ {if $permission_addmods} +
+ + + Add mod + +
+ {/if}
{/if}
From 234ce1a5e661fd9569bf6372bca03503e30b0483 Mon Sep 17 00:00:00 2001 From: Rushaway Date: Tue, 22 Sep 2026 10:57:44 +0200 Subject: [PATCH 2/4] feat(admin-groups): reflect exhausted select-all/select-none state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Port of upstream sbpp/sourcebans-pp#1573's tri-state "select all" checkbox for the web-group permission flag grid, adapted to this fork's existing two-button shape (#1436) instead of swapping in a single . A straight swap to upstream's checkbox would have thrown away the explicit Select-all/Select-none affordance and forced re-proving two things the #1436 spec (admin-groups-select-all-flags.spec.ts) already locks in against the button pair: the unsigned bit-31 OR-fold guard (#1272, SbppFoldFlags' `>>> 0`) and the dirty-tracker-arming contract in SbppGroupsToggleAllFlags. Neither is exercised for free by a native checkbox's indeterminate state. Instead, both buttons now reflect the grid's current state via aria-disabled + a dimmed style once nothing is left to do — not the native `disabled` attribute, because SbppGroupsToggleAllFlags is already a safe no-op on a fully-selected grid and the existing spec deliberately re-clicks "Select all" in that state as an idempotency check; a truly disabled button can't be clicked at all. SbppGroupsRefreshSelectAllButtons() is wired into the three places the grid's checked state can change: the existing `change` listener (manual clicks and the bulk toggle's own dispatched event), the master-detail paintGroup() repaint (which sets .checked directly with no change event), and a bootstrap call for the SSR-rendered initial state. Extends the existing E2E spec (rather than adding a new file) with aria-disabled assertions at each state transition, including after a fresh page reload to prove the state is re-derived from the SSR- checked grid rather than carried in JS memory. Co-Authored-By: Claude Sonnet 5 --- .../admin-groups-select-all-flags.spec.ts | 24 +++++++- web/themes/default/page_admin_groups_list.tpl | 61 +++++++++++++++++++ 2 files changed, 83 insertions(+), 2 deletions(-) diff --git a/web/tests/e2e/specs/flows/admin-groups-select-all-flags.spec.ts b/web/tests/e2e/specs/flows/admin-groups-select-all-flags.spec.ts index 1996c4b48..c7f961f22 100644 --- a/web/tests/e2e/specs/flows/admin-groups-select-all-flags.spec.ts +++ b/web/tests/e2e/specs/flows/admin-groups-select-all-flags.spec.ts @@ -128,6 +128,13 @@ test.describe('flow: admin groups select-all permission flags (upstream #1436)', await expect(selectAll).toHaveAttribute('type', 'button'); await expect(selectNone).toHaveAttribute('type', 'button'); await expect(bitmaskBadge).toHaveText(/^0 bitmask$/); + // Nothing checked yet: "Select none" starts out reflecting that + // there's nothing left to clear (upstream #1573 parity — see the + // SbppGroupsRefreshSelectAllButtons docblock for why this fork + // uses aria-disabled + a dimmed style instead of a real indeterminate + // checkbox or the native `disabled` attribute). + await expect(selectNone).toHaveAttribute('aria-disabled', 'true'); + await expect(selectAll).toHaveAttribute('aria-disabled', 'false'); const total = await checkboxes.count(); expect(total).toBeGreaterThan(0); @@ -146,9 +153,14 @@ test.describe('flow: admin groups select-all permission flags (upstream #1436)', } await expect(bitmaskBadge).toHaveText(`${expected} bitmask`); await expect(bitmaskBadge).not.toContainText('-'); + // Everything is now checked: "Select all" reflects there's nothing + // left to add, "Select none" flips back to actionable. + await expect(selectAll).toHaveAttribute('aria-disabled', 'true'); + await expect(selectNone).toHaveAttribute('aria-disabled', 'false'); // Idempotent: a second press changes nothing and must not corrupt - // the preview. + // the preview. This is exactly the case aria-disabled (rather than + // the native `disabled` attribute) has to stay clickable for. await selectAll.click(); await expect(bitmaskBadge).toHaveText(`${expected} bitmask`); @@ -181,13 +193,21 @@ test.describe('flow: admin groups select-all permission flags (upstream #1436)', for (let i = 0; i < reloadedTotal; i++) { await expect(reloadedChecks.nth(i)).toBeChecked(); } + // Fresh page load re-derives the same state from the SSR-checked + // grid, not just from in-memory JS state carried across the click. + const reloadedSelectAll = reloadedDetail.locator('[data-testid="flag-select-all"]'); + const reloadedSelectNone = reloadedDetail.locator('[data-testid="flag-select-none"]'); + await expect(reloadedSelectAll).toHaveAttribute('aria-disabled', 'true'); + await expect(reloadedSelectNone).toHaveAttribute('aria-disabled', 'false'); // ---- Select none → every checkbox cleared, badge back to 0 ------- - await reloadedDetail.locator('[data-testid="flag-select-none"]').click(); + await reloadedSelectNone.click(); for (let i = 0; i < reloadedTotal; i++) { await expect(reloadedChecks.nth(i)).not.toBeChecked(); } await expect(reloadedBadge).toHaveText(/^0 bitmask$/); + await expect(reloadedSelectNone).toHaveAttribute('aria-disabled', 'true'); + await expect(reloadedSelectAll).toHaveAttribute('aria-disabled', 'false'); }); test('a bulk toggle arms the unsaved-changes guard', async ({ page }) => { diff --git a/web/themes/default/page_admin_groups_list.tpl b/web/themes/default/page_admin_groups_list.tpl index 61fce5012..17133a035 100644 --- a/web/themes/default/page_admin_groups_list.tpl +++ b/web/themes/default/page_admin_groups_list.tpl @@ -615,6 +615,58 @@ function SbppGroupsToggleAllFlags(checked) { if (preview) preview.textContent = SbppFoldFlags(grid) + ' bitmask'; } +/** + * Port of upstream sbpp/sourcebans-pp#1573's tri-state "select all" + * checkbox, adapted to this fork's two-button shape instead of a + * single ``: a checkbox here would have lost + * the explicit Select-all/Select-none affordance the #1436 spec + * (admin-groups-select-all-flags.spec.ts) already locks in — including + * the unsigned bit-31 OR-fold guard (#1272) and the dirty-tracker-arming + * contract, both of which are exercised through `SbppGroupsToggleAllFlags` + * and would need re-proving from scratch against a checkbox's native + * `indeterminate` handling. Reflecting the grid's current state via each + * button's `aria-disabled` + dimmed style gives the same "you can see + * there's nothing left to do" feedback without touching that logic. + * + * Deliberately uses `aria-disabled` + a visual dim, NOT the native + * `disabled` attribute: `SbppGroupsToggleAllFlags` is already a safe + * no-op when everything is already on/off (it bails before dispatching + * `change`, per the comment above it), and the #1436 spec exercises a + * redundant "Select all" press on a fully-selected grid as a deliberate + * idempotency check. A `disabled` button can't receive a click at all + * (Playwright's actionability check would hang waiting for it), which + * would turn that intentional no-op assertion into a broken test. + * `aria-disabled` communicates the same "nothing left to do" state to + * assistive tech and sighted users alike while staying clickable. + * + * Call after anything that can change the grid's checked state without + * going through a user click on an individual checkbox: the master-detail + * `paintGroup()` repaint and the bootstrap call below (`change` events — + * manual clicks and the bulk toggle's own dispatch — are covered by the + * listener wired further down this file). + */ +function SbppGroupsRefreshSelectAllButtons() { + var grid = document.querySelector('[data-testid="flag-grid"]'); + var selectAll = document.querySelector('[data-testid="flag-select-all"]'); + var selectNone = document.querySelector('[data-testid="flag-select-none"]'); + if (!grid || !(selectAll || selectNone)) return; + + var checks = grid.querySelectorAll('input[name="flags[]"]:not([disabled])'); + var total = checks.length; + var checked = grid.querySelectorAll('input[name="flags[]"]:not([disabled]):checked').length; + + /** @param {Element|null} btn @param {boolean} exhausted */ + function reflect(btn, exhausted) { + if (!btn) return; + btn.setAttribute('aria-disabled', exhausted ? 'true' : 'false'); + /** @type {HTMLElement} */ (btn).style.opacity = exhausted ? '0.5' : ''; + /** @type {HTMLElement} */ (btn).style.cursor = exhausted ? 'default' : ''; + } + + reflect(selectAll, total === 0 || checked === total); + reflect(selectNone, checked === 0); +} + /** * Inline replacement for the legacy `applyApiResponse` global from * `web/scripts/sourcebans.js` (deleted at #1123 D1, see AGENTS.md @@ -769,7 +821,12 @@ function SbppServerGroupsDelete(gid, name, type, btn) { if (!target || !target.matches || !target.matches('input[name="flags[]"]')) return; preview.textContent = SbppFoldFlags(grid) + ' bitmask'; + SbppGroupsRefreshSelectAllButtons(); }); + + // Reflect the SSR-rendered initial state (e.g. a group whose flags + // already cover every checkbox loads with "Select all" disabled). + SbppGroupsRefreshSelectAllButtons(); })(); // --- Client-side master-detail selection --- @@ -852,6 +909,10 @@ function SbppServerGroupsDelete(gid, name, type, btn) { if (bitmaskEl) { bitmaskEl.textContent = flags + ' bitmask'; } + // paintGroup() sets .checked directly (no change event), so the + // Select all/Select none disabled state needs an explicit refresh + // here too — the grid's `change` listener alone won't catch it. + SbppGroupsRefreshSelectAllButtons(); var rows = list.querySelectorAll('[data-testid="group-row"]'); for (var r = 0; r < rows.length; r++) { From ac754b9530fd36087bceef5a4295a8f4ed0b5df0 Mon Sep 17 00:00:00 2001 From: Cedric Mercier Date: Sat, 26 Sep 2026 14:24:03 +0200 Subject: [PATCH 3/4] fix(admin-groups): refresh select-all/none state after bulk toggle SbppGroupsToggleAllFlags dispatches its synthetic change event on the flag grid itself, which the grid listener filters out (it only reacts to input[name="flags[]"] targets). The Select all / Select none aria-disabled state was therefore never refreshed after a bulk toggle, failing admin-groups-select-all-flags.spec.ts. Refresh explicitly. --- web/themes/default/page_admin_groups_list.tpl | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/web/themes/default/page_admin_groups_list.tpl b/web/themes/default/page_admin_groups_list.tpl index 17133a035..70959201a 100644 --- a/web/themes/default/page_admin_groups_list.tpl +++ b/web/themes/default/page_admin_groups_list.tpl @@ -613,6 +613,10 @@ function SbppGroupsToggleAllFlags(checked) { var preview = document.querySelector('[data-testid="flag-bitmask"]'); if (preview) preview.textContent = SbppFoldFlags(grid) + ' bitmask'; + // The `change` above targets the grid itself, which the grid listener + // filters out (it only reacts to `input[name="flags[]"]` targets), so + // refresh the Select all / Select none state explicitly here. + SbppGroupsRefreshSelectAllButtons(); } /** @@ -640,10 +644,11 @@ function SbppGroupsToggleAllFlags(checked) { * assistive tech and sighted users alike while staying clickable. * * Call after anything that can change the grid's checked state without - * going through a user click on an individual checkbox: the master-detail - * `paintGroup()` repaint and the bootstrap call below (`change` events — - * manual clicks and the bulk toggle's own dispatch — are covered by the - * listener wired further down this file). + * going through a user click on an individual checkbox: the bulk toggle + * (`SbppGroupsToggleAllFlags` dispatches `change` on the grid itself, + * which the grid listener ignores), the master-detail `paintGroup()` + * repaint, and the bootstrap call below. Manual clicks on a checkbox are + * covered by the `change` listener wired further down this file. */ function SbppGroupsRefreshSelectAllButtons() { var grid = document.querySelector('[data-testid="flag-grid"]'); From 84e17a29cb6d6752594b9cc927c3c745f60a1c12 Mon Sep 17 00:00:00 2001 From: Cedric Mercier Date: Sat, 26 Sep 2026 14:42:24 +0200 Subject: [PATCH 4/4] test(e2e): force the idempotent Select all press on an exhausted grid Once every flag is checked the button carries aria-disabled="true", which Playwright's actionability check treats as disabled, so the plain click() hung until the 30s timeout. Force the click (a mouse user can still press it, it has no native disabled) and correct the template docblock that claimed aria-disabled kept it clickable for Playwright. --- .../specs/flows/admin-groups-select-all-flags.spec.ts | 8 +++++--- web/themes/default/page_admin_groups_list.tpl | 10 +++++----- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/web/tests/e2e/specs/flows/admin-groups-select-all-flags.spec.ts b/web/tests/e2e/specs/flows/admin-groups-select-all-flags.spec.ts index c7f961f22..1283d59b4 100644 --- a/web/tests/e2e/specs/flows/admin-groups-select-all-flags.spec.ts +++ b/web/tests/e2e/specs/flows/admin-groups-select-all-flags.spec.ts @@ -159,9 +159,11 @@ test.describe('flow: admin groups select-all permission flags (upstream #1436)', await expect(selectNone).toHaveAttribute('aria-disabled', 'false'); // Idempotent: a second press changes nothing and must not corrupt - // the preview. This is exactly the case aria-disabled (rather than - // the native `disabled` attribute) has to stay clickable for. - await selectAll.click(); + // the preview. The button now carries aria-disabled="true", which + // Playwright's actionability check treats as disabled, so force + // the click: a mouse user can still press it (no native + // `disabled`), and that press must stay a no-op. + await selectAll.click({ force: true }); await expect(bitmaskBadge).toHaveText(`${expected} bitmask`); // ---- Save round-trips the folded OR-sum -------------------------- diff --git a/web/themes/default/page_admin_groups_list.tpl b/web/themes/default/page_admin_groups_list.tpl index 70959201a..a8222b1bc 100644 --- a/web/themes/default/page_admin_groups_list.tpl +++ b/web/themes/default/page_admin_groups_list.tpl @@ -637,11 +637,11 @@ function SbppGroupsToggleAllFlags(checked) { * no-op when everything is already on/off (it bails before dispatching * `change`, per the comment above it), and the #1436 spec exercises a * redundant "Select all" press on a fully-selected grid as a deliberate - * idempotency check. A `disabled` button can't receive a click at all - * (Playwright's actionability check would hang waiting for it), which - * would turn that intentional no-op assertion into a broken test. - * `aria-disabled` communicates the same "nothing left to do" state to - * assistive tech and sighted users alike while staying clickable. + * idempotency check. A native `disabled` button can't receive a click at + * all; `aria-disabled` communicates the same "nothing left to do" state + * to assistive tech and sighted users while the button stays pressable + * (the spec forces that click, since Playwright's actionability check + * treats aria-disabled="true" as disabled). * * Call after anything that can change the grid's checked state without * going through a user click on an individual checkbox: the bulk toggle