Improve accessibility for dashboard course settings dropdown - #11393
Conversation
|
Nice sleuthing, @clytwynec. I do have one request though: could you add either a colon (":") or the word "for" after "Course options dropdown"? Even with the improved context, the lack of a pause makes it difficult to really differentiate. It could just be my inexperienced ears (maybe folks who are proficient with screen readers and use them regularly wouldn't have the issue) but a bit of a pause would be nice. However, let's see what @cptvitamin thinks first. Otherwise, nice work 👍 |
|
I agree with Chris here, but prefer a different approach. I don't want to explicitly say "dropdown" since the ARIA already reveals that this control has a popup and its current state. Instead of a pause, lets just have the link text read "Course options for [name of course]" that way no pause is needed, each link is unique, and we don't redundantly describe the type of control. Now that we have the value of the text out of the way, wouldn't this test failure go away if this were a button? I find it odd that we are using a link with an href value of a page fragment (that isn't a heading and isn't focusable) It probably seemed like a good idea to the dev at the time, but I think using a button with JS on it would be better. add a tabindex=-1 on the div and move focus to that upon activation. I also noticed that you can tab past this menu and leave it open. We should probably have it close on blur. Thoughts? |
8c43956 to
50e697d
Compare
|
@clrux @cptvitamin Please resume review. I've updated the description to reflect the changes and included a sandbox. |
50e697d to
6179c6e
Compare
| <div class="wrapper-action-more" data-course-key="course-v1:DelftX+CTB3365DWx+1T2016"> | ||
| <a href="#actions-dropdown-2" class="action action-more" id="actions-dropdown-link-2" aria-haspopup="true" aria-expanded="false" data-course-number="CTB3365DWx" data-course-name="Introduction to Drinking Water Treatment" data-dashboard-index="2"> | ||
| <span class="sr">Course options dropdown</span> | ||
| <button type='button'class="action action-more" id="actions-dropdown-link-1" aria-haspopup="true" aria-expanded="false" data-course-number="CTB3365DWx" data-course-name="Introduction to Drinking Water Treatment" data-dashboard-index="1"> |
There was a problem hiding this comment.
we should add aria-controls attribute here that references the actions-dropdown-# div?
| <div class="wrapper-action-more" data-course-key="${enrollment.course_id}"> | ||
| <a href="#actions-dropdown-${dashboard_index}" class="action action-more" id="actions-dropdown-link-${dashboard_index}" aria-haspopup="true" aria-expanded="false" data-course-number="${course_overview.number | h}" data-course-name="${course_overview.display_name_with_default_escaped | h}" data-dashboard-index="${dashboard_index}"> | ||
| <span class="sr">${_('Course options dropdown')}</span> | ||
| <button type="button" class="action action-more" id="actions-dropdown-link-${dashboard_index}" aria-haspopup="true" aria-expanded="false" aria-controls="actions-dropdown-${dashboard_index}" data-course-number="${course_overview.number | h}" data-course-name="${course_overview.display_name_with_default_escaped | h}" data-dashboard-index="${dashboard_index}"> |
There was a problem hiding this comment.
@cptvitamin I've updated his to include an aria-controls attribute.
|
This tested great with a screen reader and keyboard. Nice work. The removal of that aria-label on the parent container fixed that issue we were seeing in VO. 👍 |
clytwynec
changed the title
|
@andy-armstrong Thanks for reviewing this PR. |
|
@clytwynec Before reviewing this, I was wondering if you'd considered using the dropdown that @AlasdairSwan wrote for Drupal that he also introduced into the UI Toolkit. The plan was for this to be used in the platform as soon as we complete the FedX upgrades to support the toolkit (mostly upgrading old JS libraries). My preference would be to take what you've learned here and apply it (if necessary) to Alasdair's work in the UI Toolkit. We'll then prioritize switching the dropdown over as soon as the platform is using the UI Toolkit. FYI, here's Alasdair's implementation: https://github.com/edx/edx-ui-toolkit/tree/master/src/js/dropdown-menu |
|
@andy-armstrong What is the timeline for the UI Toolkit in platform? Is that the same dropdown currently implemented on the home page (top right corner when logged in)? |
|
@clytwynec it is the same dropdown as implemented on the marketing site top right corner. |
|
@clytwynec We are about two to three sprints away from landing the UI Toolkit in platform, so hopefully early April. The big challenge is in upgrading all the JavaScript libraries which are unbelievably outdated (over three years old, in some cases). |
|
@andy-armstrong @AlasdairSwan I'm happy to go with the implementation in the UI toolkit when it gets implemented in the platform. I can take a look at that implementation to see if there are any fixes in this PR that should be addressed there. Happy to make a PR if to the toolkit if needed, or open a JIRA ticket if that seems more appropriate to you. Does that seem ok with you @cptvitamin? There are other changes in this PR that will still need to get pushed through (e.g. a --> button, and proper labeling of said button), but I could hold off on the javascript changes. |
|
@andy-armstrong is there any harm in having @clytwynec merge her changes now (the work is already done) and then having her apply what she learned and improved here to the UI Toolkit? Once the Toolkit is in platform it should be relatively easy to switch this control to use the pattern. I just don't see the point of holding this work up if it's already complete (and a cursory review results in no obvious red flags). |
|
@cptvitamin Merging it as is makes sense to me. My original point was just that the UI Toolkit version exists and I wondered if it had been considered. @clytwynec Nice work! 👍 One minor concern is that we're having difficulty with focus-related Jasmine tests as we upgrade to the new version of JQuery. We may find that they become flaky and will have to be removed. FYI @cahrens. |
|
@andy-armstrong Thanks for taking a look. I'll take another look at the toolkit version too and make sure anything here that may need to be updated gets updated (see AC-361). |
| verifyDropdownNotVisible = function() { | ||
| expect($(dropdownSelector)).not.toBeVisible(); | ||
| }, | ||
| waitForElementToBeFocused = function(element, desc) { |
There was a problem hiding this comment.
@andy-armstrong @cahrens Are the JS tests related to focus that are flaky using any sort of waitsFor method like this? I know when I tried to use the expect().toBeFocused method, it was flaky.
Improve accessibility for dashboard course settings dropdown
6 participants
Overview
There was an a11y test failure for the course setting dropdowns on the dashboard page (see AC-238).
After looking closer, the reason the test fails is because the DIV is hidden at the time the test runs. It is only shown (and made focusable) after the link it clicked. Because this isn't an issue with the actual implementation, and instead and issue with the timing of the test, I've updated the test to explicitly ignore this element for the audit, rather than ignoring the entire rule. Once AC-179 is done, the link-href rule should be enabled to ensure that skip-links on these pages work.legacy.js(which is not intended to be developed further), I moved the Javascript to it's own module and added tests.Sandbox
https://clytwynec-ac-238.sandbox.edx.org/
Reviewers
@clrux @cptvitamin