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 breadcrumbs WCAG fails in government-frontend #3338

Merged

Conversation

unoduetre
Copy link
Contributor

@unoduetre unoduetre commented Sep 19, 2024

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

Follow these steps if you are doing a Rails upgrade.

What

goverment-frontend places breadcrumbs inside the <main> container in some places, creating a WCAG fail. I’ve investigated all the templates in the repo and this problem is present in all the page templates that import the breadcrumbs, apart from /app/views/layouts/application.html.erb:

  • app/views/histories/1_horse_guards_road.html.erb
  • app/views/histories/11_downing_street.html.erb
  • app/views/histories/king_charles_street.html.erb
  • app/views/histories/10_downing_street.html.erb
  • app/views/histories/lancaster_house.html.erb
  • app/views/content_items/service_manual_service_standard.html.erb; example page: /service-manual/service-standard
  • app/views/content_items/service_manual_guide.html.erb; example page: /service-manual/measuring-success/using-data-to-improve-your-service-an-introduction
  • app/views/content_items/service_manual_topic.html.erb; example page: /service-manual/communities

Move the breadcrumbs out of the main container in each of the impacted templates.

There is one more template using the breadcrumbs component: /app/views/content_items/manuals/_manual_section_layout.html.erb
This is used e.g. in /guidance/immigration-rules/immigration-rules-appendix-continuous-residence but it represents the second breadcrumb component, not the first one, so nothing needs to be done but as pointed out by @MartinJJones it also needs to be addressesed.

Why

Having the breadcrumbs inside the main container is a WCAG fail.

Trello card

@unoduetre unoduetre changed the title Fix breadcrumbs WCAG fails in government-frontend (move breadcrumbs out of main) Fix breadcrumbs WCAG fails in government-frontend Sep 19, 2024
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3338 September 19, 2024 15:32 Inactive
@unoduetre unoduetre force-pushed the 2857-fix-breadcrumbs-wcag-fails-in-government-frontend-m-l branch from da7817e to 6329c20 Compare September 19, 2024 15:58
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3338 September 19, 2024 15:59 Inactive
@unoduetre unoduetre force-pushed the 2857-fix-breadcrumbs-wcag-fails-in-government-frontend-m-l branch from 6329c20 to 964bd40 Compare September 19, 2024 16:02
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3338 September 19, 2024 16:02 Inactive
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3338 September 20, 2024 08:09 Inactive
@unoduetre unoduetre marked this pull request as ready for review September 20, 2024 14:25
@unoduetre unoduetre force-pushed the 2857-fix-breadcrumbs-wcag-fails-in-government-frontend-m-l branch from 5561877 to 8e934ff Compare September 20, 2024 16:02
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3338 September 20, 2024 16:02 Inactive
Copy link
Contributor

@MartinJJones MartinJJones left a comment

Choose a reason for hiding this comment

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

Nice work!, the changes look good to me overall.

I think that we should still move the breadcrumbs component before the main element in the _main_section_layout template, this will follow the design system guidance and fix the accessibility issue by allowing users to jump straight to the main content.

Another issue spotted when reviewing this is that the breadcrumbs also have the same aria-label value, I will create a new Trello card to capture this issue so it can be fixed.

@unoduetre unoduetre force-pushed the 2857-fix-breadcrumbs-wcag-fails-in-government-frontend-m-l branch from 8e934ff to 62fb680 Compare September 24, 2024 10:10
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3338 September 24, 2024 10:10 Inactive
@unoduetre
Copy link
Contributor Author

unoduetre commented Sep 24, 2024

I think that we should still move the breadcrumbs component before the main element in the _main_section_layout template, this will follow the design system guidance and fix the accessibility issue by allowing users to jump straight to the main content.

I've moved the breadcrumbs out of main, so this should now be fixed.

Pages to test:

  • /guidance/immigration-rules/immigration-rules-appendix-continuous-residence
  • /hmrc-internal-manuals/denatured-alcohol/dnalc0500

Copy link
Contributor

@MartinJJones MartinJJones left a comment

Choose a reason for hiding this comment

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

Changes look good to me, nice work 👍

@unoduetre unoduetre merged commit 99b972c into main Sep 24, 2024
11 checks passed
@unoduetre unoduetre deleted the 2857-fix-breadcrumbs-wcag-fails-in-government-frontend-m-l branch September 24, 2024 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants