Skip to content
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] Fixed a bug in aria-expanded for axe-core. #6694

Merged
merged 7 commits into from
Apr 6, 2023
Merged

[EuiAccordion] Fixed a bug in aria-expanded for axe-core. #6694

merged 7 commits into from
Apr 6, 2023

Conversation

1Copenut
Copy link
Contributor

@1Copenut 1Copenut commented Apr 5, 2023

Summary

The EuiAccordion component was throwing an axe-core violation when we included interactive content in the trigger. This was due to non-button elements receiving an aria-expanded attribute. This PR adds a logic check to only render the aria-expanded attribute on buttons, and in the correct state.

  • Added logic check for EuiAccordion aria-expanded attr.
  • Adding comment for new logic reasoning.

Closes #6689

QA

Remove or strikethrough items that do not apply to your PR.

General checklist

  • Checked in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs (using @default if default values are missing) and playground toggles
  • Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

* Added logic check for EuiAccordion aria-expanded attr.
* Adding comment for new logic reasoning.
@1Copenut 1Copenut self-assigned this Apr 5, 2023
@1Copenut 1Copenut changed the title Fixed a bug in EuiAccordion aria-expanded for axe. [WIP: Do not merge][EuiAccordion] Fixed a bug in aria-expanded for axe-core. Apr 5, 2023
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6694/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6694/

@1Copenut 1Copenut changed the title [WIP: Do not merge][EuiAccordion] Fixed a bug in aria-expanded for axe-core. [EuiAccordion] Fixed a bug in aria-expanded for axe-core. Apr 6, 2023
@1Copenut 1Copenut marked this pull request as ready for review April 6, 2023 19:54
@1Copenut 1Copenut requested review from cee-chen and breehall April 6, 2023 19:54
const button = (
<ButtonElement
css={cssButtonStyles}
{...buttonProps}
id={buttonId}
className={buttonClasses}
aria-controls={id}
aria-expanded={isOpen}
aria-expanded={isExpandableButton ? isOpen : undefined}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jest and Cypress tests broke in local testing while I was working through this logic, so I've got high confidence in this approach. The two snapshot test changes were using div | fieldset for the ButtonElement which is what I expected.

1Copenut and others added 4 commits April 6, 2023 15:12
Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com>
Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com>
Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com>
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6694/

@1Copenut 1Copenut requested a review from cee-chen April 6, 2023 21:26
Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 Woohoo!

@1Copenut 1Copenut merged commit 35b6a27 into elastic:main Apr 6, 2023
@1Copenut 1Copenut deleted the bug/euiAccordion-aria-expanded-interactive branch April 6, 2023 21:48
jbudz added a commit to elastic/kibana that referenced this pull request Apr 18, 2023
EUI `77.0.0` ➡️ `77.1.1`

## [`77.1.0`](https://github.com/elastic/eui/tree/v77.1.0)

- Updated `EuiDatePicker` to display a warning icon and correctly set
`aria-invalid` when `isInvalid` is passed
([#6677](elastic/eui#6677))
- Updated `EuiFilePicker` to display an alert icon when `isInvalid`
([#6678](elastic/eui#6678))
- Updated `EuiTextArea` to display an alert icon when `isInvalid`
([#6679](elastic/eui#6679))
- Updated `EuiTextArea` to support the `isLoading` prop
([#6679](elastic/eui#6679))
- Updated `EuiComboBox` to display a warning icon and correctly set
`aria-invalid` when `isInvalid` is passed
([#6680](elastic/eui#6680))

**Bug fixes**

- Fixed `EuiAccordion` to not set an `aria-expanded` attribute on
non-interactive `buttonElement`s
([#6694](elastic/eui#6694))
- Fixed an `EuiPopoverFooter` bug causing nested popovers within
popovers (note: not a recommended use-case) to unintentionally override
its panel padding size inherited from context
([#6698](elastic/eui#6698))
- Fixed `EuiComboBox` to only delete the last selected item on backspace
if the input caret is present
([#6699](elastic/eui#6699))

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Jon <jon@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EuiAccordion][AXE-CORE]: Interactive content in trigger must not have aria-expanded attribute
3 participants