fix: Fix Clay Components with Accessibility Issues
Jira Ticker: https://liferay.atlassian.net/browse/LPD-72486
This PR fix some accessibility issues from Clay components that were discovered during the migration of Clay Accessibility test from Poshi to Playwright
Progress Bar: Ensures every ARIA progressbar node has an accessible name. Tree View: Invalid ARIA attribute value: aria-owns, Ensures buttons have discernible text.
Steps to verify:
- Go to Storybook > Element (e.g., Tree View, Progress Bar)
- Scan the page with axeDevtool selecting all rules
The latest updates on your projects. Learn more about Vercel for GitHub.
| Project | Deployment | Review | Updated (UTC) |
|---|---|---|---|
| clayui.com | Preview, Comment | Dec 16, 2025 1:45pm | |
| storybook.clayui.com | Preview, Comment | Dec 16, 2025 1:45pm |
Hey @pat270 about:
I also noticed some other issues with this component.
The checkboxes should have aria-labelledby={THE_ID_OF_COMPONENT_TEXT}. The checkbox and component-text both have the same id. We mix role="group" and role="treeitem".
Didn't get all points, but looking at the code:
- The checkboxes should have aria-labelledby={THE_ID_OF_COMPONENT_TEXT}. That's already happening when we check if the child display name is
ClayCheckbox. - I disagree, the checkbox and the component-text do not have the same id, but I think it will fail when you try to use the
labelIdfor the fallback (which was neither the box nor the text) and, at the same time, the checkbox is present. - The
treeitemis the individual item (the row) that you click. Thegroupis the<ul>(list) that contains all the nested children. I think this combination is the correct ARIA structure for a tree. Hierarchy: tree -> treeitem (Parent) -> group (Container) -> treeitem (Child).
Let me know if I understood something wrong.
Update: I realized that in StyleBooks we need to wrap the Checkbox to avoid TypeScript errors with required properties, and this causes the component to lose its displayName property, which caused the child component of the checkbox to fall into the else block, thus overwriting the id with labelId.
- The screen reader should be announcing the title when you focus the checkbox. It only announces "unchecked checkbox" when you tab to it.
The announcement should be "Entering Repositories cell. Repositories, unchecked, checkbox".
- In the DOM, the
custom-control-inputandcomponent-textdo have the same id.
- I was wrong about this one. I was navigating incorrectly.
Hey @pat270. I realized that in StyleBooks we need to wrap the Checkbox to avoid TypeScript errors with required properties, and this causes the component to lose its displayName property, which caused the child component of the checkbox to fall into the else block, thus overwriting the id with labelId. I think the last commits should fix the extra accessibility issues. Let me know if you find some other issues with this component.
I'm seeing issues in Treeview with checkboxes. It's selecting / unselecting everything.
I'm seeing issues in Treeview with checkboxes. It's selecting / unselecting everything.
Hi @pat270. I don't think that's an issue. I believe you're using old examples from the stylebook for comparison, and as I mentioned in the stylebook examples, the Checkbox component was being wrapped to avoid TypeScript errors with required properties. This caused the component to lose its displayName property, which led the Checkbox's child component to enter the else block, overwriting id with labelId. It also caused the Checkbox to not behave as it should, missing some props. So I adjusted the examples, keeping the displayName = ClayCheckbox, making the Treeview work correctly. The behavior you're seeing is probably being caused by the property selectionMode="multiple-recursive", if you change it to selectionMode="multiple" I believe it will work as you expect.
@MarioLeandro the checkboxes shouldn't be selected / unselected when clicking to expand. It should only be selected when clicking the checkbox. It seems to be a previously existing bug with the storybook examples, but it's more noticeable here.
For
const OptionalCheckbox = Object.assign(
(props: any) => <Checkbox {...props} />,
{
displayName: 'ClayCheckbox',
}
);
why do we need displayName? I don't see that prop in https://github.com/liferay/clay/blob/master/packages/clay-form/src/Checkbox.tsx
Hi @pat270, I understand now. I just submitted a change that fixes the bug you mentioned.
For
const OptionalCheckbox = Object.assign(
(props: any) => <Checkbox {...props} />,
{
displayName: 'ClayCheckbox',
}
);
We need it for this check
@MarioLeandro We can use <ClayCheckbox /> directly without TS errors. Not sure if we needed this pattern when we were on an older version of TS or not, but we don't need OptionalCheckbox any more.
Hey @pat270, I believe this pattern is being used because <ClayCheckbox /> requires the checked property, and since the Treeview overrides it in this case, I don't think it would make sense to pass it to the component. However, I could do <ClayCheckbox {...({} as any)} /> directly. What do you think?
@MarioLeandro directly replacing seems to be working for me without issue, see my commit at https://github.com/liferay/clay/pull/6217.
@MarioLeandro directly replacing seems to be working for me without issue, see my commit at #6217.
Changed it @pat270. I was using VSCode TS version, started using the workspace one and the error no longer appears. Thanks
Hey @ethib137 found another way to validate the toggle and remove that questionable code. However, other points are using @ts-ignore, and from what I've seen, most of them relate to checking the displayName property. Since it's an old component, I believe the only way to overcome this would be by refactoring it. The idea that comes to mind would be to add the checkbox and the icon to the pattern composition as <TreeView.Checkbox> and <TreeView.Icon>, but anyway these components will need to access some item properties like labelId, loading, disabled. What do you think ? I'm not sure if this PR is the right place for this since it's focused on accessibility.
I found a bug here. Click on Minerals to expand and then click on Native Elements to expand. Native Elements will not expand. Can you add a new test for this case?
Hi @ethib137 created some tests to ensure the expansion. Regarding the arrow down behavior, i forgot to mention, but Patrick sent a comment Where it said the button represented by the arrow should no longer exist, considering that, the behavior was moved to the parent element. Because Storybook was overriding the onClick event, it ended up not working correctly. I adjusted everything and also checked all the examples, due to the removal of the button and all actions being performed by the parent element, some properties that before was passed directly on the item now it's only available by the parent (e.g., expanderDisabled, onLoadMore), and also the behavior became the following:
- Clicking on the item will expand/collapse
- Clicking on the checkbox will perform the check.
@MarioLeandro we just need to reduce the number of commits, otherwise LGTM
Hi @pat270! Just reduced the number of commits, i think it's fine now. Also i think that we will need to update the portal, mainly due to changes in the treeview, but also to unblock https://github.com/liferay-platform-experience/liferay-portal/pull/1565. Should I create a ticket for this? @ethib137
Hi @pat270! Just reduced the number of commits, i think it's fine now. Also i think that we will need to update the portal, mainly due to changes in the treeview, but also to unblock liferay-platform-experience/liferay-portal#1565. Should I create a ticket for this? @ethib137
@MarioLeandro the changes would just be updating broken tests and adding language keys, right? If that's it we can just add those changes as part of the Clay release since those changes are needed along with the release.