fix: improve a11y on some components #1045
Conversation
KirillDyachkovskiy
requested review from
korvin89,
DaffPunks,
amje,
LakeVostok,
axtk and
ValeraS
as code owners
|
Preview is ready. |
src/components/Link/Link.tsx
Outdated
| ); | ||
| } else { | ||
| return ( | ||
| <span | ||
| {...(extraProps as React.HTMLAttributes<HTMLSpanElement>)} | ||
| {...commonProps} | ||
| ref={ref as React.Ref<HTMLSpanElement>} | ||
| // eslint-disable-next-line jsx-a11y/no-noninteractive-tabindex | ||
| role="link" |
There was a problem hiding this comment.
I'm not sure that it is true
There was a problem hiding this comment.
And I think we should remove this behaviour in the next major (#600)
| @@ -201,6 +201,7 @@ export const TableColumnSetup = (props: TableColumnSetupProps) => { | |||
|
|
|||
| return ( | |||
| <div className={b(null, className)}> | |||
| {/* It is used to handle clicks on the switcher control */} | |||
There was a problem hiding this comment.
that should not work this way
we should pass handler via some context i guess to easily pass it to rendered component
| @@ -14,6 +14,16 @@ $block: '.#{variables.$ns}user-avatar'; | |||
| background-position: center; | |||
| background-size: cover; | |||
|
|
|||
| outline: none; | |||
|
|
|||
| &:focus { | |||
There was a problem hiding this comment.
we probably have SCSS mixin for it
| onKeyDown={onKeyDown} | ||
| className={b({size}, className)} | ||
| title={title} | ||
| onClick={onClick} |
There was a problem hiding this comment.
onClick supposed to be removed from this component
KirillDyachkovskiy
requested review from
NikitaCG and
goshander
as code owners
src/components/Label/Label.tsx
Outdated
| <div | ||
| ref={ref} | ||
| onClick={hasOnClick ? onClick : undefined} | ||
| role={hasOnClick ? 'role' : undefined} |
src/components/Label/Label.scss
Outdated
| @@ -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( | |||
There was a problem hiding this comment.
What about not setting is-interactive flag if disabled is true?
src/components/Label/Label.scss
Outdated
| @@ -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.
If Label is clickable and has close button this wouldn't allow to focus the Label itself
src/components/Label/Label.scss
Outdated
| &:not(#{$disabled})#{$block}_is-interactive:focus:not( | ||
| :has(#{$block}__addon_interactive:focus) | ||
| ) { | ||
| box-shadow: 0 0 0 2px var(--g-color-line-focus); |
| {/* eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions */} | ||
| <div className={b('control')} ref={refControl} onClick={handleControlClick}> | ||
| {switcher || ( | ||
| {renderSwitcher?.({onClick: handleControlClick}) || switcher || ( |
There was a problem hiding this comment.
We should create onKeyDown via useActionHandlers and pass it along with the onClick
| return ( | ||
| <DropdownMenuContext.Provider value={contextValue}> | ||
| {/*as this div has Button component as child, clicking on it one fires onClick of this div on bubbling*/} | ||
| {/* FIXME remove switcher prop and this wrapper */} |
There was a problem hiding this comment.
When is this planned to be done?
There was a problem hiding this comment.
After a major library update
| /** | ||
| * Menu toggle control. | ||
| */ | ||
| renderSwitcher?: (props: SwitcherProps) => React.ReactNode; |
There was a problem hiding this comment.
Why we need this prop? We can keep current api, but use cloneElement to add handlers to switcher
UXRFC-202