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

fix(material/input): only set as aria-invalid if the input isn't empty #21609

Merged
merged 1 commit into from
Feb 10, 2021

Conversation

crisbeto
Copy link
Member

Only sets aria-invalid on a MatInput if it is invalid and it has a value, otherwise it'll likely overlap with aria-required and cause more noise for users. Furthermore, it may be confusing if the user lands on an input that they haven't interacted with, but it's already invalid.

Fixes #18140.

@crisbeto crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent Accessibility This issue is related to accessibility (a11y) target: patch This PR is targeted for the next patch release labels Jan 15, 2021
@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 15, 2021
Only sets `aria-invalid` on a `MatInput` if it is invalid and it has a value, otherwise it'll likely
overlap with `aria-required` and cause more noise for users. Furthermore, it may be
confusing if the user lands on an input that they haven't interacted with, but it's already
invalid.

Fixes angular#18140.
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.

'[attr.aria-required]': 'required.toString()',
// Only mark the input as invalid for assistive technology if it has a value since the
// state usually overlaps with `aria-required` when the input is empty and can be redundant.
'[attr.aria-invalid]': 'errorState && !empty',
Copy link
Member

Choose a reason for hiding this comment

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

Just to clarify- will errorState be true when the control is touched (not pristine)?

Copy link
Member Author

Choose a reason for hiding this comment

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

The errorState is set based on the ErrorStateMatcher. The default one will be true if the input is invalid and touched or the form has been submitted.

@zelliott
Copy link
Collaborator

From #18140:

An input field that is required should be marked as required in the HTML (which it is), but should not be marked as aria-invalid until the user enters an invalid value.

Sounds alright to me. I took a look at WCAG for any guidance (here's the relevant technique: https://www.w3.org/WAI/WCAG21/Techniques/aria/ARIA21) and stumbled upon two different examples:

  • WCAG Example 1: Applies aria-invalid on form submission for required fields. Does not apply aria-required.
  • WCAG Example 2: Only applies aria-invalid on form submission if the required fields fail validation in some other way. Does not apply aria-invalid for empty fields that are required. Also does not apply aria-required.

The second example shows that there is WCAG precedent for omitting aria-invalid on required, empty fields, so this LGTM.

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

@jelbourn jelbourn added the action: merge The PR is ready for merge by the caretaker label Jan 20, 2021
@wagnermaciel
Copy link
Contributor

@crisbeto or @jelbourn We have some internal tests that check this exact behavior. E.g.

    it('displays an error when input is empty', () => {
        setInputValue(input, '');
        expect(input.getAttribute('aria-invalid')).toBe('true');
     });

Is it ok to delete these tests entirely or should we change how they're checking the inputs' error state?

@jelbourn
Copy link
Member

jelbourn commented Feb 3, 2021

@wagnermaciel the test should ideally be updated to reflect the new behavior

@wagnermaciel
Copy link
Contributor

@jelbourn What is the new correct way to check an inputs error state?

@jelbourn
Copy link
Member

jelbourn commented Feb 3, 2021

Probably updating the test to not have aria-invalid and instead checking the error state of the form

@mmalerba mmalerba merged commit d0c53ac into angular:master Feb 10, 2021
mmalerba pushed a commit that referenced this pull request Feb 10, 2021
#21609)

Only sets `aria-invalid` on a `MatInput` if it is invalid and it has a value, otherwise it'll likely
overlap with `aria-required` and cause more noise for users. Furthermore, it may be
confusing if the user lands on an input that they haven't interacted with, but it's already
invalid.

Fixes #18140.

(cherry picked from commit d0c53ac)
@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 Mar 13, 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 P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Input being required should not result in aria-invalid="true"
5 participants