Skip to content

fix(ui5-select): fix acc finding #13559

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
PetyaMarkovaBogdanova wants to merge 13 commits into
base: main
Choose a base branch
from
+99 −13
Open

fix(ui5-select): fix acc finding #13559

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
9a48ffa
fix(ui5-select): fix acc finding
PetyaMarkovaBogdanova May 20, 2026
fa01dc5
fix(ui5-select): fix acc finding
PetyaMarkovaBogdanova May 20, 2026
0be0532
Merge branch 'main' into fix-acc-select
PetyaMarkovaBogdanova May 20, 2026
33c5c37
fix(ui5-select): fixed failing test
PetyaMarkovaBogdanova Jun 2, 2026
f16945d
Merge branch 'fix-acc-select' of github.com:SAP/ui5-webcomponents int…
PetyaMarkovaBogdanova Jun 2, 2026
04a2ef0
fix(ui5-webcomponents): fixed failing test
PetyaMarkovaBogdanova Jun 3, 2026
317ba3a
Merge branch 'main' into fix-acc-select
PetyaMarkovaBogdanova Jun 4, 2026
310b4d3
fix(ui5-select): add accessible name for the listbox in the popover o…
PetyaMarkovaBogdanova Jun 23, 2026
1528770
Merge branch 'main' into fix-acc-select
PetyaMarkovaBogdanova Jun 23, 2026
4a41098
fix(ui5-select): fixed failing tests
PetyaMarkovaBogdanova Jun 23, 2026
466d381
fix(ui5-select): ts error is fixed
PetyaMarkovaBogdanova Jun 23, 2026
55e0544
fix(ui5-select): fix ts error
PetyaMarkovaBogdanova Jun 23, 2026
a716162
fix(ui5-select): review comments addressed
PetyaMarkovaBogdanova Jun 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions packages/main/cypress/specs/Select.cy.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,68 @@ describe("Select - Accessibility", () => {
.should("have.attr", "aria-roledescription", EXPECTED_ARIA_ROLEDESCRIPTION);
});

it("tests listbox accessible naming in popover", () => {
cy.mount(
<>
<Select id="selectWithLabel" accessibleName="Choose country">
<Option value="DE">Germany</Option>
<Option value="US">USA</Option>
</Select>
<Select id="selectWithRef" accessibleNameRef="labelRef">
<Option value="One">One</Option>
<Option value="Two">Two</Option>
</Select>
<Select id="selectWithoutLabel">
<Option value="A">A</Option>
<Option value="B">B</Option>
</Select>
<span id="labelRef">Label from ref</span>
</>
);

// Test listbox with consumer-provided accessible name
cy.get("#selectWithLabel")
.shadow()
.find("[ui5-list]")
.shadow()
.find(".ui5-list-ul")
.should("have.attr", "aria-label", "Choose country");

// Test listbox with accessibleNameRef
cy.get("#selectWithRef")
.shadow()
.find("[ui5-list]")
.shadow()
.find(".ui5-list-ul")
.should("have.attr", "aria-label", "Label from ref");

// Test listbox with fallback label when no consumer label is provided
cy.get("#selectWithoutLabel")
.shadow()
.find("[ui5-list]")
.shadow()
.find(".ui5-list-ul")
.should("have.attr", "aria-label", "All Items");
});

it("tests popover accessible name with 'Select:' prefix on mobile", () => {
cy.ui5SimulateDevice("phone");

cy.mount(
<Select id="mobileSelect" accessibleName="Countries">
<Option value="DE">Germany</Option>
<Option value="US">USA</Option>
</Select>
);

cy.get("#mobileSelect").realClick();

cy.get("#mobileSelect")
.shadow()
.find("[ui5-responsive-popover]")
.should("have.attr", "accessible-name", "Select: Countries");
});

it("tests Select with valueState Positive and aria-describedby", () => {
cy.mount(
<Select valueState="Positive">
Expand Down
18 changes: 9 additions & 9 deletions packages/main/cypress/specs/Select.mobile.cy.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,15 @@ describe("Select mobile general interaction", () => {
// Open the popover
cy.get("#select").realClick();

// Check if accessible-name is equal to select._headerTitleText
cy.get("#select").invoke("prop", "_headerTitleText").then(_headerTitleText => {
cy.get("#select")
.shadow()
.find("[ui5-responsive-popover]")
.should("have.attr", "accessible-name")
.and("equal", _headerTitleText);
});
});
// Check if accessible-name is equal to select._effectivePopoverAccessibleName
cy.get("#select").invoke("prop", "_effectivePopoverAccessibleName").then(_effectivePopoverAccessibleName => {
cy.get("#select")
.shadow()
.find("[ui5-responsive-popover]")
.should("have.attr", "accessible-name")
.and("equal", _effectivePopoverAccessibleName);
});
});

@dobrinyonkov dobrinyonkov Jun 24, 2026

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.

This assertion is now circular. It reads the value of _effectivePopoverAccessibleName from the element and then asserts the rendered attribute equals that same value — so if the getter is broken, both sides of the equality break the same way and the test still passes.

The other new test (Select.cy.tsx:325) hard-codes "Select: Countries" for the mobile case, so coverage isn't lost — but consider either deleting this case or changing it to assert a literal string. Either way, the property-readback pattern hides regressions.


it("should focus the selected option when popover opens", () => {
cy.mount(
Expand Down
29 changes: 27 additions & 2 deletions packages/main/src/Select.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import "@ui5/webcomponents-icons/dist/sys-enter-2.js";
import "@ui5/webcomponents-icons/dist/information.js";
import { isPhone } from "@ui5/webcomponents-base/dist/Device.js";
import type I18nBundle from "@ui5/webcomponents-base/dist/i18nBundle.js";
import type { I18nText } from "@ui5/webcomponents-base/dist/i18nBundle.js";
import i18n from "@ui5/webcomponents-base/dist/decorators/i18n.js";
import type { Timeout, AriaRole } from "@ui5/webcomponents-base/dist/types.js";
import InvisibleMessageMode from "@ui5/webcomponents-base/dist/types/InvisibleMessageMode.js";
Expand All @@ -49,9 +50,10 @@ import {
VALUE_STATE_TYPE_INFORMATION,
VALUE_STATE_TYPE_ERROR,
VALUE_STATE_TYPE_WARNING,
INPUT_SUGGESTIONS_TITLE,
LIST_ITEM_POSITION,
SELECT_ROLE_DESCRIPTION,
SELECT_POPOVER_ACCESSIBLE_NAME_PREFIX,
SELECT_LISTBOX_LABEL,
SELECT_DIALOG_CANCEL_BUTTON,
FORM_SELECTABLE_REQUIRED,
} from "./generated/i18n/i18n-defaults.js";
Expand Down Expand Up @@ -95,6 +97,13 @@ const isPrintableCharacter = (e: KeyboardEvent) => {
return e.key.length === 1 && !e.ctrlKey && !e.metaKey && !e.altKey;
};

const isI18nText = (value: unknown): value is I18nText => {
return typeof value === "object"
&& value !== null
&& "key" in value
&& "defaultText" in value;
};

@dobrinyonkov dobrinyonkov Jun 24, 2026

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.

The isI18nText type guard (and the two narrowed branches at lines 1058 and 1142) should be removed — it's working around a stale generated file, not a real type problem.

Generated i18n constants are always typed as I18nText (see packages/main/src/generated/i18n/i18n-defaults.ts — every key is declared const FOO: I18nText = {key, defaultText}). Sibling code in this same file just does:

return Select.i18nBundle.getText(SELECT_ROLE_DESCRIPTION);
return Select.i18nBundle.getText(FORM_SELECTABLE_REQUIRED);

no guards needed. The TS error that prompted the workaround was almost certainly because yarn generate hadn't been run after the new keys were added to messagebundle.properties, so the imports in the generated file didn't yet exist. Running yarn generate regenerates the file and the simple form compiles fine.

Leaving the guard in is bad on two counts:

  1. It's dead code (the branches are unreachable in a correctly generated build) — readers will wonder what edge case it covers.
  2. The return "" fallback at line 1062 would silently produce a Select with no listbox label and no mobile dialog label if it ever did fire — masking exactly the regression this PR is supposed to prevent.

Please remove isI18nText, the I18nText named import, and inline the getText calls in both _headerTitleText and _effectivePopoverAccessibleName.


/**
* @class
*
Expand Down Expand Up @@ -1047,7 +1056,11 @@ class Select extends UI5Element implements IFormInputElement {
}

get _headerTitleText() {
return Select.i18nBundle.getText(INPUT_SUGGESTIONS_TITLE);
if (typeof SELECT_LISTBOX_LABEL === "string" || isI18nText(SELECT_LISTBOX_LABEL)) {
return Select.i18nBundle.getText(SELECT_LISTBOX_LABEL);
}

return "";
}

get _cancelButtonText() {
Expand Down Expand Up @@ -1121,6 +1134,18 @@ class Select extends UI5Element implements IFormInputElement {
return getEffectiveAriaLabelText(this) || getAssociatedLabelForTexts(this);
}

get _effectivePopoverAccessibleName() {
const fieldName = this.ariaLabelText || this._headerTitleText;
if (!fieldName) {
return undefined;
}
const prefix = (typeof SELECT_POPOVER_ACCESSIBLE_NAME_PREFIX === "string"
|| isI18nText(SELECT_POPOVER_ACCESSIBLE_NAME_PREFIX))
? Select.i18nBundle.getText(SELECT_POPOVER_ACCESSIBLE_NAME_PREFIX)
: "";
return `${prefix} ${fieldName}`;
}

get shouldDisplayDefaultValueStateMessage() {
return !this.valueStateMessage.length && this.hasValueStateText;
}
Expand Down
3 changes: 2 additions & 1 deletion packages/main/src/SelectPopoverTemplate.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export default function SelectPopoverTemplate(this: Select) {
onBeforeOpen={this._beforeOpen}
onClose={this._afterClose}
onKeyDown={this._onkeydown}
accessibleName={this._isPhone ? this._headerTitleText : undefined}
accessibleName={this._isPhone ? this._effectivePopoverAccessibleName : undefined}
>
{this._isPhone &&
<div slot="header" class="ui5-responsive-popover-header">
Expand Down Expand Up @@ -62,6 +62,7 @@ export default function SelectPopoverTemplate(this: Select) {
onMouseDown={this._itemMousedown}
onItemClick={this._handleItemPress}
accessibleRole="ListBox"
accessibleName={this.ariaLabelText || this._headerTitleText}

@dobrinyonkov dobrinyonkov Jun 23, 2026

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.

Duplicated expression (low).

this.ariaLabelText || this._headerTitleText now appears on both line 30 and line 71. If the precedence ever changes (e.g. a third source is added, or the Select: prefix from comment above lands), it's two places to keep in sync. Consider extracting a getter in Select.ts —

get _effectivePopoverAccessibleName() {
    return this.ariaLabelText || this._headerTitleText;
}

and using it on both lines. Matches how _headerTitleText is already factored out. Optional but tidy.

@dobrinyonkov dobrinyonkov Jun 23, 2026

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.

Borrowed i18n key — please add a dedicated one (low).

_headerTitleText resolves to INPUT_SUGGESTIONS_TITLE ("All Items"), which was authored for the Input suggestions popover. Now that every Select listbox falls back to this string when the consumer hasn't set a label, it's being used as a Select-specific accessible name even though the key name says otherwise.

Please introduce a Select-specific i18n key (e.g. SELECT_LISTBOX_LABEL / SELECT_POPOVER_TITLE) and have _headerTitleText (or a new getter) resolve to it — even if the English string happens to match. Keeps translations correctly scoped per component and avoids surprises if Input's wording ever diverges.

@dobrinyonkov dobrinyonkov Jun 23, 2026

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.

No test coverage added (medium).

This PR changes ARIA wiring on two surfaces but adds no test. The branch history (two fix(...): fixed failing test commits) suggests this area is easy to regress silently. Two natural assertions belong in the existing accessibility spec at packages/main/cypress/specs/Select.cy.tsx:250:

  1. Open the popover on a <Select accessibleName="Foo"> → the inner [ui5-list] has aria-label="Foo". Add a second case for accessibleNameRef and a third for a Select with no consumer label (asserting the fallback string).
  2. Simulate _isPhone → the [ui5-responsive-popover] has aria-label matching the field's accessible name.

Without these, the next refactor of SelectPopoverTemplate will not catch a regression here.

@dobrinyonkov dobrinyonkov Jun 24, 2026

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.

Nit: since _effectivePopoverAccessibleName exists for the popover line, the listbox line could be factored the same way — a small getter _effectiveListAccessibleName returning this.ariaLabelText || this._headerTitleText. Not critical (the two expressions now correctly differ — the popover one carries the Select: prefix), but it keeps the template uniform. Skip if you'd rather not.

>
<slot></slot>
</List>
Expand Down
6 changes: 6 additions & 0 deletions packages/main/src/i18n/messagebundle.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,12 @@ SELECT_ROLE_DESCRIPTION=Listbox
#XTXT: MultiComboBox and ComboBox icon accessible name
SELECT_OPTIONS=Select Options

#XACT: ARIA announcement prefix for Select dialog accessible name on mobile (without trailing space)
SELECT_POPOVER_ACCESSIBLE_NAME_PREFIX=Select:

@dobrinyonkov dobrinyonkov Jun 24, 2026

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.

Cleaner than embedding the space in the translation — translators can't accidentally drop trailing whitespace, and the join is done explicitly in code. Good fix. (No action needed — leaving the comment here so the rationale is visible in review.)


#XACT: ARIA accessible name for Select's listbox when no explicit label is provided
SELECT_LISTBOX_LABEL=All Items

#XTXT: MultiComboBox show selected items button accessible name
SHOW_SELECTED_BUTTON=Show Selected Items Only

Expand Down
Loading