New keyboard shortcuts do not work on MacOS

MR is up.

Small update to the MR. I realized we should prevent transmission of the keypress for Alt + p as well, in case the cursor is somewhere that can receive keyboard input.

Adding static patch for testing.

Thanks a lot Michael (@justcaldwell), that's very helpful!

We didn't really carry any real testing with MacOS when working on these keyboard shortcuts and since we knew there would be issues, we added a configuration option to disable them, if needed, for the time being.

There is already a follow-up issue for this scope of work, see ✨ Allow shortcut key to be configurable Active :
which aims at making these shortcuts more flexible, with the ability to select different key combinations, see some elements of discussion in the following issue:
#3494646-6: Access Admin Toolbar search field via keyboard shortcut Alt + a →

Allowing users to configure shortcut, either through the interface or at the very least with some kind of override in code.

 
The idea behind this feature would be to take a more generic approach to solving any issues with conflicting keyboard key combinations (whatever language, system, OS, etc...).

Is this something we could trying moving to the Feature request instead and group our development efforts?

Otherwise, concerning the merge request and the use of event.code === 'KeyA' instead of event.key === 'a':

We already tried this implementation and encountered the following issue #3494646-5: Access Admin Toolbar search field via keyboard shortcut Alt + a → :

4 - But most important: Changed the condition event.code === 'KeyA' to event.key === 'a'.
When I tested the initial code, the feature didn't work with my keyboard (French azerty) 😅
Turns out on my keyboard the KeyA corresponds to the key Q 😅 (Q <==> A on French keyboards, the key positions are switched)

So it felt like the event.code property couldn't be reliable enough and a different property should be used instead.
Printing the event object allowed me to check which other properties or keys could be used and I found event.key: 'a'.

Good thing we have different types of keyboards, regional settings, languages, etc... 😅
It certainly allows us to cover more potential use cases 👍

So it definitely seems there is more complexity to try matching a specific keyboard combination.
Therefore, we thought we should keep it simple in this first version and try to extend this feature in priority, in the next version.

The current shortcuts are going to work fine in many cases (default) and probably not in other ones with conflicts for various reasons (browser plugin shortcut, language, keyboard type, etc....), but as long as they could be disabled, for the time being, it should not be a problem with the module.
Until we could ✨ Allow shortcut key to be configurable Active , so that a wider range of specific keyboard setups or combinations could be supported.

Any feedback, comments or advice would certainly be greatly appreciated.
Thanks in advance!

Hi @dydave -- thanks for the feedback! I'd be glad to help test ✨ Allow shortcut key to be configurable Active once it gets underway.

Until that happens, obviously it's up you you and the other maintainers, but I think there's potential value in trying to resolve this bug as it affects a potentially substantial set of users (i.e. MacOS).

Thanks for sharing your experience on a French keyboard -- it definitely expanded my thinking on testing for this. I configured some additional keyboard layouts on my machine for testing, and I was able to replicate the issue you described with the event.code approach.

Using W3C's Keyboard Event viewer, I tested the current versions of Chrome, Firefox and Safari with keyboard layouts U.S (standard qwerty), French (azerty), German, Spanish, U.S. Dvorak. In all cases, event.keyCode (even though it's deprecated) seems the most reliable property, and is consistent for a (65) and p (80). This is reinforced by the first table on the MDN docs for keyCode.

Given all that, I think the following would be a more fault tolerant approach, and should resolve the bug for MacOS users (and possibly others):

if (event.altKey && event.key === 'a' || event.keyCode === 65) ...

and

if (event.altKey && event.key === 'p' || event.keyCode === 80) ...

I'll update the MR with those changes. Thanks!

dydave → changed the visibility of the branch 3527344-new-keyboard-shortcuts-debug to hidden.

dydave → changed the visibility of the branch 3.x to hidden.

Thanks a lot Michael (@justcaldwell), once again for the very helpful and constructive reply!

Thank you very much for documenting and testing very carefully the changes, with various keyboard layouts and doc links, it will certainly be very useful when we try to approach the related feature request.

No problem at all for getting this change added before, as well 🙂

We would definitely want to provide support for MacOS users, especially if this could be achieved with very few changes 👌

OK, concerning the MR:

Based on your tests and comments above at #7, I would expect the changes to the checks for the keys would work on your setup and they do as well on mine, so great job! 👍

One thing though: The FunctionJavascript Tests for the Admin Toolbar Search seemed to fail with the MR. 🔴
See pipelines: https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/154/pi...

So I cloned the branch and tried debugging the build and identified the problem was caused by:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/154/di...

Removing this particular line seemed to fix the Tests and the MR came back green 🟢

Going back to #4:
Do we really need to the calls to:

            // Prevent transmitting keypress.
            event.preventDefault();

I only removed the one from the Admin Toolbar Search keyboard shortcut, but I "think" this was probably one of the main reasons you added these calls initially (to prevent the key press from being recorded by the input field)....

So I'm not sure exactly how to approach this problem, at this point, would you have any ideas or advice?

Could we maybe add a test or check for MacOS users?

Or maybe we would really need this call, probably revert my commit and try to fix the broken Tests instead and find out the reason they could be failing?

Feel free to trigger more Tests and make more changes to the debugging branch, it is very practical for triggering and debugging broken builds:
https://git.drupalcode.org/issue/admin_toolbar-3527344/-/tree/3527344-ne...

Based on your feedback we should be able to move this issue forward and hopefully get a stable solution very soon 🤞
Thanks in advance!

Ahh, the refactored conditional was too greedy and capturing keyboard input even when altKey wasn't true. Grouping the 'key' and 'keyCode' conditions did the trick.

Score one for unit tests! 🙌

Tested this on macOS 15.5 on latest Arc (chrome), Safari, and Firefox Dev. Same behaviour in all browsers:

1) With this patch opt-a does now bring up focus on the admin toolbar search, but it also inputs "å" into the search. That isn't ideal because you then have to delete that character before typing your search.

2) opt+p has no effect.

3) More advanced mac users will know that alt = option / ⌥, but ideally it'd show "⌥ + a" instead of "alt + a". If this would be trivial to change based on parsing user agent strings that'd be useful. More of a minor/optional QoL issue.

Thanks for testing @erutan!

1) Great catch. I neglected to add back the code to suppress the keystroke when I addressed the issue in #10. Should be fixed now.

2) opt+p is working for me. Can you confirm that the 'Hide or show the toolbar with shortcut (Alt + p)' setting is enabled on the Admin Toolbar Settings page at /admin/config/user-interface/admin-toolbar?

3) I agree that the shortcut "tip" text could be improved — it shouldn't even be there if the shortcut has been disabled — but that's probably out of scope for this issue and could be handled in ✨ Allow shortcut key to be configurable Active .

Back to NR for testing.

1) yup that's much nicer now.

2) user error on my part, I missed that when setting this up locally (I hadn't set that shortcut before, idk what everyone has on their machine for shortcuts that is editing the site). :)

Outside of the scope of this issue, but it is a little odd it's tucked at the bottom of "Toolbar sticky behavior", since it's only tangentially related to that. Either the header should be something like "Toolbar visibility" or it should be in it's own section. It also wouldn't be terrible if the keyboard shortcuts were just in their own section instead of below what they're related to.

3) Agreed, just seemed worth pointing out since it was related.

Great job Michael (@justcaldwell) and @erutan on this! 🥳
Thanks a lot! 🙏

The changes look good at this point and consistent with my comment above at #10.

Moving this back to Needs review to prevent the MR from getting merged abruptly.
(But we're clear: the issue was marked RTBC by @erutan above at #15 👍)

I will review and most likely merge this within the next 24 hours.

In the meantime, feel free to let us know if you see anything else that we could have missed or would have any questions, we would be surely be glad to help.
Thanks in advance! 🙂

Thanks a lot Michael (@justcaldwell) and @erutan for the great work on this issue! 🙏

I tested the changes locally once again and the keyboard shorcuts worked fine, on my PC laptop, just as they did before.
I couldn't test the changes for MacOS myself, but trust that you have been testing them yourselves successfully.

Since the tests and jobs all seemed to be passing 🟢, I went ahead and merged the changes above at #17. 🥳

Let's settle for this patch for now and as mentioned above, try improving this feature in related ✨ Allow shortcut key to be configurable Active .

I've added your comments and suggestions above at #13 (point 3) and #15 (point 2) to the related issue so we could hopefully circle back on these comments and try to include these changes in the next merge requests.

Since we've covered the follow-up tasks for this issue, at this point, it should most likely be considered as Fixed for now.

Feel free to let us know if you have any questions or concerns on any aspects of the latest code changes or this ticket in general, we would surely be glad to help.
Thanks again both for your great help! 🙂