Skip to content

feat(accessibility): Implement Issue 1410: Add accessibility attributes to A2UI components - #2141

Draft
josemontespg wants to merge 3 commits into
a2ui-project:mainfrom
josemontespg:accessibility-impl
Draft

feat(accessibility): Implement Issue 1410: Add accessibility attributes to A2UI components#2141
josemontespg wants to merge 3 commits into
a2ui-project:mainfrom
josemontespg:accessibility-impl

Conversation

@josemontespg

Copy link
Copy Markdown
Collaborator

What

This PR implements accessibility attribute mapping for the basic catalog components: Button, Image, TextField, Card, and Text.
Specifically, it maps:

  • aria-label (mapped from label)
  • aria-describedby (mapped from description)
  • role (mapped from role inside accessibility)

It updates both Lit and Angular renderers:

  • Lit components are updated using setAttribute in the updated() lifecycle or inline template bindings where applicable.
  • Angular components are updated using host bindings with signals.

It also adds unit tests for both Lit and Angular to verify the correct attribute mapping.

Why

To address Issue 1410 and ensure that A2UI components render standard ARIA attributes correctly when provided in the component properties.

Effect

Assistive technologies will now be able to read accessibility attributes from these basic components.

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request introduces accessibility attributes, including role, aria-label, and aria-describedby, to basic components (Button, Card, Image, Text, and TextField) in both Angular and Lit renderers, accompanied by schema updates and unit tests. The review feedback suggests using the nullish coalescing operator (??) instead of logical OR (||) for image alt text fallbacks to preserve empty strings for decorative images. Additionally, it recommends refactoring the Angular Button and TextField components to use a computed accessibility signal to avoid duplicate lookups and maintain consistency.

readonly fit = computed(() => this.props()['fit']?.value() || 'cover');
readonly variant = computed(() => this.props()['variant']?.value() || 'default');
readonly accessibility = computed(() => this.props()['accessibility']?.value());
readonly altText = computed(() => this.accessibility()?.label || this.accessibility()?.description || this.description());

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.

medium

Using the logical OR operator (||) for altText fallback will cause empty strings ("") to be overridden by the fallback descriptions. In HTML/accessibility standards, an empty alt attribute (alt="") is semantically meaningful and indicates a decorative image that screen readers should ignore. Using the nullish coalescing operator (??) preserves empty strings while correctly falling back when the properties are undefined or null.

Suggested change
readonly altText = computed(() => this.accessibility()?.label || this.accessibility()?.description || this.description());
readonly altText = computed(() => this.accessibility()?.label ?? this.accessibility()?.description ?? this.description());

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done. Implemented using nullish coalescing to preserve empty strings.

Comment on lines +87 to +91
const altText =
props.accessibility?.label ||
props.accessibility?.description ||
props.description ||
'';

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.

medium

Using the logical OR operator (||) for altText fallback will cause empty strings ("") to be overridden by the fallback descriptions. In HTML/accessibility standards, an empty alt attribute (alt="") is semantically meaningful and indicates a decorative image that screen readers should ignore. Using the nullish coalescing operator (??) preserves empty strings while correctly falling back when the properties are undefined or null.

Suggested change
const altText =
props.accessibility?.label ||
props.accessibility?.description ||
props.description ||
'';
const altText =
props.accessibility?.label ??
props.accessibility?.description ??
props.description ??
'';

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done. Implemented using nullish coalescing.

Comment on lines +104 to +105
readonly ariaLabel = computed(() => this.props()['accessibility']?.value()?.label ?? null);
readonly ariaDescribedBy = computed(() => this.props()['accessibility']?.value()?.description ?? null);

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.

medium

For consistency with other components (like CardComponent, ImageComponent, and TextComponent) and to avoid duplicate lookups of this.props()['accessibility']?.value(), define a computed accessibility signal and use it to resolve ariaLabel and ariaDescribedBy.

Suggested change
readonly ariaLabel = computed(() => this.props()['accessibility']?.value()?.label ?? null);
readonly ariaDescribedBy = computed(() => this.props()['accessibility']?.value()?.description ?? null);
readonly accessibility = computed(() => this.props()['accessibility']?.value());
readonly ariaLabel = computed(() => this.accessibility()?.label ?? null);
readonly ariaDescribedBy = computed(() => this.accessibility()?.description ?? null);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done. Optimized accessibility lookup with a computed signal.

Comment on lines +102 to +103
readonly ariaLabel = computed(() => this.props()['accessibility']?.value()?.label ?? null);
readonly ariaDescribedBy = computed(() => this.props()['accessibility']?.value()?.description ?? null);

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.

medium

For consistency with other components (like CardComponent, ImageComponent, and TextComponent) and to avoid duplicate lookups of this.props()['accessibility']?.value(), define a computed accessibility signal and use it to resolve ariaLabel and ariaDescribedBy.

Suggested change
readonly ariaLabel = computed(() => this.props()['accessibility']?.value()?.label ?? null);
readonly ariaDescribedBy = computed(() => this.props()['accessibility']?.value()?.description ?? null);
readonly accessibility = computed(() => this.props()['accessibility']?.value());
readonly ariaLabel = computed(() => this.accessibility()?.label ?? null);
readonly ariaDescribedBy = computed(() => this.accessibility()?.description ?? null);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done. Optimized accessibility lookup with a computed signal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant