Access Admin Toolbar search field via keyboard shortcut alt + a

Thanks @ressa once again for suggesting this feature.

Thanks a lot for contributing an initial merge request 🙏

I rebased the merge request, then added a commit:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/106/di...

Removed the maintainers section in the added README.md file, since I "think", we would better move all these sections to a specific MAINTAINERS.txt file at the root of the module, see for example:
https://git.drupalcode.org/project/token/-/merge_requests/76/diffs#7797f...

Fixed a bit the added JS code:
1 - Moved the code down inside the call to function once.
2 - Replaced the call to focus on document, with context.
3 - Fixed some minor ESLINT issues.

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 👍

Once again: front-end development is not my strongest suit, so I would greatly appreciate if you could please review these changes and let me know if this is the correct way to capture the keyboard input.
Are we sure the Alt + a keyboard shortcut is not used by default on certain browsers, software, OS, etc... ?
 

Overall, I'm fine with this feature @ressa, but could you please help me double check in the issue queue if there are other issues related with keyboard shortcuts or navigation?

I "think" I saw some tickets... not sure whether they were closed or postponed or active... I don't remember exactly, but I "think" I remember seeing issues about module's keyboard shortcuts...

This would be very helpful!
I will also do a quick search in module's tracker to look for other potentially related issues. I'd like to see if this could be related, duplicated or conflicting with any other tickets.

I will also look into adding FunctionalJavascript Tests for this 👌
 

In short: From what I can see, to move this forward we need to:
1 - Check for other similar/related issues
2 - Add automated tests
 

Since all the jobs and tests are passing 🟢, moving issue to Needs review, as an attempt to get more testing feedback and help on the remaining tasks.

Thanks very much in advance!🙂

Did a quick search in other contributed modules and found the Project Browser module seems to be using the same syntax with event.key, see:
https://git.drupalcode.org/project/project_browser/-/blob/2.0.x/sveltejs...
so we "should" be fine using this method as well to compare with the user input 👌

I also did a quick search in project's issue tracker:
https://www.drupal.org/project/issues/admin_toolbar?text=keyboard&status... →

Found the following issues worth of interest, other than the ones you had already identified and related to this issue:

• ✨ Allow shortcut key to be configurable Active : Probably the most relevant.
• ✨ Tabbing order does not satisfy 508 accessibility requirements Active : Related with keyboard navigation.
• #3056856: Create a keyboard shortcut to toggle access the search functionality → : just for reference.

 
I like very much the suggestions at #3087173-7: Allow shortcut key to be configurable → :

• Adding a tooltip over the search with information on the shortcut.
• Providing information on the project page about the shortcut.
• Allowing users to configure shortcut, either through the interface or at the very least with some kind of override in code.
• Ideally, it could be a translatable string so the default value could be changed automatically for certain languages (localize.drupal.org), for example, maybe in Polish "Alt + a" is a combination for a special character (?!) 😅

From what I could see in current module's code base, there doesn't seem to be any code for this shortcut anymore and you have to go through the accesskey, as you described in the issue summary.

So overall, I would be fine with the current MR and implementation, but maybe we should consider addressing some of the comments above at the same time.
Maybe extend a bit the scope of this ticket?

I would greatly appreciate your opinion on the potentially related tickets and this feature in general.
Thanks in advance!

Thanks for taking a look at this issue @dydave, I very much appreciate it!

And great updates to the code. Like I wrote in the Issue Summary, I simply lifted the code from the Tour module issue, and here's where the key gets set, which I coped and altered:

https://git.drupalcode.org/project/tour/-/merge_requests/67/diffs#c35c44...

Great find in Project Browser, and good to get confirmation that the method is used several places.

And lucky that your french keyboard acts differently, so we caught this in advance 😅 Your update works equally well, and Alt + a sets focus in the search field.

I think we can never be totally sure that any shortcut is not already in use in some browser. But it looks to me like Alt + a does nothing Firefox and Chromium ...

About other tickets, there is my original issue, which added access key parameter "a", or maybe it's some other issue?

Great suggestions about expanding the scope of this issue! I have added your suggestions in the Issue Summary under "Remaining tasks":

• Remove the accesskey attribute based shortcut alt + shift + a <<<< IN MR ALREADY
• Add a JavaScript based shortcut Alt + a <<<< IN MR ALREADY
• Adding a tooltip over the search with information on the shortcut << NEW TASK
• Providing information on the project page about the shortcut << NEW TASK
• Make the shortcut optional, enabled by default << NEW TASK

Future plans, maybe in a follow up issue?

Allowing users to configure shortcut, either through the interface or at the very least with some kind of override in code.
Ideally, it could be a translatable string so the default value could be changed automatically for certain languages (localize.drupal.org), for example, maybe in Polish "Alt + a" is a combination for a special character (?!) 😅

Making the shortcut configurable would be fine with me. Or, for a start (to simplify things) it could be an optional setting (checkbox) under Admin Toolbar Search, like "Enable search form shortcut Alt + a", and enable it by default? Then, if there is a occasional clash of shortcuts, these few users who experience a problem can disable it. Then later, we can consider expanding the support for configurable shortcuts, in a follow up issue?

Looking forward to your feedback!

dydave → closed merge request !106

Thanks a lot @ressa!

Looks like we're making great progress here! 🥳

I created the new merge request MR !141 above at #9, which is a complete rewrite of the initial merge request, based on your suggestions to take this in smaller steps:

• Added a setting to enable/disable the keyboard shortcut for the Search, see added config enable_keyboard_shortcut.
• Added in install, schema, settings form, in a hook_update and used in the module file to conditionally add a new library which loads the JS behavior that attaches the keyboard shortcut to the search field.
• Fixed JS code for the case where 'display_menu_item' is enabled.
• Added some refactoring: Converted existing setting display_menu_item from an integer value to a boolean.
• Added Functional Tests to check whether the correct JS files would be loaded depending on the form settings.
• Added Tooltip on hover of the search text field ('title' attribute) with text: 'Keyboard shortcut: Alt + a'.

 
Mostly, I moved out the initial JS code to its own library to avoid changing existing files and be able to conditionally add or remove this particular piece of JS, based on an added form setting (checkbox).

From what I can see all the points from your comment above at #7 should have been addressed in this new merge request, except the changes to the project page, but maybe we could add this as a post 3.6.0 task, in the ticket for the next release? Or maybe in a more generic ticket related with documentation changes on the project page?

Also, maybe we could look into adding FunctionJavascript Tests for the added JS code.

Since all the jobs and tests are still passing 🟢, moving issue back to Needs review as an attempt to get more testing, reviews and feedback on merge request MR !141.

Feel free to let me know if you spot anything else or if you have any comments on any of the latest changes, I would certainly be glad to help.
Thanks in advance for your feedback! 🙂

Fantastic work @dydave, thanks! I tried the MR, and it works flawlessly, and the shortcut is enabled after an install. I also tried installing the current dev-version, patch with this MR, and then run database updates, and the new feature was enabled, cool 😎

Nice job with also adding tests, which is always best to have, and refactoring the JavaScript code and its location, so it's only loaded if the feature is enabled. I checked the source, and it works well -- the file admin_toolbar_search.keyboard_shortcut.js is only present, if the Alt + a shortcut is turned on, nice! And it's indeed a cool touch, and very user friendly to show the shortcut tip, when hovering over the search field.

I agree, the changes to the project page can always be handled in another issue as a post 3.6.0 task, in the next release Meta issue, and I have added this under "Remaining tasks".

Also, maybe we could look into adding FunctionJavascript Tests for the added JS code.

Great idea, we should add a follow up ticket for this!

I did a quick, direct correction in the MR's README file (shortcut to "Alt + a") to not stall the issue with a "Needs Work" change, I hope it's all right.

Overall, it looks great and ready to me. Thanks yet again @dydave for another stellar job! 🎉

Thanks a lot @ressa once again for your great help and very encouraging feedback, it's greatly appreciated! 🙏

Following your last confirmation, I gave the merge request a last round of reviews and tests locally, with big_pipe and since everything still worked fine, as expected, I went ahead and merged the changes above at #14.

That's one more to scratch off the list! 🥳

Thanks a lot for the update of the 3.6.0 plan ticket, for the notes to add:
I think this is a very interesting feature that deserves being brought to the attention of more users 👍
So it's definitely worth putting forward in the release notes and on the project page.

I "think" the project page would deserve a bit of a refresh as well, but we could come back to that later on, in tickets more related to the documentation.
I'll come back on the FunctionalJavascript Tests for this feature as well, in another ticket, probably looking a bit closer at the Tests we could be missing for the module.👌

For the time being, marking issue as Fixed.

Let's keep moving forward with the work on this release 👍

Thanks again for suggesting this very nice feature and for working with me through the changes for the implementation! 🙂

You're welcome @dydave! I am very glad how this feature was re-written by you, and made into a much more complete solution with great care and attention to detail -- and how it turned out in the end. Yet another awesome addition to an already great module! 🎉

I agree that the project page could do with an update, like most project pages could on a regular basis ... I have created 📌 Update the project page Active .