Fixes accessibility issues with accordion #1176
Conversation
el-mapache
force-pushed
the
ab-accordion-accessibility
branch
from
0ffc132
to
52a176e
Compare
**Why**: previous implmentation wasn't using aria attributes correctly
el-mapache
force-pushed
the
ab-accordion-accessibility
branch
from
52a176e
to
625d6b1
Compare
|
lgtm, but maybe @hursey013 should take a look too. |
There was a problem hiding this comment.
In order to make the accordion keyboard accessible I think we'll want to have a tabindex="0" on the element that is acting as the button. We'll probably need to modify the JS a bit to allow a the keyboard to expand/collapse it as well because I think only a mouse click will work now.
|
@hursey013 I added a keyup event binding. I'm going to need to refactor how this works a bit so we can remove the duplicated logic in |
**Why**: WIA-ARIA Spec requires that an accordion component respond to `enter` and `space` keys
el-mapache
force-pushed
the
ab-accordion-accessibility
branch
from
1224d8a
to
4c7160a
Compare
|
@el-mapache Just tested it, works great! Final nitpick, would be nice to have Let me know if I can help with |
|
@hursey013 So I don't know a lot about accessibility, is making the close link behave the same way just good practice? It seems like the close link is basically not needed in this case, since the user will remain focused on the heading element while the accordion is open. This is probably very wrong wrt to how screen readers actually operate 😬 If the answer is just 'more is better' that makes sense to me too! |
|
I'm definitely in the middle of learning more about accessibility, but one of the criteria we need to achieve is: "All functionality of the content is operable through a keyboard interface". So in the case of someone that cannot use a mouse, the expected behavior is that they will be able to tab to all links or interactive elements on the page, and each element will also have some sort of visual indication that it is currently in focus. https://www.w3.org/TR/UNDERSTANDING-WCAG20/keyboard-operation.html https://www.w3.org/TR/UNDERSTANDING-WCAG20/navigation-mechanisms-focus-visible.html |
|
Yep, totally understand that some level of keyboard interaction is necessary for accessibility. Specifically, the page I was working off of (https://www.w3.org/TR/wai-aria-practices/#aria_ex) for this component specifies that only I guess my question is, do we ever stop and evaluate when to add things like a |
|
I think in this case since it appears visually to be a link it makes sense to have it behave like a link, but yeah I agree I don't think we should overdo it because that will actually have the opposite effect on accessibility. Sticking with semantic HTML will do most of the heavy lifting for us, I really just see us needing to pay extra attention to a few of our unique components like the accordions, modals and tooltips. |
|
That makes perfect sense, thanks! |
el-mapache
force-pushed
the
ab-accordion-accessibility
branch
6 times, most recently
from
0a2ffba
to
c1122fc
Compare
|
@hursey013 seems like all the CC/rspec errors have been accounted for, if you wouldn't mind taking another look. |
| } | ||
|
|
||
| close() { | ||
| this.el.setAttribute('aria-expanded', 'false'); | ||
| this.content.setAttribute('aria-hidden', 'true'); | ||
| this.setExpanded(false); |
There was a problem hiding this comment.
Would it be ok if we add this.headerControl.focus(); within this close function so that if someone closes with the bottom close link, the focus outline gets reapplied to the header?
Like this:
There was a problem hiding this comment.
yep, forgot about that.
There was a problem hiding this comment.
@hursey013 focused!
**Why**: We were duplicating some logic in a file consuming the accordion. Extra tabindicies mean extra accessibility
el-mapache
force-pushed
the
ab-accordion-accessibility
branch
from
c1122fc
to
d6330b1
Compare
Why: previous implmentation wasn't using aria attributes correctly