Conversation
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: 9d3afacfe2
ℹ️ 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".
| customColors?.['rgb-border-control'] === undefined && | ||
| customColors?.['rgb-border-medium'] !== undefined | ||
| ? { 'rgb-border-control': customColors['rgb-border-medium'] } |
There was a problem hiding this comment.
When a stored or environment theme predating this token provides rgb-border-light but omits rgb-border-medium, this fallback leaves border-control at the bundled gray; when both differ, it substitutes the medium value. Before this commit, Field (and therefore Input/Textarea), Dropdown, ControlCombobox, SecretInput, and SelectDropDown all used border-light, so restoring such a theme now discards its configured control outline and can make controls indistinguishable on its custom surfaces. The same medium-only condition is duplicated in applyTheme.ts; the compatibility mapping needs to account for the controls that previously consumed border-light.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 37c3687 (with ed24833): the legacy fallback now weighs the theme's border-light first, then border-medium, the bundled role and its text, keeping the first that clears 3:1 on its own canvases, in both resolveTheme and applyTheme; registry.spec/applyTheme.spec cover light-only, quiet and illegible themes, and all 8 control-border scenarios pass on 37c3687.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e59cb96280
ℹ️ 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".
border-medium and border-light were the only visible edge on the shared form controls, and neither clears the WCAG 1.4.11 3:1 non-text floor in any palette. Raising them would also darken every separator and card edge, which the quieter dark borders deliberately keep one step off the page. border-control is a new theme role for the outline of a field, select trigger, dropdown, combobox trigger and OTP slot. Every bundled palette gives it a value that clears 3:1 on each canvas a form can sit on, the package ships the stock light and dark defaults so the outline holds before and without a theme definition, and a custom theme that paints its own medium border but not the new role keeps the outline it drew. semanticTokens.spec.ts holds the 3:1 floor for the role in all six modes.
A theme that paints its own borders but predates border-control took its medium border as the control outline, and a quiet one left every field without a visible edge. The fallback now keeps that border only where it clears 3:1 on the theme's own canvases and otherwise uses the theme's secondary text, in both resolveTheme and the legacy applyTheme adapter. The palette scenarios spell out their tags so the runner can find them.
…line Controls drew border-light (fields, dropdowns, comboboxes) or border-medium (select, OTP) before border-control existed, so a stored theme that painted either, or the canvases they sit on, takes the first of its light border, its medium border, the bundled role or its secondary text that clears 3:1 on its own canvases. A theme that names none of them keeps the bundled role.
…ter one exists Secondary and primary text join the candidates for a legacy theme's control outline, and when none clears 3:1 on the theme's canvases the one that comes closest is used rather than the secondary text by default.
37c3687 to
8d98b45
Compare
1 participant
Summary
The shared form controls (
Input,Textarea,Select,Dropdown,ControlCombobox,SecretInput,SelectDropDown,InputOTP) drew their only visible edge withborder-lightorborder-medium, and neither clears the WCAG 1.4.11 3:1 non-text floor in any bundled palette: default lightborder-mediumis 1.59:1 on white, and the dark field outline is about 1.2:1 on the dialog. Those roles also paint separators and card edges, which #16244 deliberately keeps one step off the page in dark, so raising them was not the fix.This adds a
border-controltheme role that only the control primitives use. Every palette gives it a value that clears 3:1 on every canvas a form can sit on (default 3.12:1, dark 3.25:1, ClickHouse 3.42:1 and 3.73:1 from its own slate and neutral scales, high contrast 21:1). The package ships the stock light and dark defaults, so the outline holds with no theme definition. A custom theme that sets its ownborder-mediumbut not the new role keeps the outline it already had. Separator roles are unchanged.Follow-up to berry-13#102, raised on #16262.
Type of change
Testing
Tested environments/configuration:
lcdev stack on this branch, stock theme in light and dark, Settings > General dropdownsAutomated tests:
semanticTokens.spec.ts: new 3:1 floor forborder-controlon every canvas in default, dark, both high-contrast modes and ClickHouse light and dark (fails on the oldborder-mediumvalue, passes now), plus a check that the package CSS defaults match the registryregistry.spec.tsandapplyTheme.spec.ts: fallback to a theme's ownborder-medium, explicit value preserved, bundled value when a theme sets no bordere2e/specs/mock/scenarios/control-border.spec.ts: a real Settings dropdown measured against its painted surface in all six palettes, dark separators unchanged, custom-theme fallbacknpx jest src/theme src/componentsinpackages/client: 46 suites, 574 tests passnpx tsc --noEmitinpackages/client, eslint, prettier and sort-imports on touched files: cleanScreenshots / recordings
Settings > General, stock theme. The dropdown outlines change; the header separator does not.
Risk / compatibility
Fields and select triggers get a darker outline in light and a lighter one in dark; that is the intended change. The new role's stock defaults live in
packages/client/src/theme/controls.css, loaded byThemeProviderthe wayhighContrast.cssis, rather than inclient/src/style.css. Stored and environment themes that nameborder-mediumkeep their previous control outline until they nameborder-control.Not in this change: app call sites that pass their own
border-border-lightorborder-border-mediumto a control (preset and memory dialogs, prompt and skill forms, agent list and a few others) still override the role, as doInputNumberandFilterInput. Clearing them lowerseslint-suppressions.jsoncounts, which an open stack owns, so that sweep follows once it lands. Error-state (border-border-destructive) and deliberately borderless inline fields are unaffected.Checklist