-
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
Merged
tlabaj
merged 5 commits into
patternfly:main
from
wise-king-sullyman:modal-overflow-example
May 9, 2022
Merged
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
3fc4519
docs(Modal): add example preserving a11y when content overflows
wise-king-sullyman 68a8fa3
update overflow example copy
wise-king-sullyman 528e744
add additional props to enable better a11y for scrollable content
wise-king-sullyman d5a60fb
add tests for new aria props
wise-king-sullyman 7ac6d04
expand on body aria prop descriptions
wise-king-sullyman File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
70 changes: 70 additions & 0 deletions
70
packages/react-core/src/components/Modal/examples/ModalWithOverflowingContent.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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} | ||
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> | ||
); | ||
}; |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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?
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.
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 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.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.
@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 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.