alphagov / govuk-frontend

GOV.UK Frontend contains the code you need to start building a user interface for government platforms and services.
https://frontend.design-system.service.gov.uk/
MIT License
1.17k stars 321 forks source link

Accordion - Elements must only use allowed ARIA attributes #2472

Closed adamliversidge closed 9 months ago

adamliversidge commented 2 years ago

Description of the issue

Axe DevTools reports "ARIA attribute: aria-labelledby is not well supported. Use a different role attribute or element."

Element location #accordion-default-content-1

Need to ensure ARIA attributes are allowed for an element's role

Steps to reproduce the issue

Run the accessibility checker with an expanded accordion

Actual vs expected behaviour

Error should not be reported for the expanded accordion

Environment (where applicable)

GOV.UK Frontend v3.14.0 https://design-system.service.gov.uk/components/accordion/default/index.html

spSlaine commented 2 years ago

Suggestion - add role="region" to the div.govuk-accordion__section-content

<div class="govuk-accordion" data-module="govuk-accordion" id="accordion-default">
  <div class="govuk-accordion__section ">
    <div class="govuk-accordion__section-header">
      <h2 class="govuk-accordion__section-heading">
        <span class="govuk-accordion__section-button" id="accordion-default-heading-1">
          Writing well for the web
        </span>
      </h2>
    </div>
    <div id="accordion-default-content-1" class="govuk-accordion__section-content" aria-labelledby="accordion-default-heading-1" role="region">
      <p class='govuk-body'>This is the content for Writing well for the web.</p>
    </div>
  </div>
</div>
36degrees commented 1 year ago

Adding aria-labelledby to a <div> with no explicit role is explicitly disallowed by the 'ARIA in HTML' specification.

Unless we have good evidence that it's useful to make the accordion sections landmark regions (in which case we should probably make them into <section>s), we should just remove the aria-labelledby attribute entirely.

(Relevant discussion on the use of <section>s in a GDS Way PR: https://github.com/alphagov/gds-way/pull/781)

jpveooys commented 1 year ago

This also occurs in axe in Chrome with a collapsed accordion now that hidden="until-found" is used, for info.

dav-idc commented 1 year ago

Hi @36degrees, I've taken a look at this issue and if everything mentioned in this Github issue is accurate, I think this is a WCAG failure and something we should fix.

This is my theory as for how it's currently failing:

This is an incorrect implementation of WAI-ARIA 1.1 and a failure of WCAG success criterion 4.1.2 Name, Role, Value.

For anyone watching this thread, let me know if you think I've misinterpreted the situation 👍

dav-idc commented 1 year ago

If this is a WCAG failure, I'd like to add this to our accessibility statement as a known piece of 'non-accessible content', which is the name of the section where we record accessibility issues which constitute failures of WCAG 2.1 AA. It would be noted as a failure in the GOV.UK Frontend codebase and not one of our website products.

This is my drafted note:

The accordion component is currently using an aria-labelledby ARIA attribute on an unsupported <div> element. This is an incorrect implementation of WAI-ARIA 1.1 and a failure of WCAG success criterion 4.1.2 Name, Role, Value. We plan to fix this within 2023. Track our progress on the GitHub issue: Accordion - Elements must only use allowed ARIA attributes.

36degrees commented 1 year ago

Which aspect(s) of 4.1.2 do you think it fails on?

dav-idc commented 1 year ago

I didn't personally assess it as a failure of 4.1.2, the failure under 4.1.2 is reported by axe DevTools. Here's their explainer page for the rule it says we're breaking: https://dequeuniversity.com/rules/axe/4.6/aria-allowed-attr?application=AxeChrome

If we doubt that I could do a more thorough double-check.

36degrees commented 1 year ago

It's going to catch quite a range of issues, some of which by its own description have 'no effect on the accessibility'. Definitely some of the issues it will catch might mean you fail 4.1.2, but I don't think there's a direct correlation.

Looked at another way, unless I'm missing something, I think the success criterion still holds 'true' despite the incorrect use of aria-labelledby?

For all user interface components (including but not limited to: form elements, links and components generated by scripts), the name and role can be programmatically determined; states, properties, and values that can be set by the user can be programmatically set; and notification of changes to these items is available to user agents, including assistive technologies

I still agree with fixing it, just not sure it needs to go in the statement unless we can articulate why it's failing.

dav-idc commented 1 year ago

Hey @36degrees I agree, I wouldn't personally assess it as a failure of 4.1.2 based on our existing testing and evidence.

If we found a combination of assistive tech, browser and OS that ended up doing something funky or buggy because of the aria-labelledby, then it would be easier to call a fail. But We don't have evidence of that being the case. Right now it's just improper use of WAI-ARIA.

My recommendation: We add this to the accessibility statement under the "(c)" category that stresses that it's not a WCAG failure. That way we can test out our use of the accessibility strategy and correctly state that it's not a WCAG fail. I'll also note on the item that it is not a WCAG fail.

Here are those 3 options that are available in the accessibility statement for categorising issues:

How does that sound @36degrees? I know it's still not the same as your recommendation, which is to not add it to the accessibility statement.

36degrees commented 1 year ago

Sounds good to me 👍🏻

eschweitzer78 commented 1 year ago

Hello @davidc-gds, I was about to report this issue when I noticed it's open. FYI I found the reference text that states one must not use aria-labelledby in a div unless a role is set. W3 used capital letters as in MUST NOT in the paragraph this links to: https://www.w3.org/TR/html-aria/#example-elements-with-implicit-aria-roles-which-prohibit-naming-from-authors

stephbatliner commented 1 year ago

I was wondering if there are there any timelines around fixing this bug? Thank you

dav-idc commented 1 year ago

Hello @davidc-gds, I was about to report this issue when I noticed it's open. FYI I found the reference text that states one must not use aria-labelledby in a div unless a role is set. W3 used capital letters as in MUST NOT in the paragraph this links to: https://www.w3.org/TR/html-aria/#example-elements-with-implicit-aria-roles-which-prohibit-naming-from-authors

Hey @eschweitzer78 thanks for locating the text in the 'ARIA in HTML' specification!

At this point, we've added this to our accessibility statement under the category 'Other identified and tracked accessibility concerns'. We still don't have evidence of this being a true WCAG 2.1 AA fail, but it is definitely an incorrect implementation according to the 'WAI-ARIA 1.1' specification. We can now also count it as conflicting with the 'ARIA in HTML' specification, as you've identified.

dav-idc commented 1 year ago

Hi @stephbatliner thanks for checking in! In our accessibility statement we've set a timeline that says "We plan to fix this within 2023". I don't have a more precise timeline at the moment, but I'll see about resurfacing this.

dav-idc commented 1 year ago

Adding aria-labelledby to a <div> with no explicit role is explicitly disallowed by the 'ARIA in HTML' specification.

Unless we have good evidence that it's useful to make the accordion sections landmark regions (in which case we should probably make them into <section>s), we should just remove the aria-labelledby attribute entirely.

(Relevant discussion on the use of <section>s in a GDS Way PR: alphagov/gds-way#781)

Hey @36degrees now that that GDS Way piece has been resolved, is there anything we can learn from that work?

Here's the updated guidance on region landmarks: https://gds-way.cloudapps.digital/manuals/programming-languages/html.html#region-landmarks

colinbruce commented 1 year ago

This has broken automated axe testing with the release of aws-core-* 4.8.0 gems. We now get a failing response for accordion sections with aria-labelledby attribute cannot be used on a div with no valid role attribute

Manually adding a role into line https://github.com/alphagov/govuk-frontend/blob/f837f9c003a50f9e64a2fe3476be91e6e228f318/packages/govuk-frontend/src/govuk/components/accordion/template.njk#L54

Like so:

<div id="{{ id }}-content-{{ loop.index }}" role="region" class="govuk-accordion__section-content" aria-labelledby="{{ id }}-heading-{{ loop.index }}"> 

ensures the tests pass

colinrotherham commented 1 year ago

We've just received the Axe 4.8 update for Puppeteer and can confirm our own tests now log the issue: https://github.com/alphagov/govuk-frontend/actions/runs/6531831782/job/17733984594

@dav-idc Before I add a known Axe exception, are we still confident this is improper usage versus a failure?

Otherwise, do we have a suggested fix we're confident in?

Might be quicker

dav-idc commented 1 year ago

@colinrotherham here's a message from @36degrees in a duplicate GitHub issue that came up a couple weeks ago:

We're busy working on the next major release at the minute but I'm hoping we'll be able to resolve this issue soon. As far as I know although it's flagged by Axe we're not aware of any problems it would cause users, but it has come up quite a lot so it'd be good to get it fixed anyway!

We list this in our accessibility statement as a non-WCAG-failing accessibility concern and say "We plan to fix this within 2023." But as far as when it's fixed and how it interacts with release planning, that's up to devs (you, Ollie, others)

colinrotherham commented 1 year ago

Thanks @dav-idc

I'm happy to add a known exception like we do with the Radios Axe rule for https://github.com/alphagov/govuk-frontend/issues/979

For when this gets picked up would you suggest either we:

  1. Move to <section> elements?
  2. Add the role="region" attribute?
  3. Remove aria-labelledby attribute?
dav-idc commented 1 year ago

I would want @36degrees to weigh in if he has capacity, but I also figured I may as well just try and see what value we're currently getting out of the aria-labelledby attribute.

As far as I can tell when testing quickly with NVDA and JAWS, the aria-labelledby attribute on the hidden content div area doesn't change NVDA or JAWS annoucements for:

So basically... I don't know what adding aria-labelledby is actually doing for us in practical terms. 🤷 But I'd want someone else to test and confirm.

colinrotherham commented 11 months ago

We did go ahead with the Axe 4.8 update for Puppeteer last week

The violation rule changed to ~aria-allowed-attr~ aria-prohibited-attr but still excluded as a known issue

dav-idc commented 9 months ago

In case this issue is currently stuck because:

  1. don't want to use <section>
  2. don't want to remove aria-labelledby

You could explore using <article> instead of <div>.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/article

Based on the HTML spec for the article element, it seems like an acceptable use in many accordion cases, but probably not fully appropriate for all.

Might still be a decent calculated risk if there's hesitation to use section and hesitation to remove aria-labelledby? Not sure if that's the piece holding this up though.