Conversation
Add clickui.json, the Click UI v0.12.0 tokens the ClickHouse theme cites, and a drift guard that matches every color, radius, font, shadow and motion value to its token, naming both on a mismatch. Correct the citations that did not resolve, set shadow2xs, motionNormal and the light code text from Click UI, and document the dark scrim as a deliberate deviation. Draw the global keyboard focus outline in ring-primary under any theme definition, keeping the default theme's black and white outline and the high-contrast rule unchanged.
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0f04a7fbe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| .dark:where([data-theme]:not(.high-contrast)) :focus-visible { | ||
| outline-color: rgb(var(--ring-primary)); |
There was a problem hiding this comment.
When a valid custom or stored legacy theme omits rgb-ring-primary, resolveTheme falls back to the bundled dark value 89 89 89; this selector now replaces the previous white focus outline with that gray merely because data-theme exists. Against the bundled dark surfaces, that fallback provides only about 2.1–2.8:1 contrast, below the 3:1 non-text contrast floor, so keyboard focus becomes difficult to perceive for partial dark themes that previously had a high-contrast white outline. Only opt into the themed outline when the theme actually supplies a suitable ring, or retain an accessible dark fallback.
AGENTS.md reference: AGENTS.md:L42-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0e1eb04. The themed outline now keys on a data-theme-ring mark that ThemeProvider sets only when the active mode (or legacy RGB set) names rgb-ring-primary; a ring-less dark theme keeps the white outline. Covered by @Scenario:focus-outline-ringless-theme-keeps-default (desktop light/dark, mobile) and ThemeProvider.spec.
| /** Init scripts run in the order they were added, so the second visit resolves dark. */ | ||
| for (const mode of ['light', 'dark'] as const) { | ||
| await openChat(page, mode, CUSTOM_RING_THEME); |
There was a problem hiding this comment.
On the dark iteration, the page still has the light-mode init script registered by the first navigation and adds a second script for dark mode. Playwright does not guarantee the evaluation order of multiple init scripts, so the older script may run last and restore color-theme to light, making this two-mode scenario nondeterministic. Use a single init script whose value can be updated, or isolate the modes in separate pages/tests.
Useful? React with 👍 / 👎.
Tab moves on from the previously focused element after a blur, so it never reached the prepended probe. Focus a sentinel outside the tab order first.
A truncated theme triplet such as '0 138' matched a source whose leading channels agreed. Compare only complete, finite triplets.
A theme that leaves rgb-ring-primary to the default resolves it to #595959, which a dark surface carries at about 2.3:1. The provider now marks the root with data-theme-ring when the active mode names its ring, and the global focus outline follows the ring only under that mark; every other theme keeps the default black and white outline. Each e2e mode also gets its own page instead of relying on init-script order.
This branch has not been deployed
1 participant
Pull Request
Summary
The ClickHouse reference theme cites a Click UI token beside every value, but nothing checked those citations: a few pointed at tokens that do not exist, and a theme edit or a Click UI release could drift silently. This adds
clickui.json, the 65 light and 68 dark Click UI tokens the theme cites, copied verbatim from tag v0.12.0 (e2b3d213, token files identical to the commit the theme was built from), andclickui.spec.ts, which matches every color, radius, font, shadow and motion value to its token and fails naming the theme key and the token. Every theme key must have a source or a written reason, and the snapshot may hold only cited tokens.Pure-data cleanup: corrected the citations that did not resolve, set
shadow2xs(shadow.5),motionNormal(transition.duration.smooth) and light code text (codeblock.lightMode.color.text, #282828). The dark scrim stays black as a documented deviation: Click UI's gray scrim would leave the dialog near 2.1:1. Mismatches that need a new registry token are filed as follow-ups: berry-13#177, #178, #179, #180, #181.The global
:focus-visibleoutline instyle.csswas a literal black/white that overrode every theme. A theme that names its ring now draws the outline inring-primary, at the same layer and specificity per mode. With no definition the default theme keeps its exact outline, and the high-contrast rule is untouched. This is a prerequisite for berry-13#146, which it does not close.Type of change
Testing
Tested environments/configuration:
Automated tests:
packages/client/src/theme/themes/clickui.spec.tsand ring-mark cases inThemeProvider.spec.tsx;npx jest src/themeinpackages/client: 9 suites, 309 tests pass. Mutating one theme value and one snapshot value failed with the key and token named.e2e/specs/mock/scenarios/focus-outline.spec.tsand a scenario inclickhouse-theme-shape.spec.ts: 9 scenarios, 27 runs pass.npm run static-checks -- --against origin/canarypasses;tsc --noEmitinpackages/clientis clean.Screenshots / recordings
Keyboard focus on the sidebar Account Settings button (it has no focus utilities of its own, so the global rule draws its outline). Before is base
canary, after is this branch; the conversation list behind the button scrolls between captures, the outline region is pixel-identical wherever the color did not change.Risk / compatibility
The themed outline follows the ring only when the theme names
rgb-ring-primaryfor the active mode (adata-theme-ringmark the provider sets besidedata-theme). A theme that leaves the ring to the default keeps the black and white outline rather than the default ring gray, which a dark surface carries at about 2.3:1. A dedicated focus-outline role is filed as a follow-up, as are the other Click UI decisions that need a new token.Checklist