Repository navigation
🫥 fix: Remove the Focus Line Across the Top of Select Popovers - #200
Open
danny-avila wants to merge 1 commit into
Open
danny-avila wants to merge 1 commit into
danny-avila wants to merge 1 commit into
Conversation
An open click-ui Select moves focus into its search input. Unless the
Select shows its search, that input has zero height, and the app's
`input:not([cmdk-input]):focus-visible` ring drew it as a 2px line in the
focus color across the first option, in both themes and also when the
Select was opened with the mouse.
A new rule in src/styles.css removes the outline from
`[data-testid='select-search-input']` unless its class contains
`search-bar_show-search`. click-ui 0.9.1 and 0.15.0 render the same test id
and CSS module class, so a Select that shows its search keeps its ring. The
rule is separate from the input ring rule because a compound selector in that
rule's `:not()` would raise its specificity above
`.cui-field input:focus-visible { outline: none }` and bring back the bare
input ring inside click-ui fields.
e2e/a11y/select.spec.ts (live) opens the audit log Target type filter and the
configuration Jump to list in light and dark. It checks that the focused,
zero-height search input has no outline, then that the same input with
click-ui's shown-search class has height and a 2px solid ring. The two
Selects and the theme list are exported from e2e/live.ts as SELECTS and
THEMES, so other a11y specs can open the same Selects without copying the
setup; e2e/a11y/config.spec.ts now imports THEMES instead of declaring its
own.
Contributor
Author
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. 👍 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". |
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.
Pull Request Template
Summary
Every click-ui Select popover showed a 2px line in the focus color across its top, over the first option. It appeared in light and dark, and also when the Select was opened with the mouse. Affected Selects: the audit log's Target type filter, the configuration Jump to… list, and the Langfuse destination picker.
Cause. When the popover opens, click-ui moves focus into the Select's search input (
data-testid="select-search-input"). Unless the Select hasshowSearch, click-ui gives that input zero height, no padding andoutline: none. The app'sinput:not([cmdk-input]):focus-visiblerule insrc/styles.css(2px solid focus color, 1px offset,!important) overrides that outline. Chrome matches:focus-visibleon a focused text input however it got focus, so the zero-height ring showed as a line after mouse opens too.Fix. A rule placed right after the input ring rule removes the outline from that input unless the Select shows its search:
InternalSelectsets the test id, and a shown search adds the CSS module classsearch-bar_show-search__IhdJT, which has the same name and hash in both versions. The substring match also holds if the hash changes.:not(). A compound selector inside that:not()would raise the input rule's specificity to (0,3,1). It would then beat.cui-field input:focus-visible { outline: none !important }at (0,2,1) and bring back the bare-input ring that rule removes inside click-ui fields. The new rule, at (0,3,0), beats only the input ring rule.src/styles.cssand merge cleanly. This PR also merges cleanly with ⬆️ chore: Upgrade Click UI, i18next, TypeScript, Vitest and the Dev Tooling to Their New Majors #194.showSearchkeeps its ring on the visible search input. The app has one, the "Add field" picker inInlineField.tsx, but no page renders it today: custom endpoints are edited in the drawer, and the model spec preset renders field by field. The e2e covers this case with a stand-in (see Testing).Change Type
Testing
New live spec,
e2e/a11y/select.spec.ts. Like the configuration a11y spec, it needsE2E_LIVE=1andE2E_BASE_URL. It runs on the audit log Target type filter and on the Jump to… list (at 1024 px, where the list shows), in light and dark. Each test:outline-style: none;On
main, all four tests fail at theoutline-stylecheck (receivedsolid). With this change they pass. A probe that injects a rule removing the outline from every Select search input showed that the second half fails for that over-broad fix.The two Selects and the theme list are exported from
e2e/live.tsasSELECTSandTHEMESrather than declared in the spec.e2e/a11y/config.spec.tsnow importsTHEMESfrom there instead of keeping its own copy; it collects the same 66 tests as before.Overlap with #195. #195 adds
e2e/a11y/menus.spec.ts, which declares the sameTHEMESandSELECTS(same two Selects, same locators, same 1024 px viewport) locally. The two PRs still merge cleanly, because #195 doesn't touche2e/live.ts. Whichever merges second should delete those two local declarations frommenus.spec.tsand import them frome2e/live.ts. The definitions are identical, so that is a deletion plus an import.Live, against a LibreChat backend, in light and dark:
The Langfuse destination picker uses the same click-ui Select and gets the same fix. It wasn't opened live because it needs a Langfuse setup.
Test Configuration:
devbackend, MongoDB replica set.npx tsc --noEmit,npx eslint src/ e2e/ --max-warnings 0,npx prettier --checkon the changed files,bun run test:lint-rules,npx vitest run(3338 tests),bun run build. The ESLint config doesn't covere2e/, so the import order of the changed e2e files was checked against the project conventions by hand.e2e/a11y/select.spec.ts(4 tests), and the system-tab axe tests ofe2e/a11y/config.spec.tsin both themes after theTHEMESchange.Checklist