Skip to content

Popover: support content reading on opening - #34311

Open
EugeniyKiyashko wants to merge 27 commits into
mainfrom
feature/26_2_popover_support_content_reading
Open

Popover: support content reading on opening#34311
EugeniyKiyashko wants to merge 27 commits into
mainfrom
feature/26_2_popover_support_content_reading

Conversation

@EugeniyKiyashko

Copy link
Copy Markdown
Contributor

No description provided.

@EugeniyKiyashko EugeniyKiyashko self-assigned this Jul 15, 2026
Copilot AI review requested due to automatic review settings July 15, 2026 09:38
@EugeniyKiyashko
EugeniyKiyashko requested review from a team as code owners July 15, 2026 09:38
@EugeniyKiyashko
EugeniyKiyashko marked this pull request as draft July 15, 2026 09:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 id and target aria-describedby, including cleanup on option changes and dispose.
  • Refine role/aria-labelledby behavior 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.

Comment thread packages/devextreme/js/__internal/ui/tooltip.ts Outdated
Comment thread packages/devextreme/js/__internal/ui/popover/popover.ts Outdated
Comment thread e2e/testcafe-devextreme/tests/accessibility/popover.ts Outdated
@EugeniyKiyashko
EugeniyKiyashko force-pushed the feature/26_2_popover_support_content_reading branch from 869b7fe to 5ccace2 Compare July 15, 2026 12:44
Comment thread packages/devextreme/js/__internal/ui/popover/popover.ts
Comment thread packages/devextreme/js/__internal/ui/popover/popover.ts Outdated
Comment thread packages/devextreme/js/__internal/ui/popup/popup.ts Outdated
Comment thread packages/devextreme/js/__internal/ui/popup/popup.ts Outdated
Comment thread packages/devextreme/js/__internal/ui/tooltip.ts Outdated
Comment thread packages/devextreme/js/__internal/ui/popover/popover.ts Outdated
Comment thread packages/devextreme/js/__internal/ui/popover/popover.ts Outdated
Comment thread packages/devextreme/js/__internal/ui/popover/popover.ts Outdated
Comment thread packages/devextreme/js/__internal/ui/popover/popover.ts
Comment thread packages/devextreme/js/__internal/ui/popover/popover.ts
Comment thread packages/devextreme/js/__internal/ui/popover/popover.ts Outdated
Comment thread packages/devextreme/js/__internal/ui/popover/popover.ts

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • _syncAriaAttributes sets the overlay role via setAria('role', ...) without an explicit target element. Because Popover overrides _focusTarget() to return the first tabbable element inside the overlay, setAria will apply role to 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.

Copilot AI review requested due to automatic review settings August 3, 2026 13:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
  });

Copilot AI review requested due to automatic review settings August 3, 2026 13:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 _renderFocusTarget has 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.

Copilot AI review requested due to automatic review settings August 3, 2026 14:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot AI review requested due to automatic review settings August 3, 2026 14:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {}

Copilot AI review requested due to automatic review settings August 3, 2026 14:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 _renderFocusTarget has 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 _forceFocusLost is 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.

@dmlvr
dmlvr enabled auto-merge August 3, 2026 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants