clay icon indicating copy to clipboard operation
clay copied to clipboard

fix: Fix Clay Components with Accessibility Issues

Open MarioLeandro opened this issue 4 weeks ago • 18 comments

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:

  1. Go to Storybook > Element (e.g., Tree View, Progress Bar)
  2. Scan the page with axeDevtool selecting all rules

MarioLeandro avatar Dec 01 '25 19:12 MarioLeandro

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
clayui.com Ready Ready Preview, Comment Dec 16, 2025 1:45pm
storybook.clayui.com Ready Ready Preview, Comment Dec 16, 2025 1:45pm

vercel[bot] avatar Dec 01 '25 19:12 vercel[bot]

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:

  1. 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.
  2. 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 labelId for the fallback (which was neither the box nor the text) and, at the same time, the checkbox is present.
  3. The treeitem is the individual item (the row) that you click. The group is 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.

MarioLeandro avatar Dec 02 '25 15:12 MarioLeandro

  1. The screen reader should be announcing the title when you focus the checkbox. It only announces "unchecked checkbox" when you tab to it.

treeview-checkbox

The announcement should be "Entering Repositories cell. Repositories, unchecked, checkbox".

  1. In the DOM, the custom-control-input and component-text do have the same id.

same-id

  1. I was wrong about this one. I was navigating incorrectly.

pat270 avatar Dec 03 '25 02:12 pat270

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.

MarioLeandro avatar Dec 03 '25 11:12 MarioLeandro

I'm seeing issues in Treeview with checkboxes. It's selecting / unselecting everything.

treeview-selection

pat270 avatar Dec 03 '25 14:12 pat270

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 avatar Dec 03 '25 17:12 MarioLeandro

@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

pat270 avatar Dec 04 '25 16:12 pat270

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 avatar Dec 04 '25 17:12 MarioLeandro

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

pat270 avatar Dec 09 '25 01:12 pat270

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 avatar Dec 09 '25 14:12 MarioLeandro

@MarioLeandro directly replacing seems to be working for me without issue, see my commit at https://github.com/liferay/clay/pull/6217.

pat270 avatar Dec 09 '25 15:12 pat270

@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

MarioLeandro avatar Dec 09 '25 17:12 MarioLeandro

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.

MarioLeandro avatar Dec 09 '25 19:12 MarioLeandro

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?

ethib137 avatar Dec 11 '25 10:12 ethib137

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 avatar Dec 11 '25 19:12 MarioLeandro

@MarioLeandro we just need to reduce the number of commits, otherwise LGTM

pat270 avatar Dec 12 '25 20:12 pat270

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

MarioLeandro avatar Dec 15 '25 16:12 MarioLeandro

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.

ethib137 avatar Dec 16 '25 10:12 ethib137