Skip to content

test(desktop): expand behavioral accessibility coverage - #4202

Merged
Astro-Han merged 1 commit into
apache:mainfrom
1625567290:fix/3893-desktop-a11y-coverage
Aug 31, 2026
Merged

test(desktop): expand behavioral accessibility coverage#4202
Astro-Han merged 1 commit into
apache:mainfrom
1625567290:fix/3893-desktop-a11y-coverage

Conversation

@1625567290

@1625567290 1625567290 commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Summary

  • extend the Desktop accessibility E2E journey across data-backed chat, streaming, expandable tool output, model selection, task states, toast/error feedback, Agent Graph, and keyboard navigation
  • assert the existing change, terminal, browser, task, and Daily Review surfaces through the behavioral accessibility audit
  • seed deterministic task-ledger fixture data for the conversation journey
  • keep WorkHub inside the app-level main landmark by exposing its nested surface as a named region

Validation

  • npm run lint
  • npm run format:check
  • npm run astryx:surface-inventory
  • npx knip --workspace apps/desktop
  • npm --workspace @maka/desktop run build:with-deps
  • npm --workspace @maka/desktop run typecheck
  • GIT_CONFIG_GLOBAL=/dev/null npm --workspace @maka/desktop run test:dist
  • npx playwright test --config e2e/playwright.config.ts e2e/accessibility-coverage.spec.ts e2e/workhub-layout.spec.ts e2e/workhub-reconstruction.spec.ts (10 passed on the refreshed main base)
  • npm --workspace @maka/desktop run build-storybook
  • npm --workspace @maka/desktop run smoke:storybook

Closes #3893

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Aug 29, 2026
@1625567290
1625567290 force-pushed the fix/3893-desktop-a11y-coverage branch 2 times, most recently from f7b00de to 18bd6bf Compare August 29, 2026 17:53

@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 for removing the unrelated Storage recovery commit and keeping the current head focused on the accessibility journey and WorkHub landmark semantics.

I reviewed exact head 18bd6bf5d846ee90aa5481b62c967579830cb6c9. The core direction is correct: AppShell already owns the application main, so the nested WorkHub surface should be a named region rather than a second main. The new Electron journey also uses real transcript, tool-result, task-ledger, streaming, and Workbar paths. The exact-head test check is green.

The PR body does not contain before/after screenshots, an accessibility-tree comparison, or a keyboard recording. Please attach evidence showing one application main plus the named WorkHub region, and a complete keyboard path through skip link, tool result, model picker, composer/Send, Graph, and streaming Stop.

I found one non-blocking P2 test-contract gap. Several controls are reached with direct Playwright .focus(), including recent tasks, the model menu item, Graph collapse, and Stop. Direct focus proves that a mounted element can receive focus, but not that a keyboard user can reach it through the real Tab order. The Issue acceptance also names reaching the Send button, while the test fills the composer and presses Enter without navigating to or activating Send.

Please start from a deterministic focus boundary and use Tab or Shift+Tab to reach the controls whose keyboard accessibility the test claims. Add one explicit keyboard activation of Send. If composer Enter is intentionally the only sending contract, state and assert that narrower contract instead.

For manual UX review, please also confirm that changing WorkHub from nested main to named region does not alter visual layout, focus restoration, or screen-reader landmark order across navigation away and back.

There are no P0 or P1 findings on the current head.

Review analysis was assisted by Codex and an independent @reviewer agent. Astro-Han verified the exact head after the force-push, production landmark owner, Electron fixture path, keyboard assertions, CI, and severity judgment, and owns this review.

中文对照

谢谢你删除了无关的 Storage recovery commit,让当前 head 收敛在 accessibility journey 和 WorkHub landmark 语义上。

我审查了精确 head 18bd6bf5d846ee90aa5481b62c967579830cb6c9。核心方向正确:AppShell 已经拥有应用级 main,内层 WorkHub 应是命名 region,而不是第二个 main。新的 Electron journey 也使用真实 transcript、tool result、task ledger、streaming 和 Workbar 路径。exact-head test 已通过。

PR 正文没有前后截图、accessibility tree 对比或键盘操作录屏。请补充证据,展示应用只有一个 main、WorkHub 是命名 region,并展示 skip link、tool result、model picker、composer/Send、Graph 和 streaming Stop 的完整键盘路径。

有一个不阻塞的 P2 测试契约缺口。recent tasks、model menu item、Graph collapse 和 Stop 等控件通过 Playwright .focus() 直接获得焦点。直接 focus 只能证明元素挂载后可以接收焦点,不能证明键盘用户能通过真实 Tab 顺序到达。Issue acceptance 还明确提到 Send button,但测试只是填充 composer 并按 Enter,没有导航到或激活 Send。

请从确定的焦点边界开始,用 Tab 或 Shift+Tab 到达测试声称可键盘访问的控件,并补一次 Send 的明确键盘激活。如果 composer Enter 才是刻意定义的唯一发送契约,也应明确说明并断言这个更窄的契约。

人工 UX 验收还需要确认:WorkHub 从嵌套 main 改为命名 region 后,不会改变视觉布局、焦点恢复或离开再返回时的屏幕阅读器 landmark 顺序。

当前 head 没有 P0/P1。

本次审查分析由 Codex 和独立的 @reviewer 子代理协助;Astro-Han 核验了 force-push 后的精确 head、生产 landmark owner、Electron fixture 路径、键盘断言、CI 和问题分级,并对本次 Review 负责。

@1625567290
1625567290 force-pushed the fix/3893-desktop-a11y-coverage branch from 18bd6bf to 86f893e Compare August 30, 2026 05:22
@1625567290

Copy link
Copy Markdown
Contributor Author

Addressed on exact head 86f893ec3 and rebased onto current main (d2346707d).

Keyboard contract changes:

  • removed every direct Playwright .focus() from this journey
  • start at a deterministic document boundary, Tab to the skip link, activate it, and assert focus lands on the single application main
  • reach tool output, model trigger, composer, explicit Send, Graph collapse, recent tasks, and streaming Stop through the real forward Tab order
  • open the model menu from its keyboard-reached trigger and assert the APG menu's initial glm-4.5 item focus before Enter activation
  • activate Send explicitly with Enter in the data-backed, new-task, and streaming paths

Landmark comparison from Accessibility.getFullAXTree:

  • before: region "通知", unnamed application main, nested main "WorkHub"
  • after: region "通知", unnamed application main, named region "WorkHub"

Visual evidence (same WorkHub navigation-away/back flow):

Before After
WorkHub before WorkHub after

Keyboard recording: skip link → tool result → model picker → composer/Send → Graph → recent tasks → streaming Stop

The before/after screenshots are both 2480×1640. Their mean absolute per-channel delta is 0.143/255; the remaining 1.60% differing pixels are dynamic timing/text antialiasing, with no WorkHub geometry change. Navigation away/back still preserves the expected WorkHub projection and landmark order.

Validation on the refreshed base:

  • npm run lint
  • npm run format:check
  • npm --workspace @maka/desktop run build:with-deps
  • npm --workspace @maka/desktop run typecheck
  • targeted Desktop E2E: 10/10 passed
  • dedicated evidence recording path: passed

@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 for turning the accessibility audit into real behavioral coverage. This extends the existing E2E journey through actual Tab navigation, explicit Send activation, real transcript and task-ledger fixtures, and the major conversation surfaces, while preserving a single app-level main landmark. It reuses the existing audit and fixture authorities rather than adding another accessibility harness. The exact-head test check is green.

AI-assisted review; I verified the current head, production landmark change, regression behavior, mergeability, and review threads.

@Astro-Han
Astro-Han merged commit ac59b47 into apache:main Aug 31, 2026
1 check passed
saltand pushed a commit to saltand/maka-agent that referenced this pull request Aug 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(desktop): expand a11y coverage audit to chat, tool results, and error states

2 participants