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.

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

feat(material/form-field): make mat-errors more polite #21870

Merged
merged 1 commit into from
Mar 5, 2021

Conversation

chrisbubernak
Copy link
Contributor

Make the "politeness" of mat-error "polite" by default (instead of "assertive") and also make this configurable via an input binding.

Fixes #21781

@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Feb 10, 2021
},
providers: [{provide: MAT_ERROR, useExisting: MatError}],
})
export class MatError {
@Input() id: string = `mat-error-${nextUniqueId++}`;

@Input() live: AriaLivePoliteness = 'polite';
Copy link
Contributor

Choose a reason for hiding this comment

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

@crisbeto what's the normal strategy we use for giving a default value to an aria- attribute but allowing a custom value to be set by the user if they want?

Copy link
Member

Choose a reason for hiding this comment

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

Usually we match the attribute name so in this case it should be @Input('aria-live'). Also I think that we should type it as string since the AriaLivePoliteness is tied to the LiveAnnouncer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I remember some stuff about injecting @Attribute in the constructor in some cases, does that have any advantage over just doing @Input('aria-live')?

Copy link
Member

Choose a reason for hiding this comment

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

The problem with making it an input is that it won't pick up a value set as [attr.aria-live]. It can be worked around by doing something like:

constructor(@Attribute('aria-live') defaultAriaLive) {
  if (!defaultAriaLive) {
    element.setAttribute('aria-live', 'polite');
  }
}

I like the Input, because it allows us to document it, but I don't feel strongly about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

why not both

Copy link
Member

Choose a reason for hiding this comment

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

Sure, we can base the value on the injected @Attribute.

Copy link
Contributor

Choose a reason for hiding this comment

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

@chrisbubernak something like this:

@Input('aria-live') ariaLive: string = this._ariaLive ?? 'polite';

constructor(@Attribute('aria-live') private _ariaLive: string) {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mmalerba I updated the code to include this and verified [attr.aria-live] works, PTAL

Copy link
Contributor Author

@chrisbubernak chrisbubernak Feb 18, 2021

Choose a reason for hiding this comment

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

Hey @mmalerba , it looks like I was slightly mistaken the [attr.aria-live] case does not work in my current CL. The root issue seems to be that the injected @Attribute('aria-live') only has a non-null value when the literal "aria-live" attribute is used. Setting [attr.aria-live] or [aria-live] leave _ariaLive with a null value.

In fact, the only way that I can currently think of to detect that a value was set via [attr.aria-live] is to way until ngAfterViewInit and then inspect the dom directly.

While I don't love the idea of mucking with the native element directly this is the best idea I have right now that seems to work for all cases while correctly falling back on a default if needed. Basically just wait till after view init, see if we have a value set and if not use the Input bound property which will either have a supplied value OR make sure the default gets stamped in.

  @Input('aria-live') ariaLive: string = this._ariaLive ?? 'polite';

  constructor(@Attribute('aria-live') private _ariaLive, private _elementRef: ElementRef, ){}

  ngAfterViewInit() {
     // At this point [attr.aria-live] and aria-live will have modified the dom if 
     // either was present, just handle the case where [aria-live] syntax was used.
     if(!this._elementRef.nativeElement.ariaLive){
       this._elementRef.nativeElement.setAttribute('aria-live', this.ariaLive);
     }
   }

WDYT? Am I missing something obvious here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that seems like a reasonable solution to me. It's a pain that this isn't easier to accomplish

@chrisbubernak
Copy link
Contributor Author

I know that this change leads to some test breakages but just wanted to get verification that this approach is appropriate (or guidance on an alternative solution) before proceeding to fix everything up.

@mmalerba
Copy link
Contributor

@chrisbubernak yes, overall this seems like a good change to me

@chrisbubernak chrisbubernak force-pushed the polite-mat-error branch 4 times, most recently from bcd5cf6 to 5ee4539 Compare February 12, 2021 19:43
@chrisbubernak chrisbubernak force-pushed the polite-mat-error branch 4 times, most recently from 70a8070 to 2ba8aa8 Compare February 17, 2021 02:43
Copy link
Contributor

@stevenyxu stevenyxu left a comment

Choose a reason for hiding this comment

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

Left comment about aria-atomic.

@jelbourn jelbourn added target: minor This PR is targeted for the next minor release Accessibility This issue is related to accessibility (a11y) G This is is related to a Google internal issue P2 The issue is important to a large percentage of users, with a workaround labels Feb 17, 2021
Copy link
Contributor

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

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

LGTM

Make the "politeness" of mat-error "polite" by default (instead of "assertive") and also make this
configurable via an input binding.

Fixes #21781
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for taking the time to make this PR!

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Accessibility This issue is related to accessibility (a11y) action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement G This is is related to a Google internal issue P2 The issue is important to a large percentage of users, with a workaround target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(mat-error): Make mat-error politeness configurable and/or have a less disruptive default.
6 participants