Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions packages/react-core/src/components/Modal/Modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ export interface ModalProps extends React.HTMLProps<HTMLDivElement>, OUIAProps {
'aria-label'?: string;
/** Id to use for Modal Box descriptor */
'aria-describedby'?: string;
/** Accessible label applied to the modal box body. This should be used to communicate important information about the modal box body div if needed, such as that it is scrollable */
bodyAriaLabel?: string;
/** Accessible role applied to the modal box body. This will default to region if a body aria label is applied. Set to a more appropriate role as applicable based on the modal content and context */
bodyAriaRole?: string;
/** Flag to show the close button in the header area of the modal */
showClose?: boolean;
/** Custom footer */
Expand Down Expand Up @@ -206,6 +210,8 @@ export class Modal extends React.Component<ModalProps, ModalState> {
'aria-labelledby': ariaLabelledby,
'aria-label': ariaLabel,
'aria-describedby': ariaDescribedby,
bodyAriaLabel,
bodyAriaRole,
title,
titleIconVariant,
titleLabel,
Expand All @@ -231,6 +237,8 @@ export class Modal extends React.Component<ModalProps, ModalState> {
aria-label={ariaLabel}
aria-describedby={ariaDescribedby}
aria-labelledby={ariaLabelledby}
bodyAriaLabel={bodyAriaLabel}
bodyAriaRole={bodyAriaRole}
ouiaId={ouiaId !== undefined ? ouiaId : this.state.ouiaStateId}
ouiaSafe={ouiaSafe}
/>,
Expand Down
15 changes: 14 additions & 1 deletion packages/react-core/src/components/Modal/ModalContent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ export interface ModalContentProps extends OUIAProps {
'aria-label'?: string;
/** Id of Modal Box description */
'aria-describedby'?: string;
/** Accessible label applied to the modal box body. This should be used to communicate important information about the modal box body div if needed, such as that it is scrollable */
bodyAriaLabel?: string;
/** Accessible role applied to the modal box body. This will default to region if a body aria label is applied. Set to a more appropriate role as applicable based on the modal content and context */
bodyAriaRole?: string;
/** Flag to show the close button in the header area of the modal */
showClose?: boolean;
/** Default width of the content. */
Expand Down Expand Up @@ -82,6 +86,8 @@ export const ModalContent: React.FunctionComponent<ModalContentProps> = ({
'aria-label': ariaLabel = '',
'aria-describedby': ariaDescribedby,
'aria-labelledby': ariaLabelledby,
bodyAriaLabel,
bodyAriaRole,
showClose = true,
footer = null,
actions = [],
Expand Down Expand Up @@ -120,10 +126,17 @@ export const ModalContent: React.FunctionComponent<ModalContentProps> = ({
actions.length > 0 && <ModalBoxFooter>{actions}</ModalBoxFooter>
);

const defaultModalBodyAriaRole = bodyAriaLabel ? 'region' : undefined;

const modalBody = hasNoBodyWrapper ? (
children
) : (
<ModalBoxBody {...props} {...(!description && !ariaDescribedby && { id: descriptorId })}>
<ModalBoxBody
aria-label={bodyAriaLabel}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the aria-label have a default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure. If it should what would you want the default to be?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm this is an interesting question and actually makes me also question the default role. If we're going to default the role then I think we should default an aria-label, but do we always want the content of a model to have a role and an aria-label? When we were talking about it before, I admittedly was just thinking about the overflow example, but I'm not sure if it always makes sense to have both? For example, it seems a little heavy handed if the modal content is a sentence of information, and we have it segmented out with a role and label.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not opposed to adding a default label at all, I just have no idea what a good default would be. I think that any good label here is going to be highly variable based on what the content of the modal is.

Combined with the fact that we currently have the modal itself labeled by the modal heading, and described by the modal body, I'm not sure if having a default would be for the best? I'm not an expert on this topic by any means though and am happy to proceed however you all think is best on it 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

I will defer to @jessiehuff here.
Jessie, are you saying that the role should only default to region if there is an overflow? Should this props be tied to that having overflow?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I like Austin's approach of defaulting the region when there's an aria-label to prevent a11y issues, but I'm leaning towards not having a default aria-label. The only case that I can see that being helpful is in the case of the overflow so perhaps it's better to just allow users to customize it when they'd like.

Copy link
Contributor

Choose a reason for hiding this comment

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

it might be beneficial to mention in the prop description when it should be used?

role={bodyAriaRole || defaultModalBodyAriaRole}
Copy link
Contributor

Choose a reason for hiding this comment

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

So if the consumer defines an aria-label but not an aria-role, the role defaults to 'region'?

Copy link
Collaborator Author

@wise-king-sullyman wise-king-sullyman May 5, 2022

Choose a reason for hiding this comment

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

Correct the role defaults to 'region' if the consumer passes an aria-label to prevent Axe violations.

{...props}
{...(!description && !ariaDescribedby && { id: descriptorId })}
>
{children}
</ModalBoxBody>
);
Expand Down
56 changes: 56 additions & 0 deletions packages/react-core/src/components/Modal/__tests__/Modal.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -86,4 +86,60 @@ describe('Modal', () => {

expect(consoleErrorMock).toBeCalled();
});

test('The modalBoxBody has no aria-label when bodyAriaLabel is not passed', () => {
const props = {
isOpen: true
};

render(<Modal {...props}>This is a ModalBox</Modal>);

const modalBoxBody = screen.getByText('This is a ModalBox');
expect(modalBoxBody).not.toHaveAccessibleName('modal box body aria label');
});

test('The modalBoxBody has the expected aria-label when bodyAriaLabel is passed', () => {
const props = {
isOpen: true
};

render(
<Modal bodyAriaLabel="modal box body aria label" {...props}>
This is a ModalBox
</Modal>
);

const modalBoxBody = screen.getByText('This is a ModalBox');
expect(modalBoxBody).toHaveAccessibleName('modal box body aria label');
});

test('The modalBoxBody has the expected aria role when bodyAriaLabel is passed and bodyAriaRole is not', () => {
const props = {
isOpen: true
};

render(
<Modal bodyAriaLabel="modal box body aria label" {...props}>
This is a ModalBox
</Modal>
);

const modalBoxBody = screen.getByRole('region', { name: 'modal box body aria label' });
expect(modalBoxBody).toBeInTheDocument();
});

test('The modalBoxBody has the expected aria role when bodyAriaRole is passed', () => {
const props = {
isOpen: true
};

render(
<Modal bodyAriaLabel="modal box body aria label" bodyAriaRole="article" {...props}>
This is a ModalBox
</Modal>
);

const modalBoxBody = screen.getByRole('article', { name: 'modal box body aria label' });
expect(modalBoxBody).toBeInTheDocument();
});
});
7 changes: 7 additions & 0 deletions packages/react-core/src/components/Modal/examples/Modal.md
Original file line number Diff line number Diff line change
Expand Up @@ -869,3 +869,10 @@ class HelpModal extends React.Component {
```ts file="ModalWithForm.tsx"

```

### With overflowing content

If the content that you're passing to the modal is likely to overflow the modal content area, pass tabIndex={0} to the modal to enable keyboard accessible scrolling.

```ts file="ModalWithOverflowingContent.tsx"
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
import React from 'react';
import { Modal, ModalVariant, Button } from '@patternfly/react-core';

export const ModalWithOverflowingContent: React.FunctionComponent = () => {
const [isModalOpen, setIsModalOpen] = React.useState(false);

const handleModalToggle = () => {
setIsModalOpen(prevIsModalOpen => !prevIsModalOpen);
};

return (
<React.Fragment>
<Button variant="primary" onClick={handleModalToggle}>
Show modal
</Button>
<Modal
bodyAriaLabel="Scrollable modal content"
tabIndex={0}
Copy link
Contributor

Choose a reason for hiding this comment

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

Recently, @seanforyou23 made a comment on some a11y docs about this sort of update and suggested adding an accessible label. This is because the non-negative tabindex will make the container register as a form-control on rotor menus or elements lists and without an explicit label, various browsers or screen readers may label it differently/incorrectly.
Comment: patternfly/patternfly-org#2893 (comment)

That said, I dont think we need to update the example docs - I am adding this suggestion to the a11y docs. But maybe we should add an aria-label or aria-labelledby to elements we are intentionally adding a tabindex to?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense. I'm on board with adding an aria-label here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with the suggestion of adding a label, but I wonder if we would need to add a role in that scenario as well? I think technically just having an aria-label on a <div> will throw an Axe error since this approach is not well supported. For example, we actually have this issue right now on our Wizard. I'm not sure which role would make sense in this scenario though. We could do a basic "region" role, but there might be a better sectioning role. It might require a little bit of research to see what the best approach is to add a label.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jessiehuff from the research I've done so far I think the ideal role might depend on what the modal is for / what the content in the modal is.

For that reason I think the best approach here is probably to add another prop that the consumer can use to apply a more suitable role if applicable, but default to having it as a "region" if an aria-label is applied to ensure that (at the least) Axe errors aren't caused. For a little additional context from my research Carbon sets the role to "region" in their modal if it's scrollable. Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also @nicolethoen the modal currently applies passed aria-labels to the overall modal box rather than letting them go to the modal box body where the content is / the tab index is applied, so another prop will need to be added for consumers to be able to apply labels to the modal box body.

variant={ModalVariant.small}
title="Modal with overflowing content"
isOpen={isModalOpen}
onClose={handleModalToggle}
actions={[
<Button key="confirm" variant="primary" onClick={handleModalToggle}>
Confirm
</Button>,
<Button key="cancel" variant="link" onClick={handleModalToggle}>
Cancel
</Button>
]}
>
Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore
magna aliqua. Quis eleifend quam adipiscing vitae proin sagittis nisl rhoncus. Semper auctor neque vitae tempus.
Diam donec adipiscing tristique risus. Augue eget arcu dictum varius duis. Ut enim blandit volutpat maecenas
volutpat blandit aliquam. Sit amet mauris commodo quis imperdiet massa tincidunt. Habitant morbi tristique
senectus et netus. Fames ac turpis egestas sed tempus urna. Neque laoreet suspendisse interdum consectetur
libero id. Volutpat lacus laoreet non curabitur gravida arcu ac tortor. Porta nibh venenatis cras sed felis eget
velit. Nullam non nisi est sit amet facilisis. Nunc mi ipsum faucibus vitae. Lorem sed risus ultricies tristique
nulla aliquet enim tortor at. Egestas sed tempus urna et pharetra pharetra massa massa ultricies. Lacinia quis
vel eros donec ac odio tempor orci. Malesuada fames ac turpis egestas integer eget aliquet.
<br />
<br />
Neque aliquam vestibulum morbi blandit cursus risus at ultrices. Molestie at elementum eu facilisis sed odio
morbi. Elit pellentesque habitant morbi tristique. Consequat nisl vel pretium lectus quam id leo in vitae. Quis
varius quam quisque id diam vel quam elementum. Viverra nam libero justo laoreet sit amet cursus. Sollicitudin
tempor id eu nisl nunc. Orci nulla pellentesque dignissim enim sit amet venenatis. Dignissim enim sit amet
venenatis urna cursus eget. Iaculis at erat pellentesque adipiscing commodo elit. Faucibus pulvinar elementum
integer enim neque volutpat. Nullam vehicula ipsum a arcu cursus vitae congue mauris. Nunc mattis enim ut tellus
elementum sagittis vitae. Blandit cursus risus at ultrices. Tellus mauris a diam maecenas sed enim. Non diam
phasellus vestibulum lorem sed risus ultricies tristique nulla.
<br />
<br />
Nulla pharetra diam sit amet nisl suscipit adipiscing. Ac tortor vitae purus faucibus ornare suspendisse sed
nisi. Sed felis eget velit aliquet sagittis id consectetur purus. Tincidunt tortor aliquam nulla facilisi cras
fermentum. Volutpat est velit egestas dui id ornare arcu odio. Pharetra magna ac placerat vestibulum. Ultrices
sagittis orci a scelerisque purus semper eget duis at. Nisi est sit amet facilisis magna etiam tempor orci eu.
Convallis tellus id interdum velit. Facilisis sed odio morbi quis commodo odio aenean sed.
<br />
<br />
Eu scelerisque felis imperdiet proin fermentum leo vel orci porta. Facilisi etiam dignissim diam quis enim
lobortis scelerisque fermentum. Eleifend donec pretium vulputate sapien nec sagittis aliquam malesuada. Magna
etiam tempor orci eu lobortis elementum. Quis auctor elit sed vulputate mi sit. Eleifend quam adipiscing vitae
proin sagittis nisl rhoncus mattis rhoncus. Erat velit scelerisque in dictum non. Sit amet nulla facilisi morbi
tempus iaculis urna. Enim ut tellus elementum sagittis vitae et leo duis ut. Lectus arcu bibendum at varius vel
pharetra vel turpis. Morbi tristique senectus et netus et. Eget aliquet nibh praesent tristique magna sit amet
purus gravida. Nisl purus in mollis nunc sed id semper risus. Id neque aliquam vestibulum morbi. Mauris a diam
maecenas sed enim ut sem. Egestas tellus rutrum tellus pellentesque.
</Modal>
</React.Fragment>
);
};