Skip to content

[Repo Assist] A11y: fix tool-output body outside header Button automation subtree#603

Draft
github-actions[bot] wants to merge 1 commit into
masterfrom
repo-assist/fix-issue-590-a11y-tool-output-automation-a7e5389408e522cf
Draft

[Repo Assist] A11y: fix tool-output body outside header Button automation subtree#603
github-actions[bot] wants to merge 1 commit into
masterfrom
repo-assist/fix-issue-590-a11y-tool-output-automation-a7e5389408e522cf

Conversation

@github-actions
Copy link
Copy Markdown
Contributor

🤖 This PR was created by Repo Assist, an automated AI assistant.

Summary

Fixes #590

Root Cause

In the CompactSummary tool-burst style (multi-step tool calls), the header is a standalone Button and the expanded body (step rows) is added as a sibling Border element in the outer VStack. This means the body sits outside the Button's automation subtree — Narrator announces the button's visual text content but has no direct link to the body content below it.

Fix

Two targeted changes in OpenClawChatTimeline.cs:

  1. Explicit AutomationName on headerButton — builds a string "{summaryLine}, {stepCountLabel}, {taskStatusText}, expanded/collapsed" and passes it to .AutomationName(). When Narrator focuses the header button it now hears e.g. "Searched for files, 3 steps, Done, collapsed, button" — the full card state without needing to navigate further.

  2. AutomationName on the body Border — labels the expanded body region with "Tool steps for: {summaryLine}" so that Narrator scan-mode users who navigate past the button understand what the following content describes.

The Plain style (single-step) is unaffected — there the body is already inside the Button's content (VStack(0, headerRow, body) is the button content), so Narrator reads header and body together naturally.

Trade-offs

  • The composite automation name is a concatenated string; if summaryLine is very long, Narrator will read the whole thing. It is capped at 80 chars by the existing Truncate() logic that produces summaryLine, so this is bounded.
  • Does not use AutomationProperties.SetLabeledBy (which would be the closest ARIA-equivalent) because that requires resolving both WinUI controls in the same scope, which is awkward in the FunctionalUI reactive rendering pipeline. The AutomationName approach is simpler, fully supported, and sufficient for the stated acceptance criteria.

Test Status

  • Tray tests: ✅ Passed — 870/870 (0 failures, 2 skipped/Windows-only)
  • Shared tests: ✅ Passed (pre-existing 8 failures are Windows path/browser-profile tests that fail on Linux infrastructure; unrelated to this change)
  • WinUI build: ⚠️ Cannot build on Linux (NETSDK1100: Windows-targeting project); change is a straightforward .AutomationName() call using the existing FunctionalUI extension already used elsewhere in the file.

Generated by 🌈 Repo Assist, see workflow run. Learn more.

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/repo-assist.md@97143ac59cb3a13ef2a77581f929f06719c7402a

… body

In the CompactSummary tool-burst style, the header is a standalone Button
and the expanded body (step rows) is a sibling element outside the Button's
automation subtree. This meant Narrator only announced the button's visual
content (a FlexRow with text nodes that may not compose cleanly) without
context about the task state.

Changes:
- Build a headerAutomationName string: "<summary>, <N step(s)>, <status>,
  expanded/collapsed" and pass it to .AutomationName() on headerButton.
  Narrator now announces the full card state when the button is focused.
- Add .AutomationName("Tool steps for: <summary>") on the body Border so
  scan-mode navigation gives the body region a clear label linking it back
  to the task the header describes.

Fixes #590

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 30, 2026

Codex review: found issues before merge. Reviewed May 30, 2026, 9:02 AM ET / 13:02 UTC.

Summary
The PR adds explicit FunctionalUI AutomationName values to the TaskList tool-burst header button and expanded body border in OpenClawChatTimeline.cs.

Reproducibility: no. high-confidence Windows Narrator runtime reproduction was run. The PR defect is source-reproducible: production multi-step tool bursts use CompactSummary, while this patch only changes the later TaskList branch.

Review metrics: none identified.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Move or duplicate the automation wiring onto the production CompactSummary branch.
  • [P1] Add redacted Windows Narrator/UIA proof showing header focus and expanded body navigation for a multi-step tool card.
  • [P1] After code changes, run the repository-required build and tray/shared test validation on a suitable Windows setup.

Proof guidance:

  • [P1] Needs real behavior proof before merge: No after-fix Windows/Narrator proof is attached; add redacted screenshots, video, live output, logs, or a UIA/Narrator recording, then update the PR body so ClawSweeper can re-review automatically or via a maintainer @clawsweeper re-review comment.

Mantis proof suggestion
A native desktop/Narrator proof would materially help because source review cannot confirm UIA reading order. A maintainer can ask Mantis to capture proof by posting a new PR comment that starts with the OpenClaw Mantis account mention, followed by:

visual task: verify a multi-step WinUI tool card in production CompactSummary with Narrator; header focus and expanded body scan should announce coherent task steps without collapsing on body text selection.

Risk before merge

  • [P1] Merging as-is would not fix the production CompactSummary path named by the PR and linked issue, so the accessibility issue could remain while the PR appears to close it.
  • [P1] No after-fix Windows Narrator/UIA proof is attached, so the actual reading order and expanded body navigation remain unverified.

Maintainer options:

  1. Fix the active path before merge (recommended)
    Move or duplicate the automation wiring onto the production CompactSummary branch and require Windows Narrator proof of header focus plus expanded body navigation.
  2. Pause this draft
    Keep this PR unmerged until the branch targets the production path or close it in favor of a narrower replacement PR that does.

Next step before merge

  • [P2] Needs contributor or maintainer follow-up because the code targets the wrong render branch and the required real Windows/Narrator proof is absent.

Security
Cleared: The diff only changes local WinUI automation metadata and does not alter credentials, networking, dependencies, build scripts, or code execution paths.

Review findings

  • [P2] Apply the fix to CompactSummary — src/OpenClaw.Tray.WinUI/Chat/OpenClawChatTimeline.cs:2066
Review details

Best possible solution:

Apply the automation relationship or explicit names to the production CompactSummary summary button and expanded rows while preserving the header-only click target, then verify the multi-step tool card with Windows Narrator.

Do we have a high-confidence way to reproduce the issue?

No high-confidence Windows Narrator runtime reproduction was run. The PR defect is source-reproducible: production multi-step tool bursts use CompactSummary, while this patch only changes the later TaskList branch.

Is this the best way to solve the issue?

No. The current patch is not the best fix because it labels the inactive/default-off TaskList path while leaving the production CompactSummary path unchanged.

Full review comments:

  • [P2] Apply the fix to CompactSummary — src/OpenClaw.Tray.WinUI/Chat/OpenClawChatTimeline.cs:2066
    This adds the accessibility names only inside the TaskList branch, but production multi-step tool bursts go through ToolBurstStyle.CompactSummary; that branch still expands by appending sibling rows after summaryButton without an automation relationship. As a result, the linked screen-reader issue remains for the user-facing path this PR says it fixes.
    Confidence: 0.91

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found, but no applicable review policy affected this item.

Codex review notes: model gpt-5.5, reasoning high; reviewed against eb06fba21c44.

Label changes

Label changes:

  • add merge-risk: 🚨 other: Merging the PR could falsely close the accessibility work while leaving the production CompactSummary path unchanged.

Label justifications:

  • P2: This is a bounded tray chat accessibility fix with real user impact, but it is not a crash, data loss, security issue, or urgent workflow outage.
  • merge-risk: 🚨 other: Merging the PR could falsely close the accessibility work while leaving the production CompactSummary path unchanged.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: No after-fix Windows/Narrator proof is attached; add redacted screenshots, video, live output, logs, or a UIA/Narrator recording, then update the PR body so ClawSweeper can re-review automatically or via a maintainer @clawsweeper re-review comment.
Evidence reviewed

What I checked:

  • Repository policy read: The target AGENTS.md was read fully; its validation commands apply after code changes, but this review made no checkout changes and remained read-only. (AGENTS.md:1, eb06fba21c44)
  • Production style selection: Current production rendering uses ToolBurstStyle.Auto, and multi-step entries are mapped to ToolBurstStyle.CompactSummary, not TaskList. (src/OpenClaw.Tray.WinUI/Chat/OpenClawChatTimeline.cs:1627, eb06fba21c44)
  • CompactSummary still lacks the proposed wiring: The CompactSummary branch creates summaryButton, then appends expanded rows as sibling elements in pieces without an explicit automation name or labeled relationship. (src/OpenClaw.Tray.WinUI/Chat/OpenClawChatTimeline.cs:1798, eb06fba21c44)
  • PR diff targets TaskList: The PR file patch adds headerAutomationName, .AutomationName(headerAutomationName), and Tool steps for: inside the later TaskList branch around headerButton, leaving the earlier CompactSummary branch untouched. (src/OpenClaw.Tray.WinUI/Chat/OpenClawChatTimeline.cs:2066, 51ca460db0ee)
  • Automation API support: FunctionalUI already exposes .AutomationName() and applies it through AutomationProperties.SetName, so the concern is the target code path rather than the helper API. (src/OpenClawTray.FunctionalUI/FunctionalUI.cs:724, eb06fba21c44)
  • Related discussion and proof status: The linked issue discussion and Repo Assist comment identify the missing Narrator/UIA relationship and explicitly note that a Windows Narrator smoke test is still needed; the PR body reports tests but no after-fix runtime proof.

Likely related people:

  • RBrid: Authored the recent native chat UI fix commit that changed CompactSummary/tool-burst behavior and is directly adjacent to the linked accessibility concern. (role: recent chat timeline contributor; confidence: medium; commits: bf21e7b216aa, 0a164fc111b2; files: src/OpenClaw.Tray.WinUI/Chat/OpenClawChatTimeline.cs)
  • Scott Hanselman: Recently changed production tool-call visibility in the same chat timeline surface, making him a useful routing candidate for tool-card behavior. (role: recent area contributor; confidence: medium; commits: 50837d79ecc2, 0d4fcbd50ad5; files: src/OpenClaw.Tray.WinUI/Chat/OpenClawChatTimeline.cs)
  • the99missedcalls: Recently changed assistant thinking-state rendering in OpenClawChatTimeline.cs, near the same render-composition area. (role: adjacent chat timeline contributor; confidence: low; commits: 6a5921883c86; files: src/OpenClaw.Tray.WinUI/Chat/OpenClawChatTimeline.cs)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. labels May 30, 2026
@github-actions
Copy link
Copy Markdown
Contributor Author

github-actions Bot commented Jun 4, 2026

🤖 This is a note from Repo Assist.

PR #650 (Fix tool output accessibility labels) by @ranjeshj was submitted for the same issue (#590) and includes a regression test. It is currently marked as ready for maintainer review and appears to be a more complete fix.

Suggesting this PR (#603) be closed in favour of #650 to avoid confusion. Happy to defer to the maintainer's judgement on which approach to take.

Generated by 🌈 Repo Assist, see workflow run. Learn more.

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/repo-assist.md@97143ac59cb3a13ef2a77581f929f06719c7402a

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

Labels

automation enhancement New feature or request merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. repo-assist status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A11y: tool-output body is outside the header Button's automation subtree

0 participants