Conversation
Coverage report for library
Test suite run success580 tests passing in 24 suites. Report generated by 🧪jest coverage report action from b92e6b9 |
Playwright test resultsDetails
|
There was a problem hiding this comment.
Pull request overview
This PR updates the cps-ui-kit dialog implementation and its composition docs/examples to address accessibility issues around dialog labeling, focus behavior, keyboard interaction, and scalable sizing.
Changes:
- Added dialog accessibility options and behaviors, including ARIA labeling, configurable autofocus, focus trapping/restoration, and keyboard-based move/resize support.
- Converted dialog-related sizing/styling from fixed px values to rem-aware sizing utilities and refreshed demos/defaults/docs accordingly.
- Re-enabled dialog coverage in the Playwright accessibility suite and updated tooltip accessibility behavior.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
projects/cps-ui-kit/src/lib/services/cps-dialog/utils/cps-dialog-config.ts |
Expanded dialog config API with ARIA, autofocus, and numeric/string sizing options. |
projects/cps-ui-kit/src/lib/services/cps-dialog/internal/components/cps-dialog/cps-dialog.component.ts |
Implemented most dialog accessibility, focus, drag, resize, and size-conversion logic. |
projects/cps-ui-kit/src/lib/services/cps-dialog/internal/components/cps-dialog/cps-dialog.component.scss |
Updated dialog visuals, focus states, and rem-based styling for draggable/resizable states. |
projects/cps-ui-kit/src/lib/services/cps-dialog/internal/components/cps-dialog/cps-dialog.component.html |
Added ARIA attributes, tabindexes, keyboard hooks, and resizable handle markup. |
projects/cps-ui-kit/src/lib/services/cps-dialog/internal/components/cps-confirmation/cps-confirmation.component.scss |
Converted confirmation dialog spacing/text sizing to rem units. |
projects/cps-ui-kit/src/lib/services/cps-dialog/internal/components/cps-confirmation/cps-confirmation.component.html |
Tweaked confirmation button styling/colors. |
projects/cps-ui-kit/src/lib/services/cps-dialog/cps-dialog.service.ts |
Updated confirmation dialog default width constraints to rem values. |
projects/cps-ui-kit/src/lib/directives/cps-tooltip/cps-tooltip.directive.ts |
Adjusted tooltip focus behavior and ARIA description/linking logic. |
projects/composition/src/app/pages/dialog-page/dialog-page.component.ts |
Updated dialog demo configs to exercise new autofocus and rem sizing options. |
projects/composition/src/app/pages/dialog-page/dialog-page.component.html |
Updated demo button copy to reflect new autofocus example behavior. |
projects/composition/src/app/components/dialog-content/dialog-content.component.scss |
Converted dialog demo content spacing/icon sizing to rem units. |
projects/composition/src/app/api-data/types_map.json |
Registered the new dialog autofocus type for generated docs. |
projects/composition/src/app/api-data/cps-dialog.json |
Refreshed generated dialog API docs to match new config options and types. |
playwright/cps-accessibility.spec.ts |
Re-enabled dialog route in the accessibility smoke suite. |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
| this._document.body.appendChild(this._popup); | ||
| this._ariaTarget?.setAttribute('aria-describedby', this._tooltipId); | ||
| this._ariaTarget?.setAttribute( | ||
| 'aria-description', |
There was a problem hiding this comment.
Any specific reason to use aria-description? Docs states the following:
Note: aria-description is still in W3C Editor's Draft for ARIA 1.3. For the time being, continue to use aria-describedby, which has been supported since ARIA 1.1.
(applies also to aria-description in the dialog component)
There was a problem hiding this comment.
Yes, there was a specific issue where tooltip content wasn't announced by screen readers when used inside modal dialogs. This is likely because the modal dialog creates an accessibility boundary, so tooltips referenced through aria-describedby are not reachable, since they are rendered in another overlay.
Using aria-description avoids this problem since it attaches the description directly to the element. It produces exactly the same outcome for screen readers as aria-describedby. Since it works without any issues in Chromium, I think we can go ahead with this approach.
There was a problem hiding this comment.
close button should be disabled when disableClose is true
There was a problem hiding this comment.
good catch - fixed
| const unit = parsedHeight.unit; | ||
| const size = parsedHeight.value * 0.4; | ||
| const isPx = unit === 'px'; | ||
| if (parsedHeight) { |
There was a problem hiding this comment.
Changes of the parseSize/convertSize are not documented in the PR description.
There was a problem hiding this comment.
added to PR description
| aria-description="Use arrow keys to resize" | ||
| (mousedown)="initResize($event)" | ||
| (keydown)="onResizeHandleKeydown($event)" | ||
| (keyup)="onResizeHandleKeyup($event)"> |
There was a problem hiding this comment.
keyboard resizing seems to be buggy, can only be resized to be bigger, not smaller
There was a problem hiding this comment.
Thanks, it broke while I was addressing the feedback from copilot. Fixed now
There was a problem hiding this comment.
It's out of the scope of this PR, but it'd be great if the a11y tests were grouped by the component they target.
Opened #580 for this.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Fixing accessibility issues in dialog 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:
convertSizefunction now passes through all valid CSS values (e.g.,calc(),auto,fit-content,min(),vw, etc.). It also validates input strings, and it is now also fine to pass negative numbers.parseSizefunction now returnsnullinstead of throwing when the value doesn't have the expected unit, and its regex has been updated to support negative numbers.Release notes: