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(dialog): don't assign aria-label to close button if button has text #11093

Merged
merged 1 commit into from
Feb 13, 2019
Merged

fix(dialog): don't assign aria-label to close button if button has text #11093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 22 additions & 1 deletion src/lib/dialog/dialog-content-directives.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ let dialogElementUid = 0;
exportAs: 'matDialogClose',
host: {
'(click)': 'dialogRef.close(dialogResult)',
'[attr.aria-label]': 'ariaLabel',
'[attr.aria-label]': '_hasAriaLabel ? ariaLabel : null',
'type': 'button', // Prevents accidental form submits.
}
})
Expand All @@ -42,6 +42,12 @@ export class MatDialogClose implements OnInit, OnChanges {

@Input('matDialogClose') _matDialogClose: any;

/**
* Whether the button should have an `aria-label`. Used for clearing the
* attribute to prevent it from being read instead of the button's text.
*/
_hasAriaLabel?: boolean;

constructor(
@Optional() public dialogRef: MatDialogRef<any>,
private _elementRef: ElementRef<HTMLElement>,
Expand All @@ -56,6 +62,17 @@ export class MatDialogClose implements OnInit, OnChanges {
// be resolved at constructor time.
this.dialogRef = getClosestDialog(this._elementRef, this._dialog.openDialogs)!;
}

if (typeof this._hasAriaLabel === 'undefined') {
const element = this._elementRef.nativeElement;

if (element.hasAttribute('mat-icon-button')) {
this._hasAriaLabel = true;
} else {
const buttonTextContent = element.textContent;
this._hasAriaLabel = !buttonTextContent || buttonTextContent.trim().length === 0;
}
}
}

ngOnChanges(changes: SimpleChanges) {
Expand All @@ -64,6 +81,10 @@ export class MatDialogClose implements OnInit, OnChanges {
if (proxiedChange) {
this.dialogResult = proxiedChange.currentValue;
}

if (changes.ariaLabel) {
this._hasAriaLabel = !!changes.ariaLabel.currentValue;
}
}
}

Expand Down
23 changes: 21 additions & 2 deletions src/lib/dialog/dialog.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1143,11 +1143,26 @@ describe('MatDialog', () => {
expect(overlayContainerElement.querySelectorAll('.mat-dialog-container').length).toBe(1);
});

it('should set an aria-label on a button without text', fakeAsync(() => {
let button = overlayContainerElement.querySelector('.close-without-text')!;
expect(button.getAttribute('aria-label')).toBeTruthy();
}));

it('should not have an aria-label if a button has text', fakeAsync(() => {
let button = overlayContainerElement.querySelector('[mat-dialog-close]')!;
expect(button.getAttribute('aria-label')).toBeFalsy();
}));

it('should allow for a user-specified aria-label on the close button', fakeAsync(() => {
let button = overlayContainerElement.querySelector('.close-with-aria-label')!;
expect(button.getAttribute('aria-label')).toBe('Best close button ever');
}));

it('should always have an aria-label on a mat-icon-button', fakeAsync(() => {
let button = overlayContainerElement.querySelector('.close-icon-button')!;
expect(button.getAttribute('aria-label')).toBeTruthy();
}));

it('should override the "type" attribute of the close button', () => {
let button = overlayContainerElement.querySelector('button[mat-dialog-close]')!;

Expand Down Expand Up @@ -1493,11 +1508,13 @@ class PizzaMsg {
<mat-dialog-content>Lorem ipsum dolor sit amet.</mat-dialog-content>
<mat-dialog-actions>
<button mat-dialog-close>Close</button>
<button class="close-without-text" mat-dialog-close></button>
<button class="close-icon-button" mat-icon-button mat-dialog-close>exit</button>
<button class="close-with-true" [mat-dialog-close]="true">Close and return true</button>
<button
class="close-with-aria-label"
aria-label="Best close button ever"
[mat-dialog-close]="true">Close</button>
[mat-dialog-close]="true"></button>
<div mat-dialog-close>Should not close</div>
</mat-dialog-actions>
`
Expand All @@ -1511,11 +1528,13 @@ class ContentElementDialog {}
<mat-dialog-content>Lorem ipsum dolor sit amet.</mat-dialog-content>
<mat-dialog-actions>
<button mat-dialog-close>Close</button>
<button class="close-without-text" mat-dialog-close></button>
<button class="close-icon-button" mat-icon-button mat-dialog-close>exit</button>
<button class="close-with-true" [mat-dialog-close]="true">Close and return true</button>
<button
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you think <button mat-dialog-close aria-label="foo">bar</button> should behave? It would be great to call this out. Is it possible the current implementation wipes out the aria-label? I'd propose that this is surprising.

Copy link
Contributor

@stevenyxu stevenyxu Feb 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Also, might be worth evaluating <button mat-button-close [attr.aria-label]="'foo'">bar</button>. I think the current implementation also wipes foo out. This might be a fair bit less elegant to account for since Angular null attribute bindings clear an existing value and it's unidiomatic if not impossible to accept [attr.aria-label] as an input. However, behaving in an expected way should still be possible, either by checking if the static HTML attribute is set at constructor/init time, or some other creative way.

My 2c: If the template using mat-dialog-close specifies an explicit aria-label on that node using any conventional way, it should stick, regardless of the contents on the button.

class="close-with-aria-label"
aria-label="Best close button ever"
[mat-dialog-close]="true">Close</button>
[mat-dialog-close]="true"></button>
<div mat-dialog-close>Should not close</div>
</mat-dialog-actions>
</ng-template>
Expand Down
1 change: 1 addition & 0 deletions tools/public_api_guard/lib/dialog.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ export declare const matDialogAnimations: {
};

export declare class MatDialogClose implements OnInit, OnChanges {
_hasAriaLabel?: boolean;
_matDialogClose: any;
ariaLabel: string;
dialogRef: MatDialogRef<any>;
Expand Down