search

digitoimistodude / air-light

💨 WordPress starter theme - designed to be minimal, ultra-lightweight (< 20 kB) and easy for all kinds of WordPress projects. 7+years/1000+hours of development and still updating daily! We prefer the original WordPress way of doing things so no strange templating languages or frameworks here.
https://airwptheme.com
MIT License
929 stars 139 forks source link

JS Error with Keyboard navigation #181

Closed michaelbourne closed 1 year ago

michaelbourne commented 1 year ago

Hey folks,

Wanted to run this by you before sending a PR, just to make sure we're on the same page.

When using keyboard nav, and the last item has a drop down, the down arrow on the last subitem triggers a JS error. Easy fix though.

On https://github.com/digitoimistodude/air-light/blob/master/js/src/modules/navigation/a11y-dropdown-menu-keyboard-navigation.js#L333 we check if the parent/parent is a submenu, but we don't check for a sibling.

We just need to add a && thisElement.parentNode.nextElementSibling

I'd also recommend a return after https://github.com/digitoimistodude/air-light/blob/master/js/src/modules/navigation/a11y-dropdown-menu-keyboard-navigation.js#L333 since there doesn't appear to be any reason to keep processing the key press.

See any problems with that?

michaelbourne commented 1 year ago

Also applies to the pending PR for click-only navigation: https://github.com/digitoimistodude/air-light/pull/180/commits/94742fad799d88c7232110a72c6c5ee48edb06fc#diff-82a6675c34c589afb7e70373dd59460badfc5d6c27047b257347af48af10a65cR278

Still assuming nextElementSibling exists.

ronilaukkarinen commented 1 year ago

Thanks @michaelbourne! Sounds reasonable. We are in the process of adding click nav support, I'll test these with click nav #180 soon. We are also most probably revising the endless parentNode selectors with MutationObserver and things like .querySelector(':scope > a').

I'll let you know once I'm done with the testing!

michaelbourne commented 1 year ago

Always improving :)

:scope is interesting, really curious to see how you use it to replace parent selectors.

ronilaukkarinen commented 1 year ago

I'm pretty sure I fixed this in the last release and haven't seen it for a while. Let's reopen it if the problem occurs again.