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

Create accessible dropdown component #13019

Open
wants to merge 62 commits into
base: master
Choose a base branch
from

Conversation

rak-phillip
Copy link
Member

@rak-phillip rak-phillip commented Jan 8, 2025

Summary

This creates a new dropdown menu component that can be used to create accessible dropdown menus with built-in keyboard navigation.

Fixes #12771
Fixes #12772
Fixes #12790

Occurred changes and/or fixed issues

  • Add new RcDropdown components to Rancher Components
    • RcDropdown.vue - Root dropdown component that controls dropdown communication and behavior. Dropdowns are composed of Triggers, Items, and Separators
    • RcDropdownItem.vue
    • RcDropdownSeparator.vue
    • RcDropdownTrigger.vue
  • Add new RcButton component to Rancher Components
  • Replace Page Action Menu and User Menu with new Dropdown Menu components
  • Update unit tests

Technical notes summary

RcDropdown consists of several, tightly-coupled, composable components that are designed to be accessible and provide keyboard navigation out of the box. My primary motivation for promoting component composition over a props-driven approach was to allow for replacing multiple bespoke dropdown menu implementations without requiring too much refactoring in the process. The two dropdown menus that are replaced in this PR demonstrates this ability by showing how RcDropdown can be used to replace the native html element without requiring much effort with regards to considering component logic or keyboard navigation.

Areas or cases that should be tested

  • Page Actions Menu
  • User Menu

Areas which could experience regressions

  • Page Actions Menu
  • User Menu

Screenshot/Video

dropdown-demo.mp4

Checklist

  • The PR is linked to an issue and the linked issue has a Milestone, or no issue is needed
  • The PR has a Milestone
  • The PR template has been filled out
  • The PR has been self reviewed
  • The PR has a reviewer assigned
  • The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed
  • The PR has reviewed with UX and tested in light and dark mode, or there are no UX changes

Copy link
Member

@aalves08 aalves08 left a comment

Choose a reason for hiding this comment

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

General comment:

Kind of hard to read for me since I am not familiar with the composition API. Maybe you can guide me today through the PR on our sync and tutor me on some of the composition API aspects.

If I am not mistaken on my analysis (bear with me):

  • why is RcDropdownCollection needed instead of just using RcDropdown? seems to be a div with some props
  • why does RcDropdownItem registers event handlers for keydown for each individual item? If we have 15 items, we are registering 15x2=30 events. Can't that be achieved in a broader level with only 2 event handlers in a component like RcDropdown?
  • we should have some props on RcDropdown to control certain props of the v-dropdown, if needed. We can add those later as per need basis. Same for the other components like aria-label in the RcDropdownCollection, if we are going to keep it.

Nevertheless, the functional seems to be exactly what was needed and behaves well. Just some minor adjustments needed on focus:visible, especially on dark mode where the white border and outlines overlap in a weird way, imo.

Screenshot 2025-01-09 at 14 00 03

Copy link
Member

@richard-cox richard-cox left a comment

Choose a reason for hiding this comment

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

  • Generally
    • Compared to options api, composition api can get really confusing given the amount of types and consts that are glued together at different points. In more complex components this could lead to a lot of confusion. Is there any way we can clean these up to be more clear? I’ve made some comments but overall i feel we’re missing something.
  • Testing
    • Has this been tested (even in a hacky way) with SSO enabled?
  • UX
    • When the hover state of the action menu is visible and then it gains the focus state the box grows in side (gains the border). This feels a bit off putting. Can the focus be included with the hover and be a different colour? Need to discuss with UX team?
    • Focus state of user avatar (border not flush with button content) is different from focus state of header action menu (border flush with button content
  • Bugs
    • After clicking on the action menu the drop down is shown. Pressing tab though doesn’t take focus to the first entry in the drop down. Same issue for user avatar (first entry not selected)

Update
Missed two bugs

Dropdown location doesn't seem correct. Chrome Version 132.0.6834.83 (Official Build) (64-bit)

image

image

cypress/e2e/po/side-bars/page-actions.po.ts Show resolved Hide resolved
pkg/rancher-components/.eslintrc.js Show resolved Hide resolved
shell/components/nav/HeaderPageActionMenu.vue Show resolved Hide resolved
shell/components/nav/Header.vue Outdated Show resolved Hide resolved
shell/components/nav/Header.vue Outdated Show resolved Hide resolved
@rak-phillip
Copy link
Member Author

  • Generally
    - Compared to options api, composition api can get really confusing given the amount of types and consts that are glued together at different points. In more complex components this could lead to a lot of confusion. Is there any way we can clean these up to be more clear? I’ve made some comments but overall i feel we’re missing something.

I’m working on organization techniques. I still think some of these patterns can be improved upon.

  • Testing
    - Has this been tested (even in a hacky way) with SSO enabled?

That’s a good callout - SSO will need to be tested. I can deploy these changes and report any modification required when using github auth.

  • UX
    - When the hover state of the action menu is visible and then it gains the focus state the box grows in side (gains the border). This feels a bit off putting. Can the focus be included with the hover and be a different colour? Need to discuss with UX team?
    - Focus state of user avatar (border not flush with button content) is different from focus state of header action menu (border flush with button content

This is how the menus behave today:

image

We are actively engaging @edenhernandez-suse to develop consistent designs and patterns for dropdown menus throughout Dashboard. rancher/ux#113

  • Bugs
    - After clicking on the action menu the drop down is shown. Pressing tab though doesn’t take focus to the first entry in the drop down. Same issue for user avatar (first entry not selected)

This is not a bug and is by design. The design guidlines for the menu pattern, provided by w3, detail specific usage of arrow keys and tab behavior.

https://www.w3.org/WAI/ARIA/apg/patterns/menubar/

@rak-phillip
Copy link
Member Author

Dropdown location doesn't seem correct. Chrome Version 132.0.6834.83 (Official Build) (64-bit)

The placement has been updated

@rak-phillip rak-phillip force-pushed the chore/12771-dropdown-component branch from da2f6a3 to d8adb6e Compare January 25, 2025 01:58
Copy link
Member

@aalves08 aalves08 left a comment

Choose a reason for hiding this comment

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

Codewise, there's a lot to take in, but you've addressed my main concerns after the first review + we've been discussing details in our weekly syncs on accessibility, so I would say that on that front Richard has presented a very thorough review which seems to be enough to act on.

Functionality wise, I would say it meets all of the demands of the default behaviour expected to this type of element. I did manual tests on both elements you've replaced on the header component and they LGTM.

One last question: do you think we need aria-label's in the dropdown trigger and dropdown item? 🤔

@rak-phillip
Copy link
Member Author

One last question: do you think we need aria-label's in the dropdown trigger and dropdown item? 🤔

Agreed. I made the change.

@rak-phillip rak-phillip force-pushed the chore/12771-dropdown-component branch 3 times, most recently from 8164ecf to 14bb73d Compare January 29, 2025 20:23
Signed-off-by: Phillip Rak <[email protected]>
Signed-off-by: Phillip Rak <[email protected]>
Signed-off-by: Phillip Rak <[email protected]>
Signed-off-by: Phillip Rak <[email protected]>
Signed-off-by: Phillip Rak <[email protected]>
Signed-off-by: Phillip Rak <[email protected]>
We simplify the component interactions and rely on the `dropdownContext` instead

Signed-off-by: Phillip Rak <[email protected]>
@rak-phillip rak-phillip force-pushed the chore/12771-dropdown-component branch from 372d5fe to bc1e676 Compare February 4, 2025 16:50
@rak-phillip rak-phillip force-pushed the chore/12771-dropdown-component branch from bc1e676 to 1ddd61f Compare February 4, 2025 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants