Skip to content

fix(ui): ring the row control, not the whole settings row - #3525

Open
GabrielDrapor wants to merge 1 commit into
apache:mainfrom
GabrielDrapor:fix/settings-row-focus-ring
Open

fix(ui): ring the row control, not the whole settings row#3525
GabrielDrapor wants to merge 1 commit into
apache:mainfrom
GabrielDrapor:fix/settings-row-focus-ring

Conversation

@GabrielDrapor

@GabrielDrapor GabrielDrapor commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

What

设置里的行,在焦点落到行内控件上时会把整行画一圈焦点框。最明显的一幕是 设置 → 通用 → 「默认模型」:鼠标点开下拉框,整行出现一个蓝框,而 trigger 自己什么都没有。

Why

Astryx 的 Item(以及基于它的 ListItem,也就是每一个设置行)无条件写了 outline: 2px solid accent on :has(:focus-visible)

这条规则只对整行可点的行成立:那种行 Item 会渲染一个不可见的 <button>/<a> 当点击目标,整行的框就是它唯一的焦点指示。其他行里真正可聚焦的是右侧自带焦点态的控件,整行再画一圈就是第二个框。

下拉框那一幕的机制:Astryx Selector 的弹层是原生 popovertop layer 只改变它画在哪,不改变它在 DOM 里挂在哪 —— 它仍然是那一行的后代。带搜索框的 picker 打开时焦点落进弹层里的 input,:has() 一路向上匹配到行,于是整行亮框;trigger 自己的 wrapper 是弹层的兄弟节点,反而不亮。

How

packages/ui/src/styles.css 一条规则(产品 CSS 在最后一个 cascade layer,稳压 StyleX 原子类的 :not(#\#) 特异性):

@media (forced-colors: none) {
  .astryx-item:has(:focus-visible):not(:has(> :is(a, button):focus-visible)) {
    outline: none;
  }
}

:not(:has(> :is(a, button):focus-visible)) 是分界线:Item 根节点的直接子 <a>/<button> 只可能是那个不可见 tab stop(start/end/label 内容各自包在 span 里)。所以可点行在自己的 tab stop 上仍然亮框,只有焦点在自带指示的控件上时才不画 —— 一行同时具备两者时也各归各位。

forced-colors: none 这个 gate 是必需的:Windows 高对比度下 box-shadow 被系统抹掉、所有 border 被映射成同一个系统色,Astryx field 聚焦前后完全一样,而 trigger 按钮自己写死了 outline: none。那里整行的 outline 是唯一幸存的焦点指示(outline 是 forced-colors 唯一保留的焦点属性),所以在高对比度下这一行保留它 —— 双重框是两害相权的轻者。

另一种修法是去控件那一层补 —— 需要产品 CSS 越过设计系统伸进 Selector / Switch / SegmentedControl / Button 各自的内部,每个控件一条 forced-colors 规则,把它们本该自己画的东西补回来,不划算。

影响面

在真实窗口里对 16 个设置页做了 Tab 全扫(每页 50–60 次),逐行量 outline:

修复前 修复后
通用(默认模型/权限模式/思考级别、语言、各开关、按钮) 整行亮框 只有控件亮
工作区 / 记忆 / 每日回顾 / 关于 / 权限与能力 整行亮框 只有控件亮
模型(连接行)、远程接入(catalog 行) 整行亮框 不变 —— 整行可点,那是它自己的指示
外观(主题卡片) 卡片亮框 不变 —— SelectableCard 本身就是那个 radio

鼠标操作只有「默认模型」看得见 —— 只有它带搜索框,焦点才会跳进弹层;其他 picker 焦点留在 trigger 上,鼠标点击不产生 :focus-visible。其余各条需要键盘 Tab 才会显现。

Tests

新增 apps/desktop/e2e/settings-row-focus-ring.spec.ts(4 条),读计算样式而不是截图:

  1. 弹层打开时该行不画框 —— 先 waitForFunction 等焦点真的进入已打开的 popover 再断言(Astryx 是在 requestAnimationFrame 里才聚焦搜索框的,抢跑会让旧实现也误绿)
  2. 键盘焦点亮控件不亮行 —— 同时断言控件的焦点态确实还在
  3. 整行可点的行仍然亮框 —— 防止修过头
  4. forcedColors: 'active' 下该行保留框,且实测控件此时聚焦前后样式一致

对照跑过三态:

1 2 3 4
无规则(修复前)
有规则但无 gate
当前

另跑 settings.spec.ts + accessibility-coverage.spec.ts:10 passed。scripts/ci-test-plan.mjs 命中 astryx_surface / code / e2e / storybook,workspaces packages/ui, apps/desktop

AI 参与说明

按 CONTRIBUTING.md:本次改动由 Claude Code 实质性参与(诊断、实现、测试),commit 带 Generated-by: Claude Code。改动另经 Codex 独立 review 两轮,第一轮提出的 forced-colors 焦点指示丢失、e2e 时序竞态、缺 trailer 三点已在本 commit 中处理。最终的人工 review 与合并决定归维护者。

Co-Authored-By: Claude noreply@anthropic.com

Astryx's Item draws `outline: 2px solid accent` at `:has(:focus-visible)`
unconditionally. That is correct only for a clickable row, where Item
renders an invisible <button>/<a> as a direct child and the row outline is
that button's only focus indicator. Everywhere else the focusable thing is
a real control that rings itself, so the row drew a second outline around
the entire label + description + control band.

Opening the 默认模型 picker showed it at its worst. A Selector popup is a
native `popover`: the top layer moves where it paints, not where it sits in
the DOM, so it stays a descendant of the row. Focus moves to the popup's
search input on open, `:has()` walks up to the row, and the row rings while
the trigger — a sibling of the popup — does not.

A tab sweep over all 16 settings pages in a real window found the same
doubled ring on 通用, 工作区, 记忆, 每日回顾, 权限与能力 and 关于; the ring
is now narrowed to the row's own tab stop, which leaves the clickable rows
on 模型 and 远程接入 (and the SelectableCard on 外观) untouched.

The rule is gated on `forced-colors: none`. Under Windows High Contrast the
premise does not hold: forced-colors drops box-shadow and repaints every
border in one system color, so an Astryx field reads the same focused as
resting, and outline — which forced-colors preserves — is the only
indicator left. There the row keeps its ring.

Co-Authored-By: Claude <noreply@anthropic.com>
Generated-by: Claude Code
Claude-Session: https://claude.ai/code/session_01EbmGcSNMLJMVinv5FWit3d

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — nice diagnosis. The row drawing its own ring whenever anything inside it takes focus is correct for a whole-row target and wrong for a row that contains controls, and :not(:has(> :is(a, button):focus-visible)) splits those two cases at the right place.

Reviewed at exact head af81f68f7ffaac95e768b0d4725c3fdb79e4f2e8. test is green.

I checked the structural assumption the selector rests on rather than taking it on trust: in @astryxdesign/core's Item, a clickable row's hidden <a>/<button> really is a direct child of the Item root, while the delegate and parent-role variants keep their label inside a span with no a/button — so > :is(a, button) does distinguish them. Cascade order holds too: the product layer lands after astryx-components.

Keeping the rule inside @media (forced-colors: none) is the part I'd have most expected to be missed. Under forced colors the inner control loses its own indicator, so the row ring is the only surviving focus cue and suppressing it would leave nothing.

The e2e reads computed styles instead of comparing screenshots, waits on the popover rather than racing it, and asserts the control still has its own ring — so it would catch an over-correction, not just an under-correction.

No findings. Approving.

One note for later, not for this PR: I initially flagged that the branch carried commits from #3510, #3515 and #3519. They merged while I was checking, so the diff is now the two files you intended. Nothing for you to do.


This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are mine to correct — please push back where I got it wrong.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One follow-up before this lands — a request about the comment, not the code. The fix itself I've already approved and still stand behind.

My read is that the block is doing two different jobs, and only one of them earns 50 lines.


This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are mine to correct — please push back where I got it wrong.

it would mean product CSS reaching past the design system into the internals
of Selector, Switch, SegmentedControl and Button, one forced-colors rule per
control, to restore what each was supposed to draw itself. */
@media (forced-colors: none) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P3] Could this comment come down to roughly a third? Fifty lines of prose above four lines of CSS is a lot to keep in sync, and some of it restates what the selector already says.

I'd keep the two facts that stop someone "simplifying" this later and breaking it:

  1. Why forced-colors: none gates the rule. This is the non-obvious one. Under Windows High Contrast the control loses its own indicator — box-shadow is dropped and the trigger sets outline: none on itself — so the row ring becomes the only surviving focus cue. Without this note, removing the media query looks like a harmless tidy-up and silently costs keyboard users their focus indicator.
  2. Why the child combinator is > :is(a, button). That it matches only Item's invisible tab stop, because start/end/label content each sit in their own span, is what makes the selector correct rather than incidental.

What I'd drop: the two reproduction anecdotes, the quoted bug report, the cascade-layer paragraph (components being last is a property of the layer setup, not of this rule), and the rejected-alternative discussion — that belongs in the PR description, which is where a reviewer looks for it and where it doesn't age alongside the code.

Not blocking, and if you'd rather keep it as-is I won't argue the point twice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants