Skip to content

🎨 Palette: Link hotkey inputs to header for accessibility - #427

Open
google-labs-jules[bot] wants to merge 3 commits into
mainfrom
fix-hotkey-a11y-10757027313577988780
Open

🎨 Palette: Link hotkey inputs to header for accessibility#427
google-labs-jules[bot] wants to merge 3 commits into
mainfrom
fix-hotkey-a11y-10757027313577988780

Conversation

@google-labs-jules

Copy link
Copy Markdown
Contributor

💡 What: Added x:Name="HotkeyHeader" to the "Hotkey" settings section header and applied AutomationProperties.LabeledBy="{Binding ElementName=HotkeyHeader}" to its associated modifier ComboBox and key TextBox.
🎯 Why: To provide context to screen reader users when interacting with the hotkey inputs, as they previously had no explicit programmatically linked labels.
Accessibility: Screen readers will now announce the inputs as belonging to the "Hotkey" section, improving navigability and understanding for visually impaired users.


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

@google-labs-jules
google-labs-jules Bot requested a review from mikekthx as a code owner July 3, 2026 11:31
@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 3, 2026
Comment thread SettingsWindow.xaml Outdated
Comment thread SettingsWindow.xaml Outdated
Comment thread .jules/palette.md Outdated
@mikekthx

mikekthx commented Jul 3, 2026

Copy link
Copy Markdown
Owner

Review: Link hotkey inputs to header for accessibility

What this PR does: Adds x:Name="HotkeyHeader" to the Global Hotkey section header and applies AutomationProperties.LabeledBy="{Binding ElementName=HotkeyHeader}" to the modifier ComboBox and key TextBox, intending to give screen readers group context for those controls.

Overall assessment: Changes requested — this is a net regression, not an improvement.


Blocking issue: LabeledBy overrides existing, more specific accessible names

The PR description states the hotkey controls "had no explicit programmatically linked labels" — but this is incorrect. Checking Strings/en-US/Resources.resw (and all other 12 locales) reveals that x:Uid already provides per-control AutomationProperties.Name values:

Control Existing accessible name via x:Uid
ComboBox (Settings_HotkeyModifier) "Hotkey modifier"
TextBox (Settings_HotkeyKey) "Hotkey key"

In UI Automation, AutomationProperties.LabeledBy takes precedence over AutomationProperties.Name. After this change both controls would announce "Global Hotkey" instead of their specific names. A screen reader user tabbing between them would hear the same phrase twice with no way to distinguish the modifier picker from the key input — strictly worse than the status quo.

The LabeledBy syntax is correct but the application is wrong

The CLAUDE.md rule that LabeledBy must use {Binding ElementName=...} (not {x:Bind}) is followed correctly. The problem is that LabeledBy should only be applied when a control lacks its own accessible name. These controls already have well-scoped names through x:Uid.

Palette entry records the wrong lesson

The new .jules/palette.md entry advises using LabeledBy on controls that share a group header without dedicated labels. That advice is reasonable in isolation, but the action here was taken against controls that did have dedicated labels, so the entry encodes a flawed mental model. See the inline comment for a corrected version of the learning.

Required changes

  1. Remove AutomationProperties.LabeledBy from both the ComboBox and the TextBox in SettingsWindow.xaml.
  2. Remove or correct the .jules/palette.md entry — the existing control names via x:Uid already satisfy the accessibility requirement; no structural change is needed here.

@jules please address the issues noted above

@google-labs-jules

Copy link
Copy Markdown
Contributor Author

Review: Link hotkey inputs to header for accessibility

What this PR does: Adds x:Name="HotkeyHeader" to the Global Hotkey section header and applies AutomationProperties.LabeledBy="{Binding ElementName=HotkeyHeader}" to the modifier ComboBox and key TextBox, intending to give screen readers group context for those controls.

Overall assessment: Changes requested — this is a net regression, not an improvement.


Blocking issue: LabeledBy overrides existing, more specific accessible names

The PR description states the hotkey controls "had no explicit programmatically linked labels" — but this is incorrect. Checking Strings/en-US/Resources.resw (and all other 12 locales) reveals that x:Uid already provides per-control AutomationProperties.Name values:

Control Existing accessible name via x:Uid
ComboBox (Settings_HotkeyModifier) "Hotkey modifier"
TextBox (Settings_HotkeyKey) "Hotkey key"

In UI Automation, AutomationProperties.LabeledBy takes precedence over AutomationProperties.Name. After this change both controls would announce "Global Hotkey" instead of their specific names. A screen reader user tabbing between them would hear the same phrase twice with no way to distinguish the modifier picker from the key input — strictly worse than the status quo.

The LabeledBy syntax is correct but the application is wrong

The CLAUDE.md rule that LabeledBy must use {Binding ElementName=...} (not {x:Bind}) is followed correctly. The problem is that LabeledBy should only be applied when a control lacks its own accessible name. These controls already have well-scoped names through x:Uid.

Palette entry records the wrong lesson

The new .jules/palette.md entry advises using LabeledBy on controls that share a group header without dedicated labels. That advice is reasonable in isolation, but the action here was taken against controls that did have dedicated labels, so the entry encodes a flawed mental model. See the inline comment for a corrected version of the learning.

Required changes

  1. Remove AutomationProperties.LabeledBy from both the ComboBox and the TextBox in SettingsWindow.xaml.
  2. Remove or correct the .jules/palette.md entry — the existing control names via x:Uid already satisfy the accessibility requirement; no structural change is needed here.

@jules please address the issues noted above

Thank you for the review. I have reverted the incorrect application of AutomationProperties.LabeledBy in SettingsWindow.xaml and replaced the learning in .jules/palette.md to correctly reflect the precedence of AutomationProperties.LabeledBy over x:Uid.

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

mikekthx commented Jul 3, 2026

Copy link
Copy Markdown
Owner

Review — second push (synchronize)

What the PR does

The original commit added x:Name="HotkeyHeader" to the Hotkey section header and AutomationProperties.LabeledBy to both the modifier ComboBox and key TextBox in SettingsWindow.xaml, intending to improve accessibility. A prior review correctly identified that both controls already have per-control accessible names via x:Uid/.resw, and that LabeledBy overrides Name in UI Automation — making the change a net regression. The second commit reverts the XAML and updates .jules/palette.md with the corrected learning.

Thread resolution

All three prior review threads are now outdated and their underlying concerns are addressed:

  • ComboBox LabeledBy removed — XAML is back to original, per-control name preserved
  • TextBox LabeledBy removed — same
  • .jules/palette.md learning updated to accurately reflect when LabeledBy is appropriate

Blocking issue — fix_review.py must be removed

The second commit left a scratch Python script (fix_review.py) in the repo root. This is the tool Jules used to perform the text replacement and should never have been committed. It must be deleted before this PR can merge.

.jules/palette.md learning (minor observation)

The new entry is substantively correct and actionable. The phrasing is slightly looser than the exact wording requested in the review thread, but it conveys the essential rule: do not apply LabeledBy when a control already has an accessible name through x:Uid. No change required here.

Overall assessment

The intent of the second commit is correct. One blocking issue remains: remove fix_review.py.

@jules please address the issues noted above

@google-labs-jules

Copy link
Copy Markdown
Contributor Author

Review — second push (synchronize)

What the PR does

The original commit added x:Name="HotkeyHeader" to the Hotkey section header and AutomationProperties.LabeledBy to both the modifier ComboBox and key TextBox in SettingsWindow.xaml, intending to improve accessibility. A prior review correctly identified that both controls already have per-control accessible names via x:Uid/.resw, and that LabeledBy overrides Name in UI Automation — making the change a net regression. The second commit reverts the XAML and updates .jules/palette.md with the corrected learning.

Thread resolution

All three prior review threads are now outdated and their underlying concerns are addressed:

  • ComboBox LabeledBy removed — XAML is back to original, per-control name preserved
  • TextBox LabeledBy removed — same
  • .jules/palette.md learning updated to accurately reflect when LabeledBy is appropriate

Blocking issue — fix_review.py must be removed

The second commit left a scratch Python script (fix_review.py) in the repo root. This is the tool Jules used to perform the text replacement and should never have been committed. It must be deleted before this PR can merge.

.jules/palette.md learning (minor observation)

The new entry is substantively correct and actionable. The phrasing is slightly looser than the exact wording requested in the review thread, but it conveys the essential rule: do not apply LabeledBy when a control already has an accessible name through x:Uid. No change required here.

Overall assessment

The intent of the second commit is correct. One blocking issue remains: remove fix_review.py.

@jules please address the issues noted above

Thank you for the review. I have removed the fix_review.py script as requested.

@mikekthx

mikekthx commented Jul 3, 2026

Copy link
Copy Markdown
Owner

Review Summary

What this PR does (current state)

The original PR attempted to improve accessibility by adding x:Name="HotkeyHeader" and AutomationProperties.LabeledBy to the hotkey modifier ComboBox and key TextBox in SettingsWindow.xaml. This was a regression: those controls already had per-control AutomationProperties.Name values set via x:Uid in all 13 .resw files, and LabeledBy overrides Name in the UI Automation tree.

After the review feedback on this sync, the branch has been corrected. The net diff vs. main now only contains one change: a 3-line addition to .jules/palette.md recording the correct lesson about LabeledBy precedence.


Thread Resolution

All four prior review threads have now been resolved:

Thread Concern Status
ComboBox LabeledBy overrides existing x:Uid name XAML changes reverted — control retains its specific x:Uid-derived name Resolved (outdated)
TextBox LabeledBy overrides existing x:Uid name Same — reverted Resolved (outdated)
palette.md entry propagated incorrect premise New entry accurately captures the LabeledBy -> Name override behaviour Resolved (outdated)
fix_review.py scratch script accidentally committed File is no longer in the branch tree Resolved now

Assessment of the remaining change

.jules/palette.md addition — LGTM.

The new entry is accurate:

  • LabeledBy does override AutomationProperties.Name (including names sourced from x:Uid) in the UIA tree.
  • The action item correctly advises checking for existing per-control names before applying LabeledBy.
  • Format and date follow the existing palette conventions.

One minor observation: the PR description still describes the original (now-reverted) XAML intent. Not a blocker, but updating it to reflect "reverted bad LabeledBy changes + added corrected palette entry" would make the history clearer.


Verdict

No blocking issues remain. The branch is clean — only the documentation addition to palette.md is present. Ready to merge once the description note above is addressed (optional).

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