-
Notifications
You must be signed in to change notification settings - Fork 99
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
fix: improve a11y on some components #1045
Changes from 1 commit
161c7e2
d4de1b5
501d905
0fb21e6
29f9909
5b4bd69
024f502
95f0a6d
802f579
1120a04
ce4885c
f22f5f7
ef3dc2d
a5e5490
304feea
6a16d65
2e17d94
bc2144e
d2d22fc
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 |
---|---|---|
|
@@ -14,6 +14,20 @@ $transitionTimingFunction: ease-in-out; | |
border: var(--border-size) solid #{$borderColor}; | ||
} | ||
|
||
// focus on interactive label (excluding focus on addon) | ||
&:not(#{$disabled})#{$block}_is-interactive:focus:not( | ||
:has(#{$block}__addon_interactive:focus) | ||
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. If Label is clickable and has close button this wouldn't allow to focus the Label itself |
||
) { | ||
box-shadow: 0 0 0 2px var(--g-color-line-focus); | ||
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. Let's use outline here |
||
} | ||
|
||
//fallback for old browsers | ||
&:not(#{$disabled})#{$block}_is-interactive:focus:not( | ||
:has(#{$block}__addon_interactive:focus) | ||
):not(:focus-visible) { | ||
box-shadow: none; | ||
} | ||
|
||
// hover on interactive label (excluding hover on addon) | ||
&:not(#{$disabled})#{$block}_is-interactive:hover:not( | ||
:has(#{$block}__addon_interactive:hover) | ||
|
@@ -146,6 +160,7 @@ $transitionTimingFunction: ease-in-out; | |
|
||
&_is-interactive { | ||
cursor: pointer; | ||
outline: none; | ||
} | ||
|
||
&_theme { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ import {ClipboardIcon} from '../ClipboardIcon'; | |
import {CopyToClipboard, CopyToClipboardStatus} from '../CopyToClipboard'; | ||
import {Icon} from '../Icon'; | ||
import {block} from '../utils/cn'; | ||
import {useActionHandlers} from '../utils/useActionHandlers'; | ||
|
||
import './Label.scss'; | ||
|
||
|
@@ -86,6 +87,8 @@ export const Label = React.forwardRef<HTMLDivElement, LabelProps>(function Label | |
onClick, | ||
} = props; | ||
|
||
const actionButtonRef = React.useRef<HTMLButtonElement>(null); | ||
|
||
const hasContent = Boolean(children !== '' && React.Children.count(children) > 0); | ||
|
||
const typeClose = type === 'close' && hasContent; | ||
|
@@ -122,12 +125,25 @@ export const Label = React.forwardRef<HTMLDivElement, LabelProps>(function Label | |
} | ||
}; | ||
|
||
const handleClick = (event: React.MouseEvent<HTMLDivElement>) => { | ||
/** | ||
* Triggered only if the handler was triggered on the element itself, and not on the actionButton | ||
* It is necessary that keyboard navigation works correctly | ||
*/ | ||
if (!actionButtonRef.current?.contains(event.target as Node)) { | ||
onClick?.(event); | ||
} | ||
}; | ||
|
||
const {onKeyDown} = useActionHandlers(handleClick); | ||
|
||
const renderLabel = (status?: CopyToClipboardStatus) => { | ||
let actionButton: React.ReactNode; | ||
|
||
if (typeCopy) { | ||
actionButton = ( | ||
<Button | ||
ref={actionButtonRef} | ||
size={buttonSize} | ||
extraProps={{'aria-label': copyButtonLabel || undefined}} | ||
{...commonActionButtonProps} | ||
|
@@ -143,6 +159,7 @@ export const Label = React.forwardRef<HTMLDivElement, LabelProps>(function Label | |
} else if (typeClose) { | ||
actionButton = ( | ||
<Button | ||
ref={actionButtonRef} | ||
onClick={onClose ? handleCloseClick : undefined} | ||
size={buttonSize} | ||
extraProps={{'aria-label': closeButtonLabel || undefined}} | ||
|
@@ -154,10 +171,12 @@ export const Label = React.forwardRef<HTMLDivElement, LabelProps>(function Label | |
} | ||
|
||
return ( | ||
// eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions | ||
<div | ||
ref={ref} | ||
onClick={hasOnClick ? onClick : undefined} | ||
role={hasOnClick ? 'role' : undefined} | ||
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. role="button"? |
||
tabIndex={hasOnClick ? 0 : undefined} | ||
onClick={hasOnClick ? handleClick : undefined} | ||
onKeyDown={hasOnClick ? onKeyDown : undefined} | ||
className={b( | ||
{ | ||
theme, | ||
|
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.
What about not setting is-interactive flag if disabled is true?