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(checks/aria): mark elements missing from aria-errormessage for review #2550

Merged
merged 1 commit into from
Oct 13, 2020

Conversation

imccausl
Copy link
Contributor

@imccausl imccausl commented Oct 11, 2020

If IDs in aria-errormessage do not correspond to existing elements, return undefined to mark for review.

Closes issue #2460

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Code is reviewed for security

…view

If IDs in aria-errormessage do not correspond to existing elements, return undefined to mark for review.

Closes issue dequelabs#2460
@imccausl imccausl requested a review from a team as a code owner October 11, 2020 16:41
@@ -45,14 +45,13 @@ function ariaErrormessageEvaluate(node, options) {
-1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is most likely a separate issue (I didn't make any change because I felt it was out of scope, and I wasn't 100% sure about it), but right now it looks like the ariaErrormessageEvaluate check will fail in a valid case such as this (from https://www.w3.org/TR/2017/REC-wai-aria-1.1-20171214/#aria-errormessage)

<label for="startTime"> Please enter a start time for the meeting: </label>
<input id="startTime" type="text" aria-errormessage="msgID" value="" aria-invalid="false">
<span id="msgID" aria-live="off" style="visibility:hidden"> Invalid time:  the time must be between 9:00 AM and 5:00 PM" </span>

where aria-live is off because aria-invalid is false. Maybe this check should check for the existence of aria-invalid="true" before getting strict about announcers?

Sidenote, w3 also seems to suggest that the announcer is optional on the element referenced by aria-errormessage:

Authors MAY use live regions for the error message element applying either an aria-live property or using one of the live region roles, for example, alert. A live region scenario is when an error message is displayed to users only after they have provided invalid information.

Just curious about your thoughts regarding this 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a totally valid point. Would you mind opening an issue for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely! I wouldn't mind working on it as well.

@imccausl imccausl changed the title fix(checks/aria): Mark elements missing from aria-errormessage for review fix(checks/aria): mark elements missing from aria-errormessage for review Oct 11, 2020
@WilcoFiers WilcoFiers added the hacktoberfest Help contribute during the month of October to participate https://hacktoberfest.digitalocean.com/ label Oct 12, 2020
Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'll let @straker have the final word.

@straker straker merged commit 8f9a035 into dequelabs:develop Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest Help contribute during the month of October to participate https://hacktoberfest.digitalocean.com/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants