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.

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

hds-tooltip - Implement a11y toggle content pattern #2648

Merged
merged 7 commits into from
Feb 7, 2025

Conversation

dchyun
Copy link
Contributor

@dchyun dchyun commented Jan 14, 2025

📌 Summary
This PR implements usage of the aria-controls attribute in the hds-tooltip modifier to align show/hide behavior across the repo as follows in the initiative from HDS-3581.

The hds-tooltip modifier is used in the TooltipButton.

🛠️ Detailed description
Currently in the hds-tooltip modifier, the toggled content does not follow the proper a11y structure for toggled content, or leverage aria-controls.

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 hds-tooltip follows a pattern where the entire content block is added and removed on each toggle, and any aria- attributes are set to the id of this removable block.

In this PR a new hds-tooltip-container block is now added as a sibling of the tooltip button, and is present in the DOM at all times. The yielded content is removed or added inside this container based on the tooltip visibility.

The aria-describedby and aria-controls attributes of the tooltip element now reference the hds-tooltip-container.

Note: The tooltip library leveraged, tippy.js, does not support this DOM structure out of the box or with any properties, so the container element has been created manually, and the tippy propery appendTo has been used to attach the tooltip content to this element.

📸 Screenshots

Before

Inactive
Screenshot 2025-01-22 at 9 42 30 AM

Active
Screenshot 2025-01-22 at 9 45 49 AM

After

Inactive
Screenshot 2025-01-22 at 9 40 33 AM

Active
Screenshot 2025-01-22 at 9 41 59 AM

🔗 External links

Jira ticket: HDS-4327


👀 Component checklist

💬 Please consider using conventional comments when reviewing this PR.

Copy link

vercel bot commented Jan 14, 2025

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

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Feb 3, 2025 10:03pm
hds-website ✅ Ready (Inspect) Visit Preview Feb 3, 2025 10:03pm

@dchyun
Copy link
Contributor Author

dchyun commented Jan 16, 2025

In smoke testing this change in nomad, there is a failing test that occurs due to this proposed change. The failed test is related to the job status component which contains a Hds::TooltipButton.
Screenshot 2025-01-16 at 12 32 39 PM

Update: Making some code changes to the implementation made it so this failed test can be avoided.

shleewhite
shleewhite previously approved these changes Jan 22, 2025
Copy link
Contributor

@shleewhite shleewhite left a comment

Choose a reason for hiding this comment

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

Works great with VoiceOver! Just had one non-blocking note on the changelog.

zamoore
zamoore previously approved these changes Jan 30, 2025
Copy link
Contributor

@KristinLBradley KristinLBradley left a comment

Choose a reason for hiding this comment

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

Looks good!

@dchyun dchyun merged commit 346674c into main Feb 7, 2025
14 checks passed
@dchyun dchyun deleted the dchyun/tooltip-implement-aria-controls branch February 7, 2025 21:40
@hashibot-hds hashibot-hds mentioned this pull request Feb 7, 2025
Comment on lines +25 to +26
assert.dom('.hds-tooltip-container').exists();
assert.dom('.hds-tooltip-container').hasAttribute('id');
Copy link
Member

Choose a reason for hiding this comment

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

is this a sign that our consumers may require some adjustments in tests as well? we'll likely find in our smoke testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did smoke test this change in cloud ui, atlas, and nomad, and observed no failing tests. But this change does cause DOM changes so it is possible there could be failing tests introduced in other consumer repos.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants