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

fix(menubar): Add aria-selected and aria-activedescendant to menu bar items (fix #3911) #4888

Merged
merged 1 commit into from
Oct 20, 2023

Conversation

juliushaertl
Copy link
Member

@juliushaertl juliushaertl commented Oct 20, 2023

📝 Summary

Fixes #3911

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

Base automatically changed from a11y/toolbar to main October 20, 2023 09:07
@juliushaertl juliushaertl added bug Something isn't working 3. to review labels Oct 20, 2023
@juliushaertl juliushaertl added this to the Nextcloud 28 milestone Oct 20, 2023
@juliushaertl juliushaertl marked this pull request as ready for review October 20, 2023 09:13
susnux
susnux previously requested changes Oct 20, 2023
@@ -27,6 +27,7 @@
v-bind="state"
:container="menuIDSelector"
:aria-label="actionEntry.label"
:aria-activedescendant="!!currentChild"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be a boolean value, but the ID of the currently active (focused) element.

Copy link
Contributor

Choose a reason for hiding this comment

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

Meaning this should be set on the menu toggle with the value of the active action button in the popover

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, seems I mixed that up, will have a look

Copy link
Member Author

Choose a reason for hiding this comment

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

Adjusted to have a unique id matching with the menu entries.

@juliushaertl juliushaertl merged commit 052cf54 into main Oct 20, 2023
28 of 31 checks passed
@juliushaertl juliushaertl deleted the a11y/toolbar2 branch October 20, 2023 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants