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

feat(theme-default): image alt option and set aria hidden on title if same as image alt (close #20) #23

Merged
merged 11 commits into from
Jan 28, 2024

Conversation

nruffing
Copy link
Contributor

@nruffing nruffing commented Jan 21, 2024

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Provide a description in this PR that addresses what the PR is solving. If this PR is going to solve an existing issue, please reference the issue (e.g. close #123).

What is the purpose of this pull request?

  • Bug fix
  • New feature
  • Other

Description

Dequeue/Axe recommends not to include alt text on an image that is also already displayed next to it. A screen reader will then read it twice.
https://dequeuniversity.com/rules/axe/4.8/image-redundant-alt?application=AxeChrome

Currently the brand image in the default theme will also use the site title it displays as the alt text of the brand image.

This PR adds an option to set the brand image alt text to be different from the site title and default to the site title if not set. If the logo alt text and site title are the same then aria-hidden is set on the site title so its not read by screen readers.

Also, Dequeue recommends all HTML landmarks have a unique role, title or aria-label. If the page is using the auto link page nav there are two nav elements on the page with the same landmark role.
https://dequeuniversity.com/rules/axe/4.8/landmark-unique?application=AxeChrome

This PR adds unique labels to differentiate the navs.

Screenshots

Before

After

closes #20
closes #21

Copy link
Member

@Mister-Hope Mister-Hope left a comment

Choose a reason for hiding this comment

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

Answer the 2 question first before solving the other one.

I would say no to this PR at the moment, and I believe changing the logo alt to empty string is enough.

@Mister-Hope
Copy link
Member

Mister-Hope commented Jan 24, 2024

Also one more thing I would like to declare, we should definitely provide the best a11y support for the visually impaired people and body impaired people, i.e.:

  • Screen readers should read every page easily
  • All major contents should be interacted with keyboard

And SEO is important for documentation sites, so that we'd better made every SEO improvement if we can.


And in my mind, it doesn't mean we should try to fit every "best practice" given by different tools and website. For most of the part, edge built-in issue reporter and lighthouse for Chrome is enough, and we may omit the rest or even some of the rules in the above 2 tools.

For adding aria-label for <nav>, it sounds good, but it has little improvement about the 3 things I mentioned earlier. Body impaired people doesn't need the label and so is search engine. For visually impaired people, they obviously understand the page-nav from the text in my theme:

image

And if you want to improve it, a unvisible label with text "prev" and "next" which can be picked by the screen reader is actually more useful then a aria-label="page navigation" (by default the screen reader won't read it!) A best practice which only benefits people when they are reading the html source code is obviously useless. The "best practice score" given by tools is far away comparing with impaired people's experience.

@nruffing
Copy link
Contributor Author

Also one more thing I would like to declare, we should definitely provide the best a11y support for the visually impaired people and body impaired people, i.e.:

  • Screen readers should read every page easily
  • All major contents should be interacted with keyboard

And SEO is important for documentation sites, so that we'd better made every SEO improvement if we can.

And in my mind, it doesn't mean we should try to fit every "best practice" given by different tools and website. For most of the part, edge built-in issue reporter and lighthouse for Chrome is enough, and we may omit the rest or even some of the rules in the above 2 tools.

For adding aria-label for <nav>, it sounds good, but it has little improvement about the 3 things I mentioned earlier. Body impaired people doesn't need the label and so is search engine. For visually impaired people, they obviously understand the page-nav from the text in my theme:

image And if you want to improve it, a unvisible label with text "prev" and "next" which can be picked by the screen reader is actually more useful then a `aria-label="page navigation"` (by default the screen reader won't read it!) A best practice which only benefits people when they are reading the html source code is obviously useless. The "best practice score" given by tools is far away comparing with impaired people's experience.

I disagree that built-in accessibility scanners is all that should be used. Deque does a vast larger amount of testing and research then Google or Microsoft does. They are setting the standards that Google and Microsoft use and so will appear to be pushing the standard, because they are. When they add best practices its because they think and are trying to make it a standard.

In the case of landmarks not being unique it is a problem for screen readers, not just a best practice. You can see an example here where when a screen reader is performing landmark navigation there are multiple landmarks that just say "navigation". This is before getting into any contents of each navigation instance. Yes, the nest page and previous page links should have screen readable labels but that is a separate concern.

@Mister-Hope
Copy link
Member

Mister-Hope commented Jan 27, 2024

Also I believe you should send a pr to docs repo.

@nruffing
Copy link
Contributor Author

nruffing commented Jan 28, 2024

@Mister-Hope Actually, now I remember what the big advantage was having the logo alt read instead of the site title. When viewing the site in a mobile width, the site title is not displayed and thus hidden by screen readers. By having the alt text always on the logo we avoid having to address that scenario explicitly.

@nruffing
Copy link
Contributor Author

Documentation PR created at vuepress/docs#10

@Mister-Hope
Copy link
Member

@Mister-Hope Actually, now I remember what the big advantage was having the logo alt read instead of the site title. When viewing the site in a mobile width, the site title is not displayed and thus hidden by screen readers. By having the alt text always on the logo we avoid having to address that scenario explicitly.

Good point, that makes sense

@Mister-Hope Mister-Hope merged commit dc8042e into vuepress:main Jan 28, 2024
31 checks passed
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.

[Bug report] Nav Landmark Needs Unique Role/Title/Label [Feature request] Brand Img Alt Text
2 participants