DisclosurePrimitive - Implement a11y toggled content pattern

[dchyun]

📌 Summary

This PR implements usage of the aria-controls attribute in the DisclosurePrimitive component to align show/hide behavior across the repo as follows in the initiative from HDS-3581.

The DisclosurePrimitive is leveraged in the Accordion and Reveal.

🛠️ Detailed description

Currently in the DisclosurePrimitive, the toggled content does not follow the proper a11y structure for toggled content, or leverage aria-controls in the preferred way.

As per a11y guidance in HDS-3581, all togged content should follow a pattern leveraging aria-controls, and all toggled containers should not be removed from the DOM on each toggle. They should always be present, and only the content inside them is added and removed.

Currently the DisclosurePrimitive follows a pattern where the entire content block is added and removed on each toggle, and any aria-controls attributes are set to the id of this removable block. In this PR the hds-disclosure-primitive__content block is now in the DOM at all times, and the yielded content is removed or added based on the toggle.

In this PR, in the Accordion and Reveal, the aria-controls attribute of the toggle elements are now set equal to a contentId provided by the DisclosurePrimitive. They are no longer set to the id of content inside the yielded block.

📸 Screenshots

Before - Accordion

Closed

Open

After - Accordion

Closed

Open

🔗 External links

Jira ticket: HDS-4324

---

👀 Component checklist

• Percy was checked for any visual regression
• A changelog entry was added via Changesets if needed (see templates here)

💬 Please consider using conventional comments when reviewing this PR.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
hds-showcase	✅ Ready (Inspect)	Visit Preview	Jan 13, 2025 7:52pm
hds-website	✅ Ready (Inspect)	Visit Preview	Jan 13, 2025 7:52pm

[shleewhite]

This looks good, but we should definitely test this in products before we ship. Last year we changed something similar-ish in Dropdown and it broke tests (and led us to add @preserveContentInDom):

design-system/packages/components/src/components/hds/dropdown/index.hbs

Line 21 in 735b7bd

{{#if (or PP.isOpen @preserveContentInDom)}}

)

[dchyun]

This looks good, but we should definitely test this in products before we ship. Last year we changed something similar-ish in Dropdown and it broke tests (and led us to add @preserveContentInDom):

design-system/packages/components/src/components/hds/dropdown/index.hbs

Line 21 in 735b7bd

{{#if (or PP.isOpen @preserveContentInDom)}}

)

Tested the changes across atlas and cloud-ui and didn't run into any test failures. But I think this is an open question if these kind of DOM changes could be considered breaking or not. @alex-ju do you see any potential issues with this or think it could possibly be a breaking change based on your recent RFC?

Similar DOM changes will have to be made for the hds-tooltip updates, so can follow whatever we decide here on that as well.

[alex-ju]

This could a breaking change if consumers use hds-disclosure-primitive__content selector to check if disclosed content is visible or not. If we find through testing that it does not affect tests then we're good. Otherwise we'd either have to find another way or defer this feature to a major release.

[dchyun]

Yeah I agree it could be seen as breaking. I tested it out in atlas locally and ran it through tests in cloud-ui and nothing seems to be breaking, but not sure about other consumers.

[alex-ju]

Smoke testing sounds good. Searching for this class being used as a selector I could only find a few instances in one test in nomad. I would try and check that test suite and if it's all good we can consider releasing this as a patch.

[dchyun]

Tests seem to pass for me. If anything it may make the test they have never fail. They are testing that hds-disclosure-primitive__content is present on the page, and with the new approach it will always be present and only the inner content will be added / removed.

[alex-ju]

Great! Thanks for checking! On that basis we can class it as a patch indeed.

[dchyun]

Went back through this after I realized my changes from the testing branch weren't updating the dist for local testing in Nomad. There does seem to be a test failure now on that test you highlighted. The test is related to a component they have that uses a reveal inside of a dropdown. So we may have to hold off on this change...

[alex-ju]

We can hold this PR for now and work it from another angle. Maybe we can update their tests to not need this class, then get this change in. Otherwise, as you mentioned in the sister PR, we can roll it out in a major.

[dchyun]

Converted back to a draft pr until issues with failing tests in consumer's repos can be sorted.