LPS-201300 Fixing Accessibility Issue at Clay Panel Component #5716

AkramYoussoufi · 2023-11-09T16:43:20Z

ethib137 · 2023-11-10T11:37:03Z

Hi @AkramYoussoufi , did you try using this component after your changes with your keyboard and a screen reader? You'll notice not much info is provided when interacting with a panel. We need these roles to make it work properly with accessible technologies.

AkramYoussoufi · 2023-11-10T13:30:54Z

Hi @ethib137, I have uploaded a video in the comment section in the Ticket to check.

ethib137 · 2023-11-13T05:22:03Z

Hey @AkramYoussoufi , this change still won't work. It breaks the proper HTML semantics. Mozilla explains here:

Elements with the role tab must either be a child of an element with the tablist role, or have their id as part of the aria-owns property of a tablist.

Since you have removed the tablist role entirely, this breaks those requirements.

AkramYoussoufi · 2023-11-14T10:12:16Z

@ethib137 in this changes, I have fixed the conflict happens between ClayGroupPanel and ClayPanelComponent

Because the GroupPanel Component has its own role="tablist"


const ClayPanelGroup = ({
	children,
	className,
	fluid,
	fluidFirst,
	fluidLast,
	flush,
	small,
	...otherProps
}: IProps) => {
	return (
		<div
			{...otherProps}
			aria-orientation="vertical"
			className={classNames('panel-group', className, {
				'panel-group-fluid': fluid,
				'panel-group-fluid-first': fluidFirst,
				'panel-group-fluid-last': fluidLast,
				'panel-group-flush': flush,
				'panel-group-sm': small,
			})}
			role="tablist"
		>
			{children}
		</div>
	);
};

and on the ClayPanelComponent also has the tablist role

this cause issue when using ClayPanelGroup and ClayPanel Component together ex:

	<ClayPanel.Group>
		{['One', 'Two', 'Three'].map((item) => (
			<ClayPanel
				collapsable
				displayTitle={item}
				key={item}
				showCollapseIcon
			>
				<ClayPanel.Body>
					{`Here is some content inside for number ${item}`}
				</ClayPanel.Body>
			</ClayPanel>
		))}
	</ClayPanel.Group>

in this scenario, the ClayPanelGroup will look for an element with tab and tabpanel and will not find them because the ClayPanel assigns to it the tab and tabpanel as its own element causing the ClayPanel.Group triggering the issue of missing elements when scanning with devTools.

I have added a checker which check if the GroupPanel is used as parent if yes in this case the ClayPanel will not use role=tabpanel and leave it to the ClayPanelGroup to handle the assignment.

AkramYoussoufi · 2023-11-14T11:03:27Z

one problem present now is that ClayPanel Component uses the ClayPanel.Body as its own child this caused problems since the ClayPanel.Body has and must have the tabpanel role.

based on mozilla doc you gave me and devTools scanner

the tabpanel should not be a child to the tablist but rather it needs to be a sibling!

and the ClayPanel Component is built to receive the ClayPanel.Body as its own child! based on the code source and this is the normal implementation :

			<ClayPanel
				collapsable
				displayTitle={item}
				key={item}
				showCollapseIcon
			>
				<ClayPanel.Body>
					{`Here is some content inside for number ${item}`}
				</ClayPanel.Body>
			</ClayPanel>

we can fix this technically by making the


				<ClayPanel.Body>
					{`Here is some content inside for number ${item}`}
				</ClayPanel.Body>

outside the ClayPanel ex:

			<ClayPanel
				collapsable
				displayTitle={item}
				key={item}
				showCollapseIcon
			/>
			<ClayPanel.Body>
				{`Here is some content inside for number ${item}`}
			</ClayPanel.Body>

like this ClayPanel will act as the tab while the ClayPanel.Body act as the tabpanel but still we will need to assign ClayPanel.Group as the parent to the ClayPanel with role="tablist" to be in the correct structure

	<ClayPanel.Group>
			<ClayPanel
				collapsable
				displayTitle={item}
				key={item}
				showCollapseIcon
			>
			</ClayPanel>
	</ClayPanel.Group>
	<ClayPanel.Body>
		{`Here is some content inside for number ${item}`}
	</ClayPanel.Body>

like this we will have

-tablist
  --tab
-tabpanel

as the right structure and ClayGroup will became a must use case for the ClayPanel

AkramYoussoufi · 2023-11-24T15:04:54Z

Hello @ethib137, disclosure pattern is implemented

ethib137

Thanks @AkramYoussoufi . Couple things to change... Also the deploy preview is braking, so it would be good to look into why.

ethib137 · 2023-11-27T09:27:41Z

packages/clay-panel/src/index.tsx

@@ -146,9 +146,9 @@ function ClayPanel({
 								show: internalExpanded,
 							}
 						)}
+						data-disclosure


What is the purpose of adding this data attribute?

It is a mark defining the pattern used in this element

I removed it

ethib137 · 2023-11-27T09:28:41Z

packages/clay-panel/src/index.tsx

@@ -135,6 +134,7 @@ function ClayPanel({
 			{collapsable && (
 				<>
 					<ClayButton
+						aria-controls="panel-body"


The value of aria-controls needs to be the id of the element it controls.

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-controls

AkramYoussoufi · 2023-11-27T11:28:08Z

@ethib137 required pattern is implemented I added a logic that will look for the number of panels within the parent element and give each panel element its needed index within a group of panels.

AkramYoussoufi · 2023-11-27T13:10:53Z

@ethib137 I don't understand why the CI return failed on the test because the test in my local passed successfully:

$ npm run test

> @clayui/[email protected] test
> jest --config ../../jest.config.js --updateSnapshot

ts-jest[versions] (WARN) Version 27.5.1 of jest installed has not been tested with ts-jest. If you're experiencing issues, consider using a supported version (>=28.0.0 <29.0.0-0). Please do not report issues in ts-jest if you are using unsupported versions.
 PASS  src/__tests__/index.tsx (6.488 s)
  ClayPanel
    ✓ renders (31 ms)
    ✓ renders with different displayType (7 ms)
    ✓ renders with multiple panels (20 ms)
    ✓ renders with multiple small panels (9 ms)
    ✓ renders with custom displayTitle (5 ms)
    ✓ renders without displayTitle (4 ms)
  ClayPanel Interactions
    ✓ clicking the title should expand and close the content (21 ms)

ethib137 · 2023-11-27T13:15:03Z

packages/clay-panel/src/index.tsx

@@ -135,6 +151,7 @@ function ClayPanel({
 			{collapsable && (
 				<>
 					<ClayButton
+						aria-controls={`panel-index-${indexOfElement}`}


Hey @AkramYoussoufi can you look at other instances of how we generate an ID for aria-controls and copy that pattern?

AkramYoussoufi · 2023-11-27T13:21:46Z

@ethib137 Done!

matuzalemsteles · 2023-11-27T22:06:08Z

packages/clay-panel/src/index.tsx

@@ -201,7 +204,6 @@ function ClayPanel({
 						onExiting={(element) => {
 							element.style.height = '';
 						}}
-						role="tabpanel"


I was talking to Marcos about this and one of the things we can follow here is to use the w3c accessibility recommendation for accordion which is similar to panel https://www.w3.org/WAI/ARIA/apg/patterns/accordion/examples/accordion/, we can add role="region" here instead of tabpanel. The other implementations part is ok too.

matuzalemsteles · 2023-11-27T22:14:41Z

packages/clay-panel/package.json

@@ -18,7 +18,7 @@
 		"build": "cross-env NODE_ENV=production babel src --root-mode upward --out-dir lib --extensions .ts,.tsx",
 		"buildTypes": "cross-env NODE_ENV=production tsc --project ./tsconfig.declarations.json",
 		"prepublishOnly": "yarn build && yarn buildTypes",
-		"test": "jest --config ../../jest.config.js"
+		"test": "jest --config ../../jest.config.js --updateSnapshot"


You can remove updateSnapshot here, we don't want this command by default because it will make the snapshot unusable we should only use it when we intend to update the markup. If you maintain this, we can update in a snapshot in cases where the markup change is a bug.

AkramYoussoufi · 2023-11-28T08:37:00Z

@matuzalemsteles @ethib137 all the changes have been implemented

I don't understand what causes netlify to fail but my jest and build is working perfectly fine in my local.

Co-Authored-By: Akram Youssoufi <[email protected]>

matuzalemsteles

LGTM! These storybook and website errors don't make any sense, I'll go ahead and see if the builds still fail on master.

AkramYoussoufi force-pushed the LPS-201300 branch 3 times, most recently from cea4627 to 76b7672 Compare November 10, 2023 11:30

AkramYoussoufi force-pushed the LPS-201300 branch 3 times, most recently from f45dfc4 to 59f8e81 Compare November 14, 2023 10:03

AkramYoussoufi force-pushed the LPS-201300 branch 4 times, most recently from 9448efd to 9b5b89e Compare November 14, 2023 10:37

AkramYoussoufi force-pushed the LPS-201300 branch from 9b5b89e to 1fc4949 Compare November 24, 2023 14:52

ethib137 requested changes Nov 27, 2023

View reviewed changes

AkramYoussoufi force-pushed the LPS-201300 branch 4 times, most recently from 5d98985 to 0b3ba3c Compare November 27, 2023 11:26

AkramYoussoufi force-pushed the LPS-201300 branch from 0b3ba3c to a91b228 Compare November 27, 2023 12:46

AkramYoussoufi force-pushed the LPS-201300 branch from a91b228 to b296c1b Compare November 27, 2023 13:15

ethib137 requested changes Nov 27, 2023

View reviewed changes

AkramYoussoufi force-pushed the LPS-201300 branch from b296c1b to e369936 Compare November 27, 2023 13:21

matuzalemsteles reviewed Nov 27, 2023

View reviewed changes

AkramYoussoufi force-pushed the LPS-201300 branch from e369936 to 45bd3eb Compare November 28, 2023 08:36

AkramYoussoufi force-pushed the LPS-201300 branch from 45bd3eb to e3c7573 Compare November 28, 2023 08:44

matuzalemsteles force-pushed the LPS-201300 branch from e3c7573 to 30ca61c Compare November 29, 2023 22:35

fix(@clayui/panel): Disclosure Pattern to Clay Panel Component

6ffa238

Co-Authored-By: Akram Youssoufi <[email protected]>

matuzalemsteles force-pushed the LPS-201300 branch from 30ca61c to 6ffa238 Compare November 29, 2023 22:42

matuzalemsteles approved these changes Nov 29, 2023

View reviewed changes

matuzalemsteles merged commit 5eaf690 into liferay:master Nov 29, 2023
2 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LPS-201300 Fixing Accessibility Issue at Clay Panel Component #5716

LPS-201300 Fixing Accessibility Issue at Clay Panel Component #5716

AkramYoussoufi commented Nov 9, 2023

ethib137 commented Nov 10, 2023

AkramYoussoufi commented Nov 10, 2023 •

edited

Loading

ethib137 commented Nov 13, 2023

AkramYoussoufi commented Nov 14, 2023 •

edited

Loading

AkramYoussoufi commented Nov 14, 2023 •

edited

Loading

AkramYoussoufi commented Nov 24, 2023

ethib137 left a comment

ethib137 Nov 27, 2023

AkramYoussoufi Nov 27, 2023

AkramYoussoufi Nov 27, 2023

ethib137 Nov 27, 2023

AkramYoussoufi Nov 27, 2023

AkramYoussoufi commented Nov 27, 2023

AkramYoussoufi commented Nov 27, 2023

ethib137 Nov 27, 2023

AkramYoussoufi Nov 27, 2023

AkramYoussoufi commented Nov 27, 2023

matuzalemsteles Nov 27, 2023

matuzalemsteles Nov 27, 2023

AkramYoussoufi commented Nov 28, 2023 •

edited

Loading

matuzalemsteles left a comment

LPS-201300 Fixing Accessibility Issue at Clay Panel Component #5716

LPS-201300 Fixing Accessibility Issue at Clay Panel Component #5716

Conversation

AkramYoussoufi commented Nov 9, 2023

ethib137 commented Nov 10, 2023

AkramYoussoufi commented Nov 10, 2023 • edited Loading

ethib137 commented Nov 13, 2023

AkramYoussoufi commented Nov 14, 2023 • edited Loading

AkramYoussoufi commented Nov 14, 2023 • edited Loading

AkramYoussoufi commented Nov 24, 2023

ethib137 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkramYoussoufi commented Nov 27, 2023

AkramYoussoufi commented Nov 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkramYoussoufi commented Nov 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkramYoussoufi commented Nov 28, 2023 • edited Loading

matuzalemsteles left a comment

Choose a reason for hiding this comment

AkramYoussoufi commented Nov 10, 2023 •

edited

Loading

AkramYoussoufi commented Nov 14, 2023 •

edited

Loading

AkramYoussoufi commented Nov 14, 2023 •

edited

Loading

AkramYoussoufi commented Nov 28, 2023 •

edited

Loading