From 25bb394378ad3290871d8eefa6f7a97a67fbd8c6 Mon Sep 17 00:00:00 2001
From: Guillem Arias Fauste <accounts@gariasf.com>
Date: Wed, 20 May 2026 18:19:58 +0200
Subject: [PATCH] =?UTF-8?q?fix(design-system):=20DS::Select=20a11y=20?=
 =?UTF-8?q?=E2=80=94=20fix=20aria-expanded,=20listbox=20keyboard=20nav,=20?=
 =?UTF-8?q?label=20binding=20(#1848)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* fix(design-system): DS::Select a11y — fix aria-expanded, listbox keyboard nav, label binding

Closes #1744. Several concrete bugs from the savings-goals audit:

1. **`aria-expanded` wired to the wrong state.** The template had
   `aria-expanded="<%= @selected_value.present? ? "true" : "false" %>"`,
   which is "has a value been chosen", not "is the menu open".
   AT users heard a misleading signal on every page load.
   Init to `"false"`; the Stimulus controller's openMenu/close
   already correctly maintains the attribute after that.

2. **`aria-labelledby` referenced a nonexistent id.** The trigger
   pointed at `"#{method}_label"`, but the rendered `<label>` had
   no id at all — the binding silently failed. Add
   `id: "#{method}_label"` to `form.label` so the reference
   actually resolves to the label text. Only emit
   `aria-labelledby` when there *is* a visible label.

3. **`tabindex="0"` on every option.** Listbox options should use
   roving tabindex (only the selected option is in tab order; the
   rest are reachable via ArrowUp/Down). Set
   `tabindex="0"` on the selected option only; `"-1"` on the rest.
   The select controller's `select()` handler keeps the roving
   invariant on user interaction.

4. **No keyboard navigation between options.** Add ArrowDown/Up
   (cycle), Home (first), End (last). The existing Enter/Escape
   handlers stay. ArrowUp/Down inside the search input is left
   alone so the input's caret behavior isn't hijacked.

5. **Search input had no accessible name.** Add an explicit
   `aria-label` matching the placeholder copy so AT users hear
   "search" when focus enters the field.

API unchanged. Builder-level routing fix in
`StyledFormBuilder#select` (calling DS::Select for `f.select(...)`
the same way `f.collection_select` already does) is intentionally
out of scope — it's a separate translation pass for the choices
format. Documented as a follow-up.

* fix(review): bridge search input to visible options in DS::Select

ArrowDown/Up from the search input now focus the first/last visible
option, and keyboard navigation operates on visible options only. After
typing a search query, the controller promotes the first visible option
to tabindex="0" so Tab can land on it even when the previously
tab-eligible option is filtered out.

Addresses Codex review on PR #1848 (issue #1744).

* fix(review): include trigger in DS::Select aria-labelledby

Codex P2 follow-up on #1848: \`aria-labelledby=\"#{method}_label\"\`
makes the trigger button's accessible name come solely from the
external form label — that overrides the button's own text node
(\`selected_item[:label]\` / placeholder). Screen readers therefore
announce only "Currency" without ever hearing the selected "USD"
unless the user opens the listbox.

Give the trigger \`id=\"#{method}_trigger\"\` and reference both ids:
\`aria-labelledby=\"#{method}_label #{method}_trigger\"\`. The
accessible-name algorithm concatenates the two, so AT users now
hear \"<Label> <selected value>\" while \`aria-expanded\` /
\`aria-haspopup\` continue to convey the dropdown state.
---
 app/components/DS/select.html.erb             | 24 +++++--
 .../controllers/select_controller.js          | 66 ++++++++++++++++++-
 2 files changed, 82 insertions(+), 8 deletions(-)

diff --git a/app/components/DS/select.html.erb b/app/components/DS/select.html.erb
index 8f1ffe3e4..6c06d83f1 100644
--- a/app/components/DS/select.html.erb
+++ b/app/components/DS/select.html.erb
@@ -3,7 +3,7 @@
 <div class="relative" data-controller="select <%= "list-filter" if searchable %> form-dropdown" data-select-menu-placement-value="<%= menu_placement %>" data-action="dropdown:select->form-dropdown#onSelect">
   <div class="form-field <%= options[:container_class] %>">
     <div class="form-field__body">
-        <%= form.label method, options[:label], class: "form-field__label" if options[:label].present? %>
+        <%= form.label method, options[:label], class: "form-field__label", id: "#{method}_label" if options[:label].present? %>
         <%= form.hidden_field method,
             value: @selected_value,
             data: {
@@ -11,13 +11,22 @@
               "auto-submit-target": "auto",
               **(options.dig(:html_options, :data) || {})
             } %>
+        <%# `aria-expanded` reflects MENU open/closed state — managed by the
+            select controller's openMenu/close. Init as "false"; previously
+            this incorrectly mirrored whether a value was selected.
+
+            `aria-labelledby` points at BOTH the visible label and the
+            trigger button itself so AT users hear "<label> <selected
+            value>" — referencing only the label would override the
+            button's text node and suppress the current value. %>
         <button type="button"
+                id="<%= method %>_trigger"
                 class="form-field__input w-full"
                 data-select-target="button"
                 data-action="click->select#toggle"
                 aria-haspopup="listbox"
-                aria-expanded="<%= @selected_value.present? ? "true" : "false" %>"
-                aria-labelledby="<%= "#{method}_label" %>">
+                aria-expanded="false"
+                <%= "aria-labelledby=\"#{method}_label #{method}_trigger\"".html_safe if options[:label].present? %>>
           <%= selected_item&.dig(:label) || @placeholder %>
         </button>
     </div>
@@ -28,9 +37,10 @@
         <input type="search"
                placeholder="<%= t("helpers.select.search_placeholder") %>"
                autocomplete="off"
+               aria-label="<%= t("helpers.select.search_placeholder") %>"
                class="bg-container text-primary text-sm placeholder:text-secondary font-normal h-10 pl-10 w-full border-none rounded-lg focus:outline-hidden focus:ring-0"
                data-list-filter-target="input"
-               data-action="list-filter#filter">
+               data-action="input->list-filter#filter input->select#syncTabindex">
         <%= helpers.icon("search", class: "absolute inset-0 ml-2 transform top-1/2 -translate-y-1/2") %>
       </div>
     <% end %>
@@ -40,10 +50,14 @@
         <% is_selected = item[:value] == selected_value %>
         <% obj = item[:object] %>
 
+        <%# Roving tabindex: selected option is in tab order (`0`); others
+            are reachable only via ArrowUp/Down (`-1`). WAI-ARIA APG
+            listbox keyboard pattern. %>
         <div class="filterable-item text-primary text-sm cursor-pointer flex items-center gap-2 px-3 py-2 rounded-lg hover:bg-container-inset-hover <%= "bg-container-inset" if is_selected %>"
               role="option"
-              tabindex="0"
+              tabindex="<%= is_selected ? "0" : "-1" %>"
               aria-selected="<%= is_selected %>"
+              data-select-target="option"
               data-action="click->select#select"
               data-value="<%= item[:value] %>"
               data-filter-name="<%= item[:label] %>">
diff --git a/app/javascript/controllers/select_controller.js b/app/javascript/controllers/select_controller.js
index e4b07bb3b..19b44bb64 100644
--- a/app/javascript/controllers/select_controller.js
+++ b/app/javascript/controllers/select_controller.js
@@ -2,7 +2,7 @@ import { Controller } from "@hotwired/stimulus"
 import { autoUpdate } from "@floating-ui/dom"
 
 export default class extends Controller {
-  static targets = ["button", "menu", "input", "content"]
+  static targets = ["button", "menu", "input", "content", "option"]
   static values = {
     menuPlacement: { type: String, default: "auto" },
     offset: { type: Number, default: 6 }
@@ -70,12 +70,14 @@ export default class extends Controller {
     const previousSelected = this.menuTarget.querySelector("[aria-selected='true']")
     if (previousSelected) {
       previousSelected.setAttribute("aria-selected", "false")
+      previousSelected.setAttribute("tabindex", "-1")
       previousSelected.classList.remove("bg-container-inset")
       const prevIcon = previousSelected.querySelector(".check-icon")
       if (prevIcon) prevIcon.classList.add("hidden")
     }
 
     selectedElement.setAttribute("aria-selected", "true")
+    selectedElement.setAttribute("tabindex", "0")
     selectedElement.classList.add("bg-container-inset")
     const selectedIcon = selectedElement.querySelector(".check-icon")
     if (selectedIcon) selectedIcon.classList.remove("hidden")
@@ -130,8 +132,66 @@ export default class extends Controller {
 
   handleKeydown(event) {
     if (!this.isOpen) return
-    if (event.key === "Escape") { this.close(); this.buttonTarget.focus() }
-    if (event.key === "Enter" && event.target.dataset.value) { event.preventDefault(); event.target.click() }
+    if (event.key === "Escape") { this.close(); this.buttonTarget.focus(); return }
+    if (event.key === "Enter" && event.target.dataset.value) { event.preventDefault(); event.target.click(); return }
+
+    // WAI-ARIA APG listbox keyboard pattern: ArrowUp/Down moves focus
+    // between options (roving tabindex), Home/End jump to first/last.
+    // From the search input, ArrowDown/Up bridge into the visible
+    // options so users can reach the filtered matches; other keys
+    // (typing, caret movement) stay with the input.
+    const fromSearch = event.target.matches('input[type="search"]')
+    const visibleOptions = this.visibleOptions()
+    if (fromSearch) {
+      if (event.key !== "ArrowDown" && event.key !== "ArrowUp") return
+      if (visibleOptions.length === 0) return
+      event.preventDefault()
+      const targetIndex = event.key === "ArrowDown" ? 0 : visibleOptions.length - 1
+      this.rovingFocus(visibleOptions, targetIndex)
+      return
+    }
+
+    if (visibleOptions.length === 0) return
+    const currentIndex = visibleOptions.indexOf(event.target)
+    let nextIndex = null
+    switch (event.key) {
+      case "ArrowDown": nextIndex = currentIndex < 0 ? 0 : (currentIndex + 1) % visibleOptions.length; break
+      case "ArrowUp": nextIndex = currentIndex < 0 ? visibleOptions.length - 1 : (currentIndex - 1 + visibleOptions.length) % visibleOptions.length; break
+      case "Home": nextIndex = 0; break
+      case "End": nextIndex = visibleOptions.length - 1; break
+      default: return
+    }
+    event.preventDefault()
+    this.rovingFocus(visibleOptions, nextIndex)
+  }
+
+  // Roving tabindex helper: makes the target option tabbable (and
+  // focuses it), clears tabindex on every other option in the listbox.
+  rovingFocus(visibleOptions, index) {
+    const all = this.hasOptionTarget ? this.optionTargets : []
+    const target = visibleOptions[index]
+    all.forEach(opt => opt.setAttribute("tabindex", opt === target ? "0" : "-1"))
+    target.focus()
+  }
+
+  // Options the user can currently see — list-filter hides non-matches
+  // by setting `style.display = "none"`. Inline check keeps it cheap.
+  visibleOptions() {
+    const options = this.hasOptionTarget ? this.optionTargets : []
+    return options.filter(opt => opt.style.display !== "none")
+  }
+
+  // After list-filter#filter runs, the option holding tabindex="0" may
+  // be hidden. Promote the first visible option so Tab from the search
+  // input still lands somewhere reachable; if none match, no-op.
+  syncTabindex() {
+    const visible = this.visibleOptions()
+    if (visible.length === 0) return
+    const tabbable = visible.find(opt => opt.getAttribute("tabindex") === "0")
+    if (tabbable) return
+    const all = this.hasOptionTarget ? this.optionTargets : []
+    all.forEach(opt => opt.setAttribute("tabindex", "-1"))
+    visible[0].setAttribute("tabindex", "0")
   }
 
   handleTurboLoad() { if (this.isOpen) this.close() }