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.16k stars 319 forks source link

Axe issue: "All page content must be contained by landmarks" #1604

Open colinrotherham opened 4 years ago

colinrotherham commented 4 years ago

Hello đź‘‹

We’ve noticed for a while that prototype kit and GOV.UK Frontend examples keep the “Back link” and “Phase banner” outside landmark regions.

i.e. They’re not in a <header>, <footer> or <main>.

This throws up moderate issues in axe: https://dequeuniversity.com/rules/axe/3.3/region?application=AxeChrome

Components not in <main> or <header>

This example shows showing both components outside regions:
http://govuk-frontend-review.herokuapp.com/full-page-examples/upload-your-photo

Components outside  main

But the "Breadcrumbs" component is in <main>

This example shows breadcrumbs inside a region:
http://govuk-frontend-review.herokuapp.com/full-page-examples/renew-driving-licence

Breadcrumbs component inside  main

Here's what we see in the axe sidebar when follow the examples (with “Back link” and “Phase banner” outside landmark regions):

Axe report

It would be great to see the research behind when to use a landmark or not.

Should this be logged under https://github.com/alphagov/govuk-frontend/issues/1280#issuecomment-509588851?

Thanks

NickColley commented 4 years ago

Just to clarify, the 'govuk-frontend-review' application is not intended to be guidance and contains intentionally legacy and therefore out of date patterns for test the 'compatibility mode'.

So that's the reason why one of the examples in there is wrong.

In the official guidance on the Design System website we intentionally do not have the phase banner, back links, or breadcrumbs in the 'main' element.

This is for two reasons I know of:

I don't have the original research for the latter decision, but will ask around and see if I can dig it up.

Will leave this in triage for us to decide as a team what direction we want to take, thanks a lot for raising this I'm sure others will run into this too.

NickColley commented 4 years ago

With the help from @36degrees @selfthinker I've pulled out some of the historical stuff:

It turns out some of our nav tags were redundant. They either added no extra meaning to the links they wrapped or were unnecessary as user agents got the same information from the links' context instead.

Among those that were needed, the navigational theme that grouped their contents was sometimes unclear.

The above came out of an accessibility review by LĂ©onie Watson. In it she explained that having too many nav tags devalued their use for screen reader users. It was suggested that when multiple ones were necessary we could help those users find the links they needed by identifying them properly.

colinrotherham commented 4 years ago

@nickcolley This is exactly the sort of thing I was after! Thank you for the detective work 🕵

hannalaakso commented 4 years ago

I've added this to https://github.com/alphagov/govuk-frontend/issues/1280#issuecomment-509588851 as it can also seen on the GOV.UK Frontend Template. Thanks again for raising this @colinrotherham

NickColley commented 4 years ago

Looks like this has been resolved and added to documentation for people to find in the future so will close this one out, thanks :)

36degrees commented 4 years ago

Re-opening this in favour of #1949 as this has more context.

Whilst we previously decided against moving the phase banner, back link or breadcrumbs into the <main> element, we have recently discussed whether it would make sense instead to extend the <header> to include them alongside the header component.

36degrees commented 4 years ago

We could also revisit wrapping the breadcrumbs and back link in navigation landmarks, taking into consideration https://insidegovuk.blog.gov.uk/2013/07/03/rethinking-navigation/ and validating with user research.

robertparkinson commented 3 years ago

I have come across this issue recently in our code and have worked up a potentional solution for the phase banner being moved into the header. How do I go about getting rights to push a PR for discussion?

36degrees commented 3 years ago

@robertparkinson we do not grant write permissions to users outside of the team, but we're more than happy for you to raise a pull request from a fork.

It does sound like the change you're suggesting would be fairly substantial (and breaking), so it would need to be managed carefully. To avoid any wasted effort on your part, are you able to describe the change that you're planning on proposing first?

robertparkinson commented 3 years ago

Thanks @36degrees I've opened a PR. Would love your feedback and have just focussed on the phase banner as that should be an easier conversation than surroundign the back button with nav tags. https://github.com/alphagov/govuk-frontend/pull/1984

matthewmascord commented 3 years ago

Having the ability to add additional content before the HEADER closing tag would definitely be a step forward. This would be for content that is semantically part of the header for screen reader purposes, for example, a departmental banner, but visually under the grey bar.

Allowing this would mean screenreader users can get to the main content quicker and avoid Aria difficulties around having multiple banner landmarks on the same page.

jorgeponto commented 3 years ago

With the help from @36degrees @selfthinker I've pulled out some of the historical stuff:

It turns out some of our nav tags were redundant. They either added no extra meaning to the links they wrapped or were unnecessary as user agents got the same information from the links' context instead. Among those that were needed, the navigational theme that grouped their contents was sometimes unclear. The above came out of an accessibility review by LĂ©onie Watson. In it she explained that having too many nav tags devalued their use for screen reader users. It was suggested that when multiple ones were necessary we could help those users find the links they needed by identifying them properly.

Yes. It Is understandable the rationale of LeĂłnie Watson, but a div title="breadcrumb" and a aria-current="page" at the last link (if I'm here why I need a link?) of the sequence is a good improve to screenreader users understand the structure of this element.

brunswickcomputing commented 3 years ago

skip-link still throws the same warning in Axe. i can't see any reference to this being tackled so far. any plans or have I missed something? Screenshot 2021-03-23 at 19 18 53

36degrees commented 3 years ago

We should consider the skip link component as part of this issue, but worth noting that Deque themselves say:

It is best practice to contain all content excepting skip links, within distinct regions such as the header, nav, main, and footer.

vanitabarrett commented 3 years ago

As discussed in the V4 scoping session, this needs more investigation to find out what the recommended implementation is and ideally test with users before we make the change live. We should also talk to GOV.UK and find out what changes they're planning to the header component (and if we need to make any changes as a result), as that may affect this work.

owenatgov commented 1 year ago

We revisited this very briefly in a discussion amongst some of the devs today. The takeaways:

selfthinker commented 3 days ago

It's worth noting that Axe changed this from a "must" and flagging it as a WCAG fail to a "should" and now flags it as "best practice". See more on what Deque say about the issue from Axe.