Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DisclosurePrimitive - Implement a11y toggled content pattern #2643

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
+48 −52
Draft

DisclosurePrimitive - Implement a11y toggled content pattern #2643

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0c77982
HDS-4324 Implement a11y toggle pattern in `DisclosurePrimitive`
dchyun Jan 10, 2025
eea8504
Add changeset
dchyun Jan 10, 2025
ad829a9
Fix: Remove _contentId from accordion item
dchyun Jan 13, 2025
8c332ce
chore: Linting fix
dchyun Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/seven-eyes-whisper.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@hashicorp/design-system-components": patch
---

`Accordion` - Set `aria-controls` of `Accordion::Item::Button` to `contentId` from `DisclosurePrimitve` for a11y improvements with toggled content
`DisclosurePrimitve` - Changed DOM structure of content section and exposed `contentId` for a11y improvements with toggled content
`Reveal` - Set `aria-controls` of `Reveal::Toggle` to `contentId` from `DisclosurePrimitve` for a11y improvements with toggled content
11 changes: 2 additions & 9 deletions packages/components/src/components/hds/accordion/item/index.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
<Hds::Accordion::Item::Button
@isOpen={{t.isOpen}}
@onClickToggle={{t.onClickToggle}}
@contentId={{this._contentId}}
@contentId={{t.contentId}}
@ariaLabel={{@ariaLabel}}
@ariaLabelledBy={{this.ariaLabelledBy}}
@size={{this.size}}
Expand All @@ -35,14 +35,7 @@
</:toggle>

<:content as |c|>
<Hds::Text::Body
class="hds-accordion-item__content"
@tag="div"
@size="200"
@weight="regular"
@color="primary"
id={{this._contentId}}
>
<Hds::Text::Body class="hds-accordion-item__content" @tag="div" @size="200" @weight="regular" @color="primary">
{{yield (hash close=c.close) to="content"}}
</Hds::Text::Body>
</:content>
Expand Down
6 changes: 0 additions & 6 deletions packages/components/src/components/hds/accordion/item/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,6 @@ export interface HdsAccordionItemSignature {
}

export default class HdsAccordionItem extends Component<HdsAccordionItemSignature> {
/**
* Generates a unique ID for the Content
*
* @param _contentId
*/
private _contentId = 'content-' + guidFor(this);
private _titleId = 'title-' + guidFor(this);

get ariaLabelledBy(): string | undefined {
Expand Down
10 changes: 5 additions & 5 deletions packages/components/src/components/hds/disclosure-primitive/index.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@
}}
<div class="hds-disclosure-primitive" {{did-update this.onStateChange @isOpen}} ...attributes>
<div class="hds-disclosure-primitive__toggle">
{{yield (hash onClickToggle=this.onClickToggle isOpen=this.isOpen) to="toggle"}}
{{yield (hash onClickToggle=this.onClickToggle isOpen=this.isOpen contentId=this._contentId) to="toggle"}}
</div>
{{#if this.isOpen}}
<div class="hds-disclosure-primitive__content">
<div class="hds-disclosure-primitive__content" id={{this._contentId}}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@alex-ju alex-ju Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Screenshot 2025-01-15 at 8 54 44 PM

Copy link
Member

@alex-ju alex-ju Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{{#if this.isOpen}}
{{yield (hash close=this.close) to="content"}}
</div>
{{/if}}
{{/if}}
</div>
</div>
3 changes: 3 additions & 0 deletions packages/components/src/components/hds/disclosure-primitive/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import Component from '@glimmer/component';
import { tracked } from '@glimmer/tracking';
import { action } from '@ember/object';
import { schedule } from '@ember/runloop';
import { guidFor } from '@ember/object/internals';

export interface HdsDisclosurePrimitiveSignature {
Args: {
Expand All @@ -19,6 +20,7 @@ export interface HdsDisclosurePrimitiveSignature {
Blocks: {
toggle: [
{
contentId: string;
isOpen: boolean;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
onClickToggle: (...args: any[]) => void;
Expand All @@ -37,6 +39,7 @@ export interface HdsDisclosurePrimitiveSignature {
export default class HdsDisclosurePrimitive extends Component<HdsDisclosurePrimitiveSignature> {
@tracked private _isOpen = false;
@tracked private _isControlled = this.args.isOpen !== undefined;
private _contentId = 'content-' + guidFor(this);

get isOpen(): boolean {
if (this._isControlled) {
Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/components/hds/reveal/index.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,15 @@
<Hds::DisclosurePrimitive class="hds-reveal" @isOpen={{@isOpen}} ...attributes>
<:toggle as |t|>
<Hds::Reveal::Toggle::Button
aria-controls={{this._contentId}}
aria-controls={{t.contentId}}
@text={{this.getText t.isOpen}}
@isOpen={{t.isOpen}}
{{on "click" t.onClickToggle}}
/>
</:toggle>

<:content>
<div class="hds-reveal__content hds-typography-body-200 hds-foreground-primary" id={{this._contentId}}>
<div class="hds-reveal__content hds-typography-body-200 hds-foreground-primary">
{{yield}}
</div>
</:content>
Expand Down
8 changes: 0 additions & 8 deletions packages/components/src/components/hds/reveal/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
*/

import Component from '@glimmer/component';
import { guidFor } from '@ember/object/internals';
import { assert } from '@ember/debug';

import type { HdsDisclosurePrimitiveSignature } from '../disclosure-primitive';
Expand All @@ -23,13 +22,6 @@ export interface HdsRevealSignature {
}

export default class HdsReveal extends Component<HdsRevealSignature> {
/**
* Generates a unique ID for the Content
*
* @param _contentId
*/
private _contentId = 'content-' + guidFor(this);

/**
* @param getText
* @type {string}
Expand Down
5 changes: 2 additions & 3 deletions showcase/tests/integration/components/hds/accordion/index-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ module('Integration | Component | hds/accordion/index', function (hooks) {
.hasAttribute('aria-expanded', 'true');
});

test('the AccordionItem toggle button has an aria-controls attribute with a value matching the content id', async function (assert) {
test('the AccordionItem toggle button has an aria-controls attribute with a value matching the DisclosurePrimitive content id', async function (assert) {
await render(
hbs`
<Hds::Accordion as |A|>
Expand All @@ -201,14 +201,13 @@ module('Integration | Component | hds/accordion/index', function (hooks) {
);
await click('.hds-accordion-item__button');
assert.dom('.hds-accordion-item__button').hasAttribute('aria-controls');
assert.dom('.hds-accordion-item__content').hasAttribute('id');

assert.strictEqual(
this.element
.querySelector('.hds-accordion-item__button')
.getAttribute('aria-controls'),
this.element
.querySelector('.hds-accordion-item__content')
.querySelector('.hds-disclosure-primitive__content')
.getAttribute('id')
);
});
Expand Down
39 changes: 23 additions & 16 deletions showcase/tests/integration/components/hds/disclosure-primitive/index-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,15 @@ module(
<:toggle>
<button type="button" id="test-disclosure-primitive-button" />
</:toggle>
<:content>
<a id="test-disclosure-primitive-link" href="#">test</a>
</:content>
</Hds::DisclosurePrimitive>
`);
assert.dom('.hds-disclosure-primitive__toggle').exists();
assert.dom('button#test-disclosure-primitive-button').exists();
assert.dom('.hds-disclosure-primitive__content').doesNotExist();
assert.dom('.hds-disclosure-primitive__content').exists();
assert.dom('a#test-disclosure-primitive-link').doesNotExist();
});
test('it should render the "content" when the "toggle" is clicked', async function (assert) {
await render(hbs`
Expand All @@ -52,7 +56,6 @@ module(
</Hds::DisclosurePrimitive>
`);
await click('button#test-disclosure-primitive-button');
assert.dom('.hds-disclosure-primitive__content').exists();
assert.dom('a#test-disclosure-primitive-link').exists();
});

Expand All @@ -70,11 +73,9 @@ module(
</:content>
</Hds::DisclosurePrimitive>
`);
assert.dom('.hds-disclosure-primitive__content').exists();
assert.dom('a#test-disclosure-primitive-link').exists();

this.set('isOpen', false);
assert.dom('.hds-disclosure-primitive__content').doesNotExist();
assert.dom('a#test-disclosure-primitive-link').doesNotExist();
});

Expand All @@ -90,11 +91,9 @@ module(
</Hds::DisclosurePrimitive>
`);
await click('button#test-toggle-button');
assert.dom('.hds-disclosure-primitive__content').exists();
assert.dom('a#test-disclosure-primitive-link').exists();

this.set('isOpen', false);
assert.dom('.hds-disclosure-primitive__content').doesNotExist();
assert.dom('a#test-disclosure-primitive-link').doesNotExist();
});

Expand All @@ -109,11 +108,9 @@ module(
</:content>
</Hds::DisclosurePrimitive>
`);
assert.dom('.hds-disclosure-primitive__content').doesNotExist();
assert.dom('a#test-disclosure-primitive-link').doesNotExist();

this.set('isOpen', true);
assert.dom('.hds-disclosure-primitive__content').exists();
assert.dom('a#test-disclosure-primitive-link').exists();
});

Expand All @@ -129,11 +126,9 @@ module(
</:content>
</Hds::DisclosurePrimitive>
`);
assert.dom('.hds-disclosure-primitive__content').exists();
assert.dom('a#test-disclosure-primitive-link').exists();

await click('button#test-toggle-button');
assert.dom('.hds-disclosure-primitive__content').doesNotExist();
assert.dom('a#test-disclosure-primitive-link').doesNotExist();
});

Expand All @@ -149,14 +144,28 @@ module(
</:content>
</Hds::DisclosurePrimitive>
`);
assert.dom('.hds-disclosure-primitive__content').doesNotExist();
assert.dom('a#test-disclosure-primitive-link').doesNotExist();

await click('button#test-toggle-button');
assert.dom('.hds-disclosure-primitive__content').exists();
assert.dom('a#test-disclosure-primitive-link').exists();
});

// contentId

test('it should set the contentId on the content block', async function (assert) {
await render(hbs`
<Hds::DisclosurePrimitive>
<:toggle>
<button type="button" id="test-disclosure-primitive-button" />
</:toggle>
<:content>
<a id="test-disclosure-primitive-link" href="#">test</a>
</:content>
</Hds::DisclosurePrimitive>
`);
assert.dom('.hds-disclosure-primitive__content').hasAttribute('id');
});

// CLOSE DISCLOSED CONTENT ON CLICK

test('it should hide the "content" when an interactive element triggers `close`', async function (assert) {
Expand All @@ -171,10 +180,8 @@ module(
</Hds::DisclosurePrimitive>
`);
await click('button#test-toggle-button');
assert.dom('.hds-disclosure-primitive__content').exists();
assert.dom('button#test-content-button').exists();
await click('button#test-content-button');
assert.dom('.hds-disclosure-primitive__content').doesNotExist();
assert.dom('button#test-content-button').doesNotExist();
});

Expand All @@ -196,11 +203,11 @@ module(
// toggle to open
await click('button#test-toggle-button');
assert.true(opened);
assert.dom('.hds-disclosure-primitive__content').exists();
assert.dom('a#test-disclosure-primitive-link').exists();
// toggle to close
await click('button#test-toggle-button');
assert.false(opened);
assert.dom('.hds-disclosure-primitive__content').doesNotExist();
assert.dom('a#test-disclosure-primitive-link').doesNotExist();
});
}
);
7 changes: 4 additions & 3 deletions showcase/tests/integration/components/hds/reveal/index-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,19 +66,20 @@ module('Integration | Component | hds/reveal/index', function (hooks) {
.hasAttribute('aria-expanded', 'true');
});

test('the toggle button has an aria-controls attribute with a value matching the content id', async function (assert) {
test('the toggle button has an aria-controls attribute with a value matching the DisclosurePrimitive content id', async function (assert) {
await render(
hbs`<Hds::Reveal @text="More options">Additional content</Hds::Reveal>`
);
await click('.hds-reveal__toggle-button');
assert.dom('.hds-reveal__toggle-button').hasAttribute('aria-controls');
assert.dom('.hds-reveal__content').hasAttribute('id');

assert.strictEqual(
this.element
.querySelector('.hds-reveal__toggle-button')
.getAttribute('aria-controls'),
this.element.querySelector('.hds-reveal__content').getAttribute('id')
this.element
.querySelector('.hds-disclosure-primitive__content')
.getAttribute('id')
);
});

Expand Down
Loading