Skip to content

🎨 Palette: [UX improvement] Fix XAML accessibility anti-patterns - #442

Open
google-labs-jules[bot] wants to merge 2 commits into
mainfrom
palette-a11y-improvements-2023129919356152755
Open

🎨 Palette: [UX improvement] Fix XAML accessibility anti-patterns#442
google-labs-jules[bot] wants to merge 2 commits into
mainfrom
palette-a11y-improvements-2023129919356152755

Conversation

@google-labs-jules

Copy link
Copy Markdown
Contributor

💡 What:

Added explicit accessibility labeling to the global hotkey form controls in the Settings window.

🎯 Why:

Currently, the "Global Hotkey" section has a TextBlock serving as a visual header, but the actual ComboBox and TextBox next to it do not have programmatic relationships to that header. Screen readers focus on these elements but cannot announce what they are for, reducing the app's intuitiveness for visually impaired users. By linking them using AutomationProperties.LabeledBy, we create a proper semantic relationship.

♿ Accessibility:

  • Screen readers (like Narrator) will now correctly announce "Global Hotkey, Modifier" or "Global Hotkey, Key" when focusing on the form inputs.
  • Preserves Windows UX conventions for linked form inputs.

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

@google-labs-jules
google-labs-jules Bot requested a review from mikekthx as a code owner July 24, 2026 11:35
@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.

@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 24, 2026
Comment thread SettingsWindow.xaml Outdated
Comment thread SettingsWindow.xaml Outdated
Comment thread .jules/palette.md
Comment thread pr_body.txt Outdated
@mikekthx

Copy link
Copy Markdown
Owner

Code Review

What this PR does

Adds x:Name to the Global Hotkey section header TextBlock in SettingsWindow.xaml and links it to the adjacent ComboBox and TextBox via AutomationProperties.LabeledBy, with the intent of improving screen-reader accessibility. It also adds a learning entry to .jules/palette.md and (incorrectly) commits the PR body as pr_body.txt.


Verdict: ❌ Changes Requested — blocking issues

1. Accessibility regression (blocking)

The core premise of this PR is incorrect, and the XAML changes make accessibility worse, not better.

Both the ComboBox (x:Uid="Settings_HotkeyModifier") and the TextBox (x:Uid="Settings_HotkeyKey") already have specific AutomationProperties.Name values set via the .resw resource files:

x:Uid Existing AutomationProperties.Name (all 13 locales)
Settings_HotkeyModifier "Hotkey modifier"
Settings_HotkeyKey "Hotkey key"

When both AutomationProperties.Name (from x:Uid) and AutomationProperties.LabeledBy are present on the same element, UIA resolves LabeledBy as the accessible name — it overrides the resource-based Name. Both controls now announce as "Global Hotkey", which is the same generic label for both, and less specific than what was there before.

The LabeledBy approach (pointing at a shared section header) makes sense when controls have no other accessible name. Here it actively degrades the already-correct, per-control names. The correct fix is to revert the XAML changes to SettingsWindow.xaml; the resources already handle this correctly.

2. pr_body.txt committed to the repo (blocking)

pr_body.txt is a leftover from the automated PR creation process. It has no place in the repository and must be deleted.

3. Shell substitution not expanded in .jules/palette.md (minor)

The literal string $(date +%Y-%m-%d) appears in the file instead of the actual date (2026-07-24). CLAUDE.md explicitly requires running date in the terminal and using its output.


What is correct in this PR

  • Using {Binding ElementName=...} (not {x:Bind}) for AutomationProperties.LabeledBy is the right syntax per CLAUDE.md — but applying LabeledBy here at all is the problem.
  • The palette.md learning entry documents the right pattern for cases where controls genuinely lack accessible names.

@jules please address the issues noted above

@google-labs-jules

Copy link
Copy Markdown
Contributor Author

Code Review

What this PR does

Adds x:Name to the Global Hotkey section header TextBlock in SettingsWindow.xaml and links it to the adjacent ComboBox and TextBox via AutomationProperties.LabeledBy, with the intent of improving screen-reader accessibility. It also adds a learning entry to .jules/palette.md and (incorrectly) commits the PR body as pr_body.txt.


Verdict: ❌ Changes Requested — blocking issues

1. Accessibility regression (blocking)

The core premise of this PR is incorrect, and the XAML changes make accessibility worse, not better.

Both the ComboBox (x:Uid="Settings_HotkeyModifier") and the TextBox (x:Uid="Settings_HotkeyKey") already have specific AutomationProperties.Name values set via the .resw resource files:

x:Uid Existing AutomationProperties.Name (all 13 locales)
Settings_HotkeyModifier "Hotkey modifier"
Settings_HotkeyKey "Hotkey key"

When both AutomationProperties.Name (from x:Uid) and AutomationProperties.LabeledBy are present on the same element, UIA resolves LabeledBy as the accessible name — it overrides the resource-based Name. Both controls now announce as "Global Hotkey", which is the same generic label for both, and less specific than what was there before.

The LabeledBy approach (pointing at a shared section header) makes sense when controls have no other accessible name. Here it actively degrades the already-correct, per-control names. The correct fix is to revert the XAML changes to SettingsWindow.xaml; the resources already handle this correctly.

2. pr_body.txt committed to the repo (blocking)

pr_body.txt is a leftover from the automated PR creation process. It has no place in the repository and must be deleted.

3. Shell substitution not expanded in .jules/palette.md (minor)

The literal string $(date +%Y-%m-%d) appears in the file instead of the actual date (2026-07-24). CLAUDE.md explicitly requires running date in the terminal and using its output.


What is correct in this PR

  • Using {Binding ElementName=...} (not {x:Bind}) for AutomationProperties.LabeledBy is the right syntax per CLAUDE.md — but applying LabeledBy here at all is the problem.
  • The palette.md learning entry documents the right pattern for cases where controls genuinely lack accessible names.

@jules please address the issues noted above

Thank you for the review. I have reverted the changes to SettingsWindow.xaml since the accessible names were already correctly handled by the .resw files. I have also deleted pr_body.txt and fixed the unexpanded shell substitution in .jules/palette.md.

@github-actions github-actions Bot removed the ui Front-end changes, WinUI layouts, styling (e.g., Acrylic), or system tray updates. label Jul 24, 2026
@mikekthx

Copy link
Copy Markdown
Owner

Review Summary — Synchronize event (new commits)

What this PR does

Attempted to improve accessibility in the Settings window by adding AutomationProperties.LabeledBy to the hotkey ComboBox and TextBox controls. The new commits reverted the XAML changes after it was identified that they would actually degrade accessibility (both controls already had specific accessible names from their x:Uid .resw bindings; LabeledBy would have overridden those with the generic section header). The update that remains is a learning log entry in .jules/palette.md.

Thread resolution

All four previously open threads were addressed by the new commits and have been resolved:

Thread Concern Resolution
ComboBox LabeledBy Would override specific x:Uid accessible name ✅ XAML change reverted
TextBox LabeledBy Same issue ✅ XAML change reverted
palette.md date literal $(date +%Y-%m-%d) was unevaluated ✅ Fixed to 2026-07-24
pr_body.txt artifact Repo artifact that should not be committed ✅ File removed

Current diff assessment

The only remaining change is the addition to .jules/palette.md:

Content accuracy: The learning entry correctly captures the core lesson — LabeledBy should only be applied when controls lack an existing specific accessible name, and that adding it on top of an x:Uid-sourced AutomationProperties.Name causes the generic header text to win over the specific control name.

Format: Consistent with existing entries (date heading, **Learning:**, **Action:**).

{Binding ElementName=...} usage in the Action note: Correctly documents the right XAML syntax per CLAUDE.md's sharp-edge note on LabeledBy.

Overall assessment

Approved. The net result of this PR is correct: the regression was reverted and a useful learning note was added. No blocking issues remain.

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant