-
Notifications
You must be signed in to change notification settings - Fork 155
Path selector accessibility #4621
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Hmmm, seems like there should be more, but I'll check it out and report back. |
<%= form.text_field(attrib.id, class: 'form-control', **options) %> | ||
|
||
<button type="button" class="btn btn-primary" data-bs-toggle="modal" data-bs-target="#<%= path_selector_id %>"> | ||
<button type="button" class="btn btn-primary" data-bs-toggle="modal" data-bs-target="#<%= path_selector_id %>" aria-label="<%= I18n.t('dashboard.select_path_for', element: attrib.label) %>"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this may not be an improvement, I like the idea of relating it to the form item label, but this gets read out as "#{aria-label} button". So if I have a form item called "working directory", it reads as "Select path for form element working directory button" which might be confusing since it sounds like "working directory button" is the element (when working directory label should really go with the input not the button).
A quick test shows that aria-labelledby will make it read "working directory button", which is even worse. Maybe we just rearrange the message so that it reads "for form item working directory select path button"? I don't love that though and am very open to other solutions. Whatever we go with needs to be less confusing than "select path button" which was the previous behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can phrase it "Select path button for working directory" to avoid that while keeping a phrase that doesn't sound strange?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though that does have the screen reader not reading "button" last
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I also would prefer it didn't read 'button' last, just not sure if its possible given the label-type format I was noticing. Don't want to discourage you though if you have an idea to get that behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because "button" isn't actually in the string but is just the screen reader describing the element, it might actually be more clear to someone using a screenreader regularly, though I'm not certain about this.
|
||
<div class="<%= favorites ? 'col-sm-7' : 'col-sm-12' %>"> | ||
<ol id="<%= breadcrumb_id %>" class="breadcrumb breadcrumb-no-delimiter rounded"> | ||
<ol role="navigation" id="<%= breadcrumb_id %>" class="breadcrumb breadcrumb-no-delimiter rounded"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The navigation role doesn't seem to change anything with how this modal is read (it could still be helpful), but here are all the additional issues I ran into testing it out. When the modal opens the screenreader reads the modal title (but maybe this could have additional description as the modal title), then reads the close button, then the left column without issues. When it gets to the right column, it reads out the text in the breadcrumb but skips the icon buttons for home and back. The table and Close Select Path buttons get read out fine.
A bigger issue with the modal is that most of the important buttons cannot be tabfocused. When I open the modal my tabfocus starts at the close button, then skips to the filter (skipping the left column and the breadcrumb completely) then goes to the table headers, but skips the table content (where all the navigation happens) and goes straight to the close button.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just stumbled upon at least one of these issues,
id="<%= path_selector_id %>" tabindex="-1" role="dialog" | |
aria-labelledby="modal-path-selector" aria-hidden="true" | |
data-show-files="<%= show_files %>" |
we set aria-hidden=true
for the whole table, which blocks the focus from any child elements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The role="navigation"
prevents the items from being read twice in my testing, both as text and as a list item. Also, thanks for catching that the table was aria-hidden
!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, even though our table is filled with links, it might still be appropriate to allow for table navigation using arrow keys from the header. Non-interactive tables do this, such as the ones in these examples: https://www.w3.org/WAI/tutorials/tables/one-header/
I'd imagine that needing to press tab for every single row to get to the Select Path
button would be very tedious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know you could navigate a table via arrow keys, but that does sound easier than tabbing through the whole thing, so I would be more than fine with that
…d by screen reader
It still is not possible to select elements in the table with a keyboard. This requires |
<%= fa_icon((path.to_s == Dir.home ? 'home' : path.icon), classes: '') %> | ||
<%= path.title || path %> | ||
</li> | ||
<a href="#" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this should still be ol
and li
even if the internal is an a
.
To me, it makes sense to present it as a list of anchors, much like navbar entries, favorites in the files index and interactive apps in my interactive sessions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was mainly used to follow bootstrap's recommendations. If we use <div>
and <a>
for lists with interactive elements, list-group
handles accessibility for us without a bunch of aria-*
attributes.
Here is where they recommend to use these elements in list groups
I think I opened #4680 because of this. You can't click to select items either. Is that what you mean by 'select' or (selecting and closing the modal) or do you simply mean something more like choosing (highlighting then having to press 'select path')? |
I just mean choosing. There's a setting in datatables in a newer version, but that changes a lot of looks and functionality so I think it's best in its own issue. Doing this on our own results in some very strange code that will no longer work if we upgrade datatables. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with this.
@Bubballoo3 do you have more thoughts/change requests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Fixes #3153