[#23254] - [4.0][a11y] Amenu
- Fixed in Code Base
- 18 Mar 2019
- Medium
- Build: 4.0-dev
- # 23254
- Diff
- ylahav:amenu
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Success continuous-integration/drone/pr the build was successful Details
- Success continuous-integration/appveyor/pr AppVeyor build succeeded Details
User tests: Successful: Unsuccessful:
Pull Request for Issue # .
Summary of Changes
adding keyboard navigation to menu
Testing Instructions
create different types of menus (with sub-menus and without)
Reach (with 'tab') the menu under test and check the following behavior:
- right arrow - next item (same level). If last - back to first item
- left arrow - prevision item (same level) . if first - go to last item
- down arrow - if sub-menu exist - go to sub level, first item
- if not - behave as right arrow - up arrow - if first item in sub-menu - go to parent item
- if not first item or no parent item - same as left arrow
Expected result
The behavior was explained above
Actual result
Documentation Changes Required
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Unit Tests Repository Administration com_admin |
| Labels |
Added:
?
?
|
||
@brianteeman - Yes, I realize it... I will commit the missing file soon
| Labels |
Added:
?
Removed: ? |
||
| Category | Unit Tests Repository Administration com_admin | ⇒ | JavaScript Repository Modules Front End |
| Labels |
Removed:
?
|
||
Please move the script to media_src and convert it to ES6
This code does a lot more than adding the keyboard arrow support you describe. It is also adding various markup that really should be part of the view not the js - otherwise it cant easily be overriden. Also you really should have documented those markup changes in the original report so that they can be tested correctly.
This code does a lot more than adding the keyboard arrow support you describe. It is also adding various markup that really should be part of the view not the js - otherwise it cant easily be overriden. Also you really should have documented those markup changes in the original report so that they can be tested correctly.
I Agree. The markup left from a plugin I am writing to old versions of Joomla where markup will be added as well... I will remove it from the code.
| Title |
|
||||||
FWIW this is still not es6. Try to lint the script and you’ll figure out what needs to be done
FWIW this is still not es6. Try to lint the script and you’ll figure out what needs to be done
I did it with - https://jshint.com. can you recommended another lint?
Just exetute the one in the repo:
npm run lint:js
@dgrammatiko our linter runs fine against this too (see the drone results since i just merged in staging)
EDIT: Ignore me @ylahav can you edit the file name to be .es6.js please
Updated to joomla code standards
This is still not ES6...
Then you need to explain in more detail what the issue is because as you can see from the drone output, it is happy with the linter
I agree with Brian and will fix it. No need to explain...
On Sun, 30 Dec 2018, 15:35 George Wilson <notifications@github.com wrote:
Then you need to explain in more detail what the issue is because as you
can see from the drone output, it is happy with the linter—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#23254 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGOuEwl3auKSJ-VkIb3dKzmPRBhZyWl9ks5u-MEsgaJpZM4ZKYUk
.
Surely the js should be included in the tmpl - otherwise it can't be overridden
See this as an example
What more I have to do in order this change will be included in J4?
It needs tests - presumably as the a11y team leader you can get the team members to test this
@ylahav FWIW having 3 different scripts to render a menu is not the right way...
@dgrammatiko I know only about one - menu.es6.js (that after George rename mine)... what I miss here, thanks!
It needs tests - presumably as the a11y team leader you can get the team members to test this
@brianteeman - thanks - I asked them
Yes ther is one more in the module menu and another script called metis menu. Also I’m not sure if this menu doesn’t have any dependency on bootstrap.js as I’m observing some classes there.
And finally some comments on the actual code: you’re traversing the dom way too much you shouldn’t! This will be laggy in any low end mobile
Should it even be active on a mobile?
Should it even be active on a mobile?
Good question - I don't think it "give" something if it is active in mobile....
@dgrammatiko - metismenu is used only in the administrator menu... so for the FE it is not applicable...
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/23254.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/23254.
I have tested this item
I can tab through the page and navigate through the menu. Looks nice.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/23254.
I am in vacation until March 10... Will do it when coming back home..
On Wed, 27 Feb 2019, 20:14 Franciska Perisa, notifications@github.com
wrote:
@ylahav https://github.com/ylahav I would really like to see this
merged. Please could you solve the remaining issues? :)—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#23254 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGOuE2Ybd63NSZ1oE7hdK6mECvsHmHkDks5vRoS2gaJpZM4ZKYUk
.
I'm merging this as it is to clear the path for those who want this in. If someone could make a PR here to solve the issue mentioned here with the target element it would be useful #23254 (comment)
Thanks @ylahav !
| Status | Pending | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-03-18 13:50:15 |
| Closed_By | ⇒ | wilsonge |
grrh - what was the rush to merge broken code
What's broken about it? I see the anonymous function issue addressed since the 2 good tests. The fact it's got a slightly generic selector didn't seem to cause any issues when testing - it's sub-optimal for sure - but doesn't appear to be breaking anything immediately. And it's simple to add a fix for.
I still dont see what the rush was
Still broken for IE. IE doesn't support iteration over node collections (eg document.querySelectAll IS NOT an array and you cannot iterate it on IE).
Also, this script is traversing the DOM on each event, this is plain wrong as is a performance hog if you ever get a reasonable menu...
Also not IIFE as requested by @Fedik...
As it has only just been merged please revert it so that it can be fixed BEFORE it enters the codebase
Also not IIFE as requested by @Fedik...
Yes it is. The change request was not marked reviewed, completed, or whatnot.
Still broken for IE. IE doesn't support iteration over node collections (eg document.querySelectAll IS NOT an array and you cannot iterate it on IE).
I'm not seeing this issue noted in this PR, so kind of hard for someone to address an unknown issue. That would've been helpful feedback in December with your "this is not ES6" comments.
I can confirm that this does not work in
IE11
Edge
Console with Edge reports SCRIPT5022: SCRIPT5022: SyntaxError
I looked into the JS and fixed most of the IE11 issues:
- querySelectorAll => [].slice
- correct selector (instead of .nav)
Still working on:
- :scope call, which does not work in IE11
- the classes where I'm not sure if they're needed
Will do a PR when everything is resolved
thank you
@bembelimen fwiw I would create a class and in the constructor, I would cache ALL the elements in some arrays. Then the event listeners won't have to traverse the DOM on each call, instead, they will do whatever is needed to the appropriate element(s) from the cache. Properly done...
me thinks there are some files missing