From e07d641eadcdbd23b222308d392c3b05cfd4b6fc Mon Sep 17 00:00:00 2001 From: Guillem Arias Fauste Date: Wed, 20 May 2026 18:12:36 +0200 Subject: [PATCH] =?UTF-8?q?fix(design-system):=20DS::Button=20a11y=20audit?= =?UTF-8?q?=20=E2=80=94=20focus=20ring,=20touch=20target,=20type=20default?= =?UTF-8?q?,=20icon-only=20label=20(#1840)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(design-system): DS::Button a11y audit Closes #1738. Four concrete fixes surfaced by the savings-goals audit + #1737 universal checklist: 1. Focus ring (WCAG 2.4.7). `base.css` had `focus-visible:outline-gray-900` which is **1.07:1** against the primary button's gray-900 background — invisible. Widen to `outline-2 outline-offset-2`, place outline outside the button via offset, and add a dark-mode `outline-white` so the ring is always visible against the page chrome regardless of the button surface. 2. Touch target (WCAG 2.5.5). Icon-only buttons at the default `:md` size were `w-9 h-9` = 36×36, below the 44×44 enhanced target. Bump `md.icon_container_classes` to `w-11 h-11` and `lg.icon_container_classes` to `w-12 h-12` to keep the size scale intact. `sm` stays at 32×32 (already passes WCAG 2.5.8 AA's 24×24 minimum; intentional compact-density variant). 3. Default button type. `content_tag(:button, ...)` inherits the HTML default `type="submit"`, so a DS::Button rendered inside a form steals Enter-key submission from the first text input (reproducible in the form stepper). Default to `type="button"` in the non-`href` branch; existing form submitters pass `type: "submit"` explicitly and continue to work. The `button_to` (href) branch keeps the submit default because button_to wraps its own form. 4. Icon-only accessible name. Icon-only buttons render no text node, so AT users hear "button" with no name. Derive a humanized aria-label from the icon key (e.g. `icon: "more-horizontal"` → `aria-label="More horizontal"`); explicit `aria: { label: }` on the caller still wins. Soft fallback — callers should still pass meaningful labels for richer copy. Plus: replace the stale `fg-white` icon class on the destructive variant with `text-inverse` (the `fg-*` namespace was deprecated in #1626 so `fg-white` resolved to nothing; the icon was using its helper-default color rather than the white the design intended). Out of scope: - Menu avatar trigger (custom 36×36 button bypassing DS::Button) — belongs to #1743 DS::Menu audit. - DS::FilledIcon `lg` size container (decorative, not interactive) — belongs to #1742. * fix(design-system): force type=submit on StyledFormBuilder#submit The DS::Button default-type-button change in the previous commit broke every `form.submit "Log in"` callsite because `StyledFormBuilder#submit` (app/helpers/styled_form_builder.rb) renders a DS::Button under the hood with no explicit `type:`. After the default flip, those submit buttons rendered as `type="button"`, so submitting forms (login, password reset, every form using `form.submit`) silently no-ops. CI surfaced this via ~30 system tests failing in the `sign_in` helper, which couldn't get past the login page. Pin `type: "submit"` on the DS::Button rendered by `StyledFormBuilder#submit`. The 22 view-level `f.submit` / `render DS::Button.new(type: :submit, ...)` callers already pass type explicitly and are unaffected. * fix(review): href-branch type-button bug + focus-ring tokens + profile Save submit CodeRabbit P1+P2 review on #1840: 1. button.rb: `merged_opts.delete(:href)` always returned nil because Buttonish#initialize strips :href from opts into @href, so the `if href.blank?` guard was ALWAYS true. Every DS::Button rendered via button_to (the href branch) got `type="button"` on the inner button, breaking submission of those button_to-generated forms (e.g. imports/_ready.html.erb publish button, imports/_failure.html.erb try-again button). Drop the local `href = merged_opts.delete(:href)` so the guard now reads the @href reader, leaving the href branch's HTML default intact. 2. settings/profiles/show.html.erb: the Save button is rendered with `render DS::Button.new(...)` inside `styled_form_with` (not via form.submit), so the StyledFormBuilder#submit type-pin from 624e9794 doesn't cover it. Pass `type: :submit` explicitly so the profile form submits again under the default-type-button policy. 3. base.css: replace raw `outline-gray-900` / `outline-white` with the established alpha-ring focus pattern (focus-visible:ring-alpha-black-300 + theme-dark:ring-alpha-white-300) already used by app/components/settings/provider_card.html.erb and sure-design-system/components.css. Keeps a11y focus ring while using DS tokens. * fix(review): add type: :submit to DS::Button submitters inside forms CI test_system on #1840 surfaced 6 failures (confirm-dialog close, property create/edit, transaction filter apply) caused by the same gap that db563f3d started addressing: the default-type-button policy on DS::Button means every \`render DS::Button.new(...)\` inside a \`
\` (or \`styled_form_with\`) that relies on the HTML default to submit is now an inert \`type="button"\`. Audited every \`render DS::Button.new(\` callsite repo-wide for the combination (no \`type:\`, no \`href:\`, inside a form context) and pinned \`type: :submit\` explicitly on the 12 forms that need it: - layouts/shared/_confirm_dialog.html.erb: Confirm button inside the global \`\` — fixes test_should_allow_revoking_API_key_with_confirmation. - properties/{new,edit,balances}.html.erb: Save/Next submitter inside \`styled_form_with\` — fixes test_can_create_property_account, test_can_persist_property_subtype. - transactions/searches/_menu.html.erb: Apply inside the filter form — fixes test_can_filter_uncategorized_transactions, test_all_filters_work_and_empty_state_shows_if_no_match, test_can_open_filters_and_apply_one_or_more. - transactions/bulk_updates/new.html.erb: Save in bulk-edit drawer. - account_sharings/show.html.erb: Save in account-sharing form. - category/deletions/new.html.erb, tag/deletions/new.html.erb: destructive + safe submit buttons in deletion dialog forms. - family_merchants/merge.html.erb: Submit in merge form. - subscriptions/upgrade.html.erb: contribute_and_support_sure submit. - rules/_category_rule_cta.html.erb: Dismiss inside the rule_prompts_disabled form. Cancel/close DS::Button instances inside these same forms intentionally keep the \`type=button\` default since they drive JS-only actions (\`DS--dialog#close\`, \`DS--menu#close\`). * fix(review): add type: :submit to 4 remaining form-context DS::Button callers Second sweep for the same default-type-button regression that 24c517eb fixed for 12 callsites. The latest CI run on this branch narrowed the failures from 6 to 2 (the property wizard's Address step still failed because that view was not in the first sweep). Audited via a wider 4000-char form-context window: - app/views/properties/address.html.erb: Save inside styled_form_with — fixes the remaining test_can_create_property_account + test_can_persist_property_subtype by letting Step 3 of the property wizard complete. - app/views/onboardings/goals.html.erb: Submit inside form_with so the onboarding goals step submits. - app/views/account_sharings/show.html.erb (owner-side form): Save button for the family-share permissions form (the non-owner Save was already fixed in 24c517eb). - app/views/transactions/_attachments.html.erb: Upload inside styled_form_with — kept the JS-driven hook (attachment_upload_target) but explicit type:submit covers the no-JS fallback. * fix(review): pin type=submit on the Save currencies button Codex P1 (third pass) caught one more in-form DS::Button I missed in the earlier sweeps: \`app/views/settings/preferences/show.html.erb:185\` renders the Save currencies submit deep inside a long \`styled_form_with\` block. The form-context scan I used had a finite look-back window which missed it because the matching \`styled_form_with\` opener sits ~80 lines / 4k+ characters above the button. Switched to a whole-file scan to confirm no further callsite remains. --- .../tailwind/sure-design-system/base.css | 6 ++++- app/components/DS/button.rb | 23 ++++++++++++++++++- app/components/DS/buttonish.rb | 6 ++--- app/helpers/styled_form_builder.rb | 1 + app/views/account_sharings/show.html.erb | 4 ++-- app/views/category/deletions/new.html.erb | 2 ++ app/views/family_merchants/merge.html.erb | 1 + .../layouts/shared/_confirm_dialog.html.erb | 1 + app/views/onboardings/goals.html.erb | 1 + app/views/properties/address.html.erb | 1 + app/views/properties/balances.html.erb | 1 + app/views/properties/edit.html.erb | 1 + app/views/properties/new.html.erb | 1 + app/views/rules/_category_rule_cta.html.erb | 2 +- app/views/settings/preferences/show.html.erb | 2 +- app/views/settings/profiles/show.html.erb | 2 +- app/views/subscriptions/upgrade.html.erb | 1 + app/views/tag/deletions/new.html.erb | 2 ++ app/views/transactions/_attachments.html.erb | 1 + .../transactions/bulk_updates/new.html.erb | 2 +- .../transactions/searches/_menu.html.erb | 2 +- 21 files changed, 51 insertions(+), 12 deletions(-) diff --git a/app/assets/tailwind/sure-design-system/base.css b/app/assets/tailwind/sure-design-system/base.css index 991cfc4ef..522cdcf37 100644 --- a/app/assets/tailwind/sure-design-system/base.css +++ b/app/assets/tailwind/sure-design-system/base.css @@ -1,6 +1,10 @@ @layer base { button { - @apply cursor-pointer focus-visible:outline-gray-900; + @apply cursor-pointer focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-offset-2 focus-visible:ring-alpha-black-300; + + @variant theme-dark { + @apply focus-visible:ring-alpha-white-300; + } } hr { diff --git a/app/components/DS/button.rb b/app/components/DS/button.rb index a253c04c7..ca5644225 100644 --- a/app/components/DS/button.rb +++ b/app/components/DS/button.rb @@ -22,7 +22,6 @@ class DS::Button < DS::Buttonish def merged_opts merged_opts = opts.dup || {} extra_classes = merged_opts.delete(:class) - href = merged_opts.delete(:href) data = merged_opts.delete(:data) || {} if confirm.present? @@ -33,6 +32,28 @@ class DS::Button < DS::Buttonish data = data.merge(turbo_frame: frame) end + # `content_tag(:button, ...)` defaults to `type="submit"` per the HTML + # spec — meaning a DS::Button rendered inside a form will steal Enter-key + # submission from the first text input. Default to `type="button"` so + # callers must opt into submit behavior explicitly. `button_to` (href + # branch) wraps the button in its own form, so submit there is correct + # and we leave its default alone. + if href.blank? + merged_opts[:type] ||= "button" + end + + # Icon-only buttons have no visible text node, so screen readers fall + # back to announcing "button" with no name. Derive a humanized fallback + # from the icon key so AT users hear *something* meaningful; explicit + # `aria: { label: }` on the caller still wins. + if icon_only? && icon.present? + aria = (merged_opts[:aria] || {}).symbolize_keys + if aria[:label].blank? && merged_opts[:"aria-label"].blank? + aria[:label] = icon.to_s.tr("-_", " ").capitalize + merged_opts[:aria] = aria + end + end + merged_opts.merge( class: class_names(container_classes, extra_classes), data: data diff --git a/app/components/DS/buttonish.rb b/app/components/DS/buttonish.rb index bd8894be6..f1e864511 100644 --- a/app/components/DS/buttonish.rb +++ b/app/components/DS/buttonish.rb @@ -10,7 +10,7 @@ class DS::Buttonish < DesignSystemComponent }, destructive: { container_classes: "text-inverse bg-red-500 theme-dark:bg-red-400 hover:bg-red-600 theme-dark:hover:bg-red-500 disabled:bg-red-200 theme-dark:disabled:bg-red-600", - icon_classes: "fg-white" + icon_classes: "text-inverse" }, outline: { container_classes: "text-primary border border-secondary bg-transparent hover:bg-surface-hover", @@ -43,13 +43,13 @@ class DS::Buttonish < DesignSystemComponent }, md: { container_classes: "px-3 py-2", - icon_container_classes: "inline-flex items-center justify-center w-9 h-9", + icon_container_classes: "inline-flex items-center justify-center w-11 h-11", radius_classes: "rounded-lg", text_classes: "text-sm" }, lg: { container_classes: "px-4 py-3", - icon_container_classes: "inline-flex items-center justify-center w-10 h-10", + icon_container_classes: "inline-flex items-center justify-center w-12 h-12", radius_classes: "rounded-xl", text_classes: "text-base" } diff --git a/app/helpers/styled_form_builder.rb b/app/helpers/styled_form_builder.rb index ba3b5f81e..277d34aa3 100644 --- a/app/helpers/styled_form_builder.rb +++ b/app/helpers/styled_form_builder.rb @@ -89,6 +89,7 @@ class StyledFormBuilder < ActionView::Helpers::FormBuilder @template.render( DS::Button.new( text: value, + type: "submit", data: (options[:data] || {}).merge({ turbo_submits_with: "Submitting..." }), full_width: true ) diff --git a/app/views/account_sharings/show.html.erb b/app/views/account_sharings/show.html.erb index b53a6bdff..5700136ba 100644 --- a/app/views/account_sharings/show.html.erb +++ b/app/views/account_sharings/show.html.erb @@ -46,7 +46,7 @@ <% end %>
- <%= render DS::Button.new(text: t(".save"), class: "md:w-auto w-full justify-center") %> + <%= render DS::Button.new(text: t(".save"), type: :submit, class: "md:w-auto w-full justify-center") %>
<% end %> <% else %> @@ -83,7 +83,7 @@
- <%= render DS::Button.new(text: t(".save"), class: "md:w-auto w-full justify-center") %> + <%= render DS::Button.new(text: t(".save"), type: :submit, class: "md:w-auto w-full justify-center") %>
<% end %> diff --git a/app/views/category/deletions/new.html.erb b/app/views/category/deletions/new.html.erb index cf06b137d..d594dba7a 100644 --- a/app/views/category/deletions/new.html.erb +++ b/app/views/category/deletions/new.html.erb @@ -16,6 +16,7 @@ <%= render DS::Button.new( variant: "destructive", + type: :submit, text: t(".delete_and_leave_uncategorized", category_name: @category.name), full_width: true, data: { deletion_target: "destructiveSubmitButton" } @@ -23,6 +24,7 @@ <%= render DS::Button.new( text: t(".delete_and_recategorize", category_name: @category.name), + type: :submit, data: { deletion_target: "safeSubmitButton" }, hidden: true, full_width: true diff --git a/app/views/family_merchants/merge.html.erb b/app/views/family_merchants/merge.html.erb index d2fca9c7d..879118d56 100644 --- a/app/views/family_merchants/merge.html.erb +++ b/app/views/family_merchants/merge.html.erb @@ -26,6 +26,7 @@ <%= render DS::Button.new( text: t(".submit"), + type: :submit, full_width: true ) %> <% end %> diff --git a/app/views/layouts/shared/_confirm_dialog.html.erb b/app/views/layouts/shared/_confirm_dialog.html.erb index a25c36f72..cefc5aa13 100644 --- a/app/views/layouts/shared/_confirm_dialog.html.erb +++ b/app/views/layouts/shared/_confirm_dialog.html.erb @@ -17,6 +17,7 @@ <%= render DS::Button.new( text: t(".confirm"), variant: variant, + type: :submit, autofocus: true, full_width: true, value: "confirm", diff --git a/app/views/onboardings/goals.html.erb b/app/views/onboardings/goals.html.erb index 646a892d1..8cb36cd99 100644 --- a/app/views/onboardings/goals.html.erb +++ b/app/views/onboardings/goals.html.erb @@ -46,6 +46,7 @@
<%= render DS::Button.new( text: t("onboardings.goals.submit"), + type: :submit, full_width: true ) %>
diff --git a/app/views/properties/address.html.erb b/app/views/properties/address.html.erb index 489ab62b1..a8b873c7c 100644 --- a/app/views/properties/address.html.erb +++ b/app/views/properties/address.html.erb @@ -41,6 +41,7 @@ <%= render DS::Button.new( text: t(".save"), variant: "primary", + type: :submit, ) %> <% end %> diff --git a/app/views/properties/balances.html.erb b/app/views/properties/balances.html.erb index 3cd96041d..9477213f7 100644 --- a/app/views/properties/balances.html.erb +++ b/app/views/properties/balances.html.erb @@ -21,6 +21,7 @@ <%= render DS::Button.new( text: @account.active? ? t(".save") : t(".next"), variant: "primary", + type: :submit, ) %> <% end %> diff --git a/app/views/properties/edit.html.erb b/app/views/properties/edit.html.erb index 51d674f96..428e60059 100644 --- a/app/views/properties/edit.html.erb +++ b/app/views/properties/edit.html.erb @@ -18,6 +18,7 @@ <%= render DS::Button.new( text: @account.active? ? "Save" : "Next", variant: "primary", + type: :submit, ) %> <% end %> diff --git a/app/views/properties/new.html.erb b/app/views/properties/new.html.erb index 32ab6d960..5e790cce0 100644 --- a/app/views/properties/new.html.erb +++ b/app/views/properties/new.html.erb @@ -18,6 +18,7 @@ <%= render DS::Button.new( text: t(".next"), variant: "primary", + type: :submit, ) %> <% end %> diff --git a/app/views/rules/_category_rule_cta.html.erb b/app/views/rules/_category_rule_cta.html.erb index ff3481f4d..3c7f2ed41 100644 --- a/app/views/rules/_category_rule_cta.html.erb +++ b/app/views/rules/_category_rule_cta.html.erb @@ -13,7 +13,7 @@ <%= f.hidden_field :rule_prompt_dismissed_at, value: Time.current %> <%= tag.div class:"flex gap-2 justify-end" do %> - <%= render DS::Button.new(text: "Dismiss", variant: "secondary") %> + <%= render DS::Button.new(text: "Dismiss", variant: "secondary", type: :submit) %> <% rule_href = new_rule_path(resource_type: "transaction", action_type: "set_transaction_category", action_value: cta[:category_id], name: cta[:merchant_name]) %> <%= render DS::Link.new(text: "Create rule", variant: "primary", href: rule_href, frame: :modal) %> <% end %> diff --git a/app/views/settings/preferences/show.html.erb b/app/views/settings/preferences/show.html.erb index 67c04b44a..d2e0fd9a1 100644 --- a/app/views/settings/preferences/show.html.erb +++ b/app/views/settings/preferences/show.html.erb @@ -182,7 +182,7 @@
<%= render DS::Button.new(text: t("shared.cancel"), type: :button, variant: :ghost, data: { action: "DS--dialog#close" }) %> - <%= render DS::Button.new(text: t(".save_currencies")) %> + <%= render DS::Button.new(text: t(".save_currencies"), type: :submit) %>
<% end %> <% end %> diff --git a/app/views/settings/profiles/show.html.erb b/app/views/settings/profiles/show.html.erb index dc3e081b9..5c431449d 100644 --- a/app/views/settings/profiles/show.html.erb +++ b/app/views/settings/profiles/show.html.erb @@ -19,7 +19,7 @@
- <%= render DS::Button.new(text: t(".save"), class: "md:w-auto w-full justify-center") %> + <%= render DS::Button.new(text: t(".save"), type: :submit, class: "md:w-auto w-full justify-center") %>
<% end %> diff --git a/app/views/subscriptions/upgrade.html.erb b/app/views/subscriptions/upgrade.html.erb index fd7d49936..2fbf5839a 100644 --- a/app/views/subscriptions/upgrade.html.erb +++ b/app/views/subscriptions/upgrade.html.erb @@ -48,6 +48,7 @@ <%= render DS::Button.new( text: t("subscriptions.upgrade.contribute_and_support_sure"), variant: "primary", + type: :submit, full_width: true ) %> diff --git a/app/views/tag/deletions/new.html.erb b/app/views/tag/deletions/new.html.erb index c069518ec..dcadf6e1d 100644 --- a/app/views/tag/deletions/new.html.erb +++ b/app/views/tag/deletions/new.html.erb @@ -16,6 +16,7 @@ <%= render DS::Button.new( variant: "destructive", + type: :submit, text: t(".delete_and_leave_uncategorized", tag_name: @tag.name), full_width: true, data: { deletion_target: "destructiveSubmitButton" } @@ -23,6 +24,7 @@ <%= render DS::Button.new( text: t(".delete_and_reassign"), + type: :submit, data: { deletion_target: "safeSubmitButton" }, hidden: true, full_width: true diff --git a/app/views/transactions/_attachments.html.erb b/app/views/transactions/_attachments.html.erb index dba4b464f..b771ca448 100644 --- a/app/views/transactions/_attachments.html.erb +++ b/app/views/transactions/_attachments.html.erb @@ -51,6 +51,7 @@ text: t(".upload"), variant: :primary, size: :sm, + type: :submit, data: { attachment_upload_target: "submitButton" } ) %> diff --git a/app/views/transactions/bulk_updates/new.html.erb b/app/views/transactions/bulk_updates/new.html.erb index 713d38588..47b38bf3f 100644 --- a/app/views/transactions/bulk_updates/new.html.erb +++ b/app/views/transactions/bulk_updates/new.html.erb @@ -21,7 +21,7 @@
<%= render DS::Button.new(text: t(".cancel"), variant: "ghost", data: { action: "click->DS--dialog#close" }) %> - <%= render DS::Button.new(text: t(".save"), data: { bulk_select_scope_param: "bulk_update", action: "bulk-select#submitBulkRequest" }) %> + <%= render DS::Button.new(text: t(".save"), type: :submit, data: { bulk_select_scope_param: "bulk_update", action: "bulk-select#submitBulkRequest" }) %>
<% end %> <% end %> diff --git a/app/views/transactions/searches/_menu.html.erb b/app/views/transactions/searches/_menu.html.erb index 0f2e9b09a..db7370e30 100644 --- a/app/views/transactions/searches/_menu.html.erb +++ b/app/views/transactions/searches/_menu.html.erb @@ -38,7 +38,7 @@
<%= render DS::Button.new(text: t(".cancel"), type: "button", variant: "ghost", data: { action: "DS--menu#close" }) %> - <%= render DS::Button.new(text: t(".apply")) %> + <%= render DS::Button.new(text: t(".apply"), type: :submit) %>