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

feat(flyout-menu): accessibility improvements for flyout menu component #1495

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

d-rita
Copy link
Contributor

@d-rita d-rita commented May 6, 2024

Implements LIBS-563


Description

This feature improves the accessibility of the flyout menu component

  • A user can close the flyout menu by pressing the Escape key
  • By pressing the Right Arrow key on a focused menu item containing a submenu, a flyout menu is opened and focus is placed on the 1st item.
  • By pressing the Left Arrow key on any focused item inside a submenu, it will close and focus the parent menu item if any.

Checklist

  • API docs are generated
  • Tests were added
  • Storybook demos were added

All points above should be relevant for feature PRs. For bugfixes, some points might not be relevant. In that case, just check them anyway to signal the work is done.


Screenshots

supporting text

@d-rita d-rita requested a review from a team as a code owner May 6, 2024 06:33
@dhis2-bot
Copy link
Contributor

dhis2-bot commented May 6, 2024

🚀 Deployed on https://pr-1495--dhis2-ui.netlify.app

@dhis2-bot dhis2-bot temporarily deployed to netlify May 6, 2024 06:37 Inactive
Copy link

cypress bot commented May 6, 2024

Passing run #3355 ↗︎

0 584 0 0 Flakiness 0

Details:

feat: flyout accessibility improvements
Project: ui Commit: 0675f993c2
Status: Passed Duration: 06:42 💡
Started: May 6, 2024 6:38 AM Ended: May 6, 2024 6:45 AM

Review all test suite changes for PR #1495 ↗︎

@tomzemp tomzemp requested a review from Chisomchima June 18, 2024 10:10
@d-rita d-rita force-pushed the LIBS-563/flyout-menu-accessibility branch from 0675f99 to 0342c0c Compare June 28, 2024 23:51
@dhis2-bot dhis2-bot temporarily deployed to netlify June 28, 2024 23:55 Inactive
@d-rita d-rita force-pushed the LIBS-563/flyout-menu-accessibility branch from 0342c0c to de7d976 Compare June 29, 2024 00:01
@dhis2-bot dhis2-bot temporarily deployed to netlify June 29, 2024 00:11 Inactive
@d-rita d-rita force-pushed the LIBS-563/flyout-menu-accessibility branch from de7d976 to 94f69df Compare June 29, 2024 00:17
@dhis2-bot dhis2-bot temporarily deployed to netlify June 29, 2024 00:21 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify June 29, 2024 00:48 Inactive
@d-rita d-rita force-pushed the LIBS-563/flyout-menu-accessibility branch from d14f666 to f31fd08 Compare June 30, 2024 16:50
@dhis2-bot dhis2-bot temporarily deployed to netlify June 30, 2024 16:54 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify June 30, 2024 18:54 Inactive
@d-rita d-rita force-pushed the LIBS-563/flyout-menu-accessibility branch from 841d270 to 3a6e749 Compare June 30, 2024 19:07
@dhis2-bot dhis2-bot temporarily deployed to netlify June 30, 2024 19:11 Inactive
@d-rita d-rita force-pushed the LIBS-563/flyout-menu-accessibility branch from 3a6e749 to 51a0207 Compare July 1, 2024 19:09
@dhis2-bot dhis2-bot temporarily deployed to netlify July 1, 2024 19:13 Inactive
@d-rita d-rita force-pushed the LIBS-563/flyout-menu-accessibility branch from 51a0207 to b930acb Compare July 4, 2024 15:48
@dhis2-bot dhis2-bot temporarily deployed to netlify July 4, 2024 15:52 Inactive
- focus the popper's first child element when it is rendered
- move focus to flyout menu's first child, i.e. the menu when it receives focus
- the Right arrow key opens the submenu, Left arrow key closes submenu and focuses its parent item
- close flyout menu with the Escape key by passing it a closeMenu function as a prop
- add unit tests simulating opening and closing of submenus
@d-rita d-rita force-pushed the LIBS-563/flyout-menu-accessibility branch from b930acb to 9c66c4d Compare July 4, 2024 16:19
@dhis2-bot dhis2-bot temporarily deployed to netlify July 4, 2024 16:23 Inactive
@alaa-yahia
Copy link
Member

alaa-yahia commented Jul 16, 2024

Hi @d-rita
I've tested the Flyout menu in the aggregate data entry app. However, I wasn't able to move from the DropdownButton to the Flyout menu as demonstrated in the screen recording. Is this functionality outside the scope of this Flyout menu accessibility pull request?
Here is the code for options dropdownButton in aggregate data entry app.
Screen recording: https://github.com/user-attachments/assets/f4c19f5c-654e-49fa-90c8-9112a529f1f3

@d-rita
Copy link
Contributor Author

d-rita commented Jul 16, 2024

I've tested the Flyout menu in the aggregate data entry app. However, I wasn't able to move from the DropdownButton to the Flyout menu as demonstrated in the screen recording. Is this functionality outside the scope of this Flyout menu accessibility pull request?
Here is the code for options dropdownButton in aggregate data entry app.
Screen recording: https://github.com/user-attachments/assets/f4c19f5c-654e-49fa-90c8-9112a529f1f3

In order to reproduce this, I have tested with one of the DropDownButton storybook examples with a somewhat similar implementation, i.e. the WithMenu example. Then I also tested with the aggregate data entry app Options component implementation from above on a test branch

  1. WithMenu storybook example: The first menu item receives focus immediately the menu is rendered
with.menu.mov
  1. Data entry app's Options component (as is):
    Code implementation found here. Only the child items within the menu that are not conditionally rendered as a group receive focus, i.e. the last one
options.menu.mov
  1. Data entry app's Options component (after modification):
    Code implementation found here
    The modification was to conditionally render each child item instead of all of them wrapped in a fragment.
    Outcome: all menu items are able to receive the tabIndex attribute, and hence be focused as needed.
modified.options.menu.mov

Conclusion:

  1. Most likely, the implementation in the data entry app will need an update to allow for navigation with a keyboard.
  2. Ideally, this PR only handles the FlyoutMenu component hence updates to the DropDownButton regarding the menu (for example, passing the closeMenu prop which is a function to close the flyout with the Esc key will be handled in another PR) cc: @Chisomchima

Copy link
Member

@alaa-yahia alaa-yahia left a comment

Choose a reason for hiding this comment

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

Thank you for investigating the root cause @d-rita. Nice digging!
I am approving the pr based on your conclusions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants