-
Notifications
You must be signed in to change notification settings - Fork 355
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.
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
docs(Modal): add example preserving a11y when content overflows #7293
Changes from 1 commit
3fc4519
68a8fa3
528e744
d5a60fb
7ac6d04
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
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 | ||
tabIndex={0} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. I'm on board with adding an aria-label here. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wolfeallison could you please review the copy here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could be a little more explicit on what "failing to do so" is. Not sure if this is technically correct, but what about "Failing to pass will cause the modal content to not be scrollable via keyboard." Otherwise, looks great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wolfeallison what do you think about rewording that whole line to just be:
"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."There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it! @wise-king-sullyman