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

KButton: Add aria-haspopup and aria-expanded for dropdown menus #856

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

josephinoo
Copy link

Description

This PR enhances KButton accessibility by adding ARIA dropdown attributes. When KButton is used with a dropdown menu, it now automatically includes aria-haspopup="menu" and manages aria-expanded state for improved screen reader feedback and navigation.

Issue addressed

Reported by @AlexVelezLl in #832

Before/after screenshots

Changelog

  • Description: Adds aria-haspopup and aria-expanded attributes to KButton when used with dropdown menus to improve screen reader feedback

  • Products impact: Anhancement

  • Addresses: [KButton]: Add aria-haspopup and aria-expanded to KButton when it has a KDropdownMenu related #832

  • Components: KButton

  • Breaking: no

  • Impacts a11y: yes

  • Guidance: WNo changes required for existing implementations. The ARIA attributes are automatically added when KButton is used with dropdown menus through the menu slot.

Steps to test

  1. Add a dropdown menu to a KButton using the menu slot:
<KButton text="Test Menu">
  <template #menu>
    <KDropdownMenu :options="[...]" />
  </template>
</KButton>
  1. Open browser dev tools and inspect the button element
  2. Verify aria-haspopup="menu" is present
  3. Click the button and verify aria-expanded changes from "false" to "true"
  4. Optional: Test with a screen reader to verify proper announcements

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical and brittle code paths are covered by unit tests
  • The change is described in the changelog section above

Reviewer guidance

  • Is the code clean and well-commented?
  • Are there tests for this change?
  • Are all UI components LTR and RTL compliant (if applicable)?
  • Add other things to check for here

@MisRob
Copy link
Member

MisRob commented Dec 9, 2024

Thank you @josephinoo! Let me invite my colleague @akolson for review. @akolson would you also coordinate and confirm with @radinamatic please? @radinamatic here's where you can test dropdown https://deploy-preview-856--kolibri-design-system.netlify.app/buttons/#dropdowns

@MisRob MisRob requested review from radinamatic and akolson December 9, 2024 09:16
@@ -26,6 +26,23 @@
<KButton text="Secondary flat button" :primary="false" appearance="flat-button" />
</KButtonGroup>
</DocsShow>
<p>
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate you think of documentation @josephinoo. I wonder whether we need to mention this in the docs though since many KDS components have a11 baked in, and generally from a user-developer point of view, there's no need to know the details.

@radinamatic any thoughts about this? https://deploy-preview-856--kolibri-design-system.netlify.app/kbutton/ this is the new section
Screenshot from 2024-12-09 10-19-03

Copy link
Author

Choose a reason for hiding this comment

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

Thank you, @MisRob, for the feedback! You make a good point about the built-in accessibility features. I can remove the explicit documentation about ARIA attributes, since they work automatically without developer intervention. The focus should be on the component's functionality rather than its internal accessibility implementation. Let me update the PR accordingly.

@MisRob MisRob added the TODO: needs review Waiting for review label Dec 9, 2024
@akolson
Copy link
Member

akolson commented Dec 9, 2024

hi @josephinoo is this pr still a work in progress, or we can proceed to review it -- I see it's still in draft?

@josephinoo josephinoo marked this pull request as ready for review December 9, 2024 12:10
@josephinoo
Copy link
Author

Hi @akolson, Can you review it ?

Copy link
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

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

Code changes look great. Thanks @josephinoo. I'll add a little blocker as we await feedback about the documentation pieces from @radinamatic. We should be clear after this. Thanks again

@AlexVelezLl
Copy link
Member

Hi @josephinoo! Can you please rebase your branch on top of the latest KDS develop? We recently updated the KDS nodejs version to v18 and vue to v2.7, and we need to do the rebase for the actions to run successfully :). There are also new linting rules, so after rebasing, please run yarn lint-fix again to comply with the new rules. Thanks!

@josephinoo josephinoo force-pushed the develop branch 3 times, most recently from 40df99a to 421a745 Compare December 11, 2024 05:15
@josephinoo josephinoo force-pushed the develop branch 3 times, most recently from f81d4b9 to 27a9978 Compare December 13, 2024 03:53
@josephinoo
Copy link
Author

Can you check it again @akolson?. I had to update the code for edge cases :)

@josephinoo josephinoo requested a review from akolson December 13, 2024 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants