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

Use landmark roles to improve navigation for screen reader users #138

Closed

Conversation

jcarstairs-scottlogic
Copy link
Contributor

@jcarstairs-scottlogic jcarstairs-scottlogic commented Dec 5, 2023

Some screen reader users use landmark roles to quickly navigate around webpages. We can use landmarks in our blog to make navigation easier for these users.

In particular, this PR:

  • uses <main>, which has an implicit landmark role of main
  • uses <aside>, which has an implicit landmark role of complementary
  • uses aria-label and aria-labelledby to explain to screen reader users what each landmark is for, when there's more than one landmark of the same type on the same page (see MDN on labelling landmarks)
  • chucks in an <article> tag around blog posts. Doesn't have an associated landmark role and doesn't seem to affect screen reader behaviour for me, but it can't hurt. See MDN, or have a look at how the BBC use it.

@jcarstairs-scottlogic jcarstairs-scottlogic force-pushed the use-landmarks branch 2 times, most recently from c2ca612 to fe1f5fe Compare December 6, 2023 11:40
@jcarstairs-scottlogic jcarstairs-scottlogic marked this pull request as ready for review December 6, 2023 11:44
@ColinEberhardt
Copy link
Contributor

Can you please link to a published version of the blog with these changes so that I can check them visually?

@jcarstairs-scottlogic
Copy link
Contributor Author

Can you please link to a published version of the blog with these changes so that I can check them visually?

https://ithake.github.io/blog-slogic/

It should be visually indistinguisable ;) the difference I'm hoping to make is that by exposing landmark roles, it will be easier for screen reader users to navigate through each page. I'm pressing d with NVDA.

category/ai.html Outdated Show resolved Hide resolved
@ithake
Copy link
Contributor

ithake commented Dec 11, 2023

It should be visually indistinguisable ;) the difference I'm hoping to make is that by exposing landmark roles, it will be easier for screen reader users to navigate through each page. I'm pressing d with NVDA.

@jcarstairs-scottlogic - some thoughts to consider for style and content:
On the majority of projects, you'll be submitting PRs with attached unit tests, with the expectation that they'll be tested by QA specialist either manually or via existing test suite.
Here, there's no expectation of an external tester, so there's more need for you to show your what testing you've done.

For example - #124 - I have screen shots before and after a functional change and a reference to doing (admittedly inconclusive!) non-functional testing for a non-functional change.
It doesn't "prove" I've tested everything, but at least it shows I've tested something, and there some information there for the reviewer to pick up on - can they spot something else I should be testing?

"It should be visually indistinguisable ;)" in itself, this is light-hearted, but when you follow up with "the difference I'm hoping .. " the cumulative effect is that the reader wonders just how much uncertainty is involved here?

@jcarstairs-scottlogic jcarstairs-scottlogic force-pushed the use-landmarks branch 2 times, most recently from fc551dc to 85ae097 Compare December 11, 2023 13:51
@ithake
Copy link
Contributor

ithake commented Dec 11, 2023

,,, by exposing landmark roles, it will be easier for screen reader users to navigate through each page. I'm pressing d with NVDA.

This all belongs in the introduction/description of the change.

Some readers use landmark roles to navigate.... Currently when you read blog site using NVDA (example screen reader) [this happens].

After making the change, when testing locally [something else happens].

Bonus points for:

The changes have been made in common include files so will effect all posts equally. I've tested using the list of representative posts that were agreed with our project sponsor previously.

Finally, your links to external documents in the PR description are really helpful, well done. If they're already detailed in a separate "Issues" document, then you don't need to repeat it all, instead, add a link to the document on sharepoint, and either ensure that it's publicly available or explicitly given the reviewer access to it as well.

@ithake
Copy link
Contributor

ithake commented Dec 11, 2023

+1 for the semantic structure!

Is it worth adding a main element to _layouts/default.html as well?
On one hand it's not used AFAIK, on the other, it means we consistently have all the .html files in the _layouts folder defining a main for consistency.

index.html Outdated Show resolved Hide resolved
@jcarstairs-scottlogic jcarstairs-scottlogic force-pushed the use-landmarks branch 2 times, most recently from f7afb46 to 308b2c7 Compare December 11, 2023 17:55
@jcarstairs-scottlogic
Copy link
Contributor Author

jcarstairs-scottlogic commented Dec 12, 2023

Regarding the failing pipeline: I think we're good to go.

I've looked through all the failures picked up by the pipeline on this PR. Almost all of them are colour contrast. The only other one is a <select> without an accessible name -- this is not new, and is fixed by #141.

It is unexpected that it picks up colour contrast issues: these should be ignored. I can't figure out why they're not being ignored on this PR. But it looks like when you make a new branch from gh-pages now, the problem goes away: see my dummy blog post PR. It's maybe something to do with the finer details of Git. I'll keep my fingers crossed and an eye on incoming PRs.

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