[2.x] fix: tell assistive technology which dropdown option is selected - #4900
Merged
Conversation
A `SelectDropdown` showed which option was in effect two ways, neither of which reaches a screen reader: a class on the item, and the toggle button borrowing that option's label. Moving through the menu announced an undifferentiated list of options. The selected item now carries `aria-current="true"`. `aria-current` rather than the `aria-selected` the issue asks for: that attribute is only valid on roles this menu does not claim — `option`, `tab`, `row`, `gridcell`, `treeitem` — and the menu is a plain `ul` of `li` wrapping buttons, with no role declared. Claiming those roles would mean honouring the whole listbox keyboard contract, which is a much larger piece of work and belongs with the keyboard-navigation issues. `aria-current` is valid on any element and says what is meant: the current item within a set. Unselected items are left without the attribute rather than given `"false"`. Only one item can be current, so its absence carries the meaning — and Mithril omits an attribute set to `false` anyway. Scoped to `SelectDropdown` rather than `listItems`, which would have been the tidier place since it already works out which item is active. That helper wraps every list in Flarum, and the only component implementing `isActive` is `LinkButton`, which is route-based — nav items want `aria-current="page"`, so marking them `"true"` would be wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3362.
A
SelectDropdownshows which option is in effect two ways — a class on the item, and the toggle button borrowing that option's label. Neither reaches a screen reader, so moving through the menu announces an undifferentiated list of options with no indication of which one is active.The selected item now carries
aria-current="true".Why
aria-currentand notaria-selectedThe issue asks for
aria-selected, but that attribute is only valid on elements with roleoption,tab,row,gridcellortreeitem. The menu is a plain<ul>of<li>wrapping buttons:with no role declared in
Dropdown,SelectDropdownorSelect. Addingaria-selectedthere would be ignored by assistive tech at best, and flagged as invalid by axe/Lighthouse at worst.Giving the menu
role="listbox"and its itemsrole="option"would make it valid, but that is a contract — it commits us to arrow-key navigation andaria-activedescendant, which is a much bigger piece of work and belongs with #3361 and the rest of the keyboard-navigation issues.aria-currentis valid on any element and states what is actually meant: the current item within a set.Unselected items are left without the attribute rather than given
"false". Only one item can be current, so the absence carries the meaning — and Mithril omits an attribute set tofalseregardless.Scoping
The tidier place would have been
listItems, which already works out which item is active and adds theactiveclass. It is scoped toSelectDropdowninstead because that helper wraps every list in Flarum, and the only component implementingisActiveisLinkButton— which is route-based. Nav items are genuinely navigation and wantaria-current="page", so marking them"true"would be wrong.Tests
Three added to the existing
SelectDropdown.test.ts:[aria-current="true"], and it is the right oneI checked the first two fail without the change rather than assuming it.
All integration suites green: 49 suites, 162 tests.
check-typingsclean.To try it
Open a select dropdown — the sort dropdown on the discussion index is the clearest — and inspect the menu. The current option's
<li>should carryaria-current="true"and the others should have no such attribute.