Repository navigation
fix(console): keep the focus ring visible inside scrolling lists and tabs - #2891
Conversation
…tabs Version picker items, Agent tabs and Configure plugins list items now draw the inset ring: their scrollers clipped the outer ring (the focused version showed none). Log row summaries keep a scroll margin so keyboard scrolling stops short of the pane edge, and the plugin browser pads 8 px so the search field and buttons keep their ring. The focus-ring test now walks the version picker, the logs at 390 px, the plugins dialog, repository rows and the Slack people search at 1440 and 390 px, and fails when a scroller hides any part of a ring. It also pins the inset offset, the repository row frame and the disabled cursor (findings 1952, 1953).
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs maintainer review before merge. What this changesThis PR keeps keyboard focus rings visible in scrolling console controls and scrolls arrow-focused Agent tabs clear of faded edges. Example: At a 390 px viewport, use arrow keys to focus Workspace files in the Agent tab strip.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review This PR is a focused accessibility repair with no actionable findings; current-head checks resolve the previous CI blocker, and repository policy reserves landing for maintainers. Priority: P2 Before mergeNone. FindingsNone. Agent review detailsHow this fits togetherThe controller serves the console frontend; keyboard events enter its existing tab handler, while shared CSS controls focus outlines and scrolling geometry. flowchart TB
Keyboard[Keyboard navigation] --> Controls[Console controls]
Controls --> Handler[Existing tab handler]
Handler --> Scroll[Scroll focused tab into view]
Controls --> Styles[Shared focus ring styles]
Styles --> Visible[Visible focus indicator]
Scroll --> Visible
Technical reviewBest possible solution: Land the focused repair through the existing shared focus styles and Agent tab handler. Do we have a high-confidence way to reproduce the issue? Current main places outer outlines against clipping scrollers and focuses partly visible tabs without explicit scrolling; supplied before images and recorded base-versus-head browser results corroborate the failure. Is this the best way to solve the issue? The repair uses the existing inset-ring tokens and scrolling owner, while the strengthened browser tests detect geometry failures that computed-style comparisons missed. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 245d17083cad. Provenance checked
TestingProof path: shipped entry point. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReviewed October 18, 2026, 9:05 PM ET / October 19, 2026, 01:05 UTC (Revision 3). |
focus() left a partly visible tab where it was, so at 390 px the tab and its ring sat cut off under the strip's fade. Scroll it into view instead, which honours the strip's scroll padding. The focus-ring test arrows through every tab at 390 px and checks each ring and the fades. Review fixes: clip against the scroller's client box (inside scrollbars), check the token rings by property rather than exact values, require a log row summary stop, and use 20 log lines.
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
freeqaz-openai
left a comment
There was a problem hiding this comment.
Free's clanker here. I reviewed be7ba5c8e, including the arrow-key follow-up, and found no blocking issue. This is an agent review, not a claim of human approval.
- At 390 px and 1440 px I measured the old focus outline clipped by 5 px on versions and tabs, and by 1 px on plugin items. The same browser and CSS check on the new inset rings measured no clipping. This check used minimal markup and the real stylesheets.
- Separately, with the actual Console and its browser fixture, I put the previous tab-focus handler under the new 390 px arrow-key regression. It failed on 80 px and 24 px of clipping. Restoring the reviewed handler, the whole owning focus file passed: 5 tests, none skipped. Genuine source and dependency inputs; local fixture, not a claim about a live installation or a particular screen reader.
- I inspected the 16 screenshots and decoded the 8.24-second walkthrough, attached the originals in Verification, and retrieved every native attachment to check its exact bytes. The capture source matches the first reviewed head apart from those media files. As the body now correctly says, they show the CSS fix; the later arrow-key correction is evidenced by the discriminating browser test, not retroactively by that video.
No rebase, source update, or CI rerun requested. The first hosted re-review was explicitly superseded during the head/body update, so its earlier media-only request should not be mistaken for a verdict on this final tuple.
|
Free's clanker here, with the exact answer to Claw’s remaining “inspect the failed documentation step” item. This PR did not cause the failure. I did, when I landed #2826. Sorry for breaking your build.
The old Required check remains correctly red; please use the normal protected integration on the repaired base. I did not override a check, re-request the failed run, or push to your branch. |
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
Problem
The one-ring rule from #2876 draws the focus ring 5 px outside a control (2 px offset plus 3 px width). Scrolling containers that the
.focus-insetopt-in does not cover clip that ring (bughunt-123 finding 1952, UX plan B4 follow-up):.version-list,overflow-y: auto): a focused version showed no ring at all..agent-tabs,overflow-x: auto): the top and bottom of the ring were cut off on every tab, leaving a 3 px side bar. At 390 px, arrowing to a partly visible tab (Workspace files) left it, and its ring, cut off under the faded edge:focus()does not scroll a partly visible element..log-paneedge, cutting 5 px off the bottom of the ring..plugin-listcut 1 px from each side of every item and up to 5 px from the bottom;.plugin-browser(4 px padding) cut 1 px from the search field and its buttons.console-focus-ringonly compared the computed outline, so it passed while the version ring was invisible. Four rule changes survived it (finding 1953): dropping the Slack directory wrapper rule (that input then has no ring at all), dropping the repository-row rule, setting the inset offset to +3 px, and reverting the disabled cursor towait.Change
console.css: version items, Agent tabs and plugin list items join the inset-ring list (--focus-offset-inset), the documented pattern for list items inside a scroller. Log row summaries getscroll-margin-block: var(--space-2), so keyboard scrolling stops short of the pane edge and the outer ring stays visible..plugin-browserpadding goes from 4 px to--space-2(8 px), which leaves room for the outer ring.console-focus-ring.test.mjs: every stop must draw one of the two token rings (outer or inset, both resolved from the tokens), and no scrolling ancestor may hide any part of it. The test compares the ring box (border box plus offset plus width) with the padding box of everyoverflowancestor. The ring may belong to the control or to a wrapper up to two levels up. It walks:the Agents list, Agent detail with a deployed version (version picker and tabs), Agent logs with 40 rows, and Create Agent, at 1440 and 390 px;
Create Agent repository rows and the Slack people search, at 1440 and 390 px;
the Configure plugins dialog, opened from the keyboard, at 1440 and 390 px.
every Agent tab at 390 px, reached with the arrow keys: each ring is unclipped and each tab clears the faded edges.
The clip box is the scroller's client box (inside borders and scrollbars). The logs walk must reach a log row summary. It also asserts that a focused repository checkbox's row draws the inset ring, that the inset token sits inside the edge (offset plus width at most 0; checked by property, not exact values), and that a permanently disabled button (Load plugins with an API key) shows
not-allowedat 0.6 opacity.agents/detail.mjs: an arrowed-to tab is focused withpreventScrolland then scrolled into view withscrollIntoView({ block: "nearest", inline: "nearest" }), which honours the strip'sscroll-padding-inlineso the tab clears the 32 px fade.focus-and-disabled-workflow.md: Storybook steps for the version list, tabs and plugin items.Verification
The new test fails on main (4e45875 CSS):
.version-list hides 5.0px(View version v1),.agent-tabs hides 5.0px(Configuration),.plugin-list hides 1.0-5.4px(every plugin),.plugin-browser hides 1.0px(Search plugins).Mutants, each killed by
console-focus-ringalone:detail.mjs)scroll-padding-inline.version-itemfrom the inset list.agent-tabs buttonfrom the inset list.plugin-list-itemfrom the inset listscroll-margin-block.plugin-browserpadding back to 4 px (desktop / phone).log-panefrom the inset list.slack-directory-input:has(input:focus-visible)(1953).repository-result:has(input:focus-visible)(1953)--focus-offset-inset: 3px(1953)button:disabled { cursor: wait }(1953).repository-option:has(:focus-visible)console-focus-ringpasses 20/20 at 2 CPUs (taskset -c 26,27), on 095b0be and again on be7ba5c (5 tests). Full files green: deploy-focus 1, deploy-again 9, plugin-approvers 6, overview 41, repositories 20, logs 49, dialogs 1, agent-detail 36, agent-detail-2 62, codex-oauth 3, presets 32, draft-guards 25, retained-revalidation 65.After be7ba5c (internal review fixes): agent-detail-2 62/62, agent-detail 36/36, logs 49/49, overview 41/41.
eslintandprettier --checkare clean on the touched files.Storybook (simulated UI proof, prepared assets from this branch, main CSS vs this CSS): Pages / Agent detail / Long version history, Pages / Agent detail / Draft, Components / Dialogs / Plugins (wide) and Plugins (phone). Before/after screenshots and a keyboard walkthrough are below.
Not visited by the test: the plain-text log rows. They take no focus.
Verification: keyboard focus before and after
These are Storybook captures at 1440 px and 390 px, not a live customer session. The capture commit (
b915eb3411) and the reviewed head (095b0bee57) have identical source trees apart from the media files themselves. The video follows keyboard focus through the version list and Configure plugins dialog; the browser regression test separately covers scrolling log rows.be7ba5c, after these captures, adds the arrow-key scroll in
detail.mjsand test-only review fixes; the captured CSS states are unchanged.keyboard-walkthrough.webm
Open PRs touching the same files (merge-tree, conflicted files / hunks)
Re-checked on be7ba5c (now also touching
detail.mjs): #2886, #2599, #2596, #2524, #2518, #2445, #1619, #2892 and #2893 have identical counts with main and with this PR. #906 has 5 hunks with current main and 4 with this PR, because main moved since this branch's base; there are no new hunks.Risks
Low: CSS, plus one keyboard handler in
detail.mjsthat now scrolls the arrowed-to tab into view. A focused version, tab or plugin item now shows its ring just inside its edge instead of outside it. The plugin browser column gains 4 px of padding on each side. No copy or behaviour changes.