search

alphagov / tech-docs-gem

Gem to distribute the tech docs project
https://tdt-documentation.london.cloudapps.digital/
MIT License
15 stars 38 forks source link

Move the `aria-expanded` attribute to the correct element #238

Closed colinbm closed 3 years ago

colinbm commented 3 years ago

⚠️ Don't forget to update the gem version in the CHANGELOG before merging! When you're ready to release bump version file and generate a tag. ⚠️

What

Why

From the DAC review:

There are sections of content with Show/ Hide functionality. Neither the expandable behaviour of the control nor its expanded or collapsed state are programmatically determinable as the aria-expanded attribute has not been utilised (please note, it appears this attribute has been incorrectly associated with the expandable region rather than the trigger).

36degrees commented 3 years ago

Whilst looking at this, I spotted an issue with the nav list that I've also fixed... currently we have too many ul elements. We should only have these at the open/close of the nav, and at the open/close of each new level of nav. As it is we have a ul element for each li.

I'm not sure this change is quite right – now everything in the nav sits at the same level.

On the current version in master, the nav looks like this:

Screenshot 2021-06-22 at 09 41 53

On this branch, it looks like this:

Screenshot 2021-06-22 at 09 42 35

colinbm commented 3 years ago

Odd - it's not doing that on mine - investigating.

36degrees commented 3 years ago

Can we consider splitting the list structure changes into a separate PR? As I understand it, they're unrelated to the aria-expanded change which looks good to go?

colinbm commented 3 years ago

@36degrees seperated that off

36degrees commented 3 years ago

Thanks @colinbm! Would you mind tidying up the PR description to remove the changes that have been split out?

Unfortunately, there's another place in the code where aria-expanded is being set which needs fixing too. When the code is first initialised, aria-expanded is still getting set to false on the collapsible__body:

https://github.com/alphagov/tech-docs-gem/blob/4ba92c82317d59051bef93810089bc316af7e12f/lib/assets/javascripts/_modules/collapsible-navigation.js#L35-L37

This means that the collapsed state is not announced when you first navigate to a button, only once it's been toggled at least once.

We're also still including a non-sensical aria-expanded on the toggled region, except now it's permanently set to false.

Sorry that I didn't spot that as part of my previous review!

We should really have tests that cover all of this included in /spec/javascripts/collapsible-navigation-spec.js. However, I don't think it's fair to ask you add them as you're only fixing the existing implementation.

colinbm commented 3 years ago

@36degrees Ah, apologies - I had fixed that, but somehow it failed to go into the commit.