Open
Conversation
Co-authored-by: Copilot <copilot@github.com>
fateeand
requested review from
TerranceKhumalo-absa,
korel-san and
lukasmatta
as code owners
Playwright test results
Details
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR targets accessibility improvements in the sidebar menu and related menu behaviors by adding explicit ARIA attributes/roles, improving focus-ring behavior based on input modality, and expanding a11y validation coverage in both unit and Playwright tests.
Changes:
- Added input-modality tracking and focus-ring suppression to reduce unwanted focus indicators after pointer interactions.
- Reworked
cps-sidebar-menumarkup/styles for improved semantics, keyboard operability, and contrast/focus styling. - Updated API docs generation + composition examples, and re-enabled Playwright a11y coverage for the sidebar menu page.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/cps-ui-kit/styles/_mixins.scss | Extends focus-ring mixin to support suppressing focus-visible styling. |
| projects/cps-ui-kit/src/lib/services/input-modality.service.ts | Adds a modality-tracking service/token for keyboard vs pointer heuristics. |
| projects/cps-ui-kit/src/lib/components/cps-sidebar-menu/cps-sidebar-menu.component.ts | Updates sidebar menu behavior (signals for height, focus/menu open handling). |
| projects/cps-ui-kit/src/lib/components/cps-sidebar-menu/cps-sidebar-menu.component.spec.ts | Adds unit tests for sidebar menu behavior and accessibility-related rendering. |
| projects/cps-ui-kit/src/lib/components/cps-sidebar-menu/cps-sidebar-menu.component.scss | Updates sizing to rem units and adds focus/disabled styling improvements. |
| projects/cps-ui-kit/src/lib/components/cps-sidebar-menu/cps-sidebar-menu.component.html | Refactors template structure, adds ARIA attributes/roles, replaces div triggers with buttons/links. |
| projects/cps-ui-kit/src/lib/components/cps-menu/cps-menu.component.ts | Tracks how the menu was opened and restores focus with optional focus-ring suppression. |
| projects/cps-ui-kit/src/lib/components/cps-menu/cps-menu.component.html | Adds optional aria-label support for menu items. |
| projects/composition/src/app/pages/sidebar-menu-page/sidebar-menu-page.component.scss | Converts spacing/typography to rem units on the sidebar menu docs page. |
| projects/composition/src/app/pages/menu-page/menu-page.component.ts | Adds ariaLabel to loading menu items in examples. |
| projects/composition/src/app/api-data/cps-table.json | Updates generated type docs to reflect optional properties. |
| projects/composition/src/app/api-data/cps-sidebar-menu.json | Documents new ariaLabel prop + optionality updates for sidebar menu item type. |
| projects/composition/src/app/api-data/cps-radio-group.json | Updates generated type docs to reflect optional properties. |
| projects/composition/src/app/api-data/cps-menu.json | Updates generated type docs to include ariaLabel and optional properties. |
| projects/composition/src/app/api-data/cps-button-toggle.json | Updates generated type docs to reflect optional properties. |
| playwright/cps-accessibility.spec.ts | Re-enables Playwright accessibility coverage for cps-sidebar-menu. |
| api-generator/api-generator.js | Skips emitting empty docs and marks optional properties with ? in generated type values. |
Co-authored-by: Copilot <copilot@github.com>
Coverage report for library
Test suite run success522 tests passing in 24 suites. Report generated by 🧪jest coverage report action from d9379fa |
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
lukasmatta
reviewed
May 7, 2026
| if (!isPlatformBrowser(this._platformId)) return; | ||
|
|
||
| this._document.addEventListener('keydown', (e) => { | ||
| const navigationKeys = new Set([ |
Collaborator
There was a problem hiding this comment.
Does this set need to be recreated on every event trigger? Maybe it can be moved to be a private readonly field.
Comment on lines
+134
to
+141
| if (menu.isVisible()) { | ||
| this.focusedItemWithMenu = item; | ||
| menu.hide(); | ||
| } else { | ||
| this.focusedItemWithMenu = item; | ||
| this.allMenus?.forEach((m) => m.hide()); | ||
| menu.show(null, event.currentTarget as HTMLElement, 'tr'); | ||
| } |
Collaborator
There was a problem hiding this comment.
Suggested change
| if (menu.isVisible()) { | |
| this.focusedItemWithMenu = item; | |
| menu.hide(); | |
| } else { | |
| this.focusedItemWithMenu = item; | |
| this.allMenus?.forEach((m) => m.hide()); | |
| menu.show(null, event.currentTarget as HTMLElement, 'tr'); | |
| } | |
| this.focusedItemWithMenu = item; | |
| if (menu.isVisible()) { | |
| menu.hide(); | |
| } else { | |
| this.allMenus?.forEach((m) => m.hide()); | |
| menu.show(null, event.currentTarget as HTMLElement, 'tr'); | |
| } |
Comment on lines
+284
to
+292
| const hasItemsA11yViolation = this.items.some( | ||
| (item) => !item.title?.trim() && !item.ariaLabel?.trim() | ||
| ); | ||
|
|
||
| if (hasItemsA11yViolation) { | ||
| console.error( | ||
| 'CpsMenuComponent: all untitled menu items must have an ariaLabel for accessibility.' | ||
| ); | ||
| } |
Collaborator
There was a problem hiding this comment.
I think this could be moved under an if (changes.items) condition
| [class.disabled]="item.disabled" | ||
| [attr.aria-disabled]="item.disabled || null" | ||
| [attr.aria-label]="item.title"> | ||
| <cps-icon [icon]="item.icon" size="normal"> </cps-icon> |
Collaborator
There was a problem hiding this comment.
Shouldn't this icon be aria-hidden? Same for lines 58, 74
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.
Fixing accessibility issues in sidebar menu component
Validation rules:
Validated using Playwright accessibility tests, Lighthouse tool, axe DevTools extension, Accessibility Insights for Web extension, and manual checks including keyboard tab navigation and screen reader testing.
Full doc with rules
Playwright axe-core validation results:
State before:
State after:
Checklist
Keyboard Navigation
All interactive elements are fully operable via keyboard only, including buttons, inputs, menus, dialogs, sliders, drag-and-drop, tree views, multi-selects, and composite widgets. No traps or dead ends.
Focus Management
Focus is visible, logical, moves in predictable order, trapped where necessary (modals/popovers), and restored after closing. Focus is perceivable in all interactive widgets.
Semantics / ARIA
Color / Contrast
Screen Reader / Assistive Technology
Responsive & Zoom
[N/A] Error Handling
Dynamic Content / Updates
Interaction Feedback / States
[N/A] Authentication & Sensitive Actions
Predictable & Controllable UI
Additional changes
CpsInputModalityServicehas been implemented and is currently only used by thecps-menucomponent. It tracks whether the most recent input event came from a keyboard or a pointer. This helps prevent focus rings from being shown on menu trigger elements while still maintaining proper focus order. So when a menu is closed using the Escape key and the target element wasn't reached through tab navigation, we won't show the focus ring. The service is exposed via an injection token in the public API, allowing consuming apps to override or disable it if necessary.Created a work item about showcasing public services API in the composition app: #582
Release notes: