[ACS-10242] SR: Personal files: Filter by Size popup announces entire content instead of dialog title#11782
Conversation
… content instead of dialog title
dominikiwanekhyland
requested review from
DenysVuika,
eromano and
popovicsandras
as code owners
|
There was a problem hiding this comment.
Pull request overview
This PR addresses an accessibility issue in the Content Services search header filter popup (notably “Filter by Size”), where screen readers announce the entire popup contents instead of a meaningful title.
Changes:
- Updates the filter popup template to use semantic grouping (
fieldset/legend) and revised focusable container markup. - Adjusts SCSS to support the new structure and spacing (removes container padding, adds fieldset/title/checklist spacing rules).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/content-services/src/lib/search/components/search-filter-container/search-filter-container.component.html | Restructures the popup container markup for improved accessible labeling/announcement. |
| lib/content-services/src/lib/search/components/search-filter-container/search-filter-container.component.scss | Adds styling for the new fieldset wrapper and updates padding/margins to match the new layout. |
| <mat-menu #filter="matMenu" class="adf-filter-menu adf-search-filter-menu" (closed)="onClosed()"> | ||
| <div #filterContainer role="menuitem" tabindex="0" (keydown.tab)="$event.stopPropagation()"> | ||
| <div (click)="$event.stopPropagation()" role="button" tabindex="0" (keyup.enter)="$event.stopPropagation()" class="adf-filter-container"> | ||
| <div class="adf-filter-title">{{ category?.name | translate }}</div> | ||
| <adf-search-widget-container | ||
| (keypress)="onKeyPressed($event, menuTrigger)" | ||
| [id]="category?.id" | ||
| [selector]="category?.component?.selector" | ||
| [settings]="category?.component?.settings" | ||
| [value]="initialValue" | ||
| [useHeaderQueryBuilder]="true" | ||
| /> | ||
| </div> | ||
| <fieldset | ||
| #filterContainer | ||
| tabindex="0" | ||
| [attr.aria-label]="category?.name | translate" | ||
| (click)="$event.stopPropagation()" | ||
| (keyup.enter)="$event.stopPropagation()" | ||
| (keydown.tab)="$event.stopPropagation()" | ||
| class="adf-filter-container adf-filter-fieldset" |
There was a problem hiding this comment.
mat-menu defaults to role="menu", but this popup behaves like a dialog (focus trap, form controls). To avoid screen readers announcing it as a menu (and potentially reading the whole contents as one item), consider aligning with the existing pattern in search-widget-chip.component.html: set the mat-menu to role="presentation" and set the focus-trapped container to role="dialog" with an aria-labelledby pointing at the title element (instead of relying on menu semantics).
| [attr.aria-label]="category?.name | translate" | ||
| (click)="$event.stopPropagation()" | ||
| (keyup.enter)="$event.stopPropagation()" | ||
| (keydown.tab)="$event.stopPropagation()" | ||
| class="adf-filter-container adf-filter-fieldset" | ||
| > | ||
| <legend class="adf-filter-title">{{ category?.name | translate }}</legend> |
There was a problem hiding this comment.
The fieldset currently has both an aria-label and a <legend> containing the same title. This can lead to duplicate/odd announcements in some screen readers and makes the labeling harder to reason about. Prefer a single source of truth by using aria-labelledby (referencing the legend) or dropping the redundant label.
| [attr.aria-label]="category?.name | translate" | |
| (click)="$event.stopPropagation()" | |
| (keyup.enter)="$event.stopPropagation()" | |
| (keydown.tab)="$event.stopPropagation()" | |
| class="adf-filter-container adf-filter-fieldset" | |
| > | |
| <legend class="adf-filter-title">{{ category?.name | translate }}</legend> | |
| [attr.aria-labelledby]="'filter-legend-' + category?.id" | |
| (click)="$event.stopPropagation()" | |
| (keyup.enter)="$event.stopPropagation()" | |
| (keydown.tab)="$event.stopPropagation()" | |
| class="adf-filter-container adf-filter-fieldset" | |
| > | |
| <legend class="adf-filter-title" [id]="'filter-legend-' + category?.id"> | |
| {{ category?.name | translate }} | |
| </legend> |
| <fieldset | ||
| #filterContainer | ||
| tabindex="0" | ||
| [attr.aria-label]="category?.name | translate" | ||
| (click)="$event.stopPropagation()" |
There was a problem hiding this comment.
This change is meant to fix a screen-reader announcement issue, but there’s no unit test asserting the popup container’s accessible semantics (e.g., that it’s labelled by the title / has the expected role). Since this component already has an Accessibility test section, please add an assertion to cover the intended a11y behavior to prevent regressions.
|
@dominikiwanekhyland as per new standards - please address all copilot's comments. By addressing I mean - push code with fix and answer/close comment. If any comment is invalid then just answer to that comment and close it. |
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x")
What is the current behaviour? (You can also link to an open issue here)
https://hyland.atlassian.net/browse/ACS-10242
What is the new behaviour?
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information: