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

[#378] Fixed error message with screen reader. #439

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

febdao
Copy link
Collaborator

@febdao febdao commented Dec 1, 2024

Fixed: #378

Jira ticket:

Checklist before requesting a review

  • I have formatted the subject to include the issue number
    as [#123] Verb in past tense with a period at the end.
  • I have provided information in the Changed section about WHY something was
    done if this was a bespoke implementation.
  • I have performed a self-review of my code.
  • I have commented on my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have run new and existing relevant tests locally with my changes,
    and they have passed.
  • I have provided screenshots, where applicable.

Changed

  1. Add the aria-describedby attribute to all inputs that may require an error message.
  2. When an input is in an error state, inject the error container with an error message.
  3. Add the aria-invalid attribute to all required fields with a value of "false". Update this to "true" when the input is in an error state.

Screenshots

Screenshot 2024-12-01 at 11 23 16 pm
Screenshot 2024-12-01 at 11 22 41 pm

@febdao febdao added the PR: Needs review Pull request needs a review from assigned developers label Dec 1, 2024
@febdao febdao requested a review from richardgaunt December 1, 2024 12:31
@febdao febdao self-assigned this Dec 1, 2024
@@ -29,8 +29,9 @@
name="{{ name }}"
value="{{ value }}"
id="{{ id }}"
{% if is_invalid and id %}aria-describedby="{{ id }}-message"{% endif %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this work with Drupal?

Copy link
Collaborator

@richardgaunt richardgaunt Dec 16, 2024

Choose a reason for hiding this comment

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

ie seems like we would need to pass the entire ID of the message block at the top of the screen

@@ -7,6 +7,7 @@ exports[`Field Message Component allows HTML content 1`] = `

<div
class="ct-field-message ct-theme-light ct-field-message--information "
id=""
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should never have an empty id

{% if is_disabled %}disabled{% endif %}
{% if is_required %}required{% endif %}
{% if is_required %}required aria-invalid="{{ is_invalid ? is_invalid : 'false' }}"{% endif %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

If it is not invalid why do we want to add this attribute?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Think this should be outside of the is_required attribute check

{% if is_multiple %}multiple{% endif %}
{% if is_disabled %}disabled{% endif %}
{% if is_required %}required{% endif %}
{% if is_required %}required aria-invalid="{{ is_invalid ? is_invalid : 'false' }}"{% endif %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above

Copy link
Collaborator

@richardgaunt richardgaunt left a comment

Choose a reason for hiding this comment

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

@febdao I have some questions about the use of aria-invalid and its position within the component

@richardgaunt richardgaunt added PR: Requires more work Pull request was reviewed and reviver(s) asked to work further on the pull request and removed PR: Needs review Pull request needs a review from assigned developers labels Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Requires more work Pull request was reviewed and reviver(s) asked to work further on the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WCAG 1.3.1 A: Error messages not programmatically associated with input (Issue 7)
2 participants