From e30ccd94afe51f96a7b223f247e142bff1770cf8 Mon Sep 17 00:00:00 2001
From: Guillem Arias Fauste <accounts@gariasf.com>
Date: Wed, 20 May 2026 18:17:51 +0200
Subject: [PATCH] =?UTF-8?q?fix(design-system):=20DS::Tooltip=20a11y=20?=
 =?UTF-8?q?=E2=80=94=20focusable=20trigger,=20keyboard=20parity,=20Esc=20d?=
 =?UTF-8?q?ismiss=20(#1845)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* fix(design-system): DS::Tooltip a11y — focusable trigger, keyboard parity, Esc dismiss

Closes #1747. Five fixes on the tooltip primitive.

1. **Tooltip anchor not in a11y tree.** The trigger was a bare
   Lucide icon, which Lucide renders with `aria-hidden="true"`.
   The tooltip target had `role="tooltip"` but nothing referenced
   it, so AT users had no way to discover the description. Wrap
   the icon in a focusable `<button type="button">` with
   `aria-describedby="<tooltip-id>"` so the underlying icon stays
   `aria-hidden` and the button picks up the description binding.

2. **Stable per-instance id.** Each DS::Tooltip now mints a
   `tooltip-<8-char hex>` id wired between the trigger's
   `aria-describedby` and the tooltip's `id`.

3. **Keyboard parity.** Hover-only triggers locked keyboard-only
   users out. Add `focusin` / `focusout` listeners on the
   controller element so Tab onto the trigger reveals the
   tooltip, Tab away dismisses it.

4. **Esc-to-dismiss.** Matches the WAI-ARIA tooltip pattern.
   `Escape` while the tooltip is open closes it without removing
   focus from the trigger.

5. **Resize-safe width cap.** Replace the hard-coded
   `max-w-[200px]` with `max-w-[20rem]` so the tooltip scales
   with the user's root font-size setting (large-text accessibility
   pref). Slightly wider visual cap (320px @ default) but no longer
   clips on text-zoom.

Plus: docstring note that tooltip content must be non-interactive
(no buttons / links / form controls inside) — `aria-describedby`
exposes content as a description, not as an interactive subtree.
Callers needing actions should reach for a popover/menu primitive.

API unchanged. Existing 30+ DS::Tooltip callsites work without
modification — they all pass `text:`-only payloads, which still
render correctly under the new markup.

* fix(review): as: option + alpha focus-ring on DS::Tooltip

Addresses two AI review findings on #1845:

1. **Button-inside-summary spec violation.** Wrapping the icon in
   `<button>` regressed keyboard/AT behavior at 13 callsites where
   DS::Tooltip lives inside a `<summary>` (8 provider items, lunchflow
   disclosure, activity_date, 4 simplefin badges). HTML's content
   model forbids interactive content inside `<summary>`; browsers
   and AT can drop focus or conflate activation with the disclosure
   toggle. Add `as:` parameter — default `:button` preserves the
   standalone a11y wrap; `:span` renders a non-focusable wrapper for
   summary-nested usage. `focusin` bubbles up to the controller from
   the ancestor `<summary>`, so keyboard tooltips still appear on
   tab. Migrate the 13 in-summary callsites to `as: :span`.

2. **Raw palette focus ring → alpha tokens.** Swap
   `outline-gray-900 theme-dark:focus-visible:outline-white` to the
   established focus-ring pattern `focus-visible:ring-2
   focus-visible:ring-alpha-black-300
   theme-dark:focus-visible:ring-alpha-white-300` — matches the
   DS::Toggle fix landed in #1843 review and provider_card /
   form-field tokens.

* fix(review): bind tooltip focus on ancestor <summary>

Codex P2 follow-up on #1845: \`as: :span\` renders a non-focusable
trigger inside the disclosure \`<summary>\`. Keyboard users hit Tab
and focus lands on the summary itself; \`focusin\` fires on the
summary and bubbles UP — never down to a descendant span — so the
existing listener on \`this.element\` never fires and the tooltip
stays hidden for keyboard-only users on every in-summary row
(provider _item partials, lunchflow disclosure, activity_date,
simplefin badges). My earlier reply that the focusin "bubbles up to
the Stimulus controller on the outer span" was wrong about the
direction; \`focusin\` only bubbles upward.

In \`addEventListeners\`, resolve \`this.element.closest("summary")\`
and bind \`focusin\` / \`focusout\` / \`keydown\` on it too. Track the
ancestor on the controller and undo the bindings in
\`removeEventListeners\` so reconnect-on-Turbo cycles don't leak.
Update the template comment to reflect the actual mechanism.

* docs(ds-tooltip): correct as=:span comment to match controller mechanism

---------

Signed-off-by: Juan José Mata <juanjo.mata@gmail.com>
Co-authored-by: Juan José Mata <juanjo.mata@gmail.com>
---
 app/components/DS/tooltip.html.erb            | 37 ++++++++++++++++--
 app/components/DS/tooltip.rb                  | 25 +++++++++++-
 app/components/DS/tooltip_controller.js       | 39 +++++++++++++++++++
 .../UI/account/activity_date.html.erb         |  2 +-
 app/views/brex_items/_brex_item.html.erb      |  2 +-
 app/views/ibkr_items/_ibkr_item.html.erb      |  2 +-
 .../_indexa_capital_item.html.erb             |  2 +-
 .../lunchflow_items/_lunchflow_item.html.erb  |  2 +-
 .../mercury_items/_mercury_item.html.erb      |  2 +-
 .../simplefin_items/_simplefin_item.html.erb  |  9 +++--
 .../snaptrade_items/_snaptrade_item.html.erb  |  2 +-
 .../sophtron_items/_sophtron_item.html.erb    |  2 +-
 config/locales/views/components/en.yml        |  2 +
 .../previews/tooltip_component_preview.rb     |  6 ++-
 14 files changed, 115 insertions(+), 19 deletions(-)

diff --git a/app/components/DS/tooltip.html.erb b/app/components/DS/tooltip.html.erb
index a10c1147f..fe0c04366 100644
--- a/app/components/DS/tooltip.html.erb
+++ b/app/components/DS/tooltip.html.erb
@@ -1,8 +1,39 @@
 <span data-controller="DS--tooltip" data-DS--tooltip-placement-value="<%= placement %>" data-DS--tooltip-offset-value="<%= offset %>" data-DS--tooltip-cross-axis-value="<%= cross_axis %>" class="inline-flex">
-  <%= helpers.icon icon_name, size: size, color: color %>
+  <% if as == :button %>
+    <%# Wrap the trigger icon in a focusable `<button>` so AT users (and
+        keyboard-only users) can land on the tooltip anchor. The Lucide
+        icon itself carries `aria-hidden`, so the button's accessible
+        name comes from `aria-label`, and the tooltip text is exposed
+        via `aria-describedby`. %>
+    <button type="button"
+            class="inline-flex items-center cursor-default focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-alpha-black-300 theme-dark:focus-visible:ring-alpha-white-300 rounded"
+            aria-describedby="<%= tooltip_id %>"
+            aria-label="<%= t("ds.tooltip.trigger_label", default: "More info") %>">
+      <%= helpers.icon icon_name, size: size, color: color %>
+    </button>
+  <% else %>
+    <%# `as: :span` — used when the tooltip is rendered inside an
+        already-focusable interactive ancestor (e.g. `<summary>`),
+        where the HTML spec forbids nested interactive content.
+        Keyboard reveal is wired in the controller, which (in addition
+        to listening on the outer span for the standalone case) also
+        binds `focusin/focusout/keydown` on the closest `<summary>`
+        ancestor — because `focusin` only bubbles UP, a listener on
+        this descendant span would never fire when the ancestor
+        disclosure receives focus. %>
+    <span class="inline-flex items-center"
+          aria-describedby="<%= tooltip_id %>">
+      <%= helpers.icon icon_name, size: size, color: color %>
+    </span>
+  <% end %>
 
-  <div role="tooltip" data-DS--tooltip-target="tooltip" class="hidden absolute z-50 bg-inverse text-sm px-1.5 py-1 rounded-md">
-    <div class="text-inverse font-normal max-w-[200px]">
+  <div role="tooltip"
+       id="<%= tooltip_id %>"
+       data-DS--tooltip-target="tooltip"
+       class="hidden absolute z-50 bg-inverse text-sm px-1.5 py-1 rounded-md">
+    <%# `max-w-[20rem]` scales with the root font-size so AT users with
+        a larger base font don't see the tooltip clipped. %>
+    <div class="text-inverse font-normal max-w-[20rem]">
       <%= tooltip_content %>
     </div>
   </div>
diff --git a/app/components/DS/tooltip.rb b/app/components/DS/tooltip.rb
index 7189c4f35..757f77bcf 100644
--- a/app/components/DS/tooltip.rb
+++ b/app/components/DS/tooltip.rb
@@ -1,7 +1,26 @@
 class DS::Tooltip < ApplicationComponent
-  attr_reader :placement, :offset, :cross_axis, :icon_name, :size, :color
+  AS_OPTIONS = %i[button span].freeze
+
+  attr_reader :placement, :offset, :cross_axis, :icon_name, :size, :color, :tooltip_id, :as
+
+  # NOTE: tooltip content must be non-interactive — no buttons, links,
+  # or form controls inside. Tooltips are exposed via `aria-describedby`,
+  # which announces the content as a description but does not expose
+  # interactive descendants to AT. Use a popover/menu primitive when
+  # the surface needs to host actions.
+  #
+  # `as:` controls the trigger element.
+  #   :button (default) — renders `<button type="button">`, focusable on
+  #     its own. Use for tooltips placed in standalone, non-interactive
+  #     surrounding markup.
+  #   :span — renders `<span>` with no `tabindex`. Use when the tooltip
+  #     sits inside an already-focusable interactive ancestor (most
+  #     commonly `<summary>`, where the HTML spec forbids nested
+  #     interactive content). The ancestor's focus still triggers the
+  #     tooltip because `focusin` bubbles up to the Stimulus controller.
+  def initialize(text: nil, placement: "top", offset: 10, cross_axis: 0, icon: "info", size: "sm", color: "default", as: :button)
+    raise ArgumentError, "as: must be one of #{AS_OPTIONS.inspect}" unless AS_OPTIONS.include?(as)
 
-  def initialize(text: nil, placement: "top", offset: 10, cross_axis: 0, icon: "info", size: "sm", color: "default")
     @text = text
     @placement = placement
     @offset = offset
@@ -9,6 +28,8 @@ class DS::Tooltip < ApplicationComponent
     @icon_name = icon
     @size = size
     @color = color
+    @as = as
+    @tooltip_id = "tooltip-#{SecureRandom.hex(4)}"
   end
 
   def tooltip_content
diff --git a/app/components/DS/tooltip_controller.js b/app/components/DS/tooltip_controller.js
index 8f198baed..5f0a4b396 100644
--- a/app/components/DS/tooltip_controller.js
+++ b/app/components/DS/tooltip_controller.js
@@ -29,11 +29,44 @@ export default class extends Controller {
   addEventListeners() {
     this.element.addEventListener("mouseenter", this.show);
     this.element.addEventListener("mouseleave", this.hide);
+    // Keyboard parity: keyboard users hit the trigger via Tab + focus,
+    // not hover. Without these the tooltip never appears for them.
+    this.element.addEventListener("focusin", this.show);
+    this.element.addEventListener("focusout", this.hide);
+    // Esc-to-dismiss matches the WAI-ARIA Authoring Practices for the
+    // tooltip pattern.
+    this.element.addEventListener("keydown", this.handleKeydown);
+
+    // `as: :span` renders a non-focusable trigger inside an
+    // already-focusable ancestor (typically `<summary>`). When the
+    // ancestor receives keyboard focus the `focusin` event fires on
+    // *it* and bubbles UP to the document — it never reaches a
+    // descendant span. Without a listener on the ancestor itself,
+    // the tooltip stays hidden for keyboard users on in-summary rows.
+    // Bind the same handlers on the closest `<summary>` (if any) so
+    // focusing the disclosure reveals the tooltip and Esc still
+    // dismisses it.
+    this.summaryAncestor = this.element.closest("summary");
+    if (this.summaryAncestor) {
+      this.summaryAncestor.addEventListener("focusin", this.show);
+      this.summaryAncestor.addEventListener("focusout", this.hide);
+      this.summaryAncestor.addEventListener("keydown", this.handleKeydown);
+    }
   }
 
   removeEventListeners() {
     this.element.removeEventListener("mouseenter", this.show);
     this.element.removeEventListener("mouseleave", this.hide);
+    this.element.removeEventListener("focusin", this.show);
+    this.element.removeEventListener("focusout", this.hide);
+    this.element.removeEventListener("keydown", this.handleKeydown);
+
+    if (this.summaryAncestor) {
+      this.summaryAncestor.removeEventListener("focusin", this.show);
+      this.summaryAncestor.removeEventListener("focusout", this.hide);
+      this.summaryAncestor.removeEventListener("keydown", this.handleKeydown);
+      this.summaryAncestor = null;
+    }
   }
 
   show = () => {
@@ -47,6 +80,12 @@ export default class extends Controller {
     this.stopAutoUpdate();
   };
 
+  handleKeydown = (event) => {
+    if (event.key === "Escape" && !this.tooltipTarget.classList.contains("hidden")) {
+      this.hide();
+    }
+  };
+
   startAutoUpdate() {
     if (!this._cleanup) {
       const reference = this.element.querySelector("[data-icon]");
diff --git a/app/components/UI/account/activity_date.html.erb b/app/components/UI/account/activity_date.html.erb
index 6fd158d53..e788bfa4b 100644
--- a/app/components/UI/account/activity_date.html.erb
+++ b/app/components/UI/account/activity_date.html.erb
@@ -21,7 +21,7 @@
         <div class="flex items-center gap-4">
           <div class="flex items-center gap-2">
             <span class="font-medium privacy-sensitive"><%= end_balance_money.format %></span>
-            <%= render DS::Tooltip.new(text: t(".balance_tooltip"), placement: "left", size: "sm") %>
+            <%= render DS::Tooltip.new(text: t(".balance_tooltip"), placement: "left", size: "sm", as: :span) %>
           </div>
           <%= helpers.icon "chevron-down", class: "group-open:rotate-180" %>
         </div>
diff --git a/app/views/brex_items/_brex_item.html.erb b/app/views/brex_items/_brex_item.html.erb
index 4b97ceb84..150d267f7 100644
--- a/app/views/brex_items/_brex_item.html.erb
+++ b/app/views/brex_items/_brex_item.html.erb
@@ -46,7 +46,7 @@
             </div>
           <% elsif brex_item.sync_error.present? %>
             <div class="text-secondary flex items-center gap-1">
-              <%= render DS::Tooltip.new(text: brex_item.sync_error, icon: "alert-circle", size: "sm", color: "destructive") %>
+              <%= render DS::Tooltip.new(text: brex_item.sync_error, icon: "alert-circle", size: "sm", color: "destructive", as: :span) %>
               <%= tag.span t(".error"), class: "text-destructive" %>
             </div>
           <% else %>
diff --git a/app/views/ibkr_items/_ibkr_item.html.erb b/app/views/ibkr_items/_ibkr_item.html.erb
index 511550b5e..905eda0dd 100644
--- a/app/views/ibkr_items/_ibkr_item.html.erb
+++ b/app/views/ibkr_items/_ibkr_item.html.erb
@@ -32,7 +32,7 @@
             </div>
           <% elsif ibkr_item.sync_error.present? %>
             <div class="text-secondary flex items-center gap-1">
-              <%= render DS::Tooltip.new(text: ibkr_item.sync_error, icon: "alert-circle", size: "sm", color: "destructive") %>
+              <%= render DS::Tooltip.new(text: ibkr_item.sync_error, icon: "alert-circle", size: "sm", color: "destructive", as: :span) %>
               <%= tag.span t(".error"), class: "text-destructive" %>
             </div>
           <% else %>
diff --git a/app/views/indexa_capital_items/_indexa_capital_item.html.erb b/app/views/indexa_capital_items/_indexa_capital_item.html.erb
index e6f608e33..7f90af7bd 100644
--- a/app/views/indexa_capital_items/_indexa_capital_item.html.erb
+++ b/app/views/indexa_capital_items/_indexa_capital_item.html.erb
@@ -34,7 +34,7 @@
             </div>
           <% elsif indexa_capital_item.sync_error.present? %>
             <div class="text-secondary flex items-center gap-1">
-              <%= render DS::Tooltip.new(text: indexa_capital_item.sync_error, icon: "alert-circle", size: "sm", color: "destructive") %>
+              <%= render DS::Tooltip.new(text: indexa_capital_item.sync_error, icon: "alert-circle", size: "sm", color: "destructive", as: :span) %>
               <%= tag.span t(".error"), class: "text-destructive" %>
             </div>
           <% else %>
diff --git a/app/views/lunchflow_items/_lunchflow_item.html.erb b/app/views/lunchflow_items/_lunchflow_item.html.erb
index a1aec313b..0b0fecc76 100644
--- a/app/views/lunchflow_items/_lunchflow_item.html.erb
+++ b/app/views/lunchflow_items/_lunchflow_item.html.erb
@@ -35,7 +35,7 @@
             </div>
           <% elsif lunchflow_item.sync_error.present? %>
             <div class="text-secondary flex items-center gap-1">
-              <%= render DS::Tooltip.new(text: lunchflow_item.sync_error, icon: "alert-circle", size: "sm", color: "destructive") %>
+              <%= render DS::Tooltip.new(text: lunchflow_item.sync_error, icon: "alert-circle", size: "sm", color: "destructive", as: :span) %>
               <%= tag.span t(".error"), class: "text-destructive" %>
             </div>
           <% else %>
diff --git a/app/views/mercury_items/_mercury_item.html.erb b/app/views/mercury_items/_mercury_item.html.erb
index 0159551e8..6485da548 100644
--- a/app/views/mercury_items/_mercury_item.html.erb
+++ b/app/views/mercury_items/_mercury_item.html.erb
@@ -35,7 +35,7 @@
             </div>
           <% elsif mercury_item.sync_error.present? %>
             <div class="text-secondary flex items-center gap-1">
-              <%= render DS::Tooltip.new(text: mercury_item.sync_error, icon: "alert-circle", size: "sm", color: "destructive") %>
+              <%= render DS::Tooltip.new(text: mercury_item.sync_error, icon: "alert-circle", size: "sm", color: "destructive", as: :span) %>
               <%= tag.span t(".error"), class: "text-destructive" %>
             </div>
           <% else %>
diff --git a/app/views/simplefin_items/_simplefin_item.html.erb b/app/views/simplefin_items/_simplefin_item.html.erb
index c6e90e810..d09d9b202 100644
--- a/app/views/simplefin_items/_simplefin_item.html.erb
+++ b/app/views/simplefin_items/_simplefin_item.html.erb
@@ -52,7 +52,7 @@
             <% if has_warnings %>
               <div class="flex items-center gap-2 mt-1">
                 <% if stats["accounts_skipped"].to_i > 0 %>
-                  <%= render DS::Tooltip.new(text: t(".accounts_skipped_tooltip"), icon: "alert-triangle", size: "sm", color: "warning") %>
+                  <%= render DS::Tooltip.new(text: t(".accounts_skipped_tooltip"), icon: "alert-triangle", size: "sm", color: "warning", as: :span) %>
                   <span class="text-xs text-warning"><%= t(".accounts_skipped_label", count: stats["accounts_skipped"].to_i) %></span>
                 <% end %>
 
@@ -63,14 +63,15 @@
                         text: (ago ? t(".rate_limited_ago", time: ago) : t(".rate_limited_recently")),
                         icon: "clock",
                         size: "sm",
-                        color: "warning"
+                        color: "warning",
+                        as: :span
                       ) %>
                 <% end %>
 
                 <% if stats["total_errors"].to_i > 0 || (stats["errors"].is_a?(Array) && stats["errors"].any?) %>
                   <% tooltip_text = simplefin_error_tooltip(stats) %>
                   <% if tooltip_text.present? %>
-                    <%= render DS::Tooltip.new(text: tooltip_text, icon: "alert-octagon", size: "sm", color: "warning") %>
+                    <%= render DS::Tooltip.new(text: tooltip_text, icon: "alert-octagon", size: "sm", color: "warning", as: :span) %>
                   <% end %>
                 <% end %>
               </div>
@@ -119,7 +120,7 @@
             </div>
           <% elsif simplefin_item.sync_error.present? && !duplicate_only_errors %>
             <div class="text-secondary flex items-center gap-1">
-              <%= render DS::Tooltip.new(text: simplefin_item.sync_error, icon: "alert-circle", size: "sm", color: "destructive") %>
+              <%= render DS::Tooltip.new(text: simplefin_item.sync_error, icon: "alert-circle", size: "sm", color: "destructive", as: :span) %>
               <%= tag.span t(".error"), class: "text-destructive" %>
             </div>
           <% elsif duplicate_only_errors %>
diff --git a/app/views/snaptrade_items/_snaptrade_item.html.erb b/app/views/snaptrade_items/_snaptrade_item.html.erb
index f36712ec2..636d09dbe 100644
--- a/app/views/snaptrade_items/_snaptrade_item.html.erb
+++ b/app/views/snaptrade_items/_snaptrade_item.html.erb
@@ -42,7 +42,7 @@
             </div>
           <% elsif snaptrade_item.sync_error.present? %>
             <div class="text-secondary flex items-center gap-1">
-              <%= render DS::Tooltip.new(text: snaptrade_item.sync_error, icon: "alert-circle", size: "sm", color: "destructive") %>
+              <%= render DS::Tooltip.new(text: snaptrade_item.sync_error, icon: "alert-circle", size: "sm", color: "destructive", as: :span) %>
               <%= tag.span t(".error"), class: "text-destructive" %>
             </div>
           <% else %>
diff --git a/app/views/sophtron_items/_sophtron_item.html.erb b/app/views/sophtron_items/_sophtron_item.html.erb
index a8c71d900..d91450a86 100644
--- a/app/views/sophtron_items/_sophtron_item.html.erb
+++ b/app/views/sophtron_items/_sophtron_item.html.erb
@@ -41,7 +41,7 @@
             </div>
           <% elsif sophtron_item.sync_error.present? %>
             <div class="text-secondary flex items-center gap-1">
-              <%= render DS::Tooltip.new(text: sophtron_item.sync_error, icon: "alert-circle", size: "sm", color: "destructive") %>
+              <%= render DS::Tooltip.new(text: sophtron_item.sync_error, icon: "alert-circle", size: "sm", color: "destructive", as: :span) %>
               <%= tag.span t(".error"), class: "text-destructive" %>
             </div>
           <% else %>
diff --git a/config/locales/views/components/en.yml b/config/locales/views/components/en.yml
index 8c49e0461..11d896222 100644
--- a/config/locales/views/components/en.yml
+++ b/config/locales/views/components/en.yml
@@ -89,6 +89,8 @@ en:
       default_label: Preview
     dialog:
       close: Close
+    tooltip:
+      trigger_label: More info
     link:
       opens_in_new_tab: (opens in new tab)
   provider_sync_summary:
diff --git a/test/components/previews/tooltip_component_preview.rb b/test/components/previews/tooltip_component_preview.rb
index 68bd6c320..a6345ef0b 100644
--- a/test/components/previews/tooltip_component_preview.rb
+++ b/test/components/previews/tooltip_component_preview.rb
@@ -6,7 +6,8 @@ class TooltipComponentPreview < ViewComponent::Preview
   # @param icon text
   # @param size select [xs, sm, md, lg, xl, 2xl]
   # @param color select [default, white, success, warning, destructive, current]
-  def default(text: "This is helpful information", placement: "top", offset: 10, cross_axis: 0, icon: "info", size: "sm", color: "default")
+  # @param as select [button, span]
+  def default(text: "This is helpful information", placement: "top", offset: 10, cross_axis: 0, icon: "info", size: "sm", color: "default", as: "button")
     render DS::Tooltip.new(
       text: text,
       placement: placement,
@@ -14,7 +15,8 @@ class TooltipComponentPreview < ViewComponent::Preview
       cross_axis: cross_axis,
       icon: icon,
       size: size,
-      color: color
+      color: color,
+      as: as.to_sym
     )
   end