fix(css): raise the remaining 44 px controls to the 48 px touch-target minimum (#235) - #239
Conversation
) test-touch-targets.js now loads every local stylesheet in index.html order and has a viewport meta, so the iPhone harness really is 390 px wide and the max-width: 640px rules apply. It lists the controls from #235 and the other 44 px controls found on master (the coarse-pointer Kpa-clawbot#630 block, Leaflet zoom and live toggles, the channel-proposals buttons, the live node-filter hit area and region tap pad). The channel header's back button is .ch-back (40 px); .ch-back-btn has no markup. test-issue-2052-touch-target-e2e.js raises the mobile sidebar check to 48 and measures, at 390x844, the open channel's back button, every sender avatar and the add-channel dialog's close button, with no horizontal overflow. Both fail on master: 24 harness assertions, 4 E2E steps. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t minimum (#235) Every control that was still sized at 44 px now uses the 48 px house minimum from Kpa-clawbot#2052: - .theme-toggle; .modal-close / .ch-modal-close and .ch-avatar.ch-tappable (max-width: 640px); .suggest-claim; .ch-back-btn (no markup renders it). - The channel header's back button, .ch-back, which was 40 px. - The Kpa-clawbot#630 coarse-pointer block: .filter-bar/.filter-group .btn, .tab-btn, .region-pill, .region-dropdown-trigger, .multi-select-trigger, .node-count-pill, .analytics-time-range button, .detail-back-btn, .filter-toggle-btn, and the .filter-bar input/select height. - Leaflet zoom buttons and the live page's map toggles. - The live region dropdown's ::after tap pad and the node-filter hit area, whose inline style in live.js moves to a live.css rule. - The channel-proposals toolbar and action buttons. At 390 px the region pills no longer fit beside the channel title and + Add. The mobile sidebar header kept max-height: 56px, which on master already let the list cover the lower half of the 44 px pills; with 48 px pills + Add wrapped under the list and could not be tapped. The cap goes, title and + Add share the first row and the pills take the second (115 px). test-issue-1224-channels-mobile-ux-e2e.js now checks that the header holds its controls and the list starts below it (<=120 px) instead of the <=60 px bound that only the cap kept true. .feed-show-btn and .legend-toggle-btn keep their 44 px rule: live.css hides both at <=640px, the only width where it applies. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…235) The header check ran only at 375 px without touch, where the region pills are 24 px, so dropping the rule that puts the pills on their own row (header grows to three rows, 139 px) passed. The check is now a helper and also runs at 390x844 with touch, where the pills and + Add are 48 px; that mutant now fails there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rapport — CS-pve-agent3 PR#239 #235 — head bd56fecStatus: Draft, all acceptance criteria met and tested; CI green on attempt 2 (attempt 1 hit a pre-existing, time-dependent Evidence marks: [T] a test run here (command and result below), [A] ad-hoc measurement or screenshot taken here with Playwright, [K] known from elsewhere, not re-run here. Local setup, as in CI: a Go server built from this branch, on a copy of Acceptance criteria
One mutant survived, as expected:
The Selected measurements [A] (master → branch):
CI (run
|
| Job | Attempt 1 | Attempt 2 (failed jobs re-run) |
|---|---|---|
| Go Build & Test | success | (not re-run) success |
| Playwright E2E Tests | failure: test-issue-1122-details-row-clamp-e2e.js, [desktop-1200] and [tablet-900] "advert link in Details is not hit-testable" (R5-D4 300D Rak) |
success. In the log: test-touch-targets.js: OK, test-issue-2052-touch-target-e2e.js: 14 passed, 0 failed, Kpa-clawbot#1224 6/6 including the new 390×844 touch step, test-bottom-nav-1061-e2e.js: 31 passed, Kpa-clawbot#1122 passed 18 failed 0 |
| Build & Publish Docker Image | skipped (E2E failed) | success |
| Release Artifacts | skipped | skipped |
| Deploy Staging / Publish Badges & Summary | skipped (fork guard) | skipped (fork guard) |
The attempt-1 failure is not from this PR [T]:
- It reproduces locally on master's
public/as well as on this branch, once the freshened fixture is about 27 minutes old: 2/2 runs fail on each, with the same message. - On a fresh fixture both pass 3/3.
- The measured geometry shows the link's box growing to about 52 px tall (it wraps), so the probe point at
r.top + r.height / 2lands on thetdor the clip, not the link. - The unrelated
codex/issue-96-hide-control-packetsrun failed on the same assertion at 07:46 UTC [A]. - This PR changes nothing on the packets table at desktop width. Its desktop-visible changes are the nav theme toggle, map zoom and live toggles.
Worth its own issue.
Residuals
- Design choice to review: the 390 px channel header grows from 56 px (clipped) to 115 px. The alternative is the region dropdown in the channel header, which keeps one row but changes desktop and needs three channel E2Es reworked. Details are in the PR description.
.ch-back-btnhas no markup since regression(channels mobile): #1227 picker rows too cramped — restore prod's roomier layout (keep header wins) Kpa-clawbot/CoreScope#1367. It is raised to 48 for consistency, but it could be deleted in a cleanup PR, together with the dead.feed-show-btn/.legend-toggle-btn44 px rule in live.css.- Not in scope, all pre-existing and identical on master [A]:
- At 768 px touch and on desktop the channel sidebar's region pills do not wrap; the last pills (SFO, SJC) are clipped by the sidebar.
- The touch-only rules (
≤640pxandpointer: coarse) leave some controls under 48 at 768 px touch:.ch-avatar.ch-tappable36,.ch-modal-close32×29,.legend-toggle-btn38×39. - The node-detail back arrow sits at the top of its box, not centred.
- Controls below 44 px were not part of this issue:
.feed-hide-btnand.live-controls-toggle(36, under the bug(live mobile): chrome-reduction pass 2 — header to single row, hide top navbar, collapse VCR >6h buttons Kpa-clawbot/CoreScope#1234 header cap), VCR buttons (32, test(ingestor): pin failed-tick, NULL route type and originator exclusion (#200) #207), and the inline-styled close buttons of the area and packet-path map dialogs.
- The generic
.modal > .modal-closeis checked in the harness only. No observer in the E2E fixture has a neighbour sparkline, so the real dialog cannot be opened there. test-issue-1122-details-row-clamp-e2e.jsis time-dependent (see CI above); it needs its own issue.- Workflow note: the harness also runs
live.css,home.cssand the other local sheets now. A future sheet that changes a listed control will show up there, as intended.
Review — CS-Macmini PR#239 touch-targets-48 — head bd56fecDom: APPROVE med nits F1 is a one-line CSS fix and should go in before merge. Independent, read-only review. Evidence marks: [T] a test run here, [A] an ad-hoc measurement or screenshot taken here with Playwright, [K] known from elsewhere, not re-run here. Setup [T]: Findings
Everything else I checked holds up; see below. 1. Every #235 control measures ≥48×48Real DOM, master → branch [A]:
My own grep [A] of
2. Layout at 390 px and on desktop
Channel header, my assessment: I agree with the design choice (pills mode, 2–4 regions), with F1 fixed for the dropdown mode.
[A] from route-mocked Why I agree:
The alternative, the region dropdown on mobile only, would save about 54 px in the 2–4-region case. But it needs breakpoint-dependent JS ( Simpler suggestion: keep this PR's CSS, add the one-line menu anchor from F1, and optionally F3. 3. Tests and mutants[T] Each listed control is asserted in Mutants, each a copy of the merged tree served by its own server [T]:
10 of 11 killed. 4. Colours, tokens, behaviour, XSS
5. Rules
Tests run
Not verified
|
…and the pill label (#239) Review findings on #235: - F1: test-issue-1224-channels-mobile-ux-e2e.js mocks 6 regions, so the channel header renders the region dropdown, and checks at 320, 360, 390, 430 and 640 px touch that its open menu stays inside the viewport with every option hit at its centre. With order: 1 the trigger sits at the right edge and the left-anchored menu runs off-screen. - F2: test-touch-targets.js measures .live-node-filter-hitarea in its real .live-toggles context and checks that it keeps cursor: text, which .live-toggles label (cursor: pointer) overrides. - F3: test-touch-targets.js checks that a 48px .region-pill centres its label. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… and centred pills (#239) - F1: at <=640px the channel header's region dropdown menu opens from the trigger's right edge (left: auto; right: 0), since order: 1 puts the trigger at the right edge of the strip. - F2: the live node filter's hit-area rule is scoped to .live-toggles, so it wins over .live-toggles label and keeps the text cursor it had as an inline style before #235. - F3: .region-pill centres its label (justify-content: center). Only a pill wider than its content, the 48px touch minimum, changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rapport — CS-pve-agent1 PR#239 runde 2 — head 23778ebStatus: F1–F3 fixed with red-then-green tests, each mutant killed. Master Review feedback addressed (commit Evidence: [T] test run here · [A] measurement or screenshot here · [K] from the review or issue, not re-run. Findings
In the harness, Tests run locally
Screenshots (local, 390×844 touch, 6 mocked regions, channel header with the region menu open) [A]
The screenshots were not uploaded; they are described here only. CI (run 37292852773, head
|
| Job | Result |
|---|---|
| ✅ Go Build & Test | pass |
| 🎭 Playwright E2E Tests | pass |
| 🏗️ Build & Publish Docker Image | pass (local build; push guarded) |
| 📦 Release Artifacts / 🚀 Deploy Staging / 📝 Publish Badges | skipped |
No rerun was needed; neither #244 nor #250 occurred.
Residuals
- On the live page the hit area measured 0×0 in my check, because the controls panel is collapsed on load. Its 48 px height is covered by the harness only (as before), and Macmini's M9 (inline style restored in
live.js) is still not caught. That was optional and is not done here. - I did not test on real devices; Chromium emulation only.
- Everything else is unchanged from round 1 (see the author's report and the review).
Review — CS-Macmini PR#239 runde 2 — head 23778ebDom: APPROVE med nits This is a read-only re-review of round 2. F1–F3 from my round-1 review are fixed, each with a test that is red before and green after, and F4 is unchanged and tracked in #249. Nothing blocks the merge. The two new nits below already exist on master and belong in follow-ups, not in this PR. Evidence: [T] test run here · [A] measurement or screenshot here · [K] from the author's report or CI, not re-run. Setup:
1. My round-1 findingsF1: the channel-header region menu, ≤640 px. Fixed. [T][A]
F2: the live node-filter hit area. Fixed. [T][A]
F3: region-pill labels. Fixed. [T][A]
F4. Not changed, and tracked in #249, as agreed. [A] 2. Red before, green after, and mutants
Mutants, each applied alone to the merged tree:
3. Merge commit
|
| # | Severity | Finding | Since | Action |
|---|---|---|---|---|
| F1 | — | The region menu was off-screen ≤640 px | round 1 | Fixed [T][A] |
| F2 | — | The hit-area cursor was pointer |
round 1 | Fixed [T][A] |
| F3 | — | Pill labels were off-centre | round 1 | Fixed [T][A] |
| F4 | Low | Add-channel dialog buttons at 32 px | round 1 | Tracked in #249 [A] |
| N1 | Low | Between 641 and 767 px, the channel region menu (6 regions) runs about 40 px past the right edge: 461–681 at 641, 520–740 at 700, 587–807 at 767. Options are still hit at their centre, and there is no page overflow. Identical on master. | pre-existing | Follow-up issue; not this PR [A] |
| N2 | Low | The region dropdown items are 28 px tall at every width, touch included. They are below the 48 px minimum, but they were never 44 px controls, so they are outside #235's 44→48 scope. Identical on master. | pre-existing | Follow-up issue (touch targets) [A] |
| N3 | Info | MH survives: nothing catches the inline 44px style coming back in live.js |
round 1 | Optional, as before [T] |
Tests run (merged tree vs. origin/master c12aa60b)
sh test-all.sh: 218 passed, 0 failed (218 files).node test-frontend-helpers.js: 707 passed, 0 failed. [T]- 41 E2E files against the local server, all exit 0 [T]:
- every channels, live and touch-target E2E listed in
deploy.yml, includingtest-issue-122416/16,test-issue-205214/0 andtest-touch-targetsOK; - the self-starting
test-channel-proposals-e2e.js24/0 andtest-channels-client-state-152-decrypt-e2e.js18/0.
- every channels, live and touch-target E2E listed in
- CI run 37292852773 on
23778ebd: Go Build & Test, Playwright E2E and Docker build passed; release, deploy and badges were skipped. [K] I confirmed this viagh run view.
Screenshots, 390×844 touch, 6 mocked regions, menu open [A]
- Before (round 1, light): "+ Add" and "All Regions" sit at the right of the header. The menu opens under the trigger at x=300 and is cut at the right edge. Only "All", "SJC - Sa…", "SFO - Sa…", "OAK - Oa…", "MRY - Mo…", "SMF - Sa…" and "LAX - Los…" show, and the option centres are off-screen.
- After (round 2, light and dark): the menu opens leftwards from the trigger's right edge (x=160–380) and sits fully on-screen over the channel list. All labels are complete: "All", "SJC - San Jose", "SFO - San Francisco", "OAK - Oakland", "MRY - Monterey", "SMF - Sacramento", "LAX - Los Angeles". Dark mode uses the themed card background and border.
- Pills mode (fixture, 4 regions), after: "Region:" followed by All, MRY, OAK, SFO and SJC as 48×48 pills on the second header row, with labels centred.
Not verified
- Real devices: Chromium emulation only.
- Firefox and WebKit.
- The live hit area on a real page with the controls panel expanded by a user: measured through computed style and the harness only.
- The Docker image.
- Staging and production: not accessed.
Review — CS-Macmini PR#239 runde 2 — head 23778ebDom: APPROVE Read-only re-review of round 2 ( Setup [T]:
Findings
No new issues introduced by round 2. 1. Round-1 findingsF1 — fixed.
F2 — fixed.
F3 — fixed.
F4 — unchanged, tracked in #249 (open: "add-channel dialog buttons are 32 px tall on touch") [T]. 2. Tests red before, green after
[T]. The new Kpa-clawbot#1224 steps tap the real trigger, wait for the menu, and check the viewport bounds, the hit at each option's centre and page overflow at five widths. That is the test I suggested in round 1. Mutants: each a copy of merged
6 of 7 are killed. The survivor is the known round-1 residual. 3. Merge commit
|
Relates to #235
Every control in
public/that was still sized at 44 px now uses the 48 px house minimum from Kpa-clawbot#2052 (as.fav-stardid in #229). Tests first: two test files go red on master, then the fix turns them green.public/live.js, where an inlinestyle="… min-height:44px …"moves to alive.cssrule. No colours added; only sizes, oneorderand the removal of onemax-height.deploy.yml, 1 inrelease-fast-path.yml;.github/is not in the diff.scripts/check-xss-sinks.sh --diffis clean.The list from the issue, checked against master
aff158c7.theme-toggle.modal > .modal-close,.ch-modal-close(≤640 px).ch-back-btn.ch-back, the channel header's real back button.ch-avatar.ch-tappable(≤640 px).suggest-claim.detail-back-btn,.filter-toggle-btn(pointer: coarse)Other 44 px controls found
pointer: coarseblock:.filter-bar .btn,.filter-group .btn,.tab-btn,.region-pill,.region-dropdown-trigger,.multi-select-trigger,.node-count-pill,.analytics-time-range button, and theheightof.filter-bar input/selectstyle.css.leaflet-control-zoom astyle.css, every map.live-leaflet-toggle alive.css::aftertap padlive.css.live-node-filter-hitarealive.jslive.cssrule (.live-toggles .live-node-filter-hitarea, see round 2).ch-proposals-toolbar button,.ch-proposals-actions buttonchannel-proposals.cssLeft at 44 on purpose:
.feed-show-btnand.legend-toggle-btnin live.css's@media (max-width: 640px)block. The same block hides both withdisplay: none !important, so the 44 px rule never renders. Also left alone: the 44 px values that are not controls (.live-node-detail .panel-headermin-height, the Kpa-clawbot#1234 live header cap, a 44 px column in analytics).The channel sidebar header at 390 px
At 390 px the mobile sidebar header holds the title, the region pills and + Add. Master caps it with
max-height: 56px(Kpa-clawbot#1224), and that cap was already hiding an overflow: the channel list was drawn over the lower half of the 44 px pills. With 48 px pills, + Add wrapped to a third row under the list and could not be tapped. The new mobile E2E step caught this.The cap is removed, and the pills get
order: 1: title and + Add share the first row and the pills take the second. The header is 115 px (was 56 px clipped).test-issue-1224-channels-mobile-ux-e2e.jsused to assert ≤60 px, which only the cap kept true. It now asserts that the header holds all its controls, that the list starts below it, and that it is ≤120 px, at 375 px and, new, at 390×844 with touch.This is a design trade-off. The alternative was to keep a one-row header by putting the channel region filter into dropdown mode (
RegionFilter.init(el, { dropdown: true }), as packets and live already do). That keeps the header at ≤60 px, but it changes the desktop sidebar too and needs three channel E2Es that click the pills (152,152-decrypt,154-155) reworked. If you prefer that, say so.Screenshots (before = master, after = this branch, 390×844 touch unless noted): https://github.com/dborup/CoreScope/tree/codex/issue-235-screenshots, an evidence-only branch that is not meant to be merged.
Tests
test-touch-targets.js(CI E2E step, no server) lists every control above, 24 more assertions. Two harness fixes were needed to measure them at all:max-widthrule ever applied in this test.index.htmlorder, not onlystyle.css.home.cssrestyles.suggest-claim, andlive.cssandchannel-proposals.csshold controls of their own.(pointer: coarse)and(max-width: 640px), and checks the::aftertap pad's box.test-issue-2052-touch-target-e2e.js, mobile 390×844 touch, in the real DOM:.region-pillwas the 44 px exception)..ch-backis ≥48, unclipped and hit at its centre.test-issue-1224-channels-mobile-ux-e2e.js: the header check described above.On master: the harness fails 24 assertions, the Kpa-clawbot#2052 E2E fails 4 steps, and the Kpa-clawbot#1224 header check fails.
Round 2: review feedback
This round merges
origin/master(9989b97f) with a merge commit, then adds8a8c72ab(tests, red before the fix) and23778ebd(fix).RegionFilterrenders a dropdown, andorder: 1puts its trigger at the right edge of the strip. Its menu was anchoredleft: 0, so at 390 px it ran from 302 to 522 px. It now opens leftwards from the trigger's right edge:.ch-header-region .region-dropdown-menu { left: auto; right: 0; }, in the same≤640pxblock. At 390 px it spans 160–380 px. Desktop is unchanged..live-toggles .live-node-filter-hitarea(specificity 0,2,0). It wins over.live-toggles label(0,1,1,cursor: pointer), so the label keepscursor: textas before fix(css): raise the remaining 44 px controls to the 48 px touch-target minimum #235..region-pillgetsjustify-content: center. Only a pill wider than its label changes, which means the 48 px touch minimum; desktop pills are content-sized.Tests:
test-issue-1224-channels-mobile-ux-e2e.jsmocks 6 regions through/api/config/regions; the fixture has 4, so it never shows the dropdown. At 320, 360, 390, 430 and 640 px touch, the step checks that the header strip holds its controls, and that the open menu is inside the viewport with all 7 options hit at their centre and no page overflow. Before the fix, 5 steps fail.test-touch-targets.jsmeasures the hit area in its real.live-toggles/.live-node-filter-wrapcontext and checkscursor: text(F2). It also checks that a 48 px.region-pillcentres its label within 1 px (F3).Perf
There is no hot path: static CSS values only, no JS logic.
🤖 Generated with Claude Code