Popover: support content reading on opening - #34311
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves accessibility behavior for Popover-family overlays by ensuring assistive technologies can consistently read popover content via aria-describedby when appropriate, while keeping role/aria-labelledby behavior correct across different interaction modes (tooltip-like vs dialog-like).
Changes:
- Add Popover-side syncing of overlay content
idand targetaria-describedby, including cleanup on option changes and dispose. - Refine
role/aria-labelledbybehavior for Popover/Tooltip/Popup (e.g., dialog-mode vs tooltip-mode, and titleTemplate edge cases). - Extend unit (QUnit) and e2e a11y coverage for the new role/label/description behaviors across Popover, Tooltip, Popup, ActionSheet, and Lookup.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/devextreme/testing/tests/DevExpress.ui.widgets/tooltip.tests.js | Adds a11y regression tests for Tooltip role behavior with title/close button and toolbar items. |
| packages/devextreme/testing/tests/DevExpress.ui.widgets/popup.tests.js | Adds tests ensuring aria-labelledby isn’t set when the title label element is missing due to a custom titleTemplate. |
| packages/devextreme/testing/tests/DevExpress.ui.widgets/popover.tests.js | Adds comprehensive tests for target aria-describedby wiring, stability, cleanup, and role/label interactions. |
| packages/devextreme/testing/tests/DevExpress.ui.widgets/actionSheet.tests.js | Verifies ActionSheet popover mode does not add aria-describedby to its target. |
| packages/devextreme/testing/tests/DevExpress.ui.widgets.editors/lookup.tests.js | Ensures Lookup popover mode stays dialog-role and does not describe the input element. |
| packages/devextreme/js/__internal/ui/tooltip.ts | Adjusts Tooltip to preserve legacy role rules and avoid Popover aria-describedby syncing (but needs a fix to keep role syncing). |
| packages/devextreme/js/__internal/ui/popup/popup.ts | Prevents dangling aria-labelledby when a custom titleTemplate doesn’t render a label element; resync on titleTemplate changes. |
| packages/devextreme/js/__internal/ui/popover/popover.ts | Implements aria role computation/sync, stable overlay content id generation, and target aria-describedby sync/cleanup. |
| packages/devextreme/js/__internal/ui/lookup.ts | Switches Lookup popover role forcing to internal _overlayContentRole instead of direct DOM mutation. |
| packages/devextreme/js/__internal/ui/action_sheet.ts | Switches ActionSheet popover role forcing to internal _overlayContentRole instead of direct DOM mutation. |
| packages/devextreme/js/__internal/scheduler/header/calendar.ts | Disables target description (_describeTarget: false) for an interactive calendar overlay to avoid noisy descriptions. |
| e2e/testcafe-devextreme/tests/accessibility/popover.ts | Updates option matrix and a11y configuration/selector to cover new behaviors for visible and hidden popovers. |
869b7fe to
5ccace2
Compare
Signed-off-by: Dmitry Lavrinovich <52966626+dmlvr@users.noreply.github.com>
58ba85c
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/devextreme/js/__internal/ui/popover/popover.ts:257
_syncAriaAttributessets the overlay role viasetAria('role', ...)without an explicit target element. Because Popover overrides_focusTarget()to return the first tabbable element inside the overlay,setAriawill applyroleto that first focusable child instead of the overlay content container, leaving$overlayContent().attr('role')unset/incorrect.
_syncAriaAttributes(): void {
this.setAria('role', this._getEffectiveAriaRole());
this._syncTargetAriaDescription();
e2e/testcafe-devextreme/tests/accessibility/popover.ts:23
- The NOTE comment ends mid-sentence (“for the combinations where.”), which reads like an unfinished thought and makes the intent unclear.
// NOTE: dialog-mode popovers (toolbarItems or showTitle + showCloseButton) have no
// accessible name unless a title is set. Providing a default dialog name is a separate
// dialog-labeling task, so the best-practice rule is disabled for the combinations where.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.
Suppressed comments (6)
packages/devextreme/js/__internal/ui/popover/tests/popover.aria_role.test.ts:40
- fx.off is set to true in a beforeAll hook but never restored, which can leak global animation settings into other Jest tests and cause order-dependent failures. Reset fx.off after each test and re-enable it at the start of each test run.
describe('Popover overlay content aria role', () => {
beforeAll(() => {
fx.off = true;
});
packages/devextreme/js/__internal/ui/tests/tooltip.aria_role.test.ts:40
- fx.off is set to true but never restored, which can leak the global animation flag into other Jest tests. Reset it after each test and turn it on at the start of each tooltip scenario creation.
describe('Tooltip overlay content aria role', () => {
beforeAll(() => {
fx.off = true;
});
e2e/testcafe-devextreme/tests/accessibility/popover.ts:23
- The NOTE comment ends with an incomplete sentence ("...for the combinations where.") which is likely a leftover and makes the intent unclear.
// NOTE: dialog-mode popovers (toolbarItems or showTitle + showCloseButton) have no
// accessible name unless a title is set. Providing a default dialog name is a separate
// dialog-labeling task.
packages/devextreme/js/__internal/ui/lookup.ts:682
- The NOTE comment has grammatical issues ("popover set", "do not have toolbar"), which makes it harder to understand quickly.
// NOTE: popover set role based on toolbarOptions, but
// Lookup with showCancelButton: false, do not have toolbar, so we use option.
packages/devextreme/js/__internal/ui/action_sheet.ts:183
- The NOTE comment has grammatical issues ("popover set", "ActionSheet rendered"), which reduces readability.
// NOTE: popover set role based on toolbarOptions, but
// ActionSheet rendered buttons not in a toolbar, so we use option.
packages/devextreme/js/__internal/ui/popup/tests/popup.aria_role.test.ts:39
- fx.off is enabled in beforeAll but never restored, so this file can leak the global animation toggle into unrelated Jest suites. Restore fx.off after each test and re-enable it when creating the popup for a scenario.
describe('Popup overlay content aria role', () => {
beforeAll(() => {
fx.off = true;
});
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.
Suppressed comments (3)
packages/devextreme/js/__internal/ui/popover/popover.ts:270
- The comment above
_renderFocusTargethas grammatical issues ("uses" / "do not need"), which makes it harder to understand the intent. Please rephrase for clarity.
// Intentional no-op: Focus target logic is inherited from Widget,
// uses in Popup and do not need here.
packages/devextreme/js/__internal/ui/action_sheet.ts:183
- This NOTE is grammatically incorrect and refers to "toolbarOptions" even though the role predicate is based on
toolbarItems(and title/close button). Rewording would make the rationale clearer.
// NOTE: popover set role based on toolbarOptions, but
// ActionSheet rendered buttons not in a toolbar, so we use option.
packages/devextreme/js/__internal/ui/lookup.ts:682
- This NOTE is both grammatically incorrect and slightly misleading: the role is derived from
toolbarItems(and title/close button), not "toolbarOptions". Rewording will make future maintenance clearer.
// NOTE: popover set role based on toolbarOptions, but
// Lookup with showCancelButton: false, do not have toolbar, so we use option.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/devextreme/js/__internal/ui/overlay/overlay.ts:842
- The new inline comment has grammatical issues (missing article, inconsistent capitalization) which reduces clarity.
// Note: method helps Scheduler Appointments to avoid Focus Race Condition
packages/devextreme/js/__internal/ui/popover/popover.ts:270
- The comment has grammatical issues (“uses in Popup and do not need here”), which makes it harder to understand the intent.
// Intentional no-op: Focus target logic is inherited from Widget,
// uses in Popup and do not need here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.
Suppressed comments (3)
packages/devextreme/js/__internal/ui/lookup.ts:682
- Comment grammar/capitalization: “popover set role … do not have toolbar” should be corrected (Popover → Popover sets; Lookup … does not have a toolbar).
// NOTE: popover set role based on toolbarOptions, but
// Lookup with showCancelButton: false, do not have toolbar, so we use option.
packages/devextreme/js/__internal/ui/action_sheet.ts:183
- Comment grammar/capitalization: “popover set role … rendered buttons not in a toolbar” should be corrected for readability.
// NOTE: popover set role based on toolbarOptions, but
// ActionSheet rendered buttons not in a toolbar, so we use option.
packages/devextreme/js/__internal/ui/popover/popover.ts:271
- Comment grammar: “uses in Popup and do not need here” is ungrammatical; please rephrase for clarity.
// Intentional no-op: Focus target logic is inherited from Widget,
// uses in Popup and do not need here.
_renderFocusTarget(): void {}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.
Suppressed comments (4)
packages/devextreme/js/__internal/ui/popover/popover.ts:271
- The comment above
_renderFocusTargethas grammatical issues ("uses" / "do not") that make it harder to understand.
// Intentional no-op: Focus target logic is inherited from Widget,
// uses in Popup and do not need here.
_renderFocusTarget(): void {}
packages/devextreme/js/__internal/ui/lookup.ts:682
- This NOTE comment has grammatical issues ("popover set", "do not have toolbar").
// NOTE: popover set role based on toolbarOptions, but
// Lookup with showCancelButton: false, do not have toolbar, so we use option.
packages/devextreme/js/__internal/ui/overlay/overlay.ts:842
- The comment above
_forceFocusLostis grammatically awkward (missing article / casing).
// Note: method helps Scheduler Appointments to avoid Focus Race Condition
packages/devextreme/js/__internal/ui/action_sheet.ts:183
- This NOTE comment has grammatical issues ("popover set", missing verb in the second line).
// NOTE: popover set role based on toolbarOptions, but
// ActionSheet rendered buttons not in a toolbar, so we use option.
No description provided.