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

[crisbeto]

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]

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]

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.

[crisbeto]

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

[jelbourn]

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

[crisbeto]

Done.

[jelbourn]

LGTM

[josephperrott]

@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]

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

[josephperrott]

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]

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

[josephperrott]

LGTM

[andrewseguin]

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]

Updated to avoid having the _hasAriaLabel be reset by ngOnInit.

[stevenyxu]

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.

[stevenyxu]

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.

[angular-automatic-lock-bot]

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