refactor(settings): give each capability its own disclosure - #2100
Merged
Conversation
The Permission Center's diagnostics were one page-level boolean that every capability row read. Pressing 展开详情 unfolded four MetadataLists on all of them at once — to reach the one capability you came to diagnose you scrolled past every other capability's five layers, required permissions, guidance and audit slot. It is now an Astryx CollapsibleGroup with one Collapsible per capability: `type="single"` so opening a row closes the last one, `hasDividers` + `density="compact"` for the edge-to-edge hairline rows this settings surface already speaks in — except the rows are the component's now, not ours. `density` is stated explicitly because `hasDividers` otherwise defaults items to `balanced`. Collapsible owns the trigger button, aria-expanded, aria-controls and the keyboard semantics, so `diagnosticsOpen`, its toggle button, and the expandDetails/collapseDetails copy (zh + en) are all gone. Three visual corrections from design review, all in the same pass: - The chevron was centered against a three-line trigger, which parked it beside the `computer_use` id line — pointing at the least important of the three. It top-aligns now, with one title line box of height so its own centering resolves onto the capability name. - The diagnostics were flush with the capability name, so 功能开关 read as a sibling row rather than as that row's detail; these rows have no icon column to supply a gutter. One indent step fixes it. Indent only — a tint or rounded container would be a card nested inside a row list, which this surface does not do. - The audit slot was a bare 暂无审计记录 line under nothing. It gets its own 子标签 the way 所需系统权限 has one; the missing parent was the actual reason it read as cramped, and now the empty and populated states hang off the same heading as parallel sub-groups. The DiagnosticsExpanded story drove both anchors this change removes — it found the button by its 展开详情 text and waited on `data-diagnostics="open"`. It now opens the first capability row and asserts on `aria-expanded`, which is Collapsible's own contract rather than an attribute the page maintained by hand for the test, scoped through `data-readiness` so it cannot latch onto another expandable button. No e2e touches this page: permission-mode-surface.spec covers the composer's permission mode, not the Permission Center. Verified: build, typecheck, format:check, check-dead-css, test:checks, check-story-annotations, build-storybook, smoke:storybook (the rewritten story passes), and the desktop + ui workspace suites. Screenshots of all three states came from the real Electron app, whose e2e fixture leaves the permissions IPC unstubbed, so the rows showed real TCC state. storage and cli remain the known Node v25 node:sqlite ExperimentalWarning failures. runtime-host failed once in the parallel run and passed twice on retry — an ownership-election timing flake; this change touches only the renderer.
Review caught that the section lost its accessible name in the move off `List`. CollapsibleGroup's wrapper is a bare div — it sets no role of its own and spreads consumer props straight onto it — so the aria-label landed on a `generic` element, and ARIA prohibits naming that role. The label was inert: the rows previously sat in a `List`, which ships `role="list"` and can be named, so "功能能力列表" simply disappeared for assistive tech. `role="group"` passes through the same spread and is a valid host for aria-label, which brings the name back. Confirmed by A/B against the real app rather than by inspection, since check-a11y does not catch this class of defect: querying for a group named 功能能力列表 returns 0 matches without the role and 1 with it.
Member
Author
|
Review by maka-审美专家 — 通过：三处视觉裁定（chevron 锚标题行 / 展开内容 space-4 缩进不加卡 / 审计位补子标签）全部正确落地；story 改用 Collapsible 自带 aria-expanded 契约；review 提出的 role="group" 可及名回接已修并经 A/B 实证（getByRole('group', {name:'功能能力列表'}) 0→1）。CI 全绿（含 storybook smoke）。合入。 |
jackwener
added a commit
that referenced
this pull request
Aug 4, 2026
ARIA prohibits naming the `generic` role, so `aria-label` on a bare `<div>` or `<span>` is not a weak name — it is no name at all. The failure is silent: the markup looks labelled, and review reads as if it were. This is how the Permission Center lost its capability list's accessible name when those rows moved off Astryx `List` (`role="list"`, nameable) onto a CollapsibleGroup whose wrapper is a role-less div (#2100). The new rule flags aria-label / aria-labelledby on tags whose implicit role is name-prohibited, and only those: div, span, p, pre, code, em, … . Tags with a nameable implicit role — nav, section, img, table, ul, svg, form — are legitimately labelled and never fire. An explicit `role=` is the fix, so it silences the rule; a `{...spread}` may carry a role we cannot see, so those stay quiet rather than cry wolf. Honest limit, documented at the rule: only intrinsic lowercase elements are judged. A capitalized component may set its own role internally — `List` does — so flagging `<SomeComponent aria-label>` would be guesswork over 51 sites. Which means this rule would NOT have caught the #2100 regression itself. It catches the directly checkable half; the component half stays a review question. The gate found 27 pre-existing violations, so the rule ships with them resolved rather than with a red build: - 18 are container semantics and take the obvious role. Action clusters, panels and list wrappers become `role="group"`; the artifact and browser panels `role="region"`; the two loading skeletons (already aria-busy) `role="status"`; the plan step marker, whose span renders a glyph and whose label IS its content, `role="img"`. - 9 are a different shape: `<span aria-label={richer}>{visible text}</span>`, where the label is meant to give screen readers more than the visible text. Those labels do nothing today, and the fix — delete them, or move them onto the interactive ancestor that should carry the name — changes what gets announced. That is a UX decision, not a mechanical one, so they carry an `a11y-allow` with that reasoning and are raised for design. Adding those exceptions exposed that the allow mechanism did not actually work where this rule fires: `// a11y-allow:` is not legal inside a JSX tag, and no exception existed anywhere in the tree to prove the convention. The rule now also accepts the JSX block form (`{/* a11y-allow: … */}`) on the line above, and inside an attribute expression, which is the only comment syntax valid in each of those positions. Verified: build, typecheck, format:check, check-dead-css, test:checks, and the desktop + ui workspace suites. storage and cli remain the known Node v25 node:sqlite failures. runtime-host failed twice under the parallel runner — two different ownership/lifecycle tests — and then passed 3/3 standalone; it is flaky under load, and this change touches no runtime code.
jackwener
added a commit
that referenced
this pull request
Aug 4, 2026
…#2108) * feat(a11y): flag aria-label on elements whose role cannot hold a name ARIA prohibits naming the `generic` role, so `aria-label` on a bare `<div>` or `<span>` is not a weak name — it is no name at all. The failure is silent: the markup looks labelled, and review reads as if it were. This is how the Permission Center lost its capability list's accessible name when those rows moved off Astryx `List` (`role="list"`, nameable) onto a CollapsibleGroup whose wrapper is a role-less div (#2100). The new rule flags aria-label / aria-labelledby on tags whose implicit role is name-prohibited, and only those: div, span, p, pre, code, em, … . Tags with a nameable implicit role — nav, section, img, table, ul, svg, form — are legitimately labelled and never fire. An explicit `role=` is the fix, so it silences the rule; a `{...spread}` may carry a role we cannot see, so those stay quiet rather than cry wolf. Honest limit, documented at the rule: only intrinsic lowercase elements are judged. A capitalized component may set its own role internally — `List` does — so flagging `<SomeComponent aria-label>` would be guesswork over 51 sites. Which means this rule would NOT have caught the #2100 regression itself. It catches the directly checkable half; the component half stays a review question. The gate found 27 pre-existing violations, so the rule ships with them resolved rather than with a red build: - 18 are container semantics and take the obvious role. Action clusters, panels and list wrappers become `role="group"`; the artifact and browser panels `role="region"`; the two loading skeletons (already aria-busy) `role="status"`; the plan step marker, whose span renders a glyph and whose label IS its content, `role="img"`. - 9 are a different shape: `<span aria-label={richer}>{visible text}</span>`, where the label is meant to give screen readers more than the visible text. Those labels do nothing today, and the fix — delete them, or move them onto the interactive ancestor that should carry the name — changes what gets announced. That is a UX decision, not a mechanical one, so they carry an `a11y-allow` with that reasoning and are raised for design. Adding those exceptions exposed that the allow mechanism did not actually work where this rule fires: `// a11y-allow:` is not legal inside a JSX tag, and no exception existed anywhere in the tree to prove the convention. The rule now also accepts the JSX block form (`{/* a11y-allow: … */}`) on the line above, and inside an attribute expression, which is the only comment syntax valid in each of those positions. Verified: build, typecheck, format:check, check-dead-css, test:checks, and the desktop + ui workspace suites. storage and cli remain the known Node v25 node:sqlite failures. runtime-host failed twice under the parallel runner — two different ownership/lifecycle tests — and then passed 3/3 standalone; it is flaky under load, and this change touches no runtime code. * fix(a11y): delete the nine inert labels instead of allow-listing them Follow-up to review on the aria-label-on-generic rule. `blockquote` leaves NAME_PROHIBITED: ARIA 1.2 gives the blockquote role Name From: author, so listing it would have flagged legitimate labelling. (Only div, span, p and pre actually carry aria-label in this tree today, so the entry was wrong rather than harmful — but wrong is enough.) The nine `<span aria-label={richer}>{visible text}</span>` sites are now deleted rather than excepted. Two reasons, both decided in review: Removing them is a no-op for users. The span is role-less, so the label was never announced; keeping it only preserved markup that reads as labelled without being labelled, which is the exact thing this rule exists to find. And they should not come back with a role bolted on. Screen readers and eyes should get the same thing. Everything these labels wanted to inject — default markers, connection status, card descriptions — already has a visible carrier beside the text (Badge, StatusDot, description), which assistive tech reaches through normal reading order. Putting it in the accessible name just says it twice, and an information channel only screen readers can hear is its own kind of asymmetry. The two markdown-body cases keep their `title` tooltip, which is what actually reaches the user; if the "why is this not clickable" reason should reach AT later, that is VisuallyHidden text, not a name on a generic span. Orphaned copy follows the labels out: chipAria / chipAriaLabel, manageAria, connectAria, both cardAria variants, currentAriaLabel, redactedAriaLabel, in zh and en. One test changes with the behavior, deliberately and not to make anything pass: markdown-body's unsafe-scheme case asserted `aria-label="Unsafe link"`. It now asserts `title="Unsafe link"` and asserts the aria-label is gone — the guarantee that the reason reaches the user is preserved, and pinned to the attribute that delivers it. Verified: build, typecheck, lint, format:check, check-dead-css, test:checks (the new rule reports clean with zero exceptions in the tree), and the desktop + ui workspace suites. storage and cli remain the known Node v25 node:sqlite failures. * fix(a11y): keep the five row labels, which name the row through its content The e2e failure was a real regression from deleting them, and the mechanism in review is right: the accessible-name algorithm walks descendants, so a label on an inner span still lands in an interactive ancestor's name. What does not hold is the prescribed fix — moving the label onto the Item. Measured it: `aria-label` on an Astryx Item lands on the outer `<li>`, while the click target is a separate invisible `<button>` Item renders inside, which has no aria-label and takes its name from content. So the label never reaches the element the test queries by role. Item exposes no naming prop either. That leaves the inner label as the only mechanism that names these rows, which is what the code already did. So the five row labels come back with the exception reason corrected to say what they actually do, and their copy comes back with them. The four standalone cases stay deleted — no interactive ancestor is reading those, so they really were inert. settings.spec's remote-access assertion passes with its original expectations untouched. The rule keeps its scope but gains the missing instruction: before deleting a flagged label, check whether it is naming something else, and prefer moving it onto the genuinely interactive element — where a component gives you no way to do that, the inner label is the mechanism and belongs in an a11y-allow rather than in the bin. Verified: build, typecheck, lint, format:check, check-dead-css, test:checks, settings.spec e2e (5/5), and the desktop + ui workspace suites. storage and cli remain the known Node v25 node:sqlite failures.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Task #139 —【Astryx 落地 ③】CollapsibleGroup 重做权限与能力页的诊断展开。
问题
诊断展开是一个页面级布尔，所有能力行共读。按一次「展开详情」，每一行都同时铺开四个 MetadataList —— 要看你真正关心的那个能力，得先滚过其他所有能力的五层状态 + 所需权限 + 处理建议 + 审计位。一面墙。
做法
CollapsibleGroup+ 每行一个Collapsible：type="single"—— 开一行自动收起上一行hasDividers+density="compact"—— 就是这个设置页到处在用的 edge-to-edge hairline 行，只不过现在是组件的，不是我们手写的density必须显式写：hasDividers会把 items 默认成balancedtrigger button、
aria-expanded、aria-controls、键盘语义全归 Collapsible。所以diagnosticsOpenstate、那个 toggle 按钮、以及expandDetails/collapseDetails文案（中英）全部删除。设计 review 的三处修正（同一个 PR 内）
computer_use那行，指着三行里最不重要的一行选择器影响面（先查后改，全列出来）
PermissionCenterDiagnosticsExpandedstory 必须改 —— 它 play 函数两个锚点都被这个 PR 删掉了：按展开详情文本找按钮 → 点击 → 等[data-diagnostics="open"]。改成：找
[data-readiness]内aria-expanded="false"的 trigger → 点击 → 等[data-readiness] button[aria-expanded="true"]。用
aria-expanded而不是再造一个data-diagnostics：那是 Collapsible 自己的 ARIA 契约，不需要页面手工维护一个属性专门给测试看。data-readiness用来收窄作用域，避免抓到页面上别的可展开按钮。✅ e2e 无影响 ——
permission-mode-surface.spec.ts测的是 composer 的权限模式，不是这个页面。全仓搜过没有别的测试引用这页。验证
npm run buildnpm run typechecknpm run format:checkcheck-dead-css.mjs --checktest:checks（console / a11y / copy）check-story-annotations.mjsbuild-storybooksmoke:storybookpermission-center-diagnostics-expanded通过截图（三态：收起 / 展开 / 单开互斥）跑的是真实 Electron app —— e2e fixture 不 stub permissions IPC，所以能力行显示的是这台机器真实的 TCC 状态，已发任务线程经设计侧确认。
已知失败，与本 PR 无关：
storage/cli—— Node v25 打node:sqliteExperimentalWarning，那些测试断言 stderr 为空。既有问题。runtime-host—— 并行跑挂过一次（elects one owner…所有权选举），单独重跑连过两次。timing flake。本 PR 只动 renderer，因果上不可能影响它。@maka-审美专家 请 review diff（三处视觉修正按你的裁定做的，你说不用再发截图）。