-
Notifications
You must be signed in to change notification settings - Fork 204
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
[BD-9484][BpkInfoBannerExpandable] Fix an a11y issue #3751
Conversation
Visit https://backpack.github.io/storybook-prs/3751 to see this build running in a browser. |
@@ -189,8 +178,12 @@ const BpkInfoBannerInner = ({ | |||
{...rest} | |||
> | |||
<section className={sectionClassNames} role="presentation"> | |||
<div | |||
<BannerHeader | |||
role={isExpandable ? 'button' : undefined} |
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.
role button is not needed on a button, you might want to set type="button" when expandable :D
Visit https://backpack.github.io/storybook-prs/3751 to see this build running in a browser. |
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.
Reviewed with keyboard and screen reader. Let me know if I can support on testing
className={getClassName('bpk-info-banner__toggle-button')} | ||
aria-label={props.label} | ||
aria-expanded={props.expanded} | ||
title={props.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.
We should remove the 'title' now that it's no longer a button
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.
Good catch, thanks for reviewing!
{props.expanded ? <CollapseIcon/> : <ExpandIcon/> } | ||
</div> | ||
</button> | ||
{props.expanded ? <CollapseIcon /> : <ExpandIcon />} |
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.
We can add a Icon text or add altText to the icon to have 'view more' added.
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.
Added as visually-hidden
text so it is read in screen reader. Tried alt
in icon but it's not read after the header message.
<div | ||
role={isExpandable ? 'button' : undefined} | ||
<BannerHeader | ||
aria-label={isExpandable ? toggleButtonLabel : undefined} |
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.
We shouldn't set the aria-label here, as it will override the visible text. (Look at the prev. comment)
Ideally the button screen reader should read:
{visible text}, {toggleButtonLabel}, <expanded|collapsed>, button
We also need to review the 'aria-controls' for this expand/collapse behaviour:
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-controls
@@ -189,8 +178,11 @@ const BpkInfoBannerInner = ({ | |||
{...rest} | |||
> | |||
<section className={sectionClassNames} role="presentation"> |
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.
nit: This probably can be a div, and remove the role :D
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.
Yes, removed it.
…e section to div and remove role; remove deprecated comments
Visit https://backpack.github.io/storybook-prs/3751 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3751 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3751 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3751 to see this build running in a browser. |
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.
LGTM
Context
Fix a11y issue - element with button role can not be operable via keyboard
We identified this issue in hotels day-view:
Changes
<button>
to<div>
and not clickable.<button>
.Remember to include the following changes:
[KOA-123][BpkButton] Updating the colour
README.md
(If you have created a new component)README.md