Conversation
MenuRow renders its content as arbitrary child elements, which — unlike Switch, which derives an accessible name from its own label text — carries no accessible name on its own; a caller has to set one via the generic aria_label() builder GPUI already provides. Most call sites never did, so NVDA announced only the generic button role with no name for direction pickers, thumbwheel presets, the app catalog, per-key power-user actions, and the profile-removal menu item.
|
The explicit aria_label on workflow-step rows, gesture-direction rows, and application rows in the profile picker only announced each row's primary title, dropping the secondary text sighted users see (the step's payload, the direction's assigned action, the app's stable identifier) — so a screen reader couldn't distinguish rows that look identical otherwise (two "Type Text" steps, two apps sharing a display name). - WorkflowStepRow: extracted step_preview_text() so the aria_label and the visible preview text stay in sync. - Gesture direction rows: append the assigned action's localized label. - Profile picker application rows: append the app's stable identifier. Addresses the Greptile P1 finding on AprilNEA#1448.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Root cause
MenuRow(crates/openlogi-desktop/src/ui/components.rs) renders its content as arbitrary child elements (ParentElement/children: Vec<AnyElement>). UnlikeSwitch, which derives an accessible name from its ownlabeltext automatically,MenuRowhas no such fallback — it needs an explicit accessible name set via GPUI's genericaria_label()(InteractiveElement/StatefulInteractiveElement), which some call sites already do correctly (features/mouse/picker.rs,features/action_ring/editor.rs) and most simply never called.I initially added a bespoke
accessibility_label()builder toMenuRowitself before realizing GPUI already providesaria_label()generically on anything implementingStatefulInteractiveElement(whichMenuRowalready does) — reverted that in favor of the pattern this codebase already uses elsewhere, for consistency.Changes
Added
.aria_label(...)(reusing text already computed for each row's visible content — no new locale strings) to theMenuRowcall sites that were missing it:features/keyboard/editors.rs— workflow step rows (type label: "Type Text" / "Press Key" / etc.)features/keyboard/function_row.rs— per-key power-user action rows (Type Text / AppleScript / Shell / Workflow)features/mouse/inspector.rs— gesture-direction rows and thumbwheel preset rowsfeatures/profiles/picker.rs— app-catalog rows (the app's display name)features/profiles/shell.rs— the profile-removal menu itemui/gallery.rs— the demo rows in the component gallery, per this crate's own rule that a changed reusable component's gallery entry must be updated in the same commitLeft alone:
features/mouse/picker.rsandfeatures/action_ring/editor.rs(already correct), and the two#[cfg(test)]harness call sites (test scaffolding, not shipped UI).Scope note
I found (but did not fix) an adjacent, larger gap: gpui-component's
Sliderwidget has noaccessibility_label/aria_labelsupport at all, at either the base or component layer — so every slider in this app (DPI, SmartShift threshold, thumbwheel/scroll sensitivity, camera controls) is unlabeled by the underlying library, not by anything in this repo's own code. Fixing that needs an upstream change to gpui-kit, out of scope here.Testing
cargo fmt --all -- --check,cargo clippy -p openlogi-desktop --all-targets -- -D warnings,cargo test -p openlogi-desktop— all green (219 passed, including the gallery render test covering the two rows I touched there).Fixes #1226 (partially — the
Slidergap noted above remains and would need a separate, upstream-dependent fix)