Improve accessibility #535
Conversation
Chapters and sections are ordered, so we should use the appropriate HTML tag.
…creen readers Screen readers have this functionality build-in, no need to present this. Ideally, this should not even be in the DOM tree, since the numbers can be shown by using CSS.
Divs are not focusable by default
Using aria-hidden (together with tabIndex) takes the links out of the tab order. http://heydonworks.com/practical_aria_examples/#progressive-collapsibles
The main tag helps users skip additional content on the page.
The main content is identified by the main tag, not by auto-focusing it on page load.
- Use ul and li (since it is a list) - Add aria-expanded and aria-haspopup to the toggle button - Use button instead of div (buttons are accessible by default) - Handle Esc key (close popup) - Adjust CSS to keep same visual style
Links now expand to fill the entire row.
Previously, the header had a fixed height, which meant that sometimes the print button was not visible. Animating the left property is expensive, which lead to laggy animations - transform is much cheaper and has the same effect.
Bug introduced while making the popup accessible
Bug introduced when switching from animating left to using transform.
|
Looking through the commit messages it seems like a lot of the changes you are proposing are quite logical and will help out a lot with accessibility 👍 Unfortunately I'm not a massive expert on CSS and making accessible websites, so I don't know if I'm the best person to review this PR. @projektir mentioned he's done some front-end work in the past, so maybe he has some thoughts? Something I noticed is that we may have an issue in 29dbcdc where you're changing from using unordered lists to ordered ones. A book is actually composed of "front matter" chapters (e.g. "introduction"), the numbered chapters themselves, and some "back matter" chapters (where you'd put "conclusion" and appendices). The front and back matter chapters are meant to be un-numbered, I think we were using a I think the best fix for this would be to change the underlying |
|
I don't know much about accessibility, unfortunately. I can't figure out how to reach the sidebar with keys alone, though. I've pulled this down and looked at RBE and everything looks fine. |
You can press
|
|
Oh, yeah, I got it to work. Seems like this works in the current RBE as well. Anyway, LGTM! There's a Travis failure, though, but I am not sure what's up with it. |
Yes, but the chapters are focusable even when the sidebar is hidden.
Looks like a timeout issue to me? |
Yeah I've noticed that all the Mac jobs for the last several days are "waiting to be queued" indefinitely, until travis must kill it for taking too long. I think it's alright to merge these types of PRs without the Mac builds seeing as all the changes are purely browser-based and should be platform agnostic. |
|
Oh wow, I just checked out your branch and it really makes a difference! Just from a normal user's perspective, having keyboard navigation is pretty sweet, with the theme selector being a really nice touch. Overall that's a big 👍 from me! Is there anything else you'd like to add to the PR, or should we think about merging it? |
|
No, I think this is it for now - I might follow up later with other fixes. |
|
Merged 🎉 |
* fix(theme/index): Use nav element for Table of Content * fix(renderer/html_handlebars/helpers/toc): Use ol instead of ul Chapters and sections are ordered, so we should use the appropriate HTML tag. * fix(renderer/html_handlebars/helpers/toc): Hide section number from screen readers Screen readers have this functionality build-in, no need to present this. Ideally, this should not even be in the DOM tree, since the numbers can be shown by using CSS. * fix(theme/index): Remove tabIndex="-1" from .page Divs are not focusable by default * fix(theme): Make sidebar accessible Using aria-hidden (together with tabIndex) takes the links out of the tab order. http://heydonworks.com/practical_aria_examples/#progressive-collapsibles * fix(theme/index): Wrap content inside main tag The main tag helps users skip additional content on the page. * fix(theme/book): Don't focus .page on page load The main content is identified by the main tag, not by auto-focusing it on page load. * fix(theme/index): Make page controls accessible * fix: Make theme selector accessible - Use ul and li (since it is a list) - Add aria-expanded and aria-haspopup to the toggle button - Use button instead of div (buttons are accessible by default) - Handle Esc key (close popup) - Adjust CSS to keep same visual style * fix(theme/stylus/sidebar): Make link clickable area wider Links now expand to fill the entire row. * fix(theme): Wrap header buttons and improve animation performance Previously, the header had a fixed height, which meant that sometimes the print button was not visible. Animating the left property is expensive, which lead to laggy animations - transform is much cheaper and has the same effect. * fix(theme/stylus/theme-popup): Theme button inherits color Bug introduced while making the popup accessible * fix(theme/book): Handle edge case when toggling sidebar Bug introduced when switching from animating left to using transform.
Most commits have a body explaining the reasoning behind the changes.