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

crisbeto · 2018-05-01T17:46:44Z

No longer adds the aria-label to the mat-dialog-close directive, if the element that it's being applied to has text. This prevents the label from being read instead of the button text.

Fixes #11084.

crisbeto · 2018-05-01T17:47:45Z

src/lib/dialog/dialog-content-directives.ts

@@ -56,6 +62,9 @@ export class MatDialogClose implements OnInit, OnChanges {
      // be resolved at constructor time.
      this.dialogRef = getClosestDialog(this._elementRef, this._dialog.openDialogs)!;
    }
+
+    const buttonText = this._elementRef.nativeElement.innerText;
+    this._hasText = !!buttonText && buttonText.trim().length > 0;


A note on this: this could technically become inaccurate if the button has a data binding that starts off with a value, but then gets cleared. I decided not to handle that case, because of the potential performance implications.

jelbourn · 2018-05-01T17:48:57Z

src/lib/dialog/dialog-content-directives.ts

@@ -56,6 +62,9 @@ export class MatDialogClose implements OnInit, OnChanges {
      // be resolved at constructor time.
      this.dialogRef = getClosestDialog(this._elementRef, this._dialog.openDialogs)!;
    }
+
+    const buttonText = this._elementRef.nativeElement.innerText;


I had been thinking we could special-case mat-icon and ignore its text if it's present, but I realized we can actually just ignore the text if it's a mat-icon-button.

Done. I also reworded it a bit so it's more clear about what it does.

jelbourn · 2018-05-01T18:09:56Z

src/lib/dialog/dialog-content-directives.ts

+    const element = this._elementRef.nativeElement;
+
+    this._hasAriaLabel = element.hasAttribute('mat-icon-button') ||
+                         element.innerText.trim().length === 0;


Use textContent since it doesn't require a style calculation?

jelbourn

LGTM

josephperrott · 2018-05-03T21:08:23Z

@crisbeto In the check to determine if an aria-label should be applied during ngOnInit, on line 72 we should also check to confirm that the aria-label has not changed.

Even if there is text content inside of the button, the user should be able to have screen readers read their own aria-label rather than button content.

Something like this should work:

this._hasAriaLabel = (
    !buttonTextContent || 
    buttonTextContent.trim().length === 0 || 
    this.ariaLabel !== 'Close dialog');

crisbeto · 2018-05-03T21:11:57Z

@josephperrott that's what this line does. If the consumer specified an aria-label, we won't override anything.

josephperrott · 2018-05-03T21:31:55Z

Correct, however I think currently the only paths to _hasAriaLabel being true is if the element is a MatIconButton or if there is no text content, or the text content is empty once trimmed.

We need to cover the case where an aria-label is provided by the user and the button has text content.

crisbeto · 2018-05-03T21:46:46Z

Ah I see. I've changed it to use the ngOnChanges instead.

josephperrott

LGTM

andrewseguin · 2019-01-23T22:06:35Z

Hey @crisbeto - looks like we've got a small problem on this PR. If the button has an aria-label, it is getting removed when there is button text.

The flow is that ngOnChanges is called and sets _hasAriaLabel to true because the input is true. But then ngOnInit runs and sets it back to false since there is button text.

crisbeto · 2019-01-26T08:39:44Z

Updated to avoid having the _hasAriaLabel be reset by ngOnInit.

stevenyxu · 2019-02-07T23:04:32Z

src/lib/dialog/dialog.spec.ts

@@ -1466,11 +1483,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


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.

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.

No longer adds the `aria-label` to the `mat-dialog-close` directive, if the element that it's being applied to has text. This prevents the label from being read instead of the button text. Fixes angular#11084.

…xt (#11093) No longer adds the `aria-label` to the `mat-dialog-close` directive, if the element that it's being applied to has text. This prevents the label from being read instead of the button text. Fixes #11084.

angular-automatic-lock-bot · 2019-09-10T12:03:48Z

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.}

crisbeto added the pr: needs review label May 1, 2018

crisbeto assigned jelbourn May 1, 2018

crisbeto requested a review from jelbourn as a code owner May 1, 2018 17:46

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 1, 2018

crisbeto commented May 1, 2018

View reviewed changes

jelbourn reviewed May 1, 2018

View reviewed changes

crisbeto force-pushed the 11084/dialog-close-aria-label branch from 78bbb43 to 0ecd595 Compare May 1, 2018 17:59

jelbourn reviewed May 1, 2018

View reviewed changes

crisbeto force-pushed the 11084/dialog-close-aria-label branch from 0ecd595 to 9c53272 Compare May 1, 2018 18:21

jelbourn approved these changes May 1, 2018

View reviewed changes

jelbourn added pr: lgtm Accessibility This issue is related to accessibility (a11y) action: merge The PR is ready for merge by the caretaker G This is is related to a Google internal issue target: patch This PR is targeted for the next patch release and removed pr: needs review labels May 1, 2018

josephperrott removed the action: merge The PR is ready for merge by the caretaker label May 3, 2018

crisbeto force-pushed the 11084/dialog-close-aria-label branch from 9c53272 to c99df8c Compare May 3, 2018 21:46

crisbeto force-pushed the 11084/dialog-close-aria-label branch from c99df8c to a6323c9 Compare May 9, 2018 21:10

crisbeto force-pushed the 11084/dialog-close-aria-label branch from a6323c9 to 54b0257 Compare October 22, 2018 20:40

josephperrott approved these changes Oct 22, 2018

View reviewed changes

josephperrott added the action: merge The PR is ready for merge by the caretaker label Oct 22, 2018

crisbeto force-pushed the 11084/dialog-close-aria-label branch from 54b0257 to 3e9f89c Compare January 11, 2019 21:05

andrewseguin removed the action: merge The PR is ready for merge by the caretaker label Jan 23, 2019

crisbeto force-pushed the 11084/dialog-close-aria-label branch from 3e9f89c to 15fec92 Compare January 26, 2019 08:36

crisbeto force-pushed the 11084/dialog-close-aria-label branch from 15fec92 to 8893005 Compare January 26, 2019 08:37

crisbeto added the action: merge The PR is ready for merge by the caretaker label Jan 26, 2019

stevenyxu reviewed Feb 7, 2019

View reviewed changes

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

ffb4a79

No longer adds the `aria-label` to the `mat-dialog-close` directive, if the element that it's being applied to has text. This prevents the label from being read instead of the button text. Fixes angular#11084.

crisbeto force-pushed the 11084/dialog-close-aria-label branch from 8893005 to ffb4a79 Compare February 12, 2019 18:21

jelbourn merged commit 3163cc6 into angular:master Feb 13, 2019

stevenyxu mentioned this pull request Mar 19, 2019

mat-dialog-close wrongly adds an aria-label when the button text content is bound rather than static #15542

Closed

dgfranklin mentioned this pull request Mar 27, 2019

MatDialogClose shouldn't set aria-label if it has text #11084

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

crisbeto commented May 1, 2018

crisbeto May 1, 2018

jelbourn May 1, 2018

crisbeto May 1, 2018

jelbourn May 1, 2018

crisbeto May 1, 2018

jelbourn left a comment

josephperrott commented May 3, 2018

crisbeto commented May 3, 2018

josephperrott commented May 3, 2018

crisbeto commented May 3, 2018

josephperrott left a comment

andrewseguin commented Jan 23, 2019

crisbeto commented Jan 26, 2019

stevenyxu Feb 7, 2019

stevenyxu Feb 7, 2019 •

edited

Loading

angular-automatic-lock-bot bot commented Sep 10, 2019

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

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

Conversation

crisbeto commented May 1, 2018

crisbeto May 1, 2018

Choose a reason for hiding this comment

jelbourn May 1, 2018

Choose a reason for hiding this comment

crisbeto May 1, 2018

Choose a reason for hiding this comment

jelbourn May 1, 2018

Choose a reason for hiding this comment

crisbeto May 1, 2018

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

josephperrott commented May 3, 2018

crisbeto commented May 3, 2018

josephperrott commented May 3, 2018

crisbeto commented May 3, 2018

josephperrott left a comment

Choose a reason for hiding this comment

andrewseguin commented Jan 23, 2019

crisbeto commented Jan 26, 2019

stevenyxu Feb 7, 2019

Choose a reason for hiding this comment

stevenyxu Feb 7, 2019 • edited Loading

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Sep 10, 2019

stevenyxu Feb 7, 2019 •

edited

Loading