[2.x] fix: name the pane pin button and expose whether it is pinned - #4890
Merged
Conversation
The button that pins the discussion list carries only an icon, and unlike the back and drawer buttons either side of it, nothing that names it. A screen reader reaches it and can say no more than "button". Core already notices: Button warns in the console when it finds no content and no accessible label, and this button has been tripping that warning rather than being read as an omission. It is also a toggle, and its state was carried entirely by the icon rotating forty-five degrees when unpinned. Nothing about being pinned was exposed to assistive technology at all. The label names the control rather than the action, so it holds still while the state changes underneath it; `aria-pressed` reports whether it is currently on. Naming it for the action instead would mean a screen reader reading a button whose name changes under it, with the state never stated. `aria-pressed` is given as a string rather than a boolean on purpose. Mithril renders a boolean as an HTML boolean attribute, so `false` omits the attribute entirely and `true` renders it empty. Neither is valid ARIA, and an absent `aria-pressed` reads as a plain button rather than as a toggle that happens to be off. Core has no other use of the attribute, so there was no local precedent to follow here.
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.
The button that pins the discussion list is icon-only and has no accessible name, so a screen reader can announce it as nothing more than "button". The back and drawer buttons on either side of it in the same component both label themselves, so this is an omission rather than a house style.
Core already detects it.
Button.oncreatefires a debug warning when it finds a button with no content and no accessible label, and this button has been tripping that warning:It is also a toggle. Its state was carried entirely by CSS — the thumbtack rotates 45° when unpinned — so whether the list was pinned was never exposed to assistive technology.
Change
aria-labelfrom a newcore.lib.nav.pin_pane_buttonkey, sitting alongside the existingdrawer_button.aria-pressedreflectingpane.pinned.The label names the control ("Pin Discussion List") rather than the action, so it stays stable while the state changes beneath it. Labelling it for the action instead ("Pin" / "Unpin") would mean a screen reader reading a button whose name changes under the user, with the state only ever implied.
Why
aria-pressedis a stringPassing the boolean straight through does not work, and it fails silently. Mithril treats a boolean as an HTML boolean attribute:
truearia-pressed=""false"true"/"false"aria-pressed="true"/aria-pressed="false"ARIA needs the words. An empty value is invalid, and an absent
aria-pressedmakes the control read as a plain button rather than a toggle that happens to be off — so the boolean form is broken in both states. The first revision of this usedaria-pressed={pane.pinned}and rendered no attribute at all; it typechecked and built cleanly, and only showed up when the rendered DOM was inspected.There is no other use of
aria-pressedin core, so there was no existing precedent to match.Verified
Driven in a browser at a viewport wide enough for the pane, on a discussion page so the button renders:
Pin Discussion List, rolebuttonaria-pressedmovesfalse→true→falseacross two toggles, in step with theactiveclassNot in scope
Only this button. Since core uses
aria-pressednowhere else, other icon-only toggles may have the same gap — worth a sweep as separate work rather than widening this.