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

aria-errormessage is hidden or removed when not pertinent #1588

Merged
merged 16 commits into from
Apr 25, 2023

Conversation

jongund
Copy link
Contributor

@jongund jongund commented Aug 3, 2021

No description provided.

@jongund jongund linked an issue Aug 3, 2021 that may be closed by this pull request
@spectranaut
Copy link
Contributor

spectranaut commented Aug 12, 2021

I just fixed the conflict in the README and added a json file for the AXE results. It seems to me there is a bug in AXE, but I'm having trouble reporting it...

Here are the results:

  Violation of "aria-valid-attr-value" with 4 occurrences!
    Ensures all ARIA attributes have valid values. Correct invalid elements at:
     - #input-1
     - #input-2
     - #input-3
     - #input-7
    For details, see: https://dequeuniversity.com/rules/axe/4.3/aria-valid-attr-value

According to Jon's examples, inputs 1 - 4 should fail, and 5 - 8 should not, so the AXE results do not match the expectation. The more detailed error says aria-errormessage value id-message-1 must use a technique to announce the message (e.g., aria-live, aria-describedby, role=alert, etc.),. Seems to me this is wrong, and there is an additional bug in that the hiddenness of the element pointed to by aria-errormessage is not reported?

edit: I wrote up a bug: dequelabs/axe-core-npm#328

Copy link
Contributor

@spectranaut spectranaut left a comment

Choose a reason for hiding this comment

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

I approve this PR but I also added a AXE file, so @joanmarie or @schne324 should take a look too.

@pkra
Copy link
Member

pkra commented Jan 10, 2022

ping @joanmarie @jnurthen

This testcase resolves #1576 (which is technically in the 1.2 milestone).

@pkra pkra added this to the ARIA 1.3 milestone Jan 12, 2022
@spectranaut spectranaut self-assigned this Sep 1, 2022
@jnurthen jnurthen requested review from MelSumner and removed request for joanmarie January 19, 2023 18:30
{
"description": "Ensures all ARIA attributes have valid values",
"help": "ARIA attributes must conform to valid values",
"helpUrl": "https://dequeuniversity.com/rules/axe/4.3/aria-valid-attr-value?application=webdriverjs",
Copy link
Contributor

Choose a reason for hiding this comment

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

Question, non-blocker: Is the query param needed here? It seems like the URL is valid without it. Just curious.

Copy link
Member

Choose a reason for hiding this comment

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

This is the file retrieved from aXe. I don't think we should be changing it.


<!--
URL: https://www.w3.org/TR/wai-aria-1.2/#aria-errormessage
RULE: " Authors MUST use aria-invalid in conjunction with aria-errormessage and when aria-errormessage is pertinent, authors MUST ensure the content is not hidden so users can navigate to and examine the error message"
Copy link
Contributor

@MelSumner MelSumner Jan 31, 2023

Choose a reason for hiding this comment

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

Question, potential blocker: Should an example be included that has no aria-invalid attribute?

From the spec, emphasis mine:

Initially, the object is in a valid state and either has aria-invalid set to false or no aria-invalid attribute, and the element referenced by aria-errormessage is not applicable.

WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I think we would if this were a test file for all of aria-errormessage.

However this is only a testfile for the added authors MUST statements in aria 1.2. Without aria-invalid, aria-errormessage is not pertinent so the entire sentence would not apply.

@pkra
Copy link
Member

pkra commented Apr 20, 2023

@jnurthen can we merge this? It's actually closing the last open issue for the 1.2 milestone.

Copy link
Contributor

@schne324 schne324 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.

@spectranaut, you probably have already seen this but it looks like the underlying axe-core issue you raised has been resolved: dequelabs/axe-core#3149

@pkra pkra added the Agenda label Apr 22, 2023
@jnurthen jnurthen merged commit 11902fc into main Apr 25, 2023
@pkra pkra deleted the errormessage-hidden-removed branch April 25, 2023 19:36
jnurthen added a commit that referenced this pull request Oct 10, 2023
Co-authored-by: Valerie Young <spectranaut@igalia.com>
Co-authored-by: James Nurthen <jnurthen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Author test case: aria-errormessage (ARIA 1.2)
6 participants