Add missing tabindex and keyboard functions#311
Conversation
| return html` | ||
| <div style="display:flex; justify-content:flex-end; padding-right: 1rem; font-size: 14px; margin-top: 16px;"> | ||
| <span @click="${(e) => expandCollapseAll.call(this, e, true)}" style="color:var(--primary-color); cursor: pointer;">Expand</span> | ||
| <span @click="${(e) => expandCollapseAll.call(this, e, true)}" style="color:var(--primary-color); cursor: pointer;" role="button" tabindex="0" @keydown="${(e) => { if (e.key === 'Enter') { e.target.click(); }}}">Expand</span> |
There was a problem hiding this comment.
I'm looking at this and wondering if it wouldn't make more sense to change this to a button and the other one to a link, that feels like a better fix, and we'll have to adjust the css to account for that. The PR description says there are "a lot of these", but I only count 4 in this PR. If there are only four we can inline the css, otherwise we'll likely want to a new css class to handle this stuff.
There was a problem hiding this comment.
I'm looking at this and wondering if it wouldn't make more sense to change this to a button and the other one to a link, that feels like a better fix
That would be ideal, and much cleaner in my opinion!
The PR description says there are "a lot of these", but I only count 4 in this PR.
There are others where much more extensive changes (usually around aria-expanded and aria-controls) are needed to the same element, and I was hoping to the keyboard navigation AND other needed changes to those together in separate PRs, to prevent merge conflicts.
There was a problem hiding this comment.
It's hard for me to understand what that would entail to give advice. Is there such a thing as a perfectly configurable accessible button that then we can go and put everywhere? Or are there complex changes needed for each button? For me it mainly feels like a styling problem, and aria label application, is there something else too? Do you have some examples we can iterate on to figure out the complexity? You've looked at this already, what are you thinking exactly?
| ? html`<h1 style="display: inline; align-self: center" class="title tag-link" role="heading" aria-level="1" data-content-id="${tag.elementId}" | ||
| @click="${(e) => this.scrollToEventTarget(e, false)}"> ${tag?.name} ${tagSummary}</h1>` | ||
| ? html`<h1 style="display: inline; align-self: center" class="title" data-content-id="${tag.elementId}" | ||
| ><span class="tag-link" role="link" tabindex="0" @click="${(e) => this.scrollToEventTarget(e, false)}" @keydown="${(e) => { if (e.key === 'Enter') { e.target.click(); }}}"> ${tag?.name} ${tagSummary}</span></h1>` |
There was a problem hiding this comment.
What is using tag-link for, and is it safe to mov elike that?
wparad
left a comment
wparad
left a comment
There was a problem hiding this comment.
This is a much better PR, I think a learning here is that we should opt for transitioning to the available html tags that already do what we need, rather than trying to fix the hacked elements that are present. Unless there is a core reason we can't do that, it feels much better than attempting to hack in a pseudo implementation of the actual html tag we are trying to emulate.
I would absolutely be happy to do that, it's really the preferred solution. I had just assumed things were set up this way on purpose; that they were not set up using native elements for some specific reason. But using The links can be a bit weird, since they do not all have href equivalents and most do not actually link to a new web addresses (though some change the window url history/state). But I can move as many of those as seem sensible to How would you want this broken up to make the PRs a reasonable size for you? Like, should I transition a bunch of button equivalents to buttons in 1 PR, or would that be overwhelming? If you would like it done over multiple PRs, how should those be divided? |
|
@ahamelers size is less important than "types of changes", 1000 changes that all make the exact same change is infinitely easier to review as compared to a PR that changes two lines of code for completely unrelated reasons. Having a PR "fix accessibility of all buttons" makes sense to me so long as they are all the exact same change. If we need to start handling some of the buttons differently then each "type" of button change likely needs its own PR, but I don't expect we are there yet. That being said, I only see two button related role changes in this PR, so it's a moot point right? Or are there other button changes you would make that follow the exact same pattern? And lastly, don't assume how things are being done here, were done correctly. They surely have value in the way they are done, (Chesterton's Fence), but don't necessarily need to stay that way to achieve the best possible end goal. If there is no downside to switching out a component, let's do it. For me, switches to => |
Yes there are many button changes I would make that would follow the same pattern, or the same pattern plus aria-expanded settings (for all the buttons associated with disclosure elements). If given leave to change a bunch of interactive elements to native HTML interactive elements, I can do that in many places without breaking functionality or style. I am going to close this PR and start with the next simplest change I've got, while I work on the button change, since you've requested 1 PR at a time |
2 participants
Currently, there are a lot of interactive links that are inaccessible by keyboard and screen reader.
Any interactive/clickable elements that do not use default interactive HTML elements (a, button) need to have tabindex set so they can be navigated to via the keyboard, and keydown functionality set so they can also be used via the keyboard. Setting an aria role for these interactive elements is also good for accessibility.
This PR sets the role, tabindex, and keydown functionality for the simplest elements these are needed for.
In addition, an element cannot have more than 1 aria role, so one change involves separating a heading and a link element.