Skip to content

🎨 Palette: Link screen reader labels to inputs - #425

Open
google-labs-jules[bot] wants to merge 3 commits into
mainfrom
palette-ux-a11y-labels-2102851808157902841
Open

🎨 Palette: Link screen reader labels to inputs#425
google-labs-jules[bot] wants to merge 3 commits into
mainfrom
palette-ux-a11y-labels-2102851808157902841

Conversation

@google-labs-jules

Copy link
Copy Markdown
Contributor

💡 What: Added AutomationProperties.LabeledBy to Hotkey inputs.
🎯 Why: Improves screen reader support.
♿ Accessibility: Screen readers will now announce the 'Hotkey' label for the Modifier ComboBox and Key TextBox.


PR created automatically by Jules for task 2102851808157902841 started by @mikekthx

@google-labs-jules

Copy link
Copy Markdown
Contributor Author

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@google-labs-jules
google-labs-jules Bot requested a review from mikekthx as a code owner July 1, 2026 11:23
@github-actions github-actions Bot added documentation Improvements or additions to READMEs, wikis, or inline code comments. ui Front-end changes, WinUI layouts, styling (e.g., Acrylic), or system tray updates. labels Jul 1, 2026
Comment thread SettingsWindow.xaml
Comment thread .jules/palette.md Outdated
@mikekthx

mikekthx commented Jul 1, 2026

Copy link
Copy Markdown
Owner

Review: 🎨 Link screen reader labels to inputs

What this PR does: Adds x:Name="HotkeyHeader" to the Hotkey section header TextBlock and wires AutomationProperties.LabeledBy="{Binding ElementName=HotkeyHeader}" to both the modifier ComboBox and key TextBox in SettingsWindow.xaml. Also appends a learning entry to .jules/palette.md.

Overall assessment: The intent is right, and the XAML technique is correct ({Binding ElementName=...} for LabeledBy is exactly what CLAUDE.md requires). However the change has two issues — one of which is a functional accessibility regression — so it needs revision before merging.


🔴 Blocking issues

1. Accessibility regression — both hotkey controls lose their specific accessible names

Both Settings_HotkeyModifier and Settings_HotkeyKey already carry AutomationProperties.Name values via x:Uid in all 13 .resw files ("Hotkey modifier" and "Hotkey key" respectively). In WinUI 3, AutomationProperties.LabeledBy takes precedence over AutomationProperties.Name when the automation framework computes an element's accessible name. Pointing both controls at the same HotkeyHeader means a screen reader announces "Global Hotkey" for both — the generic section title — rather than the per-control names that previously differentiated them. This is worse for screen reader users than the status quo, not better.

The fix is one of:

  • Simplest: Remove both LabeledBy attributes — the .resw names are already correct and sufficient.
  • Pattern-consistent: Follow the IconSizeLabel approach (line 130) and give each control its own visible TextBlock label with x:Name, then point LabeledBy at the per-control label, not the section header.

2. Wrong date in .jules/palette.md

The new entry is dated 2024-11-20, which is 18 months in the past and precedes the existing 2026-05-15 entry. Today is 2026-07-01. CLAUDE.md explicitly requires running date +%Y-%m-%d before writing any date to log/journal files.


✅ What's correct

  • {Binding ElementName=HotkeyHeader} is the right syntax for LabeledBy (not {x:Bind}) — matches CLAUDE.md's sharp-edge note exactly.
  • x:Name added to the header TextBlock is required for the ElementName binding to resolve.
  • The + spacer TextBlock retains AutomationProperties.AccessibilityView="Raw" to hide it from the accessibility tree — correct.
  • The XAML formatting (multi-line attributes) follows project style.

@jules please address the issues noted above

@google-labs-jules

Copy link
Copy Markdown
Contributor Author

Review: 🎨 Link screen reader labels to inputs

What this PR does: Adds x:Name="HotkeyHeader" to the Hotkey section header TextBlock and wires AutomationProperties.LabeledBy="{Binding ElementName=HotkeyHeader}" to both the modifier ComboBox and key TextBox in SettingsWindow.xaml. Also appends a learning entry to .jules/palette.md.

Overall assessment: The intent is right, and the XAML technique is correct ({Binding ElementName=...} for LabeledBy is exactly what CLAUDE.md requires). However the change has two issues — one of which is a functional accessibility regression — so it needs revision before merging.


🔴 Blocking issues

1. Accessibility regression — both hotkey controls lose their specific accessible names

Both Settings_HotkeyModifier and Settings_HotkeyKey already carry AutomationProperties.Name values via x:Uid in all 13 .resw files ("Hotkey modifier" and "Hotkey key" respectively). In WinUI 3, AutomationProperties.LabeledBy takes precedence over AutomationProperties.Name when the automation framework computes an element's accessible name. Pointing both controls at the same HotkeyHeader means a screen reader announces "Global Hotkey" for both — the generic section title — rather than the per-control names that previously differentiated them. This is worse for screen reader users than the status quo, not better.

The fix is one of:

  • Simplest: Remove both LabeledBy attributes — the .resw names are already correct and sufficient.
  • Pattern-consistent: Follow the IconSizeLabel approach (line 130) and give each control its own visible TextBlock label with x:Name, then point LabeledBy at the per-control label, not the section header.

2. Wrong date in .jules/palette.md

The new entry is dated 2024-11-20, which is 18 months in the past and precedes the existing 2026-05-15 entry. Today is 2026-07-01. CLAUDE.md explicitly requires running date +%Y-%m-%d before writing any date to log/journal files.


✅ What's correct

  • {Binding ElementName=HotkeyHeader} is the right syntax for LabeledBy (not {x:Bind}) — matches CLAUDE.md's sharp-edge note exactly.
  • x:Name added to the header TextBlock is required for the ElementName binding to resolve.
  • The + spacer TextBlock retains AutomationProperties.AccessibilityView="Raw" to hide it from the accessibility tree — correct.
  • The XAML formatting (multi-line attributes) follows project style.

@jules please address the issues noted above

I have addressed the blocking issues. I removed the AutomationProperties.LabeledBy bindings from the ComboBox and TextBox to prevent the regression where they both announced the global 'Hotkey' section title, reverting to their .resw provided specific names. I also updated the date in the .jules/palette.md entry to the correct date 2026-07-01.

Comment thread .jules/palette.md Outdated
@mikekthx

mikekthx commented Jul 1, 2026

Copy link
Copy Markdown
Owner

Review — PR #425 (synchronize)

What this PR does (current commits): The PR title and body describe adding AutomationProperties.LabeledBy to the hotkey controls for accessibility. In practice the current code contains no such addition — the XAML changes are pure cosmetic reformatting (multi-line attribute style for the hotkey ComboBox, TextBlock, and TextBox). The only substantive change is the new .jules/palette.md entry.


Thread resolution

Thread Concern Status
PRRT_kwDORLRSls6NkDxK — LabeledBy accessibility regression LabeledBy pointing to HotkeyHeader for both controls overrode per-control .resw names, making the controls indistinguishable to screen readers ResolvedLabeledBy is absent from the current code; the regression no longer exists
PRRT_kwDORLRSls6NkD6D — Wrong date in palette.md Entry was dated 2024-11-20, 18 months before the preceding entry Resolved — Date is now 2026-07-01; thread was already marked outdated by GitHub

Remaining issue (blocking)

.jules/palette.md lines 25–26 — lesson is the inverse of what was learned.

The new entry tells future contributors to explicitly link controls to section headers via LabeledBy — which is exactly the anti-pattern that was reverted in this PR after review feedback. If a future agent follows this guidance, it will reproduce the same accessibility regression. The entry must be corrected to reflect the actual lesson: LabeledBy to a shared section header is harmful; per-control .resw AutomationProperties.Name (via x:Uid) is sufficient when descriptive names are already present; and LabeledBy should only be used with a dedicated per-control visible label element (see the IconSizeLabel + IconSizeCombo pattern in SettingsWindow.xaml). See inline comment for a full description.

The XAML and C# changes are otherwise clean and raise no correctness, security, or threading concerns.

@jules please address the issues noted above

@mikekthx

mikekthx commented Jul 1, 2026

Copy link
Copy Markdown
Owner

Review PR 425 (synchronize)

What this PR does: The title and body describe adding AutomationProperties.LabeledBy to the hotkey controls. In practice the current code contains no such addition — the XAML changes are pure cosmetic reformatting (multi-line attributes for the hotkey ComboBox, TextBlock, and TextBox). The only substantive change is the new .jules/palette.md entry.

Thread resolution: Both prior threads are now resolved. The LabeledBy accessibility regression thread (PRRT_kwDORLRSls6NkDxK) is resolved because LabeledBy is absent from the current code — the regression no longer exists. The wrong-date thread (PRRT_kwDORLRSls6NkD6D) is resolved because the date is now 2026-07-01 and GitHub had already marked it outdated.

Remaining issue (blocking): .jules/palette.md lines 25-26 capture the inverse of the lesson actually learned. The Learning and Action lines instruct future contributors to explicitly link controls to section headers via LabeledBy — which is exactly the anti-pattern reverted in this PR. The correct lesson: LabeledBy pointing to a shared section header overrides per-control .resw names and harms accessibility; rely on x:Uid-driven AutomationProperties.Name from .resw when descriptive per-control names are already present; use LabeledBy only with a dedicated per-control visible label element (the IconSizeLabel + IconSizeCombo pattern is the right model). See the inline comment on palette.md for details.

The XAML and C# changes are otherwise clean — no correctness, security, or threading concerns.

@jules please address the issues noted above

@google-labs-jules

Copy link
Copy Markdown
Contributor Author

Review — PR #425 (synchronize)

What this PR does (current commits): The PR title and body describe adding AutomationProperties.LabeledBy to the hotkey controls for accessibility. In practice the current code contains no such addition — the XAML changes are pure cosmetic reformatting (multi-line attribute style for the hotkey ComboBox, TextBlock, and TextBox). The only substantive change is the new .jules/palette.md entry.


Thread resolution

Thread Concern Status
PRRT_kwDORLRSls6NkDxK — LabeledBy accessibility regression LabeledBy pointing to HotkeyHeader for both controls overrode per-control .resw names, making the controls indistinguishable to screen readers ResolvedLabeledBy is absent from the current code; the regression no longer exists
PRRT_kwDORLRSls6NkD6D — Wrong date in palette.md Entry was dated 2024-11-20, 18 months before the preceding entry Resolved — Date is now 2026-07-01; thread was already marked outdated by GitHub

Remaining issue (blocking)

.jules/palette.md lines 25–26 — lesson is the inverse of what was learned.

The new entry tells future contributors to explicitly link controls to section headers via LabeledBy — which is exactly the anti-pattern that was reverted in this PR after review feedback. If a future agent follows this guidance, it will reproduce the same accessibility regression. The entry must be corrected to reflect the actual lesson: LabeledBy to a shared section header is harmful; per-control .resw AutomationProperties.Name (via x:Uid) is sufficient when descriptive names are already present; and LabeledBy should only be used with a dedicated per-control visible label element (see the IconSizeLabel + IconSizeCombo pattern in SettingsWindow.xaml). See inline comment for a full description.

The XAML and C# changes are otherwise clean and raise no correctness, security, or threading concerns.

@jules please address the issues noted above

I have updated the entry in .jules/palette.md to reflect the correct lesson regarding AutomationProperties.LabeledBy and .resw fallback names. It now warns against linking multiple controls to a generic shared section header and advises relying on .resw names or dedicated 1:1 labels.

@google-labs-jules

Copy link
Copy Markdown
Contributor Author

Review PR 425 (synchronize)

What this PR does: The title and body describe adding AutomationProperties.LabeledBy to the hotkey controls. In practice the current code contains no such addition — the XAML changes are pure cosmetic reformatting (multi-line attributes for the hotkey ComboBox, TextBlock, and TextBox). The only substantive change is the new .jules/palette.md entry.

Thread resolution: Both prior threads are now resolved. The LabeledBy accessibility regression thread (PRRT_kwDORLRSls6NkDxK) is resolved because LabeledBy is absent from the current code — the regression no longer exists. The wrong-date thread (PRRT_kwDORLRSls6NkD6D) is resolved because the date is now 2026-07-01 and GitHub had already marked it outdated.

Remaining issue (blocking): .jules/palette.md lines 25-26 capture the inverse of the lesson actually learned. The Learning and Action lines instruct future contributors to explicitly link controls to section headers via LabeledBy — which is exactly the anti-pattern reverted in this PR. The correct lesson: LabeledBy pointing to a shared section header overrides per-control .resw names and harms accessibility; rely on x:Uid-driven AutomationProperties.Name from .resw when descriptive per-control names are already present; use LabeledBy only with a dedicated per-control visible label element (the IconSizeLabel + IconSizeCombo pattern is the right model). See the inline comment on palette.md for details.

The XAML and C# changes are otherwise clean — no correctness, security, or threading concerns.

@jules please address the issues noted above

I have updated the entry in .jules/palette.md to reflect the correct lesson regarding AutomationProperties.LabeledBy and .resw fallback names. It now warns against linking multiple controls to a generic shared section header and advises relying on .resw names or dedicated 1:1 labels.

@mikekthx

mikekthx commented Jul 1, 2026

Copy link
Copy Markdown
Owner

Thread Resolution

Thread resolved: The palette.md **Learning:**/**Action:** entry that originally documented the anti-pattern (using LabeledBy against a shared section header) as the correct approach has been rewritten to correctly describe the harm. The concern is fully addressed — resolving the outdated thread.


Code Review

What this PR does

After review feedback on the original commit (which added AutomationProperties.LabeledBy pointing both the modifier ComboBox and the key TextBox at the shared HotkeyHeader element), the implementation was correctly rolled back. The net change in this PR is:

  • SettingsWindow.xaml — Pure formatting: three single-line elements are reformatted to multi-line attribute style. No functional or accessibility change.
  • .jules/palette.md — A new entry documents the correct lesson: LabeledBy pointing multiple controls at a shared section header overrides their per-control .resw-driven AutomationProperties.Name entries, harming screen reader experience.

Assessment: ✅ Approved

The code is correct. The original accessibility regression was reverted, and the existing x:Uid-driven names (already present in all 13 .resw files) continue to provide the right per-control accessible names without any extra XAML plumbing.

Observations (non-blocking)

  1. PR description is stale. The description says "Added AutomationProperties.LabeledBy to Hotkey inputs" and "Screen readers will now announce the 'Hotkey' label", but the final code does neither. The PR metadata was not updated after the implementation was revised. Not a code issue, but could cause confusion for anyone reading PR history.

  2. Palette entry — extra sentence scope. The sentence beginning "Additionally, do NOT apply AutomationProperties.Name to non-interactive structural layout panels…" covers guidance not evidenced anywhere in this PR's diff. It reads as if it addresses a change that was made and then reverted. The advice is accurate (it aligns with the 2026-03-27 grid toggle entry), but a reader might be puzzled about which change prompted it. Low risk — the lesson itself is valid.

  3. XAML formatting is correct. Multi-line attribute style is the right choice per project conventions for elements with multiple attributes.


No blocking issues. The accessibility concern from the previous review is correctly not implemented.

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

Labels

documentation Improvements or additions to READMEs, wikis, or inline code comments. ui Front-end changes, WinUI layouts, styling (e.g., Acrylic), or system tray updates.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant