Repository navigation
💍 fix: Unclip the Keyboard Highlight Ring in Select Popovers - #125
Closed
dustinhealy wants to merge 3 commits into
Closed
dustinhealy wants to merge 3 commits into
dustinhealy wants to merge 3 commits into
Conversation
Inline @clickhouse/click-ui in vitest server deps and exclude .claude/** from test discovery. click-ui 0.9.1 ships ESM that imports .css files, which Node's loader cannot parse, so Vitest must process the package through Vite.
click-ui renders the select option list at the trigger width while the popover border-box loses 2px to its 1px borders, so the overflow hidden popover clips the outermost 2px of the options on the right. The keyboard-highlight ring click-ui draws on the active option (2px outline at -2px offset) falls entirely inside that clipped band on the right edge, so the ring rendered on three sides only. Inset the ring by the overhang with a scoped override on the click-ui modality attribute pair, following the existing scoped focus overrides in styles.css.
dustinhealy
changed the title
…oken click-ui paints the keyboard-highlight ring on popover options with its own accent token, which the app theme never overrides, so the ring rendered near-black in light mode while every other focus ring uses the outline token. The options are not buttons, so the global focus-visible override never reaches them; the existing scoped popover rule now also sets the outline color to var(--cui-color-outline), matching the app focus convention in both themes.
dustinhealy
marked this pull request as ready for review
August 11, 2026 23:04
Contributor
Author
|
@codex review |
dustinhealy
marked this pull request as draft
August 14, 2026 15:05
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
dustinhealy
marked this pull request as ready for review
August 14, 2026 15:23
7 tasks done
danny-avila
added a commit
that referenced
this pull request
Oct 28, 2026
…ers (#195) The keyboard-highlighted option in a click-ui Select popover showed its ring on three sides only. The popover is as wide as the trigger and clips its content (overflow: hidden) inside 1px borders, but the option list is also as wide as the trigger, so options overhang the clip box by 2px on the right. click-ui draws a 2px ring at a -2px offset, in the option's outer 2px, so the right side fell entirely in the overhang. The ring also used click-ui's accent, near-black in the light theme, instead of the app's focus color. A scoped rule in styles.css, next to the other focus-ring overrides, draws the ring of a keyboard-highlighted option in --focus-ring-color, 4px inside the option. data-input-modality is only set on click-ui menu panels, so the rule reaches click-ui Select, Dropdown and ContextMenu options only, and only while the keyboard drives them. Dropdown menus (the sidebar user menu, the row action menus) already showed a whole ring; they now draw the same ring as the Selects. e2e/a11y/menus.spec.ts (live backend) measures the highlighted option's ring in two Selects and the user menu, in light and dark mode: solid, in the focus color, and inside every clipping ancestor up to the menu. Supersedes #125 by @dustinhealy, whose root-cause analysis and inset ring this reuses.
Contributor
|
Thanks @dustinhealy for measuring exactly why the ring's right side was clipped. #195 is now merged and redoes this PR on current The Title method and Title timing selects you checked are native selects on Closing this in favor of #195. If you see the ring clipped anywhere else, please comment or open an issue. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Keyboard-navigating the options of any click-ui Select shows the highlight ring on only three sides; the right edge is cut off by the popover.
Root cause (measured in the browser): click-ui's Select popover content is
overflow: hiddenwith 1px borders, so its inner clip box is 2px narrower than the popover, but the option list and rows render at the full trigger width and overhang the clip box by exactly 2px on the right. click-ui's keyboard-highlight rule draws a 2px outline atoutline-offset: -2pxon the active option, placing the ring in the outermost 2px band, so the right band falls entirely inside the clipped overhang. Pixel sampling on the highlighted row confirmed zero outline pixels between the option's visible right edge and the popover border. Systemic to every click-ui Select in the app and theme-independent.Fix: a scoped override in
styles.css, following the existing scoped focus-override convention, insets the ring past the overhang:[data-input-modality='keyboard'] [data-highlighted] { outline-offset: -4px !important; }. The selector pair only matches click-ui popover descendants because the input-modality attribute lives on the click-ui popover content element; other click-ui popovers (e.g. the user-menu dropdown) also pick up the inset, verified visually with no regression (their rings stay complete and comfortably inside the menu). The width mismatch itself is a click-ui bug worth reporting upstream; this override keeps the ring fully visible regardless.Change Type
Testing
Verified in the browser at 1280x800 with keyboard modality: the highlighted option's ring now renders on all four sides (computed
outline-offset: -4px, outline pixels present at the right band inside the popover), checked on multiple selects (Title method, Title timing). jsdom cannot assert outline geometry, so the proof is visual plus the pixel sampling; no unit test carries signal for this rule.Full gates green: bunx tsc --noEmit, bunx eslint src/ --max-warnings 0, bunx vitest run (799/799), bun run build, prettier on the touched file.
Before/after proof:
Test Configuration:
Checklist