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

[EuiAccordion] Add configurable role prop and change default role to group #7326

Merged
merged 6 commits into from
Oct 30, 2023
Merged

[EuiAccordion] Add configurable role prop and change default role to group #7326

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dbb4fd7
[EuiAccordion] Add `role` prop and default to `group` (was previously…
cee-chen Oct 27, 2023
ac093b0
Update tests/snapshots
cee-chen Oct 27, 2023
d894a5b
Update downstream snapshots
cee-chen Oct 27, 2023
d34ac51
[EuiCollapsibleNavBeta] Update static group components to set a `role…
cee-chen Oct 27, 2023
6e2599c
changelog
cee-chen Oct 27, 2023
bb4f0aa
Fix props table not showing `role` prop
cee-chen Oct 27, 2023
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
46 changes: 23 additions & 23 deletions src/components/accordion/__snapshots__/accordion.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ exports[`EuiAccordion behavior closes when clicked twice 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isClosed"
id="25"
inert=""
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down Expand Up @@ -95,7 +95,7 @@ exports[`EuiAccordion behavior does not open when isDisabled 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isClosed"
id="23"
inert=""
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down Expand Up @@ -148,7 +148,7 @@ exports[`EuiAccordion behavior opens when clicked once 1`] = `
aria-labelledby="generated-id"
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isOpen"
id="22"
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down Expand Up @@ -201,7 +201,7 @@ exports[`EuiAccordion behavior opens when div is clicked if element is a div 1`]
aria-labelledby="generated-id"
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isOpen"
id="24"
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down Expand Up @@ -257,7 +257,7 @@ exports[`EuiAccordion is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isClosed"
id="0"
inert=""
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down Expand Up @@ -309,7 +309,7 @@ exports[`EuiAccordion isDisabled is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isClosed"
id="21"
inert=""
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down Expand Up @@ -344,7 +344,7 @@ exports[`EuiAccordion props arrowDisplay none is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isClosed"
id="14"
inert=""
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down Expand Up @@ -394,7 +394,7 @@ exports[`EuiAccordion props arrowDisplay right is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isClosed"
id="13"
inert=""
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down Expand Up @@ -446,7 +446,7 @@ exports[`EuiAccordion props arrowProps is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isClosed"
id="15"
inert=""
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down Expand Up @@ -500,7 +500,7 @@ exports[`EuiAccordion props buttonContent is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isClosed"
id="3"
inert=""
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down Expand Up @@ -550,7 +550,7 @@ exports[`EuiAccordion props buttonContentClassName is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isClosed"
id="2"
inert=""
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down Expand Up @@ -598,7 +598,7 @@ exports[`EuiAccordion props buttonElement is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isClosed"
id="10"
inert=""
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down Expand Up @@ -650,7 +650,7 @@ exports[`EuiAccordion props buttonProps is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isClosed"
id="4"
inert=""
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down Expand Up @@ -701,7 +701,7 @@ exports[`EuiAccordion props buttonProps paddingSize l 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isClosed"
id="7"
inert=""
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down Expand Up @@ -752,7 +752,7 @@ exports[`EuiAccordion props buttonProps paddingSize m 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isClosed"
id="6"
inert=""
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down Expand Up @@ -803,7 +803,7 @@ exports[`EuiAccordion props buttonProps paddingSize s 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isClosed"
id="5"
inert=""
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down Expand Up @@ -851,7 +851,7 @@ exports[`EuiAccordion props element is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isClosed"
id="1"
inert=""
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down Expand Up @@ -908,7 +908,7 @@ exports[`EuiAccordion props extraAction is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isClosed"
id="11"
inert=""
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down Expand Up @@ -958,7 +958,7 @@ exports[`EuiAccordion props forceState closed is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isClosed"
id="16"
inert=""
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down Expand Up @@ -1011,7 +1011,7 @@ exports[`EuiAccordion props forceState open is rendered 1`] = `
aria-labelledby="generated-id"
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isOpen"
id="17"
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down Expand Up @@ -1064,7 +1064,7 @@ exports[`EuiAccordion props initialIsOpen is rendered 1`] = `
aria-labelledby="generated-id"
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isOpen"
id="12"
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down Expand Up @@ -1127,7 +1127,7 @@ exports[`EuiAccordion props isLoading is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isClosed"
id="19"
inert=""
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down Expand Up @@ -1186,7 +1186,7 @@ exports[`EuiAccordion props isLoadingMessage is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isClosed"
id="20"
inert=""
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down
11 changes: 10 additions & 1 deletion src/components/accordion/accordion.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,15 @@ describe('EuiAccordion', () => {
});
});

test('role', () => {
const { queryByRole } = render(
<EuiAccordion id="role-region" role="region" />
);

expect(queryByRole('region')).toBeInTheDocument();
expect(queryByRole('group')).not.toBeInTheDocument();
});

describe('buttonContentClassName', () => {
it('is rendered', () => {
const { container } = render(
Expand Down Expand Up @@ -313,7 +322,7 @@ describe('EuiAccordion', () => {

it('moves focus to the content when expanded', () => {
const component = mount(<EuiAccordion id={getId()} />);
const childWrapper = component.find('div[role="region"]').getDOMNode();
const childWrapper = component.find('div[role="group"]').getDOMNode();

expect(childWrapper).not.toBeFalsy();
expect(childWrapper).not.toBe(document.activeElement);
Expand Down
13 changes: 12 additions & 1 deletion src/components/accordion/accordion.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,21 @@ export const PADDING_SIZES = ['none', 'xs', 's', 'm', 'l', 'xl'] as const;
export type EuiAccordionPaddingSize = (typeof PADDING_SIZES)[number];

export type EuiAccordionProps = CommonProps &
Omit<HTMLAttributes<HTMLElement>, 'id'> & {
Omit<HTMLAttributes<HTMLElement>, 'id' | 'role'> & {
id: string;
/**
* Applied to the entire .euiAccordion wrapper.
* When using `fieldset`, it will enforce `buttonElement = legend` as well.
*/
element?: 'div' | 'fieldset';
/**
* Defaults to the [group role](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/group_role).
*
* If your accordion contains significant enough content to be a document
* [landmark role](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/region_role#accessibility_concerns), consider using the `region` role instead.
* @default group
*/
role?: HTMLAttributes<HTMLElement>['role'];
Comment on lines +36 to +43
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@1Copenut Do you feel like this needs its own documentation section in our Accordion docs? Or is the prop documentation sufficient?

To be honest, I could be way off here, but I strongly suspect that 90%+ of accordion use cases do not fit a 'landmark' role, and I'd rather have people under rather than overindexing on that 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

@cee-chen I agree with your assessment here, especially now that I've had a chance to drive it with multiple screen readers. You mentioned that you felt landmarks were designated waypoints on the page last week. I started considering landmarks like points on a map and yeah, those shouldn't be ephemeral. They should be present and predictable. Accordions—and by proxy their content—can be expanded and collapsed, so the content is not always present and is not always predictable.

Let's keep the documentation just in props for now. If folks really need to upgrade a content block's status they'll have a means to do so. But my hunch is a good content strategy would recommend making content worthy of a role="region" an always-on-screen block.

/**
* Class that will apply to the trigger for the accordion.
*/
Expand Down Expand Up @@ -124,6 +132,7 @@ export class EuiAccordionClass extends Component<
isLoadingMessage: false,
element: 'div' as const,
buttonElement: 'button' as const,
role: 'group' as const,
};

state = {
Expand Down Expand Up @@ -184,6 +193,7 @@ export class EuiAccordionClass extends Component<
children,
className,
id,
role,
element: Element = 'div',
buttonElement,
buttonProps,
Expand Down Expand Up @@ -239,6 +249,7 @@ export class EuiAccordionClass extends Component<
/>

<EuiAccordionChildren
role={role}
id={id}
aria-labelledby={buttonId}
paddingSize={paddingSize}
Expand Down
5 changes: 3 additions & 2 deletions src/components/accordion/accordion_children/accordion_children.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,15 @@ import {
type _EuiAccordionChildrenProps = HTMLAttributes<HTMLDivElement> &
Pick<
EuiAccordionProps,
'children' | 'paddingSize' | 'isLoading' | 'isLoadingMessage'
'role' | 'children' | 'paddingSize' | 'isLoading' | 'isLoadingMessage'
> & {
isOpen: boolean;
accordionChildrenRef: Ref<HTMLDivElement>;
};
export const EuiAccordionChildren: FunctionComponent<
_EuiAccordionChildrenProps
> = ({
role,
children,
accordionChildrenRef,
paddingSize,
Expand Down Expand Up @@ -90,7 +91,7 @@ export const EuiAccordionChildren: FunctionComponent<
css={wrapperCssStyles}
style={heightInlineStyle}
ref={accordionChildrenRef}
role="region"
role={role}
tabIndex={-1}
// @ts-expect-error - inert property not yet available in React TS defs. TODO: Remove this once https://github.com/DefinitelyTyped/DefinitelyTyped/pull/60822 is merged
inert={!isOpen ? '' : undefined} // Can't pass a boolean currently, Jest throws errors
Expand Down
4 changes: 2 additions & 2 deletions ...s/collapsible_nav/collapsible_nav_group/__snapshots__/collapsible_nav_group.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ exports[`EuiCollapsibleNavGroup when isCollapsible is true accepts accordion pro
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isClosed"
id="id"
inert=""
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down Expand Up @@ -319,7 +319,7 @@ exports[`EuiCollapsibleNavGroup when isCollapsible is true will render an accord
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isClosed"
id="id"
inert=""
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down
3 changes: 3 additions & 0 deletions ...lapsible_nav_beta/collapsible_nav_group/__snapshots__/collapsible_nav_group.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ exports[`EuiCollapsibleNavGroup renders 1`] = `
aria-label="aria-label"
class="euiCollapsibleNavLink euiCollapsibleNavItem emotion-euiCollapsibleNavLink-isTopItem-isNotAccordion-euiCollapsibleNavGroup__title"
data-test-subj="test subject string"
id="generated-id"
>
<span
data-euiicon-type="home"
Expand All @@ -19,7 +20,9 @@ exports[`EuiCollapsibleNavGroup renders 1`] = `
</span>
</span>
<div
aria-labelledby="generated-id"
class="euiCollapsibleNavItem__items emotion-euiCollapsibleNavItem__items-isGroup"
role="group"
>
<span
class="euiCollapsibleNavLink euiCollapsibleNavSubItem emotion-euiCollapsibleNavLink-isSubItem"
Expand Down
12 changes: 10 additions & 2 deletions src/components/collapsible_nav_beta/collapsible_nav_group/collapsible_nav_group.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import React, { FunctionComponent, HTMLAttributes, useContext } from 'react';
import classNames from 'classnames';

import { useEuiTheme } from '../../../services';
import { useEuiTheme, useGeneratedHtmlId } from '../../../services';
import { CommonProps } from '../../common';

import { EuiCollapsibleNavContext } from '../context';
Expand Down Expand Up @@ -65,17 +65,25 @@ export const EuiCollapsibleNavGroup: FunctionComponent<
wrapperProps?.css,
];

const labelledById = useGeneratedHtmlId();

return (
<div {...wrapperProps} className={classes} css={cssStyles}>
{isCollapsed && isPush ? (
<EuiCollapsedNavPopover className={classes} items={items} {...props} />
) : (
<>
<EuiCollapsibleNavItem
id={labelledById}
{...props}
css={styles.euiCollapsibleNavGroup__title}
/>
<EuiCollapsibleNavSubItems items={items} isGroup />
<EuiCollapsibleNavSubItems
items={items}
isGroup
role="group"
aria-labelledby={props.id || labelledById}
/>
</>
)}
</div>
Expand Down
4 changes: 2 additions & 2 deletions ...sible_nav_beta/collapsible_nav_item/__snapshots__/collapsible_nav_accordion.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ exports[`EuiCollapsibleNavAccordion renders as a sub item 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isClosed"
id="generated-id"
inert=""
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down Expand Up @@ -113,7 +113,7 @@ exports[`EuiCollapsibleNavAccordion renders as a top level item 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isClosed"
id="generated-id"
inert=""
role="region"
role="group"
style="block-size: 0;"
tabindex="-1"
>
Expand Down
Loading
Loading