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

Keyboard accessibility improvements #16613

Merged
merged 12 commits into from
Sep 23, 2024
Merged

Keyboard accessibility improvements #16613

merged 12 commits into from
Sep 23, 2024

Conversation

jenswittmann
Copy link
Contributor

@jenswittmann jenswittmann commented Sep 5, 2024

What does it do?

Make the MODX manager interface more accessible when using the keyboard (WIP 🧑‍🏭 ).

Bildschirmaufnahme.2024-09-09.um.11.02.13.mov

How to test

Minify CSS and compress JS via grunt first. Use the Tab key to navigate through the Manager UI. Use the Esc key to close the subnavigation. Use the tab key to navigate through the tabs.

Related issue(s)/PR(s)

See #16612

@jenswittmann jenswittmann changed the title A11y: A11y: add a simple keyboard navigation to the main navigation Sep 5, 2024
@modxcommunity
Copy link
Collaborator

This pull request has been mentioned on MODX Community. There might be relevant details there:

https://community.modx.com/t/modx-manager-accessibilty/7990/3

@opengeek
Copy link
Member

opengeek commented Sep 5, 2024

@jenswittmann — thanks for this contribution, Jens. When submitting PRs, please do NOT commit changes to the compiled CSS or minimized JS files produced during the grunt build. You can indicate that a grunt build is required to test the changes in the PR section How to test. Let me know if you have any questions.

@opengeek
Copy link
Member

opengeek commented Sep 5, 2024

Thank you for your contribution! Before your pull request can be reviewed, please sign the MODX Contributor License Agreement (CLA). This is to ensure MODX has your written permission to distribute projects containing your contributions under the appropriate license.

Please make sure the CLA has been signed for GitHub user(s): @jenswittmann

Once you've signed, please reply with a comment letting us know so we can verify.

@jenswittmann
Copy link
Contributor Author

Thank you for your contribution! Before your pull request can be reviewed, please sign the MODX Contributor License Agreement (CLA). This is to ensure MODX has your written permission to distribute projects containing your contributions under the appropriate license.

Please make sure the CLA has been signed for GitHub user(s): @jenswittmann

Once you've signed, please reply with a comment letting us know so we can verify.

@opengeek i signed the CLA 👍🏽

@jenswittmann
Copy link
Contributor Author

jenswittmann commented Sep 5, 2024

@jenswittmann — thanks for this contribution, Jens. When submitting PRs, please do NOT commit changes to the compiled CSS or minimized JS files produced during the grunt build. You can indicate that a grunt build is required to test the changes in the PR section How to test. Let me know if you have any questions.

@opengeek thanks for the info. I remove the minified and compressed files and add a hint on how to test.

Copy link
Member

@theboxer theboxer left a comment

Choose a reason for hiding this comment

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

Awesome addition, thank you. Works as described

@jenswittmann
Copy link
Contributor Author

After testing the PR I found some problems and tried to fix them.

  • The whole navigation is marked as a landmark with <section aria-label="Navigation">.
  • In general, the subnavigation should be closed when you leave focus on the last item. The subnavigation is placed in the footer for some reason (?). So when a subnavigation is open, it is the last DOM element on the page. So focus after the last item leaves the page and brings focus to the browser UI (e.g. Current Tab). There is no way to bring the focus back to the opener button. So I move the subnavigation from the footer to the header.
  • When you leave the focus on the last subnavigation item, the subnavigation is now closed and the focus is on the opener button.
  • It is now possible to use the keyboard to navigate the sub-navigation.
  • For now, I have set the focus outline online for the navigation. On the other UI elements there are too many side effects at the moment. This should be fixed component by component.

Here is a small demo of the new update:

Bildschirmaufnahme.2024-09-09.um.11.02.13.mov

@opengeek
Copy link
Member

opengeek commented Sep 9, 2024

@jenswittmann — we had an issue with the CLA form when you attempted to sign it, so it did not record the action. Could you do that again when you get a chance? Sorry for the inconvenience.

@jenswittmann
Copy link
Contributor Author

@jenswittmann — we had an issue with the CLA form when you attempted to sign it, so it did not record the action. Could you do that again when you get a chance? Sorry for the inconvenience.

I submit the CLA form again. Is it ok now?

@opengeek
Copy link
Member

opengeek commented Sep 9, 2024

@jenswittmann — we had an issue with the CLA form when you attempted to sign it, so it did not record the action. Could you do that again when you get a chance? Sorry for the inconvenience.

I submit the CLA form again. Is it ok now?

It is! Thank you!

@theboxer
Copy link
Member

@jenswittmann I'm not sure if this was also an issue before the new update, but menu items that have JS handler (clear cache, delete locks, flush your permissions, etc.) won't trigger when hitting enter.

@theboxer theboxer self-requested a review September 11, 2024 11:41
@jenswittmann
Copy link
Contributor Author

@theboxer thanks for this bug! I think its an older bug. Now i add a href attribute to the links to properly fire the onclick event b993603

Can you check it again?

@theboxer
Copy link
Member

@jenswittmann thank you! Now those items trigger fine.

I noticed one more thing, once you're under sub menu, you can't escape from it using esc key, you have to finish going through it. You think you could adjust this as well?

@jenswittmann
Copy link
Contributor Author

jenswittmann commented Sep 12, 2024

@theboxer i restore the escape function to go back on the root button 96e2542

The way the current navigation is structured, it will be difficult to optimise the keyboard navigation even more. So i think this is the best what we can get for now. Have a look at the example "Dropdown Menus" from Floating UI. I thinks this is the goal, but its based on React. A complete rewrite of the Navigation in Vanilla JS with some help of AlpineJS Focus Plugin could be a good choice 🤔 #16612

@rthrash rthrash added this to the v3.1.0 milestone Sep 12, 2024
Copy link

codecov bot commented Sep 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 21.49%. Comparing base (9a3f82c) to head (96e2542).
Report is 2 commits behind head on 3.x.

Additional details and impacted files
@@             Coverage Diff              @@
##                3.x   #16613      +/-   ##
============================================
+ Coverage     21.47%   21.49%   +0.02%     
- Complexity    10652    10665      +13     
============================================
  Files           561      561              
  Lines         32268    32250      -18     
============================================
+ Hits           6929     6933       +4     
+ Misses        25339    25317      -22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jenswittmann jenswittmann changed the title A11y: add a simple keyboard navigation to the main navigation [A11y]: make manager interface more accessible when using keyboard Sep 13, 2024
@jenswittmann jenswittmann changed the title [A11y]: make manager interface more accessible when using keyboard [A11y] make manager interface more accessible when using keyboard Sep 13, 2024
@jenswittmann
Copy link
Contributor Author

Thanks to @theboxer and @Ibochkarev for their help 👍🏽
I have fixed some issues and moved PR #16614 to this for better handling. This PR is now for some global accessibility optimisations, not just navigation.

The checkboxes now also get an outline when focused via the keyboard. ec4e1d9

@jenswittmann
Copy link
Contributor Author

I have changed the markup for the modx-button component. The create and refresh buttons are now keyboard focusable and triggerable in the resource tree f903a73

@theboxer
Copy link
Member

@jenswittmann pls let me know once this PR will be complete so I can re-review it

@theboxer theboxer self-requested a review September 16, 2024 12:33
@jenswittmann
Copy link
Contributor Author

@theboxer I did another keyboard test. Now it should be better to focus on the main UI elements. All other tweaks like windows, tables, landmark/section movement and so on should be in a separate PR for better handling. Maybe this PR can be included in the next 3.0.6 release instead of 3.1.0 🤔

Bildschirmaufnahme.2024-09-16.um.18.mp4

@jenswittmann
Copy link
Contributor Author

@theboxer I found some problems with the tab order and added some skiplinks to avoid long tab adventures aac69f5

You can now test everything for the next MODX release 👍🏽

Bildschirmaufnahme.2024-09-18.um.21.13.34.mov

Copy link
Member

@theboxer theboxer left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thx for all the a11y improvements!

@opengeek opengeek changed the title [A11y] make manager interface more accessible when using keyboard Keyboard accessibility improvements Sep 23, 2024
@opengeek opengeek merged commit b207371 into modxcms:3.x Sep 23, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants