fix(material/input): Do not set aria-invalid on emptymatInputs
#22802
Conversation
google-cla
bot
added
the
cla: yes
|
This is actually just a continuation of #22779 but I screwed up the branch pretty severely when I: There is good discussion on that PR which needs to be followed up on. Copying over: Via @crisbeto : This behavior is intentional, see #21609. There's also a linked issue. Response: Unfortunately, setting aria-invalid="false" on these inputs suppresses any native indication to a screen reader user that the input is in an invalid state. The only information they receive is the error message itself. If worded correctly the error message can be enough (e.g. if it includes "Invalid" in the text) but this requires component users to be aware of this fact and actively account for it. Without a well-worded error message, this results in a different experience for SR users than visual users -- the input is flagged as invalid and displayed prominently to visual users as invalid, but screen reader users get no native notification at all when they focus the input. |
| @@ -1081,7 +1081,7 @@ describe('MatMdcInput with forms', () => { | |||
| expect(describedBy).toBe(errorIds); | |||
| })); | |||
|
|
|||
| it('should not set `aria-invalid` to true if the input is empty', fakeAsync(() => { | |||
| it('should set `aria-invalid` to true if the input is empty', fakeAsync(() => { | |||
There was a problem hiding this comment.
Note: This test checks that a non-required input with a FormControl which contains Validators.required sets aria-invalid.
This actually is the edge case that @zelliott pointed out in #22779 when they mentioned that the logic for setting aria-invalid should actually be (empty && required) ? null : errorState.
In this case there is no required on the form field, and therefore no aria-required. This means we must set aria-invalid to indicate to a screen reader that there is some other validation error on the form field that they need to be aware of.
I'm not certain that this test's title is informative enough in this case -- thoughts?
| @Component({ | ||
| template: ` | ||
| <mat-form-field> | ||
| <input matInput required [formControl]="formControl"> |
There was a problem hiding this comment.
I think that the ReactiveFormsModule discourages using required (and similar attributes) together with formControl, in favor of passing in Validator functions. That being said, we currently have an issue where we can't detect Validators.required from a FormControl because @angular/forms doesn't have an API for it. Are these tests capturing something that isn't captured by the other ngModel-based tests?
There was a problem hiding this comment.
Assuming I'm reading the tests correctly, what exists right now does a good job of testing a required matInput and a good job of testing a matInput which relies on the ReactiveFormsModule without the native required attribute.
These specifically test when both are present since the new logic considers both when assigning aria-required. If we are okay with leaving that scenario untested since it's discouraged by ReactiveFormsModule I'm fine to remove these tests. I included them for completeness' sake more than anything.
There was a problem hiding this comment.
These ReactiveFormsModule tests can safely be removed since they don't reflect how the component should be used.
There was a problem hiding this comment.
These tests have been removed.
…Input`s Updates the logic for setting `aria-invalid` on `matInput` to not set the attribute at all if the input is `required` and has no value. Prior to this PR `matInput` sets `aria-invalid="false"` for any empty `matInput`, including `required` ones. This suppresses screen readers' announcement to users that such inputs are in an invalid state. Fixes angular#22777
ByzantineFailure
force-pushed
the
matInput-aria-required
branch
from
13da794
to
9f0afd3
Compare
crisbeto
added
Accessibility
…Input`s (#22802) Updates the logic for setting `aria-invalid` on `matInput` to not set the attribute at all if the input is `required` and has no value. Prior to this PR `matInput` sets `aria-invalid="false"` for any empty `matInput`, including `required` ones. This suppresses screen readers' announcement to users that such inputs are in an invalid state. Fixes #22777 (cherry picked from commit d185294)
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Updates the logic for setting
aria-invalidonmatInputto not setthe attribute at all if the input is
requiredand has no value.Prior to this PR
matInputsetsaria-invalid="false"for any emptymatInput, includingrequiredones. This suppresses screen readers'announcement to users that such inputs are in an invalid state.
Fixes #22777