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

[3.1] A11y/ManagerMenu: hotfix for links not clickable 3th level #16671

Conversation

jenswittmann
Copy link
Contributor

What does it do?

Hopefully it fixes #16667.

How to test

  1. Try clicking on all the links in the manager menu. Especially in the 3rd level.
  2. Try navigating through the Manager Menu using the Tab key on your keyboard and clicking on the menu items using the Enter key.
  3. Make sure there are no new errors 😸.

Related issue(s)/PR(s)

#16530
#16667

@modxcommunity
Copy link
Collaborator

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

https://community.modx.com/t/extjs-problems-on-newly-installed-site/8247/2

Copy link
Collaborator

@smg6511 smg6511 left a comment

Choose a reason for hiding this comment

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

This does appear to fix the issue at hand ;-)

However, and I'll post an issue on this, there is a usability issue that this causes (the original a11y addition, not this PR specifically): It removes focus from the modals and overlay windows that may appear as a result of clicking a menu item. Clear Cache is a primary example. You cannot press return/enter to submit the window's form; you must click on it.

Also - I don't know if this poses a merging challenge, but I noticed this PR is applied on the current -pl rather than the 3.1.1-dev branch. Check with Jason (@opengeek) on that...

@jenswittmann
Copy link
Contributor Author

This does appear to fix the issue at hand ;-)

@smg6511 thank you for reviewing it!

However, and I'll post an issue on this, there is a usability issue that this causes (the original a11y addition, not this PR specifically): It removes focus from the modals and overlay windows that may appear as a result of clicking a menu item. Clear Cache is a primary example. You cannot press return/enter to submit the window's form; you must click on it.

I can confirm that this is a recent bug. When you click "Clear Cache", the focus leaves the navigation. The modal gets the focus, but then the eventlistener from the navigation sets the focus back to the subnav opener button.

I'll look into this.

Also - I don't know if this poses a merging challenge, but I noticed this PR is applied on the current -pl rather than the 3.1.1-dev branch. Check with Jason (@opengeek) on that...

This is my fault. Is it possible for @opengeek to change this on your site, or should I open a new PR in the 3.1.1-dev branch?

@opengeek
Copy link
Member

Also - I don't know if this poses a merging challenge, but I noticed this PR is applied on the current -pl rather than the 3.1.1-dev branch. Check with Jason (@opengeek) on that...

This is my fault. Is it possible for @opengeek to change this on your site, or should I open a new PR in the 3.1.1-dev branch?

I don't see any issue with this PR? There is no separate branch for 3.1.1-dev; it's all just 3.x. @smg6511 — are you just referring to it being applied before I bumped the version in the 3.x branch? That will not be an issue. 😉

@smg6511
Copy link
Collaborator

smg6511 commented Jan 13, 2025

@opengeek - Just wanted to point it out since usually when pulling down something to test from the 3.x branch, the UI will show the current dev # (expecting 3.1.1-dev); but when I was testing, it showed up as 3.1.0-pl. Sounds like it's just a timing thing.

@opengeek
Copy link
Member

@opengeek - Just wanted to point it out since usually when pulling down something to test from the 3.x branch, the UI will show the current dev # (expecting 3.1.1-dev); but when I was testing, it showed up as 3.1.0-pl. Sounds like it's just a timing thing.

Yep, understood. It's only an issue if the commits touch the same lines in the same files.

@jenswittmann
Copy link
Contributor Author

However, and I'll post an issue on this, there is a usability issue that this causes (the original a11y addition, not this PR specifically): It removes focus from the modals and overlay windows that may appear as a result of clicking a menu item. Clear Cache is a primary example. You cannot press return/enter to submit the window's form; you must click on it.

@smg6511 maybe this check will solve your problem?

@jenswittmann jenswittmann requested a review from smg6511 January 13, 2025 22:18
Copy link
Collaborator

@smg6511 smg6511 left a comment

Choose a reason for hiding this comment

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

Great, the tweak you made takes care of the window focus issue. Danke schön, Jens!

@opengeek opengeek added this to the v3.1.1 milestone Jan 21, 2025
jenswittmann and others added 5 commits January 21, 2025 08:58
…#16667

- remove variables subNavOpen and _this
- set focusRestoreEls from array to focusRestoreEl as object
- remove the class from the submenu instead of using hide()
- refactor to use requestAnimationFrame() instead of setTimeout()
@opengeek opengeek force-pushed the a11y_managermenu_hotfix_links_not_clickable_3th_level branch from 68f1314 to 236f484 Compare January 21, 2025 16:03
@opengeek opengeek merged commit 66a9b79 into modxcms:3.x Jan 21, 2025
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.

[3.1.0] Third level menu items can't be clicked
4 participants