From 114a3fdbc2f49eda5b5ed0a29fc811a35b78c973 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Djalma=20Ara=C3=BAjo?= Date: Mon, 22 Jun 2026 17:07:44 -0300 Subject: [PATCH 1/2] [Bug Fix] Accordion: hide closed content properly instead of zero height (#168) Closed accordion content was rendered with height:0 + overflow-hidden, which trapped form field errors in an invisible, still-focusable region. Now the content element receives the `hidden` attribute after the close animation completes (and `data-state="closed"`), fully removing it from layout and form focus. On open, `hidden` is removed before the height animation begins. Animation remains smooth via motion. --- .../ruby_ui/accordion/accordion_content.rb | 6 +- .../ruby_ui/accordion/accordion_controller.js | 23 ++++- gem/test/ruby_ui/accordion_test.rb | 87 +++++++++++++++++++ 3 files changed, 110 insertions(+), 6 deletions(-) diff --git a/gem/lib/ruby_ui/accordion/accordion_content.rb b/gem/lib/ruby_ui/accordion/accordion_content.rb index 1b6feef9d..063778f55 100644 --- a/gem/lib/ruby_ui/accordion/accordion_content.rb +++ b/gem/lib/ruby_ui/accordion/accordion_content.rb @@ -11,10 +11,12 @@ def view_template(&) def default_attrs { data: { - ruby_ui__accordion_target: "content" + ruby_ui__accordion_target: "content", + state: "closed" }, class: "overflow-y-hidden", - style: "height: 0px;" + style: "height: 0px;", + hidden: true } end end diff --git a/gem/lib/ruby_ui/accordion/accordion_controller.js b/gem/lib/ruby_ui/accordion/accordion_controller.js index 2408ce7fe..3ed2952c2 100644 --- a/gem/lib/ruby_ui/accordion/accordion_controller.js +++ b/gem/lib/ruby_ui/accordion/accordion_controller.js @@ -65,9 +65,15 @@ export default class extends Controller { // Reveal the accordion content with animation revealContent() { - const contentHeight = this.contentTarget.scrollHeight; + const content = this.contentTarget; + + // Remove hidden so the element participates in layout before measuring + content.removeAttribute("hidden"); + content.dataset.state = "open"; + + const contentHeight = content.scrollHeight; animate( - this.contentTarget, + content, { height: `${contentHeight}px` }, { duration: this.animationDurationValue, @@ -78,14 +84,23 @@ export default class extends Controller { // Hide the accordion content with animation hideContent() { + const content = this.contentTarget; + content.dataset.state = "closed"; + animate( - this.contentTarget, + content, { height: 0 }, { duration: this.animationDurationValue, easing: this.animationEasingValue, }, - ); + ).finished.then(() => { + // After animation completes, truly hide the element so it is removed + // from layout and form focus — prevents trapped validation errors + if (content.dataset.state === "closed") { + content.setAttribute("hidden", ""); + } + }); } // Rotate the accordion icon 180deg using animate function diff --git a/gem/test/ruby_ui/accordion_test.rb b/gem/test/ruby_ui/accordion_test.rb index 1d5cf5fcd..9628c269b 100644 --- a/gem/test/ruby_ui/accordion_test.rb +++ b/gem/test/ruby_ui/accordion_test.rb @@ -81,4 +81,91 @@ def test_render_with_all_items assert_match(/Yes, RubyUI is pure Ruby and works great with Rails/, output) end + + # Regression test for issue #168: + # Closed accordion content must not trap form validation errors in a + # zero-height clipped region. Verify that: + # - closed content has the `hidden` attribute (truly hidden from layout + focus) + # - closed content carries data-state="closed" for CSS/semantic targeting + # - open content does NOT have the `hidden` attribute + # - open content carries data-state="open" + + def test_closed_content_is_hidden + output = phlex do + RubyUI.Accordion do + RubyUI.AccordionItem(open: false) do + RubyUI.AccordionTrigger { "Trigger" } + RubyUI.AccordionContent do + "Hidden content" + end + end + end + end + + # The content div must carry hidden so it is fully removed from layout, + # preventing form field errors inside it from being invisible-but-focusable. + assert_match(/data-ruby-ui--accordion-target="content"[^>]*hidden/, output) + end + + def test_closed_content_has_data_state_closed + output = phlex do + RubyUI.Accordion do + RubyUI.AccordionItem(open: false) do + RubyUI.AccordionTrigger { "Trigger" } + RubyUI.AccordionContent do + "Hidden content" + end + end + end + end + + assert_match(/data-state="closed"/, output) + end + + def test_open_content_does_not_have_hidden + output = phlex do + RubyUI.Accordion do + RubyUI.AccordionItem(open: true) do + RubyUI.AccordionTrigger { "Trigger" } + RubyUI.AccordionContent do + "Visible content" + end + end + end + end + + # When open: true the JS controller removes hidden on connect — + # but at the Ruby/HTML level the content still starts with hidden. + # The Stimulus controller handles removal at runtime. What we can assert + # at the structural level is that the item is wired up with open: true + # (Phlex renders boolean true as a bare attribute, no ="true"). + assert_match(/data-ruby-ui--accordion-open-value/, output) + # Confirm the open: true value is set (bare attribute without ="false") + refute_match(/data-ruby-ui--accordion-open-value="false"/, output) + end + + # Structural test: a FormField with a FormFieldError nested inside a closed + # AccordionContent is present in the HTML (not stripped) but wrapped inside + # an element that carries the `hidden` attribute, so the browser hides it + # from layout and focus — the error cannot silently block form submission. + def test_form_field_error_inside_closed_accordion_is_wrapped_in_hidden_element + output = phlex do + RubyUI.Accordion do + RubyUI.AccordionItem(open: false) do + RubyUI.AccordionTrigger { "Form section" } + RubyUI.AccordionContent do |content| + # Simulate a form validation error message inside a closed accordion + content.span(class: "text-destructive text-sm") { "This field is required" } + end + end + end + end + + # Error text is in the DOM (server-rendered), but its ancestor content + # container must carry `hidden` so the browser skips it for layout/focus. + assert_match(/This field is required/, output) + assert_match(/hidden/, output) + # Confirm the hidden attribute belongs to the content target element + assert_match(/data-ruby-ui--accordion-target="content"[^>]*hidden/, output) + end end From 837712f9ff66363021e277ee6aa3dcbd0f824886 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Djalma=20Ara=C3=BAjo?= Date: Mon, 22 Jun 2026 17:41:09 -0300 Subject: [PATCH 2/2] [Bug Fix] Accordion: fix misnamed test and rebuild MCP registry (#433) Rename `test_open_content_does_not_have_hidden` to `test_open_item_wires_stimulus_open_value`, add a `Visible content` assertion, and clarify comments to accurately describe that the hidden attribute is removed at JS runtime (not server-render time). Addresses code-review finding from cubic (confidence 9). Also rebuild mcp/data/registry.json to include the accordion_content.rb and accordion_controller.js changes from this PR so CI "MCP registry up to date" passes. --- gem/test/ruby_ui/accordion_test.rb | 16 +++++++++------- mcp/data/registry.json | 4 ++-- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/gem/test/ruby_ui/accordion_test.rb b/gem/test/ruby_ui/accordion_test.rb index 9628c269b..6d74e0d5c 100644 --- a/gem/test/ruby_ui/accordion_test.rb +++ b/gem/test/ruby_ui/accordion_test.rb @@ -122,7 +122,7 @@ def test_closed_content_has_data_state_closed assert_match(/data-state="closed"/, output) end - def test_open_content_does_not_have_hidden + def test_open_item_wires_stimulus_open_value output = phlex do RubyUI.Accordion do RubyUI.AccordionItem(open: true) do @@ -134,14 +134,16 @@ def test_open_content_does_not_have_hidden end end - # When open: true the JS controller removes hidden on connect — - # but at the Ruby/HTML level the content still starts with hidden. - # The Stimulus controller handles removal at runtime. What we can assert - # at the structural level is that the item is wired up with open: true - # (Phlex renders boolean true as a bare attribute, no ="true"). + # The Stimulus controller removes `hidden` and sets data-state="open" at + # runtime (JS). At the server-rendered HTML level we assert that the + # AccordionItem is wired up with the open value set to true so the + # controller reveals it on connect. Phlex renders `open: true` as a bare + # data attribute (no ="true"), so we confirm the attribute is present and + # NOT set to the false string. assert_match(/data-ruby-ui--accordion-open-value/, output) - # Confirm the open: true value is set (bare attribute without ="false") refute_match(/data-ruby-ui--accordion-open-value="false"/, output) + # The content is present in the DOM (server-rendered) + assert_match(/Visible content/, output) end # Structural test: a FormField with a FormFieldError nested inside a closed diff --git a/mcp/data/registry.json b/mcp/data/registry.json index 8fa00b3f8..f1a95e96a 100644 --- a/mcp/data/registry.json +++ b/mcp/data/registry.json @@ -11,11 +11,11 @@ }, { "path": "accordion_content.rb", - "content": "# frozen_string_literal: true\n\nmodule RubyUI\n class AccordionContent < Base\n def view_template(&)\n div(**attrs, &)\n end\n\n private\n\n def default_attrs\n {\n data: {\n ruby_ui__accordion_target: \"content\"\n },\n class: \"overflow-y-hidden\",\n style: \"height: 0px;\"\n }\n end\n end\nend\n" + "content": "# frozen_string_literal: true\n\nmodule RubyUI\n class AccordionContent < Base\n def view_template(&)\n div(**attrs, &)\n end\n\n private\n\n def default_attrs\n {\n data: {\n ruby_ui__accordion_target: \"content\",\n state: \"closed\"\n },\n class: \"overflow-y-hidden\",\n style: \"height: 0px;\",\n hidden: true\n }\n end\n end\nend\n" }, { "path": "accordion_controller.js", - "content": "import { Controller } from \"@hotwired/stimulus\";\nimport { animate } from \"motion\";\n\n// Connects to data-controller=\"ruby-ui--accordion\"\nexport default class extends Controller {\n static targets = [\"icon\", \"content\"];\n static values = {\n open: {\n type: Boolean,\n default: false,\n },\n animationDuration: {\n type: Number,\n default: 0.15, // Default animation duration (in seconds)\n },\n animationEasing: {\n type: String,\n default: \"ease-in-out\", // Default animation easing\n },\n rotateIcon: {\n type: Number,\n default: 180, // Default icon rotation (in degrees)\n },\n };\n\n connect() {\n // Set the initial state of the accordion\n let originalAnimationDuration = this.animationDurationValue;\n this.animationDurationValue = 0;\n this.openValue ? this.open() : this.close();\n this.animationDurationValue = originalAnimationDuration;\n }\n\n // Toggle the 'open' value\n toggle() {\n this.openValue = !this.openValue;\n }\n\n // Handle changes in the 'open' value\n openValueChanged(isOpen, wasOpen) {\n if (isOpen) {\n this.open();\n } else {\n this.close();\n }\n }\n\n // Open the accordion content\n open() {\n if (this.hasContentTarget) {\n this.revealContent();\n this.hasIconTarget && this.rotateIcon();\n this.openValue = true;\n }\n }\n\n // Close the accordion content\n close() {\n if (this.hasContentTarget) {\n this.hideContent();\n this.hasIconTarget && this.rotateIcon();\n this.openValue = false;\n }\n }\n\n // Reveal the accordion content with animation\n revealContent() {\n const contentHeight = this.contentTarget.scrollHeight;\n animate(\n this.contentTarget,\n { height: `${contentHeight}px` },\n {\n duration: this.animationDurationValue,\n easing: this.animationEasingValue,\n },\n );\n }\n\n // Hide the accordion content with animation\n hideContent() {\n animate(\n this.contentTarget,\n { height: 0 },\n {\n duration: this.animationDurationValue,\n easing: this.animationEasingValue,\n },\n );\n }\n\n // Rotate the accordion icon 180deg using animate function\n rotateIcon() {\n animate(this.iconTarget, {\n rotate: `${this.openValue ? this.rotateIconValue : 0}deg`,\n });\n }\n}\n" + "content": "import { Controller } from \"@hotwired/stimulus\";\nimport { animate } from \"motion\";\n\n// Connects to data-controller=\"ruby-ui--accordion\"\nexport default class extends Controller {\n static targets = [\"icon\", \"content\"];\n static values = {\n open: {\n type: Boolean,\n default: false,\n },\n animationDuration: {\n type: Number,\n default: 0.15, // Default animation duration (in seconds)\n },\n animationEasing: {\n type: String,\n default: \"ease-in-out\", // Default animation easing\n },\n rotateIcon: {\n type: Number,\n default: 180, // Default icon rotation (in degrees)\n },\n };\n\n connect() {\n // Set the initial state of the accordion\n let originalAnimationDuration = this.animationDurationValue;\n this.animationDurationValue = 0;\n this.openValue ? this.open() : this.close();\n this.animationDurationValue = originalAnimationDuration;\n }\n\n // Toggle the 'open' value\n toggle() {\n this.openValue = !this.openValue;\n }\n\n // Handle changes in the 'open' value\n openValueChanged(isOpen, wasOpen) {\n if (isOpen) {\n this.open();\n } else {\n this.close();\n }\n }\n\n // Open the accordion content\n open() {\n if (this.hasContentTarget) {\n this.revealContent();\n this.hasIconTarget && this.rotateIcon();\n this.openValue = true;\n }\n }\n\n // Close the accordion content\n close() {\n if (this.hasContentTarget) {\n this.hideContent();\n this.hasIconTarget && this.rotateIcon();\n this.openValue = false;\n }\n }\n\n // Reveal the accordion content with animation\n revealContent() {\n const content = this.contentTarget;\n\n // Remove hidden so the element participates in layout before measuring\n content.removeAttribute(\"hidden\");\n content.dataset.state = \"open\";\n\n const contentHeight = content.scrollHeight;\n animate(\n content,\n { height: `${contentHeight}px` },\n {\n duration: this.animationDurationValue,\n easing: this.animationEasingValue,\n },\n );\n }\n\n // Hide the accordion content with animation\n hideContent() {\n const content = this.contentTarget;\n content.dataset.state = \"closed\";\n\n animate(\n content,\n { height: 0 },\n {\n duration: this.animationDurationValue,\n easing: this.animationEasingValue,\n },\n ).finished.then(() => {\n // After animation completes, truly hide the element so it is removed\n // from layout and form focus — prevents trapped validation errors\n if (content.dataset.state === \"closed\") {\n content.setAttribute(\"hidden\", \"\");\n }\n });\n }\n\n // Rotate the accordion icon 180deg using animate function\n rotateIcon() {\n animate(this.iconTarget, {\n rotate: `${this.openValue ? this.rotateIconValue : 0}deg`,\n });\n }\n}\n" }, { "path": "accordion_default_content.rb",