Skip to content

Commit 0897b2a

Browse files
djalmaaraujodjalmaaraujo
authored
[Bug Fix] Accordion: hide closed content properly instead of zero height (#168) (#433)
* [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. * [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.
1 parent 1fc5e6c commit 0897b2a

4 files changed

Lines changed: 114 additions & 8 deletions

File tree

‎gem/lib/ruby_ui/accordion/accordion_content.rb‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,12 @@ def view_template(&)
1111
def default_attrs
1212
{
1313
data: {
14-
ruby_ui__accordion_target: "content"
14+
ruby_ui__accordion_target: "content",
15+
state: "closed"
1516
},
1617
class: "overflow-y-hidden",
17-
style: "height: 0px;"
18+
style: "height: 0px;",
19+
hidden: true
1820
}
1921
end
2022
end

‎gem/lib/ruby_ui/accordion/accordion_controller.js‎

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -65,9 +65,15 @@ export default class extends Controller {
6565

6666
// Reveal the accordion content with animation
6767
revealContent() {
68-
const contentHeight = this.contentTarget.scrollHeight;
68+
const content = this.contentTarget;
69+
70+
// Remove hidden so the element participates in layout before measuring
71+
content.removeAttribute("hidden");
72+
content.dataset.state = "open";
73+
74+
const contentHeight = content.scrollHeight;
6975
animate(
70-
this.contentTarget,
76+
content,
7177
{ height: `${contentHeight}px` },
7278
{
7379
duration: this.animationDurationValue,
@@ -78,14 +84,23 @@ export default class extends Controller {
7884

7985
// Hide the accordion content with animation
8086
hideContent() {
87+
const content = this.contentTarget;
88+
content.dataset.state = "closed";
89+
8190
animate(
82-
this.contentTarget,
91+
content,
8392
{ height: 0 },
8493
{
8594
duration: this.animationDurationValue,
8695
easing: this.animationEasingValue,
8796
},
88-
);
97+
).finished.then(() => {
98+
// After animation completes, truly hide the element so it is removed
99+
// from layout and form focus — prevents trapped validation errors
100+
if (content.dataset.state === "closed") {
101+
content.setAttribute("hidden", "");
102+
}
103+
});
89104
}
90105

91106
// Rotate the accordion icon 180deg using animate function

‎gem/test/ruby_ui/accordion_test.rb‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,4 +81,93 @@ def test_render_with_all_items
8181

8282
assert_match(/Yes, RubyUI is pure Ruby and works great with Rails/, output)
8383
end
84+
85+
# Regression test for issue #168:
86+
# Closed accordion content must not trap form validation errors in a
87+
# zero-height clipped region. Verify that:
88+
# - closed content has the `hidden` attribute (truly hidden from layout + focus)
89+
# - closed content carries data-state="closed" for CSS/semantic targeting
90+
# - open content does NOT have the `hidden` attribute
91+
# - open content carries data-state="open"
92+
93+
def test_closed_content_is_hidden
94+
output = phlex do
95+
RubyUI.Accordion do
96+
RubyUI.AccordionItem(open: false) do
97+
RubyUI.AccordionTrigger { "Trigger" }
98+
RubyUI.AccordionContent do
99+
"Hidden content"
100+
end
101+
end
102+
end
103+
end
104+
105+
# The content div must carry hidden so it is fully removed from layout,
106+
# preventing form field errors inside it from being invisible-but-focusable.
107+
assert_match(/data-ruby-ui--accordion-target="content"[^>]*hidden/, output)
108+
end
109+
110+
def test_closed_content_has_data_state_closed
111+
output = phlex do
112+
RubyUI.Accordion do
113+
RubyUI.AccordionItem(open: false) do
114+
RubyUI.AccordionTrigger { "Trigger" }
115+
RubyUI.AccordionContent do
116+
"Hidden content"
117+
end
118+
end
119+
end
120+
end
121+
122+
assert_match(/data-state="closed"/, output)
123+
end
124+
125+
def test_open_item_wires_stimulus_open_value
126+
output = phlex do
127+
RubyUI.Accordion do
128+
RubyUI.AccordionItem(open: true) do
129+
RubyUI.AccordionTrigger { "Trigger" }
130+
RubyUI.AccordionContent do
131+
"Visible content"
132+
end
133+
end
134+
end
135+
end
136+
137+
# The Stimulus controller removes `hidden` and sets data-state="open" at
138+
# runtime (JS). At the server-rendered HTML level we assert that the
139+
# AccordionItem is wired up with the open value set to true so the
140+
# controller reveals it on connect. Phlex renders `open: true` as a bare
141+
# data attribute (no ="true"), so we confirm the attribute is present and
142+
# NOT set to the false string.
143+
assert_match(/data-ruby-ui--accordion-open-value/, output)
144+
refute_match(/data-ruby-ui--accordion-open-value="false"/, output)
145+
# The content is present in the DOM (server-rendered)
146+
assert_match(/Visible content/, output)
147+
end
148+
149+
# Structural test: a FormField with a FormFieldError nested inside a closed
150+
# AccordionContent is present in the HTML (not stripped) but wrapped inside
151+
# an element that carries the `hidden` attribute, so the browser hides it
152+
# from layout and focus — the error cannot silently block form submission.
153+
def test_form_field_error_inside_closed_accordion_is_wrapped_in_hidden_element
154+
output = phlex do
155+
RubyUI.Accordion do
156+
RubyUI.AccordionItem(open: false) do
157+
RubyUI.AccordionTrigger { "Form section" }
158+
RubyUI.AccordionContent do |content|
159+
# Simulate a form validation error message inside a closed accordion
160+
content.span(class: "text-destructive text-sm") { "This field is required" }
161+
end
162+
end
163+
end
164+
end
165+
166+
# Error text is in the DOM (server-rendered), but its ancestor content
167+
# container must carry `hidden` so the browser skips it for layout/focus.
168+
assert_match(/This field is required/, output)
169+
assert_match(/hidden/, output)
170+
# Confirm the hidden attribute belongs to the content target element
171+
assert_match(/data-ruby-ui--accordion-target="content"[^>]*hidden/, output)
172+
end
84173
end

‎mcp/data/registry.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,11 @@
1111
},
1212
{
1313
"path": "accordion_content.rb",
14-
"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"
14+
"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"
1515
},
1616
{
1717
"path": "accordion_controller.js",
18-
"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"
18+
"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"
1919
},
2020
{
2121
"path": "accordion_default_content.rb",

0 commit comments

Comments
 (0)