From af81f68f7ffaac95e768b0d4725c3fdb79e4f2e8 Mon Sep 17 00:00:00 2001
From: GabrielDrapor <gabrieldrapor@gmail.com>
Date: Sat, 22 Aug 2026 23:36:41 +0800
Subject: [PATCH] fix(ui): ring the row control, not the whole settings row
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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
---
 .../e2e/settings-row-focus-ring.spec.ts       | 175 ++++++++++++++++++
 packages/ui/src/styles.css                    |  56 ++++++
 2 files changed, 231 insertions(+)
 create mode 100644 apps/desktop/e2e/settings-row-focus-ring.spec.ts

diff --git a/apps/desktop/e2e/settings-row-focus-ring.spec.ts b/apps/desktop/e2e/settings-row-focus-ring.spec.ts
new file mode 100644
index 0000000000..060f02f143
--- /dev/null
+++ b/apps/desktop/e2e/settings-row-focus-ring.spec.ts
@@ -0,0 +1,175 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { expect, test } from './fixtures';
+import type { Locator, Page } from '@playwright/test';
+
+/**
+ * A settings row rings its OWN tab stop and nothing else.
+ *
+ * Astryx's Item draws `outline: 2px solid accent` at `:has(:focus-visible)`
+ * unconditionally. That is right only for a row whose click target is the
+ * invisible `<button>` Item renders for `onClick`/`href`. Every other row holds
+ * a control that rings itself, so the row outline was a second ring around the
+ * whole label + description + control band.
+ *
+ * Opening the 默认模型 picker showed it at its worst: a Selector popup is a
+ * native `popover`, so the top layer moves where the popup PAINTS but not where
+ * it sits in the DOM. Focus lands on the popup's search input, `:has()` walks
+ * up to the row, and the row drew a full-width ring while the trigger drew
+ * none — reported as "点开下拉框时外面整行都会出现一个蓝色的框".
+ *
+ * `packages/ui/src/styles.css` narrows the ring to the row's own tab stop.
+ * Three things have to hold together, and a fix that over-reaches passes the
+ * first two while failing the third:
+ *
+ *   1. an open picker leaves its row unringed,
+ *   2. keyboard focus on a row control rings the CONTROL and not the row,
+ *   3. a clickable row still rings, because there the ring IS the indicator.
+ *
+ * Computed style rather than a screenshot: the ring is one declaration, and
+ * reading it back needs no tolerance for anti-aliasing or theme drift.
+ */
+
+async function openSettingsPage(page: Page, section: string) {
+  await page.getByRole('button', { name: '设置' }).click();
+  await page.getByRole('button', { name: section, exact: true }).click();
+}
+
+/** Tab until `target` owns focus, so the browser treats it as keyboard focus. */
+async function tabTo(page: Page, target: Locator, limit = 40) {
+  for (let index = 0; index < limit; index++) {
+    await page.keyboard.press('Tab');
+    if (await target.evaluate((element) => element === document.activeElement)) return;
+  }
+  throw new Error('Tab order never reached the target control');
+}
+
+/** The row around whatever currently has focus, and whether it draws a ring. */
+function focusedRowOutline() {
+  const active = document.activeElement as HTMLElement | null;
+  const row = active?.closest('.astryx-item') as HTMLElement | null;
+  if (!row) return null;
+  const style = getComputedStyle(row);
+  return { outlineStyle: style.outlineStyle, outlineWidth: style.outlineWidth };
+}
+
+/** The trigger's own focus affordance: Astryx fields state it in the border. */
+const fieldChrome = (element: SVGElement | HTMLElement) => {
+  const field = (element as HTMLElement).parentElement as HTMLElement;
+  const style = getComputedStyle(field);
+  return `${style.borderColor} | ${style.boxShadow}`;
+};
+
+test('an open picker does not ring the settings row that hosts it', async ({
+  window: page,
+}) => {
+  await openSettingsPage(page, '通用');
+  await expect(page.getByRole('textbox', { name: '助手语气偏好' })).toBeEnabled();
+  await page.getByRole('button', { name: '默认模型' }).click();
+
+  // Wait for the state under test to actually exist before reading it. Astryx
+  // hands the popup's search input focus inside a requestAnimationFrame
+  // (Selector.tsx `onOpen`), so a read that wins that race would find focus
+  // still on the mouse-clicked trigger — which is not `:focus-visible`, so
+  // NOTHING rings and the assertion below passes against the bug too. Waiting
+  // on the condition rather than on a timeout is what keeps this test honest.
+  await page.waitForFunction(() => {
+    const active = document.activeElement;
+    return (
+      document.querySelector('[popover]:popover-open') != null &&
+      active instanceof HTMLElement &&
+      active.closest('[popover]:popover-open') != null
+    );
+  });
+
+  const state = await page.evaluate(() => {
+    const active = document.activeElement as HTMLElement;
+    const row = active.closest('.astryx-item') as HTMLElement | null;
+    return {
+      // The mechanism, asserted so the ring check can never pass vacuously:
+      // the focused popup input is still a DOM descendant of the row, because
+      // the top layer moves where a popover PAINTS and not where it sits. If
+      // that ever stops being true this test goes quiet instead of green.
+      focusInsideRow: row != null,
+      outlineStyle: row ? getComputedStyle(row).outlineStyle : null,
+    };
+  });
+
+  expect(state.focusInsideRow).toBe(true);
+  expect(state.outlineStyle).toBe('none');
+});
+
+test('keyboard focus rings the row control, never the row', async ({ window: page }) => {
+  await openSettingsPage(page, '通用');
+  await expect(page.getByRole('textbox', { name: '助手语气偏好' })).toBeEnabled();
+
+  const trigger = page.getByRole('button', { name: '默认模型' });
+  const resting = await trigger.evaluate(fieldChrome);
+
+  await page.getByRole('textbox', { name: '助手语气偏好' }).focus();
+  await tabTo(page, trigger);
+
+  expect((await page.evaluate(focusedRowOutline))?.outlineStyle).toBe('none');
+  // …and the control still says it has focus. Astryx's field convention is an
+  // accent border, not an outline, which is why dropping the row's outline
+  // costs no keyboard visibility. Polled because border-color transitions.
+  await expect.poll(() => trigger.evaluate(fieldChrome)).not.toBe(resting);
+});
+
+test('a row whose own tab stop is the click target keeps its ring', async ({
+  window: page,
+}) => {
+  await openSettingsPage(page, '远程接入');
+
+  // A catalog row is a clickable Item, so Item renders an invisible <button>
+  // as a direct child and the row outline is that button's only indicator.
+  const row = page.locator('.settingsRemoteAccessCatalogRow').first();
+  await expect(row).toBeVisible();
+  const rowButton = row.locator(':scope > button').first();
+
+  await page.getByRole('button', { name: '远程接入', exact: true }).focus();
+  await tabTo(page, rowButton);
+
+  expect(await page.evaluate(focusedRowOutline)).toEqual({
+    outlineStyle: 'solid',
+    outlineWidth: '2px',
+  });
+});
+
+test('the row keeps its ring under forced colors, where the control loses its own', async ({
+  window: page,
+}) => {
+  await openSettingsPage(page, '通用');
+  await expect(page.getByRole('textbox', { name: '助手语气偏好' })).toBeEnabled();
+  await page.emulateMedia({ forcedColors: 'active' });
+
+  const trigger = page.getByRole('button', { name: '默认模型' });
+  const resting = await trigger.evaluate(fieldChrome);
+
+  await page.getByRole('textbox', { name: '助手语气偏好' }).focus();
+  await tabTo(page, trigger);
+
+  // Windows High Contrast drops box-shadow and repaints every border in one
+  // system color, so the field's own affordance reads the same focused as
+  // resting — measured here rather than assumed, because it is the entire
+  // reason the row must keep its ring.
+  expect(await trigger.evaluate(fieldChrome)).toBe(resting);
+  expect((await page.evaluate(focusedRowOutline))?.outlineStyle).toBe('solid');
+});
diff --git a/packages/ui/src/styles.css b/packages/ui/src/styles.css
index 8b5b7a3ea4..fe9719eeab 100644
--- a/packages/ui/src/styles.css
+++ b/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) {
+  .astryx-item:has(:focus-visible):not(:has(> :is(a, button):focus-visible)) {
+    outline: none;
+  }
+}
+
 .maka-quote-chip-remove { display: inline-flex; flex: 0 0 auto; align-items: center; justify-content: center; border: 0; color: var(--muted-foreground); background: transparent; transition: color var(--duration-quick) var(--ease-out-strong), background var(--duration-quick) var(--ease-out-strong); }
 .maka-quote-chip-remove:hover { color: var(--foreground); background: var(--state-hover-bg); }
 .maka-quote-chip-remove:focus-visible { color: var(--foreground); background: var(--state-hover-bg); outline: var(--focus-ring-width) solid var(--focus-ring); outline-offset: 1px; }