Skip to content
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

fix(material/badge): remove the console warning #29800

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(material/badge): remove the console warning #29800

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
04dfc52
fix(material/badge): remove the console warning
naaajii Sep 28, 2024
8a979ba
fix(material/badge): move inline description element to be next sibling
naaajii Sep 28, 2024
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
28 changes: 24 additions & 4 deletions src/material/badge/badge.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,16 +243,16 @@ describe('MatBadge', () => {
badgeHostNativeElement = badgeHostDebugElement.nativeElement;
});

it('should insert the description inline after the host', () => {
it('should insert description as next sibling for host', () => {
testComponent.description.set('Extra info');
fixture.detectChanges();

const inlineDescription = badgeHostNativeElement.querySelector('.cdk-visually-hidden')!;
const inlineDescription = badgeHostNativeElement.nextSibling!;
expect(inlineDescription)
.withContext('A visually hidden description element should exist')
.toBeDefined();
expect(inlineDescription.textContent)
.withContext('The badge host next sibling should contain its description')
.withContext('The badge next sibling should contain its description')
.toBe('Extra info');

testComponent.description.set('Different info');
Expand All @@ -263,14 +263,34 @@ describe('MatBadge', () => {
.toBe('Different info');
});

it('should not apply aria-describedby for non-interactive hosts', () => {
it('should not apply aria-describedby for non-interactive host', () => {
testComponent.description.set('Extra info');
fixture.detectChanges();

expect(badgeHostNativeElement.hasAttribute('aria-description'))
.withContext('Non-interactive hosts should not have aria-describedby')
.toBeFalse();
});

it('should not insert description as next sibling if description is not provided', () => {
fixture.detectChanges();

expect(badgeHostNativeElement.nextSibling).toBeFalsy();
});

it('should not create multiple description elements if description changes', () => {
testComponent.description.set('one');
fixture.detectChanges();

let siblings = fixture.nativeElement.querySelectorAll('.cdk-visually-hidden');
expect(siblings.length).toBe(1);

testComponent.description.set('two');
fixture.detectChanges();

siblings = fixture.nativeElement.querySelectorAll('.cdk-visually-hidden');
expect(siblings.length).toBe(1);
});
});
});

Expand Down
25 changes: 10 additions & 15 deletions src/material/badge/badge.ts
View file
Open in desktop
Copy link
Contributor Author

@naaajii naaajii Sep 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@wagnermaciel @AleksanderBodurri

continuing from this reply i basically have questions regarding for not interactive elements:

  1. mat-icon also fall will same principal as quoted from link above?
  2. currently badge with mat-icon produces following markup:
<mat-icon matBadge="7">
  <span class="mat-badge">7</span>
</mat-icon>

the mat-badge should append a span with the description as the next sibling to the element that has the badge on it.

after span change is the following markup valid?

<mat-icon matBadge="7" matBadgeDescription="this is description">
  <span class="mat-badge">7</span>
</mat-icon>
<span class="cdk-visually-hidden">this is description</span>

Original file line number Diff line number Diff line change
Expand Up @@ -164,20 +164,6 @@ export class MatBadge implements OnInit, OnDestroy {
if (nativeElement.nodeType !== nativeElement.ELEMENT_NODE) {
throw Error('matBadge must be attached to an element node.');
}

// Heads-up for developers to avoid putting matBadge on <mat-icon>
// as it is aria-hidden by default docs mention this at:
// https://material.angular.io/components/badge/overview#accessibility
if (
nativeElement.tagName.toLowerCase() === 'mat-icon' &&
nativeElement.getAttribute('aria-hidden') === 'true'
) {
console.warn(
`Detected a matBadge on an "aria-hidden" "<mat-icon>". ` +
`Consider setting aria-hidden="false" in order to surface the information assistive technology.` +
`\n${nativeElement.outerHTML}`,
);
}
}
}

Expand Down Expand Up @@ -310,14 +296,23 @@ export class MatBadge implements OnInit, OnDestroy {
}

private _updateInlineDescription() {
// We do not need to need empty description element for non interactive elements.
if (!this.description) return;

// Create the inline description element if it doesn't exist
if (!this._inlineBadgeDescription) {
this._inlineBadgeDescription = this._document.createElement('span');
this._inlineBadgeDescription.classList.add('cdk-visually-hidden');
}

this._inlineBadgeDescription.textContent = this.description;
this._badgeElement?.appendChild(this._inlineBadgeDescription);
// We want to add the inline description element right after the not interactive element that
// badge is on therefore we can't use appendChild here as they will keep getting stacked in
// last.
this._elementRef.nativeElement.parentNode?.insertBefore(
this._inlineBadgeDescription,
this._elementRef.nativeElement.nextSibling,
);
}

private _removeInlineDescription() {
Expand Down
Loading