-
Notifications
You must be signed in to change notification settings - Fork 28
feat(a11y): Add keyboard navigation, focus management, and ARIA labels to pipeline nodes and groups #322
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
feat(a11y): Add keyboard navigation, focus management, and ARIA labels to pipeline nodes and groups #322
Changes from all commits
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,15 @@ | ||
| --- | ||
| id: Accessibility | ||
| section: extensions | ||
| subsection: topology | ||
| sortValue: 999 | ||
| --- | ||
|
Comment on lines
+1
to
+6
Contributor
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. This can be a followup if it isn't as straight forward to implement for an extension, but if this file is meant to be strictly accessibility for the "Pipelines" section of topology, would be nice to include it as a tab on the Pipelines page (similar to how each PF component has the "HTML", "React", ..., "Accessibility" tabs)
Member
Author
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 thought about that but I'm not sure where we would document the accessibility features for non-pipeline layouts. Once we have it for both, the information for both will likely not need to be split between layout types. We can have it all here in one section. But for now, we only support it for pipelines. |
||
|
|
||
| ## Pipelines layout | ||
|
|
||
| By default, `<TaskNode>` and `<DefaultTaskGroup>` are included in the keyboard tab order (`tabIndex` is `0`). Tab order follows the order of items in the `model`. An expanded group receives focus before the nodes inside it. To set a custom tab order, pass `tabIndex` on `<TaskNode>` or `<DefaultTaskGroup>`. | ||
|
|
||
| When you select nodes and groups with `withSelection` or `useSelection`, `raiseOnSelect` defaults to `true`. That option moves the selected item to the end of its siblings, which also moves it in the tab order. Each later selection places that item after the one you selected before it. To keep tab order stable, pass `{ raiseOnSelect: false }` to `withSelection` or `useSelection`. | ||
|
|
||
| To stop screen readers from announcing truncated labels twice, set `labelTooltipTrigger` to `mouseenter` on `<TaskNode>`. The tooltip then opens on hover only, so the truncated label is not added to the keyboard tab order. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,14 @@ | ||
| import { forwardRef } from 'react'; | ||
| import { forwardRef, useRef } from 'react'; | ||
| import { useSize } from '../../../utils'; | ||
| import { css } from '@patternfly/react-styles'; | ||
| import styles from '../../../css/topology-components'; | ||
| import { handleKeyboardSelection } from '../../../utils/accessibility-utils'; | ||
|
|
||
| interface LabelActionIconProps { | ||
| className?: string; | ||
| icon: React.ReactElement; | ||
| 'aria-label'?: string; | ||
|
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. Since we are defining aria-label as the prop name here, and adding it to the calling definition, but then immediately re-casting it as 'ariaLabel' below, can we just have it as ariaLabel here as well? Do we expect this component to be called like a normal html element where a user might pass in 'aria-label' as a prop and that's why we do it this way? I see it used a couple of times in this PR, made me curious
Contributor
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. fwiw, in the base PatternFly react repo, that's how we handle aria props (use kebab case, but later use camel case). Just for consistency probably better to keep it like this here as well.
Member
Author
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. Yes, I was just following PatternFly conventions. |
||
| tabIndex?: number; | ||
| onClick: (e: React.MouseEvent) => void; | ||
| iconOffsetX?: number; | ||
| iconOffsetY?: number; | ||
|
|
@@ -17,8 +20,24 @@ interface LabelActionIconProps { | |
| } | ||
|
|
||
| const LabelActionIcon = forwardRef<SVGRectElement, LabelActionIconProps>( | ||
| ({ icon, onClick, className, x, y, paddingX, height, iconOffsetX = 0, iconOffsetY = 0 }, actionRef) => { | ||
| ( | ||
| { | ||
| icon, | ||
| onClick, | ||
| 'aria-label': ariaLabel, | ||
| tabIndex = 0, | ||
| className, | ||
| x, | ||
| y, | ||
| paddingX, | ||
| height, | ||
| iconOffsetX = 0, | ||
| iconOffsetY = 0 | ||
| }, | ||
| actionRef | ||
| ) => { | ||
| const [iconSize, iconRef] = useSize([icon, paddingX]); | ||
| const clickRef = useRef(null); | ||
| const iconWidth = iconSize?.width ?? 0; | ||
| const iconHeight = iconSize?.height ?? 0; | ||
| const iconY = (height - iconHeight) / 2; | ||
|
|
@@ -33,7 +52,7 @@ const LabelActionIcon = forwardRef<SVGRectElement, LabelActionIconProps>( | |
| }; | ||
|
|
||
| return ( | ||
| <g className={classes} onClick={handleClick}> | ||
| <g className={classes} onClick={handleClick} ref={clickRef}> | ||
| {iconSize && ( | ||
| <rect | ||
| ref={actionRef} | ||
|
|
@@ -42,6 +61,10 @@ const LabelActionIcon = forwardRef<SVGRectElement, LabelActionIconProps>( | |
| y={y} | ||
| width={iconWidth + paddingX * 2} | ||
| height={height} | ||
| tabIndex={ariaLabel ? tabIndex : undefined} | ||
| aria-label={ariaLabel} | ||
| onKeyDown={handleKeyboardSelection(clickRef)} | ||
| data-id="context-icon" | ||
| /> | ||
| )} | ||
| <g | ||
|
|
||
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.
The context menus are not able to be opened or focused via keyboard.
If you add an actionIconAriaLabel to these TaskNodes, i think that should give it a tabindex because of the logic on LabelActionIcon.tsx:507
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.
It will, this is for the
Set tab indicesoption. When that is off, no data.tabIndex will be set.