Skip to content

fix(ui5-button): announce accessible name #11396

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
wants to merge 4 commits into
base: main
Choose a base branch
from
+58 −14
Open

fix(ui5-button): announce accessible name #11396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6f60dfd
fix(ui5-button): announce accessible name
unazko Apr 23, 2025
e630923
fix: adjust tests
unazko Apr 23, 2025
f8d4825
fix: adjust tests
unazko Apr 23, 2025
deb135d
fix: resolve lint errors
unazko Apr 23, 2025
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
37 changes: 28 additions & 9 deletions packages/main/cypress/specs/Button.cy.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import download from "@ui5/webcomponents-icons/dist/download.js";
import employee from "@ui5/webcomponents-icons/dist/employee.js";

import {
BUTTON_ARIA_TYPE_REJECT,
BUTTON_ARIA_TYPE_EMPHASIZED,
} from "../../src/generated/i18n/i18n-defaults.js";

describe("Button general interaction", () => {
Expand Down Expand Up @@ -304,28 +304,47 @@ describe("Accessibility", () => {
.should("have.attr", "role", "button");
});

it("aria-describedby properly applied on the button tag", () => {
cy.mount(<Button design="Attention">Content</Button>);
it("accessibleDescription in combination with design property applied on the button tag", () => {
Copy link
Preview

Copilot AI Apr 30, 2025

Choose a reason for hiding this comment

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

[nitpick] Ensure that the test description accurately reflects the new behavior, verifying that the aria-description is set as expected for different design scenarios.

Suggested change
it("accessibleDescription in combination with design property applied on the button tag", () => {
it("verifies that accessibleDescription is correctly applied as aria-description on the button tag, in combination with the design property", () => {

Copilot uses AI. Check for mistakes.

cy.mount(<Button design="Negative" accessibleDescription="Decline">Content</Button>);

cy.get("[ui5-button]")
.shadow()
.find("button")
.as("button");

cy.get("@button")
.shadow()
.find("button")
.should("have.attr", "aria-description", "Warning");
.should("have.attr", "aria-description", "Decline");
});

it("accessibleDescription in combination with design property applied on the button tag", () => {
cy.mount(<Button design="Negative" accessibleDescription="Decline">Content</Button>);

it("accessibleName when the button has a ui5-button-badge with more than 1 items", () => {
cy.mount(
<Button accessibleName="Download">
<ButtonBadge design="OverlayText" text="999+" slot="badge"></ButtonBadge>
</Button>
);
cy.get("[ui5-button]")
.shadow()
.find("button")
.as("button");

cy.get("@button")
.should("have.attr", "aria-label", `Download 999+ items`);
});

it("accessibleName when the button has a ui5-button-badge with 1 item", () => {
cy.mount(
<Button design="Emphasized" accessibleName="Download">
<ButtonBadge text="1" design="InlineText" slot="badge"></ButtonBadge>
</Button>
);
cy.get("[ui5-button]")
.shadow()
.find("button")
.should("have.attr", "aria-description", `${BUTTON_ARIA_TYPE_REJECT.defaultText} Decline`);
.as("button");

cy.get("@button")
.should("have.attr", "aria-label", `Download ${BUTTON_ARIA_TYPE_EMPHASIZED.defaultText} 1 item`);
});

it("setting accessible-name-ref on the host is reflected on the button tag", () => {
Expand Down
29 changes: 24 additions & 5 deletions packages/main/src/Button.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ import {
BUTTON_ARIA_TYPE_REJECT,
BUTTON_ARIA_TYPE_EMPHASIZED,
BUTTON_ARIA_TYPE_ATTENTION,
BUTTON_BADGE_ONE_ITEM,
BUTTON_BADGE_MANY_ITEMS,
} from "./generated/i18n/i18n-defaults.js";

// Styles
Expand Down Expand Up @@ -522,15 +524,32 @@ class Button extends UI5Element implements IButton {
}

get ariaLabelText() {
return getEffectiveAriaLabelText(this);
const ariaLabelText = getEffectiveAriaLabelText(this) || "";
const typeLabelText = this.hasButtonType ? this.buttonTypeText : "";
const internalLabelText = `${typeLabelText} ${this.effectiveBadgeDescriptionText}`.trim();

return `${ariaLabelText} ${internalLabelText}`.trim();
Comment on lines +529 to +531
Copy link
Preview

Copilot AI Apr 30, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider handling cases where both the effective aria label and button type text are empty to avoid potential extra spaces, even though trim() is used; a more modular construction of the final label might improve future maintenance.

Suggested change
const internalLabelText = `${typeLabelText} ${this.effectiveBadgeDescriptionText}`.trim();
return `${ariaLabelText} ${internalLabelText}`.trim();
const internalLabelText = this.effectiveBadgeDescriptionText || "";
// Combine non-empty components into an array and join with a space
const labelParts = [ariaLabelText, typeLabelText, internalLabelText].filter(part => part);
return labelParts.join(" ");

Copilot uses AI. Check for mistakes.

}

get ariaDescriptionText() {
const ariaDescribedByText = this.hasButtonType ? this.buttonTypeText : "";
const accessibleDescription = this.accessibleDescription || "";
const ariaDescriptionText = `${ariaDescribedByText} ${accessibleDescription}`.trim();
return this.accessibleDescription === "" ? undefined : this.accessibleDescription;
}

get effectiveBadgeDescriptionText() {
if (!this.shouldRenderBadge) {
return "";
}

const badgeEffectiveText = this.badge[0].effectiveText;

Copy link
Preview

Copilot AI Apr 30, 2025

Choose a reason for hiding this comment

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

[nitpick] Adding a comment to explain the rationale for using distinct i18n keys (BUTTON_BADGE_ONE_ITEM vs. BUTTON_BADGE_MANY_ITEMS) for singular and plural badge values would help improve code clarity.

Suggested change
// Use distinct i18n keys for singular and plural badge values to ensure proper localization.
// Some languages have different grammatical rules for singular and plural forms,
// so separate keys (BUTTON_BADGE_ONE_ITEM and BUTTON_BADGE_MANY_ITEMS) are necessary.

Copilot uses AI. Check for mistakes.

return ariaDescriptionText || undefined;
switch (badgeEffectiveText) {
case "":
return badgeEffectiveText;
case "1":
return Button.i18nBundle.getText(BUTTON_BADGE_ONE_ITEM, badgeEffectiveText);
default:
return Button.i18nBundle.getText(BUTTON_BADGE_MANY_ITEMS, badgeEffectiveText);
}
}

get _isSubmit() {
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 @@ -79,6 +79,12 @@ BUTTON_ARIA_TYPE_EMPHASIZED=Emphasized
#XACT: ARIA announcement for the warning button
BUTTON_ARIA_TYPE_ATTENTION=Warning

#XACT: ARIA announcement for the Button Badge with value 1 - "{0} item"
BUTTON_BADGE_ONE_ITEM={0} item

#XACT: ARIA announcement for the Button Badge with value more than 1 - "{0} items"
BUTTON_BADGE_MANY_ITEMS={0} items

#XACT: Text for Today item in CalendarLegend
CAL_LEGEND_TODAY_TEXT=Today

Expand Down
Loading