fix(material/badge): correctly apply badge description #23562

jelbourn · 2021-09-10T02:53:10Z

Previously, MatBadge was applying the provided description as an aria-label to the badge content element, which is a child element of the badge host. Then it was also applying an aria-describedby to that same content element. The end result is that the badge was being treated mainly as text content for a control, which isn't really the best way to convey its complementary nature. This behavior was causing problems in NVDA, where the badge content was overriding the host button's label.

This change makes the badge content itself aria-hidden and applies the badge description to the badge's host element via aria-describedby and completely removes any setting of aria-label. It also makes a handful of minor cleanup refactorings.

cc @zarend @amysorto

crisbeto

LGTM with a few nits.

src/material/badge/badge.ts

crisbeto · 2021-09-10T04:26:07Z

src/material/badge/badge.ts

    }

+    this._content = newContentNormalized;
+    this._hasContent = !!newContentNormalized;


Isn't this a little redundant considering that we can check this._content?

Good point, now that it always normalizes the content this is unnecessary. Removed.

crisbeto · 2021-09-10T04:31:38Z

src/material/badge/badge.ts

@@ -84,22 +85,20 @@ export class MatBadge extends _MatBadgeBase implements OnDestroy, OnChanges, Can
  @Input('matBadgePosition') position: MatBadgePosition = 'above after';

  /** The content for the badge */
-  @Input('matBadge') content: string | number | undefined | null;
+  @Input('matBadge')
+  get content(): string | number | undefined | null {


We could pull the string | number | undefined | null into a type so we don't have to repeat it in 3 places.

Ack, I'm going to leave as-is for now.

src/material/badge/badge.ts

jelbourn · 2021-09-20T22:23:32Z

I've made a small change to this PR so that the badge element isn't added to the DOM until OnInit. A handful of Google apps have brittle tests that check the text content of the badge's host element, so moving the badge content before the e.g. button's content makes the tests start failing.

zarend

This will work 👍 .

It could be less confusing for a reader of this code to understand when we need to create the badge element. I have suggestions in the above comments, but that is not a big enough problem to be a blocker for me.

zarend · 2021-09-21T21:50:54Z

src/material/badge/badge.ts

+    // ViewEngine only: when creating a badge through the Renderer, Angular remembers its index.
+    // We have to destroy it ourselves, otherwise it'll be retained in memory.
+    if (this._renderer.destroyNode) {
+      this._renderer.destroyNode!(this._badgeElement);


nit: this non-null assert shouldn't be necessary since we truthy checked it in the if-statement's condition.

zarend · 2021-09-21T22:11:37Z

src/material/badge/badge.ts

+    // on which the badge is attached won't necessarily be in the DOM until this point.
+    this._clearExistingBadges();
+
+    if (this.content && !this._badgeElement) {
      this._badgeElement = this._createBadgeElement();


IMHO: it's confusing that _updateContent sometimes handles creating the badge element, and sometimes the caller handles it. I spent a few minutes head scratching and thinking through the event lifecycle to figure this out.

From the method's description: /** Update the text content of the badge. */. In this situation it would make more sense for _updateContent to do that one thing, and not do anything else. It's up to the caller to make sure the badge element is created.

I suggest making the caller handle creating the badge element, or making _updateContent create the badge element in all situations, or making a _createBadgeElementIfNotExists (final name depends on bike shedding) method to handle creating the badge element in a single place.

I agree that the logic could be organized more cleanly here, but I want to avoid refactoring any more than I've already done here. As a middle ground, I've renamed _updateContent to _updateRenderedContent`, which I think does a better job of capturing that it may create a DOM element if it doesn't already exist.

zarend · 2021-09-21T22:13:01Z

src/material/badge/badge.ts

-  private _updateHostAriaDescription(newDescription: string, oldDescription: string): void {
-    // ensure content available before setting label
-    const content = this._updateTextContent();
+  /** Update the text content of the badge.  */


and also create the badge element for when OnChanges happens

Changed to

/** Update the text content of the badge element in the DOM, creating the element if necessary. */

Previously, `MatBadge` was applying the provided description as an `aria-label` to the badge content element, which is a child element of the badge host. Then it was _also_ applying an `aria-describedby` to that same content element. The end result is that the badge was being treated mainly as text content for a control, which isn't really the best way to convey its complmentary nature. This behavior was causing problems in NVDA, where the badge content was overriding the host button's label. This change makes the badge content itself `aria-hidden` and applies the badge description to the badge's _host_ element via `aria-describedby` and completely removes any setting of `aria-label`. It also makes a handful of minor cleanup refactorings.

Previously, `MatBadge` was applying the provided description as an `aria-label` to the badge content element, which is a child element of the badge host. Then it was _also_ applying an `aria-describedby` to that same content element. The end result is that the badge was being treated mainly as text content for a control, which isn't really the best way to convey its complmentary nature. This behavior was causing problems in NVDA, where the badge content was overriding the host button's label. This change makes the badge content itself `aria-hidden` and applies the badge description to the badge's _host_ element via `aria-describedby` and completely removes any setting of `aria-label`. It also makes a handful of minor cleanup refactorings. (cherry picked from commit f5883db)

…ar#23562)" This reverts commit 6b71169. Reverts this badge fix because it relies on a querySelector API that is not supported in IE11.

…ar#23562)" This reverts commit 6b71169. Reverts this badge fix because it relies on a querySelector API that is not supported in IE11. Related to angular#23736

…" (#23754) This reverts commit 6b71169. Reverts this badge fix because it relies on a querySelector API that is not supported in IE11. Related to #23736

angular-automatic-lock-bot · 2021-10-28T00:02:24Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

jelbourn added P2 The issue is important to a large percentage of users, with a workaround Accessibility This issue is related to accessibility (a11y) area: material/badge labels Sep 10, 2021

jelbourn requested a review from crisbeto September 10, 2021 02:53

google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 10, 2021

jelbourn force-pushed the badge-a11y branch from 10c26e6 to b342398 Compare September 10, 2021 03:01

jelbourn requested a review from a team as a code owner September 10, 2021 03:01

crisbeto approved these changes Sep 10, 2021

View reviewed changes

jelbourn added the target: patch This PR is targeted for the next patch release label Sep 10, 2021

jelbourn force-pushed the badge-a11y branch from b342398 to 45f8086 Compare September 10, 2021 20:28

jelbourn added the action: merge The PR is ready for merge by the caretaker label Sep 10, 2021

josephperrott removed the request for review from a team September 10, 2021 22:36

jelbourn force-pushed the badge-a11y branch from 45f8086 to 3abc4a1 Compare September 20, 2021 22:22

zarend approved these changes Sep 21, 2021

View reviewed changes

jelbourn force-pushed the badge-a11y branch from 3abc4a1 to d31965e Compare September 21, 2021 23:35

jelbourn mentioned this pull request Sep 22, 2021

fix(badge): put matBadgeDescription on host instead of badge element #23400

Closed

amysorto merged commit f5883db into angular:master Sep 27, 2021

sync-by-unito bot mentioned this pull request Oct 1, 2021

build(deps): bump @angular/material from 12.2.4 to 12.2.8 numbersprotocol/capture-lite#925

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(material/badge): correctly apply badge description #23562

fix(material/badge): correctly apply badge description #23562

Uh oh!

jelbourn commented Sep 10, 2021

Uh oh!

crisbeto left a comment

Uh oh!

Uh oh!

crisbeto Sep 10, 2021

Uh oh!

jelbourn Sep 10, 2021

Uh oh!

crisbeto Sep 10, 2021

Uh oh!

jelbourn Sep 10, 2021

Uh oh!

Uh oh!

jelbourn commented Sep 20, 2021

Uh oh!

zarend left a comment

Uh oh!

zarend Sep 21, 2021 •

edited

Loading

Uh oh!

jelbourn Sep 21, 2021

Uh oh!

zarend Sep 21, 2021

Uh oh!

jelbourn Sep 21, 2021

Uh oh!

zarend Sep 21, 2021

Uh oh!

jelbourn Sep 21, 2021

Uh oh!

angular-automatic-lock-bot bot commented Oct 28, 2021

Uh oh!

Uh oh!

fix(material/badge): correctly apply badge description #23562

fix(material/badge): correctly apply badge description #23562

Uh oh!

Conversation

jelbourn commented Sep 10, 2021

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jelbourn commented Sep 20, 2021

Uh oh!

zarend left a comment

Choose a reason for hiding this comment

Uh oh!

zarend Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Oct 28, 2021

Uh oh!

Uh oh!

zarend Sep 21, 2021 •

edited

Loading