-
Notifications
You must be signed in to change notification settings - Fork 841
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
[EuiAccordion] Improve keyboard accessibility of tabbable children #7064
Changes from all commits
d76c33d
493cf86
2bece39
e40a7c1
60a5ea3
9a9ebc2
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 |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
|
||
import React, { Component, HTMLAttributes, ReactNode } from 'react'; | ||
import classNames from 'classnames'; | ||
import { tabbable, FocusableElement } from 'tabbable'; | ||
|
||
import { CommonProps } from '../common'; | ||
|
||
|
@@ -108,9 +109,13 @@ export type EuiAccordionProps = CommonProps & | |
isDisabled?: boolean; | ||
}; | ||
|
||
type EuiAccordionState = { | ||
isOpen: boolean; | ||
}; | ||
|
||
export class EuiAccordionClass extends Component< | ||
WithEuiThemeProps & EuiAccordionProps, | ||
{ isOpen: boolean } | ||
EuiAccordionState | ||
> { | ||
static defaultProps = { | ||
initialIsOpen: false, | ||
|
@@ -125,21 +130,24 @@ export class EuiAccordionClass extends Component< | |
|
||
childContent: HTMLDivElement | null = null; | ||
childWrapper: HTMLDivElement | null = null; | ||
tabbableChildren: FocusableElement[] | null = null; | ||
|
||
state = { | ||
isOpen: this.props.forceState | ||
? this.props.forceState === 'open' | ||
: this.props.initialIsOpen!, | ||
}; | ||
|
||
get isOpen() { | ||
return this.props.forceState | ||
? this.props.forceState === 'open' | ||
: this.state.isOpen; | ||
} | ||
|
||
setChildContentHeight = () => { | ||
const { forceState } = this.props; | ||
requestAnimationFrame(() => { | ||
const height = | ||
this.childContent && | ||
(forceState ? forceState === 'open' : this.state.isOpen) | ||
? this.childContent.clientHeight | ||
: 0; | ||
this.childContent && this.isOpen ? this.childContent.clientHeight : 0; | ||
this.childWrapper && | ||
this.childWrapper.setAttribute( | ||
'style', | ||
|
@@ -150,17 +158,33 @@ export class EuiAccordionClass extends Component< | |
|
||
componentDidMount() { | ||
this.setChildContentHeight(); | ||
if (!this.isOpen) this.preventTabbing(); | ||
} | ||
|
||
componentDidUpdate() { | ||
componentDidUpdate( | ||
prevProps: EuiAccordionProps, | ||
prevState: EuiAccordionState | ||
) { | ||
this.setChildContentHeight(); | ||
|
||
if ( | ||
(prevProps.forceState === 'open' && this.props.forceState === 'closed') || | ||
(prevState.isOpen === true && this.state.isOpen === false) | ||
) { | ||
this.preventTabbing(); | ||
} | ||
if ( | ||
(prevProps.forceState === 'closed' && this.props.forceState === 'open') || | ||
(prevState.isOpen === false && this.state.isOpen === true) | ||
) { | ||
this.enableTabbing(); | ||
} | ||
} | ||
|
||
onToggle = () => { | ||
const { forceState } = this.props; | ||
if (forceState) { | ||
this.props.onToggle && | ||
this.props.onToggle(forceState === 'open' ? false : true); | ||
this.props.onToggle?.(forceState === 'open' ? false : true); | ||
} else { | ||
this.setState( | ||
(prevState) => ({ | ||
|
@@ -170,12 +194,49 @@ export class EuiAccordionClass extends Component< | |
if (this.state.isOpen && this.childWrapper) { | ||
this.childWrapper.focus(); | ||
} | ||
this.props.onToggle && this.props.onToggle(this.state.isOpen); | ||
this.props.onToggle?.(this.state.isOpen); | ||
} | ||
); | ||
} | ||
}; | ||
|
||
// When accordions are closed, tabbable children should not be present in the tab order | ||
preventTabbing = () => { | ||
if (this.childContent) { | ||
// Re-check for children on every close - content can change dynamically | ||
this.tabbableChildren = tabbable(this.childContent); | ||
|
||
this.tabbableChildren.forEach((element) => { | ||
// If the element has an existing `tabIndex` set, make sure we can restore it | ||
const originalTabIndex = element.getAttribute('tabIndex'); | ||
if (originalTabIndex) { | ||
element.setAttribute('data-original-tabindex', originalTabIndex); | ||
} | ||
Comment on lines
+210
to
+214
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 is good! I'm not sure I would have thought to retain the original tab index. 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. The current |
||
|
||
element.setAttribute('tabIndex', '-1'); | ||
}); | ||
} | ||
}; | ||
|
||
enableTabbing = () => { | ||
// If no tabbable children were set, we don't need to re-enable anything | ||
if (this.tabbableChildren) { | ||
this.tabbableChildren.forEach((element) => { | ||
const originalTabIndex = element.getAttribute('data-original-tabindex'); | ||
if (originalTabIndex) { | ||
// If the element originally had an existing `tabIndex` set, restore it | ||
element.setAttribute('tabIndex', originalTabIndex); | ||
element.removeAttribute('data-original-tabindex'); | ||
} else { | ||
// If not, remove the tabIndex property | ||
element.removeAttribute('tabIndex'); | ||
} | ||
}); | ||
// Cleanup - unset the list of children | ||
this.tabbableChildren = null; | ||
} | ||
}; | ||
|
||
setChildContentRef = (node: HTMLDivElement | null) => { | ||
this.childContent = node; | ||
}; | ||
|
@@ -214,8 +275,6 @@ export class EuiAccordionClass extends Component< | |
...rest | ||
} = this.props; | ||
|
||
const isOpen = forceState ? forceState === 'open' : this.state.isOpen; | ||
|
||
// Force button element to be a legend if the element is a fieldset | ||
const ButtonElement = Element === 'fieldset' ? 'legend' : _ButtonElement; | ||
const buttonElementIsFocusable = ButtonElement === 'button'; | ||
|
@@ -229,7 +288,7 @@ export class EuiAccordionClass extends Component< | |
const classes = classNames( | ||
'euiAccordion', | ||
{ | ||
'euiAccordion-isOpen': isOpen, | ||
'euiAccordion-isOpen': this.isOpen, | ||
}, | ||
className | ||
); | ||
|
@@ -252,7 +311,7 @@ export class EuiAccordionClass extends Component< | |
const iconButtonClasses = classNames( | ||
'euiAccordion__iconButton', | ||
{ | ||
'euiAccordion__iconButton-isOpen': isOpen, | ||
'euiAccordion__iconButton-isOpen': this.isOpen, | ||
'euiAccordion__iconButton--right': _arrowDisplay === 'right', | ||
}, | ||
arrowProps?.className | ||
|
@@ -276,13 +335,13 @@ export class EuiAccordionClass extends Component< | |
const childWrapperStyles = euiAccordionChildWrapperStyles(theme); | ||
const cssChildWrapperStyles = [ | ||
childWrapperStyles.euiAccordion__childWrapper, | ||
isOpen && childWrapperStyles.isOpen, | ||
this.isOpen && childWrapperStyles.isOpen, | ||
]; | ||
|
||
const iconButtonStyles = euiAccordionIconButtonStyles(theme); | ||
const cssIconButtonStyles = [ | ||
iconButtonStyles.euiAccordion__iconButton, | ||
isOpen && iconButtonStyles.isOpen, | ||
this.isOpen && iconButtonStyles.isOpen, | ||
_arrowDisplay === 'right' && iconButtonStyles.arrowRight, | ||
arrowProps?.css, | ||
]; | ||
|
@@ -312,7 +371,7 @@ export class EuiAccordionClass extends Component< | |
iconType="arrowRight" | ||
onClick={this.onToggle} | ||
aria-controls={id} | ||
aria-expanded={isOpen} | ||
aria-expanded={this.isOpen} | ||
aria-labelledby={buttonId} | ||
tabIndex={buttonElementIsFocusable ? -1 : 0} | ||
isDisabled={isDisabled} | ||
|
@@ -364,7 +423,7 @@ export class EuiAccordionClass extends Component< | |
css={cssButtonStyles} | ||
aria-controls={id} | ||
// `aria-expanded` is only a valid attribute on interactive controls - axe-core throws a violation otherwise | ||
aria-expanded={ButtonElement === 'button' ? isOpen : undefined} | ||
aria-expanded={ButtonElement === 'button' ? this.isOpen : undefined} | ||
onClick={isDisabled ? undefined : this.onToggle} | ||
type={ButtonElement === 'button' ? 'button' : undefined} | ||
disabled={ButtonElement === 'button' ? isDisabled : undefined} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
**Bug fixes** | ||
|
||
- Fixed `EuiAccordion` to remove tabbable children from sequential keyboard navigation when the accordion is closed |
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.
Nice clean up!
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.
[attaching to a random comment for threading]
Just mentioning for completeness, getters can be used inside any JS object, not just inside classes! https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get