You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "GabrielDrapor (via GitHub)" <gi...@apache.org> on 2026/08/22 17:12:43 UTC
[PR] fix(ui): ring the row control, not the whole settings row [maka]
GabrielDrapor opened a new pull request, #3525:
URL: https://github.com/apache/maka/pull/3525
## What
设置里的行，在焦点落到行内控件上时会把**整行**画一圈焦点框。最明显的一幕是 设置 → 通用 → 「默认模型」：鼠标点开下拉框，整行出现一个蓝框，而 trigger 自己什么都没有。
<img width="960" alt="before" src="https://github.com/user-attachments/assets/placeholder-before" />
## Why
Astryx 的 `Item`（以及基于它的 `ListItem`，也就是每一个设置行）无条件写了 `outline: 2px solid accent` on `:has(:focus-visible)`。
这条规则只对**整行可点**的行成立：那种行 `Item` 会渲染一个不可见的 `<button>`/`<a>` 当点击目标，整行的框就是它唯一的焦点指示。其他行里真正可聚焦的是右侧自带焦点态的控件，整行再画一圈就是第二个框。
下拉框那一幕的机制：Astryx Selector 的弹层是原生 `popover`。**top layer 只改变它画在哪，不改变它在 DOM 里挂在哪** —— 它仍然是那一行的后代。带搜索框的 picker 打开时焦点落进弹层里的 input，`:has()` 一路向上匹配到行，于是整行亮框；trigger 自己的 wrapper 是弹层的兄弟节点，反而不亮。
## How
`packages/ui/src/styles.css` 一条规则（产品 CSS 在最后一个 cascade layer，稳压 StyleX 原子类的 `:not(#\#)` 特异性）：
```css
@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 <no...@anthropic.com>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(ui): ring the row control, not the whole settings row [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #3525:
URL: https://github.com/apache/maka/pull/3525#discussion_r3836710231
##########
packages/ui/src/styles.css:
##########
@@ -20,6 +20,62 @@
/* Maka product compositions shared across desktop renderer surfaces. Astryx
owns generic component chrome; these selectors own only product semantics. */
+/* An Item row draws a focus ring for ITS OWN tab stop, never for a control
+ that already draws one.
+
+ Astryx's Item (and everything built on it — ListItem, and so every settings
+ row) applies `outline: 2px solid accent` at `:has(:focus-visible)`,
+ unconditionally. That is right for an interactive row: `onClick`/`href`
+ makes Item render an INVISIBLE `<button>`/`<a>` as a direct child, and the
+ row's ring is that button's only visible focus indicator. It is wrong for
+ every other row, where the focusable thing is a real control in the end slot
+ that rings itself — the row ring is then a second outline around a whole
+ label + description + control band.
+
+ Two ways this showed up, both measured in a real window:
+
+ 1. Tabbing onto any settings-row control — a Selector trigger, a Switch, a
+ SegmentedControl, a ghost Button — lit the control AND the full row.
+ 2. Opening the 默认模型 picker outlined the entire row and nothing else.
+ A Selector popup is a native `popover` element: the top layer moves where
+ it PAINTS, not where it sits in the DOM, so it is still a descendant of
+ the row. Focus goes to the popup's search input on open, `:has()` walks
+ up to the row, and the row rings — while the trigger, whose own wrapper
+ is a sibling of the popup, does not. Reported as "点开下拉框时外面整行
+ 出现一个蓝色的框".
+
+ `:not(:has(> :is(a, button):focus-visible))` is what separates the two: the
+ only `<a>`/`<button>` that is a DIRECT child of an Item root is that
+ invisible tab stop, since start/end/label content each sit inside their own
+ span. So an interactive row keeps its ring when its own tab stop has focus,
+ and loses it only when focus is on a control that owns its indicator — which
+ is also the case for a row carrying both.
+
+ Product CSS is the last cascade layer (`components`, see
+ cascade-layers.css), so this wins over the compiled StyleX atom regardless
+ of that atom's `:not(#\#)` specificity padding.
+
+ Gated on `forced-colors: none` because the premise — "the control already
+ draws one" — is only true when the page controls its own colors. Under
+ Windows High Contrast the control's affordance is gone: forced-colors drops
+ `box-shadow` entirely and repaints every border in the same system color, so
+ an Astryx field looks identical resting and focused, and its trigger button
+ sets `outline: none` on itself (Selector.tsx: the wrapper is supposed to
+ carry the ring). The row outline is then the ONLY indicator that survives —
+ outline is the one focus property forced-colors preserves — and taking it
+ away leaves a keyboard user with no visible focus at all. So there, the
+ doubled ring is the lesser problem and the row keeps it.
+
+ Fixing it at the control instead was the other option and is not better:
+ 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) {
Review Comment:
[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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(ui): ring the row control, not the whole settings row [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han merged PR #3525:
URL: https://github.com/apache/maka/pull/3525
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org