-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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.
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
fix(material/badge): insert inline description for non-interactive hosts #27025
Conversation
src/material/badge/badge.ts
Outdated
@@ -238,15 +254,37 @@ export class MatBadge extends _MatBadgeBase implements OnInit, OnDestroy, CanDis | |||
this._badgeElement.textContent = newContentNormalized; | |||
} | |||
|
|||
if (!this._isHostInteractive()) { | |||
if (!this._inlineBadgeDescription) { | |||
this._inlineBadgeDescription = this._renderer.createElement('span') as HTMLSpanElement; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to go through the renderer for this. The badge uses the renderer so view encapsulation works correctly, but this element doesn't need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed - I just did it since the component was already using it anyway.
src/material/badge/badge.ts
Outdated
this._inlineBadgeDescription.textContent = this.description; | ||
|
||
const host = this._elementRef.nativeElement; | ||
host.parentElement?.insertBefore(this._inlineBadgeDescription, host.nextSibling); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the nextSibling
here can be undefined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to append to the badge element
src/material/badge/badge.ts
Outdated
@@ -238,15 +254,37 @@ export class MatBadge extends _MatBadgeBase implements OnInit, OnDestroy, CanDis | |||
this._badgeElement.textContent = newContentNormalized; | |||
} | |||
|
|||
if (!this._isHostInteractive()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fires in ngOnInit
and whenever the content changes. Isn't it possible for the element to become non-interactive between those two, e.g. if it's on a button that becomes disabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good catch- I took a look at badge usages in Google, and think this is a small enough edge case that we don't need to worry about it. The vast majority of elements with matBadge
don't set disabled
, and those that do almost all also set matBadgeHidden
based on the same condition. I've added a long comment to capture this in the code.
src/material/badge/badge.ts
Outdated
this._inlineBadgeDescription.textContent = this.description; | ||
|
||
const host = this._elementRef.nativeElement; | ||
host.parentElement?.insertBefore(this._inlineBadgeDescription, host.nextSibling); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this have to be a sibling of the badge? Maybe we can add the element inside the badge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed - I had been worried that adding this as a child would potentially insert the description in an awkward place (e.g., breaking up a block of text), but upon further thought, that does more accurately reflect how the badge is applied by the developer.
src/material/badge/badge.ts
Outdated
private _updateInlineDescription() { | ||
// Create the inline description element if it doesn't exist | ||
if (!this._inlineBadgeDescription) { | ||
this._inlineBadgeDescription = document.createElement('span'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use the document from the DOCUMENT
token since it accounts for SSR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The badge current applies `aria-describedby` to surface the developer-provided description. However, assistive technology generally doesn't read these description on non-interactive elements. So, we can take advantage of our handy `InteractivityChecker` and do something different if the host is not focusable; we add the description inline as the next sibling with `.cdk-visually-hidden`. Fixes angular#26190
…sts (#27025) The badge current applies `aria-describedby` to surface the developer-provided description. However, assistive technology generally doesn't read these description on non-interactive elements. So, we can take advantage of our handy `InteractivityChecker` and do something different if the host is not focusable; we add the description inline as the next sibling with `.cdk-visually-hidden`. Fixes #26190 (cherry picked from commit f0a5008)
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
The badge current applies
aria-describedby
to surface the developer-provided description. However, assistive technology generally doesn't read these description on non-interactive elements. So, we can take advantage of our handyInteractivityChecker
and do something different if the host is not focusable; we add the description inline as the next sibling with.cdk-visually-hidden
.Fixes #26190