Add skip links and refactor filter sidebar for accessibility #140
Conversation
Pull Request Test Coverage Report for Build 8441324055Details
💛 - Coveralls |
There was a problem hiding this comment.
I'll confirm the functionality shortly and work towards approving this.
I do have reservations about this feature and may want to discuss it further (not sure who would need to be involved quite yet) as I'm nervous we may be adding additional skip links when the actual problem is potentially how we present the content to begin with.
There was a problem hiding this comment.
We need to at minimum address the id vs class for skip links. Either as timdex-ui overrides (with a ticket to move them to the theme/style guide) or directly in the theme/style guide.
We also need to resolve the bug with Safari not tabbing to our skip links. That could handled by adding an override style to timdex-ui and/or opening a ticket to resolve it at the theme gem/style guide level.
I believe you have already confirmed you will add a new feature to the theme gem to support overriding skip links rather than forking the template.
|
The latest commit updates the theme gem in order to move the changes to a skip links partial (introduced here and released in v1.4). For now, I addressed the Safari bug in TIMDEX UI only, but I opened ENGX-260 to fix it in the style guide. During our team meeting today, we discussed the possibility of wrapping multiple skip links in another tag, so we can continue to use the |
|
Before I merge, I will also move the addition of the new partial into its own commit, as is our practice, so it's clearer what the changes are. |
|
Yeah, the Safari situation is looking increasingly rough. Adding the |
Why these changes are being introduced: We are trying out custom skip links for this application, as it has a relatively complex navigation structure. The theme gem has been updated to support this, using a new `_skip_links` partial. Relevant ticket(s): * [GDT-161](https://mitlibraries.atlassian.net/browse/GDT-161) How this addresses that need: This updates the theme gem and adds the new partial as-is. It will be customized in a subsequent commit. Side effects of this change: A small amount of technical debt may be added here, but incorporating a layout that needs to remain compatible with the upstream theme gem. However, the partial itself is very simple, so it's unlikely that any maintenance will be required.
Why these changes are being introduced: This application has many focusable elements, and it may be helpful to users navigating with keyboards or screen readers if we provide links to skip to different sections. Relevant ticket(s): * [GDT-161](https://mitlibraries.atlassian.net/browse/GDT-161) How this addresses that need: This adds the following skip links to the `_skip_links` partial: * All views: nav, search form * Results: results list, filter sidebar * Record: record metadata It also adds `.skip-link` rules to our SCSS, copied from the `#skip` rules in the style guide with two changes: * The `width: 0` rule has been removed in a step towards partial Safari support, as Safari will not focus on elements with this rule. * The color of skip links on focus is now white, as the current blue-on-black is a contrast error. Safari has a significant focus bug that Apple seems unlikely to fix. [This article](https://itnext.io/fixing-focus-for-safari-b5916fef1064) details the problem and provides a polyfill that solves it. As this bug likely affects all of our applications, we should incorporate that polyfill in the style guide. I've opened [ENGX-260](https://mitlibraries.atlassian.net/browse/ENGX-260) to do so. Side effects of this change: * Some IDs have been added to top-level elements to make them selectable. * While working on this, I realized that VoiceOver doesn't recognize the filter labels as disclosure triangles, so even when they are closed, it tabs through every available filter. I've fixed this in a subsequent commit. * There is an open question as to whether this is too many skip links. [WebAIM's guidance on skip links](https://webaim.org/techniques/skipnav/#multiple) suggests that one is optimal, but more than one may be necessary in some cases. Whether our use case necessitates them is somewhat unclear, as are the best practices for how many links to provide if you are providing multiple. I plan to discuss this further with Darcy in UX review. If we can't come to a decision, it would be a good question for DAS.
Why these changes are being introduced: VoiceOver (and possibly other screen readers) do not interpret the current sidebar drawers as disclosure triangles. As such, one must tab through every available filter, even if that filter category is collapsed. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/GDT-161 How this addresses that need: This refactors filter categories as `details` tags, so screen readers will recognize them as expandable elements. Side effects of this change: * Some tests have been updated a result of changes to class names. * We had previously been opening/closing categories with an `onclick`, was a quick-and-dirty way to get things working but not very good practice. I took this opportunity to change that too an event listener and moved it to the `filters.js` file that we added for the mobile filter toggle button. * I've removed the transition rules for the filters, since `details` does not support them natively.
jazairi
force-pushed
the
gdt-161-skip-links
branch
from
7e245e0 to
5c503b3
Compare
Why these changes are being introduced:
This application has many focusable elements, and it may be helpful
to users of assistive technology to provide skip links to different
sections.
Relevant ticket(s):
How this addresses that need:
This pulls in the
applicationlayout from the theme gem and adds to it the following skip links:It also makes the filter sidebar more semantically meaningful. As currently written, VoiceOver does not parse the filter menus as disclosure triangles, and thus tabs through each available filter in a menu even while it is collapsed.. The menus are now written as
detailselements, which VoiceOver interprets correctly.In the process of refactoring the filter sidebar, I also made a small change to the menu toggler. This had been assigned to an
onclickattribute, but it is now more appropriately an event listener, and moved to the same file as the logic for the mobile filter toggler.Side effects of this change:
The main side effect is the added technical debt of maintaining a separate version of
application.html.erb. This shouldn't be especially onerous, as that file is rarely updated. Additional side effects are noted in each commit message.Developer
Accessibility
New ENV
Approval beyond code review
Additional context needed to review
Note that this PR includes three commits: one to pull in the upstream layout, one to add the skip links, and one to refactor the filter sidebar. It will likely be easier to review by looking at each commit individually, from oldest to newest.
To verify the skip links, ensure that tab/screen reader navigation focuses on the following links before any other element, and they link to the correct section:
To verify the filter refactor, ensure that tab/screen reader navigation focuses only on the filter menu label if it is collapsed. If the menu is open, it should focus first on the label, then on each available filter. Additionally, screen readers should read "apply filter" before available filters, and "remove filter" before applied filters.
You should also confirm that filter menus can be opened and closed, and that filters can be applied and removed, in both the mobile and desktop views.
Code Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing