🇫🇷France @dydave

dydave → made their first commit to this issue’s fork.

@dgwolf:

This issue was also reported in the Admin Toolbar Content → module, see:
📌 Compatibility with Admin Toolbar 3.6.0 Active
where a patch was submitted in issue's merge request MR !37.

Could you please try testing again with the suggested patch for admin_toolbar_content?

Could we maybe try fixing this issue in the Admin Toolbar Content → module instead?
Do you think we could consider Admin Toolbar is working fine and that the issue is rather with Admin Toolbar Content?

Otherwise, as a "personal" advice:
It would seem the module Admin Toolbar Content → is Minimally maintained with No further development, a rather limited user base (about 500 reported installs) and maintainers are now directing users to Navigation Extra → as a potential replacement.

Therefore, if you have a choice, I would recommend to try moving away from the Admin Toolbar Content module to another solution, since the module doesn't necessarily seem to be very well maintained.

Since a solution was provided in this comment, moving issue to Needs review for now, as an attempt to attract more attention, reviews, testing and reporting feedback.

Feel free to let us know if you have any questions on this issue or the changes suggested in the merge request, we would certainly be glad to help.
Thanks in advance!

Quick follow-up on this issue:

Created the initial merge request MR !37 above at #2 which :

Converts the __construct method to create and all the existing code or logic was kept pretty much "as-is".

After testing the changes locally, the error disappeared and the expected menu items seemed to display properly under Content, for example: this patch should fix the fatal error.👌

Moving issue to Needs review for now, as an attempt to attract more attention, reviews, testing and reporting feedback.

Feel free to let us know if you have any questions on this issue or the changes suggested in the merge request, we would certainly be glad to help.
Thanks in advance!

dydave → created an issue.

Created initial merge request MR !156 above at #13 based on latest patch at #12.

• Created new branch with issue fork from the links in the IS.
• Checked-out branch locally and applied the patch cleanly without error.
• Committed/pushed the changes to initial merge request.

 
Since the Tests seem to be failing ❌, the MR was set in Draft for now, since more work would be needed to integrate with the patch required for Drupal Core.

The MR in related core issue would also require some help updating and testing.

Could we requalify this issue as Feature request?
As it shouldn't register as an existing "bug" in the module since the feature is not completely integrated with Drupal Core yet.

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

@alex.skrypnyk: Sorry again for the additional work and noise on the updates.

A fix was committed in the DEV branch in issue #3527315-8: in admin_toolbar_toolbar_alter(), Trying to access array offset on value of type null → and should be released in a patch version hopefully soon, after a few other "urgent" issues have been addressed (as many as possible).

We will try to be more careful with these types of error cases in the future.
Sorry again for the inconvenience and thanks in advance for your understanding. 🙏

Thanks Mathew (@minoroffense) for suggesting this feature!

I thought it would be worth mentioning the contrib module Accessible Menu → in the issue summary, since it provides a very quick and concrete way to immediately test the library, for anyone interested in exploring what an integration with Drupal could look like.

Thanks in advance for your feedback and comments!

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!

Thanks a lot Peter (@pwolanin) for taking the time to look at this and give us some advice.

When we first encountered this issue in #3440852-26: Make un-hover delay configurable → , I suggested to @ressa to test with the update hook, which fixed it, but we didn't anticipate on other types of issues.

We will try to stick to this approach in the future if there are more configuration changes with update hooks, to avoid as much as possible these types of issues.

For the time being, I went ahead and merged the changes above at #8.

This will most likely be released as a patch release, along with several other issues, if possible, in a 3.6.1 corrective version, for example.

Marking issue as Fixed for now.

Feel free to let us know if you spot anything else or if we missed anything, we would certainly be glad to help.
Thanks in advance!

Thanks Peter (@pwolanin)!

Any chance this could be related to 🐛 Warning: Trying to access array offset on null in admin_toolbar_toolbar_alter() Active ?

Thanks in advance!

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

dydave → changed the visibility of the branch 3314213-add-open-by to hidden.

Re #12:

Created merge request MR !10 targeting 2.1.x above at #19 based on the work from previous merge request by @aaronpinero.

I have not had the time to review the code more than that...
But I've done some basic tests locally and it seemed to work pretty well 👌
Great job @aaronpinero! 🥳

See a few screenshots of the results below:

CKEditor: Configuration of a details section to be open or closed by default:

CKEditor: View source, see the markup generated in the background:

Text editor configuration form in the admin backend:

At this point, it looks like all the points that were requested above should have been addressed in this merge request.

Keeping this issue in Needs review for now, as an attempt to attract more attention, get more feedback and reviews.

Feel free to let us know if you have any questions or concerns on any aspects of this comment, the merge request or this issue in general, we would surely be glad to help.
Thanks in advance! 🙂

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

Thanks a lot everyone for the great work on this issue, all the contributed patches and comments! 🙏

Looks like there is already a lot of history in this issue, going back to 2017 😅
 

Let's try summing up the current status:

Currently, this issue contains two different implementations to achieve a similar goal:

Improve the way dropdown menus with many items are currently displayed, so they could be visible.

1 - Multi-column menu dropdown: MR !150
Based on patch #59 which is the latest version of the very first implementation of this feature, when the ticket was initially created, so it has quite a lot of reviews and testing feedback.

Patch MR!150 displays long lists of items in dropdown menus broken down into multiple columns, so they could be displayed horizontally and fit the viewport, see a screenshot of the result:

2 - Scrolling menu dropdown: MR !151
Based on patch #55 which is a more recent approach of the feature with fewer exchanges, reviews or feedback, but seems like a very valid approach as well.

Patch MR !151 displays long lists of items in dropdown menus with a fixed height and a scrollbar, to be displayed when scrolling up or down the menu dropdown, see a screenshot of the result:

At this point, I have not started any kind of code review or careful testing of any of these merge requests, but only very superficial testing locally and they both seemed to work very well 👌
Great job everyone! 🥳

OK, first of all, we need to clarify what implementation should be chosen in this issue:

What solution should be implemented exactly? Solution 1 or 2 or something else?

Keep in mind, it is always possible to make any of these solutions configurable through the backend, for example, to enable/disable or select one method or another (select list, checkbox, radios, etc...).
It is also possible to break down this issue into multiple smaller issues with different types of implementations.

But if we would like to make any kind of serious progress here, we should start by having a clear vision of the results that would be expected by the changes in this issue.

What would be your suggestions on this?

Otherwise from a coding standpoint, MR !150 is much less intrusive than MR !151 which changes the HTML structure and classes of the menu with a twig template override....
So we would probably need to rework some of the code implementations once we have a better idea of the expected result.

Keeping this issue in Needs review for now, as an attempt to attract more attention, get more feedback, reviews of the new merge requests and replies to the questions in this comment.

Feel free to let us know if you have any questions or concerns on any aspects of this comment, the merge request or this issue in general, we would surely be glad to help.
Thanks in advance! 🙂

This is great stuff Mathew (@minoroffense)! 😍

Thanks a lot for sharing this! 🙏

Recently, I have been searching around, looking for a good Vanilla JS alternative to the current custom scripts based on the hoverIntent JQuery plugin.... But up until now, I couldn't find anything that could look like an exploitable solution.

Additionally, I have been looking at ✨ Tabbing order does not satisfy 508 accessibility requirements Active , which seems to basically implement with custom code exactly what the accessible_menu library does 👍

So essentially, if I understand this right: we "could" replace entirely (almost) all the JS and CSS that is currently used by the module to:

• Implement a full Vanilla JS version of admin_toolbar.
• Move all basic/default CSS/JS libraries out of the code base.
• Implement ✨ Tabbing order does not satisfy 508 accessibility requirements Active

 
On paper, this looks like an amazing idea and exactly for what I had been looking for the past two months 😅
 
Now, I would have to give the Accessible Menu → module and libraries a bit of testing to see how (and if) this could potentially work.
 
But, would you have any recommendations on how this could work?
Is my understanding correct or could I be missing something?
 
Would you like to take an initial stab at the implementation of this feature and create an initial merge request?
 
Any suggestions, advice or recommendations would be greatly appreciated.
 
Feel free to let us know if you have any questions or concerns on this comment or the module in general, we would certainly be glad to help.
Thanks in advance! 🙂

Thanks a lot everyone for the great work on this issue!

Quick follow-up on #5:

I've created an initial merge request above at #13 for the 2.1.x branch, based on the patch above at #9:

• Created the branch / issue fork from the links at the top of this issue/page based on 2.1.x.
• Checked out locally and applied patch from #9 without error.
• Recompiled assets locally: build.js with webpack.
• Committed and pushed the changes to issue fork.

 
I have tested a bit the changes myself locally and they seemed to work pretty well, great job! 👍
 
One thought though:
Could this be something we would like to be able to configure? Enable/disable, for example, or maybe select a tag (from a dropdown list)?
Or do you it be fine to just switch over to use the <p> tag?
 
Therefore, at this point, moving back issue to Needs review, as an attempt to collect more feedback, comments and reviews.
 
Feel free to let us know if you have any questions, suggestions or concerns on any aspects of the merge request or this issue in general, we would surely be glad to help.
Thanks in advance! 🙂

Understandable, thanks a lot Benjamin (@mlncn)!

I've create a follow-up issue to address all the validation errors on which Wylbur (@wylbur) has been working, see:
📌 Gitlab CI: Fix validation errors reported by jobs CSPELL, PHPCS and PHPSTAN Active

and moved all his work to MR !7 👌

Any chance we could consider this particular issue Fixed, get credits assigned and move to the other issue?
Maybe this ticket's merge request MR !6 could be closed now, since it's not going to go any further anyway?

Any feedback, comments or questions would be greatly appreciated.
Thanks in advance! 🙂

Quick follow-up on this issue:

• Created a new issue fork with this issue.
• Merged the branch 3516579-configure-gitlabci locally and pushed to this issue fork.
• Added extra commit to broaden Tests coverage and require all jobs to pass.
• Created merge request MR !7 above at #2.

 
Since all the jobs are now passing 🟢, moving issue to Needs review.
 
Please note: The changes to require jobs and broaden Tests coverage (GitLab CI config file) are completely at the appreciation and preference of the maintainers.
Feel free to adjust as you would see fit 🙂
Whatever works best for you!
 
Any feedback, comments or reviews would be greatly appreciated.
Thanks in advance!

Totally! Thanks a lot @ressa.

I'm adding here as well the change of wording in the settings form we couldn't get in the release:
#3304810-72: Toggle away Admin Toolbar completely →

dydave → created an issue.

Super good catch @ressa! 🥳

Thanks a lot for the very clear ticket:
No problem, we're going to fix this: I will take a look a little bit later today, when I have a bit more time 👌
 

Otherwise, I have the feeling this release went quite well overall 👍

Apart from users having issues running DB updates, we didn't get anything else really relevant to this upgrade.

I'll take also a look later in the day at 🐛 Upgrade version 3.5.3 to 3.6 crashes site Active .

Thanks so much, once again, for all your help answering issues and helping users updating their sites correctly! 😅

Thanks a lot Thom (@thomwilhelm)!

Looks like we have the same understanding of the complaints brought up in this issue:

this doesn't seem related with Admin Toolbar specifically, but rather with the way site administrators maintain and deploy their configuration on their sites.

Sure @ressa, no problem at all 🙂

Of course we're going to release "patch" versions of the 3.6 branch if there are any urgent issues to fix 👌
But I thought this would be straight forward enough not to require a specific ticket to request help for testing and/or plan for big changes.

I have created this issue mostly for ✨ Tabbing order does not satisfy 508 accessibility requirements Active which is a very big ticket with a large amount of changes .... so I thought that could potentially require another new minor version.
Also, we "could" start considering dropping support for version below 10.3 in the next minor, since by the month of August we should be about a year away from D10 EOL....

But in any case, I wouldn't mind at all changing the target version, the title of this issue, or anything in the coming scope of work 👌

I just created this issue really to try to get a list of priorities organized for the coming months.

Feel free to make any changes, suggestions or modifications to this issue or the suggested scope of work, we would be very grateful for your help, as usual 🙏
Thanks in advance!

I mean ... Alex, this doesn't seem related with Admin Toolbar specifically, but rather with the way site administrators maintain and deploy their configuration on their sites.

This seems like a very generic problem, which is most likely the case with all other contrib modules and core 😅

As far as the module is concerned, we "should" be able to consider everything was done in a very standard way.

Closing in favor of 🐛 Warning: Trying to access array offset on null in admin_toolbar_toolbar_alter() Active

Thanks everyone!

The way we usually do this in our projects:

Update the code base locally:
composer update -W (for example with a global update)
Update the configuration:
drush cr
drush updb
Export the configuration changes:
drush cex
Commit/push the configuration changes:
git add xxxx
git commit
git push

This is always the standard procedure that we follow for any core or contrib upgrade and ensures any module is properly updated with its configuration inline before it gets imported.
(drush deploy, for example)

I mean .... if it's like that Alex (@alexgreyhead) we are never going to be able to make any changes to configuration objects in general 😅

We have to be able to update configurations in our projects, otherwise we don't do anything anymore 😅

I'm not a big fan of getting this patch applied as it adds "unnecessary" code in my sense....

@dgwolf:

It seems you are using the contrib module Admin Toolbar Content → (see admin_toolbar_content in the stack trace in the issue summary), which "could" be the problem here ...

Sorry, but I didn't have time to test this myself, but it would be worth looking into:
https://git.drupalcode.org/project/admin_toolbar_content/-/blob/2.0.x/sr...

Just make sure you have the latest versions installed in general, for this module as well... maybe this could help fixing the issue in your case.

Otherwise, I'll try taking a look a bit later when I have more time.

Thanks in advance!

Thanks @nojj for the prompt feedback and glad it worked!

I thought it would be bad practice, to require a specific version as this would not be updated to e.g. V 3.5.4

Not really no.... it's OK to use a fixed version, we call this "pinned", since in some cases your site might be running a bit behind certain versions.
Indeed, in your following updates you might have to change the version constraint again to upgrade the module.
But you can probably deal with that a bit later, once you have a bit more time to figure out what could be breaking on your site with 3.6.0.

Thanks again for the feedback!

Fantastic!

Great catch Guillaume (@guilhom)!
I had completely forgotten to add Project Browser to the Breaking changes 😅

I have just added the following paragraph to the 3.6.0 release notes:

Important: Support for older versions of the Project Browser → module has been dropped: Please upgrade to the latest version or at least the 2.x branch.
If the Project Browser 1.x module is installed in your project and you upgrade Admin Toolbar, without upgrade Project Browser, your site will crash.

Hope this helps others not to go through the same types of errors.

Thanks again for your interest in the Admin Toolbar module.

Thanks a lot @star-szr for your help answering these "support" requests.

Closing this in favor of 🐛 Warning: Trying to access array offset on null in admin_toolbar_toolbar_alter() Active .

Cheers!

I don't understand .....

Site administrators are not running database updates after updating their sites anymore?

It has always been very standard to run DB updates after a site upgrade, whatever is being updated, whether core or contrib...

Running DB updates should update the necessary configuration values, see:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...

which should prevent this type of errors from occurring.

OK, no problem, thanks @nojj

Could you maybe try the following command?

composer require 'drupal/admin_toolbar:3.5.3'

Thanks in advance!

Any chance you could upgrade the project browser module in your project?

Thanks in advance!

Thanks a lot @ressa for catching this so quickly, before we got a storm of angry messages 😅

I've set the 3.4.x release unsupported instead, so we only show the last two releases supported on the project page:
3.5.3 (3.5.x) and 3.6.0 (3.6.x)

Let me know if you catch anything else, I will be monitoring the issues closely and try to answer as soon as possible for urgent requests 👌

Marking issue as Fixed for now.

Thanks again for your great help @ressa! 🙏

Thanks @ressa!

Sorry for the noise with the emails: I've made the change immediately on the administer releases page.

Could you please double check again and see if it is better now?

Thanks in advance!

dydave → created an issue.

That's it @ressa! 🥳

The new 3.6.0 release → is out! 🤩

This is probably when we really start receiving feedback on how we've managed breaking the module on sites 😅😆

Otherwise, I tried writing a bit of documentation on the release, with a short summary on the maintenance tasks and bug fixes, but a big highlight on the added features 👍

I thought it would be good to let everybody know about the new keyboard shortcuts and the pretty cool slide in/out sticky behavior.

I've also added a paragraph on some of the BC breaking code changes... Next time, in such cases, I will probably issue a change record, it will probably be easier 😅

25 issues with 19 contributors over 3 months!! It's enormous !! 😆🥳
We've managed taking the number of active tickets from more than 80 down under 30 \o/ a much more manageable base of active tickets, more realistic and easier to maintain.
We've greatly tightened the code base with improved Tests and stricter validation jobs allowing a much more efficient validation of changes submitted by contributors.

Really happy we could work together on this @ressa and this release would certainly not have been possible without all your great help, guidance and cooperation 🙏
Many of the features in this new release are based on your ideas, functional designs and suggestions.

Thanks again so much for your help working with other contributors and I, through rounds of changes in merge requests, your reviews, advice, discussions, issue summary updates, etc...
It definitely greatly contributed to making this module much better! 🙂
 

Very happy this release is now behind us, so we are now able to start looking at the next sprint/release, with already a bunch of tickets lined up in the issue queue, in particular: accessibility improvements, ESLINT fixes (let's get this in ASAP in DEV👌), compatibility with Gin, more configuration settings, etc...
I've also started working on 🐛 zindex issue between admin toolbar and ckeditor 5 Active 😅 and made good progress already 🤞
We could already start cloning this ticket to: [Meta] Roadplan for Admin Toolbar 3.7😆
and start sticking tickets on there.
 

Lastly, I'd would like to thank once again very much all other contributors with whom I had the great chance and pleasure to interact and work on the issues included in this release. I definitely learnt a lot and met great people with whom to share the same passion for Drupal and Open Source development 🤝

Marking issue as Fixed for now.

Feel free to let us know if you have any questions or concerns on any aspects of this ticket, the latest 3.6.0 release → or the project in general, we would certainly be glad to help.
Thanks in advance!

dydave → made their first commit to this issue’s fork.

Great job Tanner (@tanner.svg)! Great catch!
Thanks a lot!

The patch works great!

The changes in the merge request MR !6 remove the code that was added in issue #3111124: Force inclusion of drupal.collapse shim → , corresponding to commit:
https://git.drupalcode.org/project/ckeditor_details/-/commit/6d2cc1d71c4...

See file history:
https://git.drupalcode.org/project/ckeditor_details/-/commits/2.1.x/cked...

The code was added 5 years ago, probably to support older versions of Internet Explorer that could maybe not be supported anymore.
I didn't check in detail which versions of Internet Explorer could be impacted, but I would assume, going forward, in particular with D11 versions, support for older versions of Internet Explorer could probably be dropped.

Bumping issue to Major, as this is definitely a significant performance improvement for the module.

Moving to RTBC, as I would personally be in favor of dropping support for these older browser versions, if this could result in a significant performance improvement.

Feel free to let us know if you would have any questions on this issue or the merge request, we would surely be glad to help.
Thanks in advance!

It seems development has moved to the 2.1.x branch now, therefore:

Created the new merge request MR !8 above at #11 based on the previous merge request but rebased for branch 2.1.x.

Updated the previous merge request to be hidden.

This MR could use a round of textual/content review and overall testing.

Any feedback, comments, reviews or suggestions would be greatly appreciated.
Thanks in advance!

dydave → changed the visibility of the branch 3336825-hookhelp-is-missing to hidden.

dydave → made their first commit to this issue’s fork.

Closing in favor of 📌 Automated Drupal 11 compatibility fixes for ckeditor_details Needs review .

Closing in favor of 📌 Automated Drupal 11 compatibility fixes for ckeditor_details Needs review .

Martin (@mandclu), could this issue be closed or fixed now?
It's been more than a year without activity.

Thanks in advance!

Quick follow-up on this issue:

Created the merge request MR !7 above at #2 which adds an initial .gitlab-ci.yml file to automate builds on GitlabCI.

Added the bare minimum configuration to enable automated build pipelines with all validation jobs:
cspell, eslint, phpcs, phpstan and stylelint.
Left all the variables and custom jobs configuration code commented, so it could be enabled in the future (uncommented).

This is left completely at the appreciation and preference of the project maintainers: feel free to adjust the file as you would see fit.

The merge request build came back with errors for all jobs, but fixing them should be considered out-of-scope in this issue.
Fixing validation jobs should be addressed in separate tickets.

Bumping issue to Major as this task should be very straight forward and unblock the validation of all pending merge requests.

Moving issue to Needs review as an attempt to attract the attention of project's maintainer and collect more feedback.
Thanks in advance!

dydave → created an issue.

dydave → made their first commit to this issue’s fork.

Yes indeed Charles (@charles belov):
Both patches should not be applied at the same time, see my comment at:
#3286466-64: Tabbing order does not satisfy 508 accessibility requirements →

Otherwise, your feedback on this particular patch is very helpful and constructive as usual, thanks a lot!

No problem: We could consider changing the font color to a darker #000000.

[added] Actually, thinking about it, that's one more reason to use the browser's default focus indicator rather than coming up with your own.

So essentially, we would be reverting or resetting all styles for the focus of the menu items?
Do you think that could be a better option overall?

Once again, all your suggestions and advice are more than appreciated! 🙏
Thanks in advance!

Thanks so much Charles (@charles belov) for the feedback once again!

Please note my comment on Focus indication clashes with active tab indication. There is code in patch 138 which conflicts with the patch 149 for that issue.

Yes indeed: these are different issues currently under development, so they are very likely to have conflicts and there is not much we could do to fix that.
Ideally, we would like to consider these issues separately, or potentially group one in the other...

But you shouldn't be applying both patches at the same time to test both issues, there are great chances it won't work.
 

If we take out the problem with the styles of the focus:

What would be your general feedback on this patch: In particular, all the work that was put into changing the tabbed navigation and the display, etc... ?

There are a lot of things that could be tested in this issue, putting aside any other known issues that could be addressed separately.

Could we perhaps try focusing on what we already have in this patch?

I personally think it is a great improvement:
I would personally say the module is much better with this patch than without.

Please let us know if you encounter more issues while testing or if you would have any questions or concerns on any aspects of this issue, we would surely be glad to help!
Thanks in advance! 🙂

@charles belov: Was your comment at #9 meant for this issue or rather ✨ Tabbing order does not satisfy 508 accessibility requirements Active ?

It's a bit confusing...

Thank you so much Charles (@Charles Belov) for taking the time to test again this issue and for all the advice and suggested changes.

Thanks a lot for translating this issue into actionable technical changes.

Based on your suggestions above at #7, I created the initial merge request MR !149 above at #8 with the following changes:

• Focus background color: #33a39d
• Focus and non-focus text color: #2d2d2d

Could you please try testing these changes and let us know if we missed or misunderstood anything in your last reply?

- Active is bold and underlined. I do find it odd that it's shown as a link (underlined), since it's active and I can't tab to it. That might be a separate issue.

OK, no problem 🙂
Let's try to maybe focus on this color contrast change first, then we could always take a closer look at the active menu items styles.

Moving issue to Needs review for now, as an attempt to collect more comments, reviews and feedback on MR !149.

Feel free to let us know if you encounter any issues while testing the patch or have more suggested changes, we would surely be glad to help.
Thanks in advance!

Thanks a lot Charles (@charles belov) for taking the time to look at this issue, it's greatly appreciated! 🙏

Indeed: the patch in the MR was outdated and had conflicts with the latest DEV changes.

I have just rebased the merge request and it should apply fine now to the 3.x version of the module.

Could you please try testing the patch again?

I've just given it another round of tests and it really looks great 👍

Please, let us know if you encounter more issues trying to test the patch or with the module in general, we would surely be glad to help.
Thanks in advance for your feedback!

raaaaa.... looks like it was a random build fail 😅

Retrying the PHPCS job came back green 🟢
So the pipelines actually seem fine.

Let's try to make this change to the form wording in the next MR 👌

@ressa, thanks a lot for the very constructive and quick feedback, it's super appreciated! 🙏

Looks like we're going to have to be careful not to keep issues RTBC for "too" long in the issue tracker...
Otherwise they just might get abruptly merged 😅

Sorry I didn't have the time to make the suggested changes for the wording of the settings form before the MR got merged 😅

No worries: "Luckily" the 3.x build pipelines are now failing with PHPCS errors 🥳
So I'll make sure the changes to the form are added in the next MR to be merged asap to fix the dev branch build pipelines 👌

Otherwise, once again, I personally think you have great UX and feature ideas: I like very much your suggestion of a dropdown list of suggested shortcuts to make it configurable 👍 (efficient, simple, clear)
We could immediately create this ticket (feature) and probably group it/do the same kind of implementation for the Search focus shortcut ( ✨ Allow shortcut key to be configurable Active ). Some tickets, in particular feature requests could already start getting planned for the following release.

Additionally, we still have a spin off/follow-up ticket to create for this one ==> Improve compatibility with the Gin theme.
We definitely want to keep the work from @thomas.frobieter specifically related to supporting GIN, mostly the CSS and JS files (MR!70).
We'll most likely end up adapting/keeping most of the code, probably organized a bit differently.

So that's already 3 or 4 new tickets on which we could start working for the following release 👍

Lastly, thanks a lot for taking the time to kindly explain to other contributors/users how to apply patches or help testing module's merge request and for all the kind words and encouragements in other issues.

Special thanks to @sascha_meissner for getting this issue started and @grevil and @thomas.frobieter for helping getting it over the line!🙏

Thanks a lot @ressa!

Worst case: we don't necessarily need to wait for Eric's (@ericmulder1980) feedback:

Mostly, what remains for this issue:

Let's get the patch validated (RTBC-ed if possible)?
Were you able to test the patch from the merge request above MR!122, check the wording, check the message and feature (display of the error message)?

Anything that could help validating and confirming the changes so they could be pending merge for the next stable ...

Basically, if you could test the patch:
On a Drupal Core version above 10.3, apply the patch, then enable the admin_toolbar_links_access_filter module.
Just check the message displays as expected and whether you think the wording is clear enough....

That should be enough for us to RTBC the issue and get it merged quickly after 👌

I wish Eric (@ericmulder1980) could have replied on this, but if we can't get a reply before the next release, then it would be great if you could maybe take a very quick look at this one, just to confirm everything is in order 🙂

The impact of this MR is very limited, so it should be very safe to merge 👍
Bear in mind all of this should be trashed once the admin_toolbar_links_access_filter module can be removed from the code base (when support below 10.3 is dropped).

Let us know if you catch anything or have more suggestions or comments on any aspects of this ticket or the merge request, it would definitely be very helpful! (as always @ressa 😉)
Thanks in advance!

Thanks a lot @ressa once again for the prompt and positive feedback! 🙏

This is rather minor, but since we're at it, we might as well try squeezing in as much clean-up as possible before the release 😅
Just like you did with the Admin Toolbar Search settings form path 👍

I personally think the comment linked in the IS to the related issue, sums up very well the different options and removing the file 'CHANGELOG.txt' probably seems like the simplest option for this module right now 🙂 (and for its maintainers 😅)

Following your confirmation, I went ahead and merged the changes above at #5: another issue Fixed for this release 🥳

Let us know if you spot any other "small" changes that could help further polishing the module before its release (in a week🤞), I would certainly be glad to take a closer look very quickly 👌
Thanks again for all the great help reviewing and testing these issues!

Adding another one to the list 😅

I would personally be in favor of removing the file from the code base to align on other "standard" contrib modules.
Less files is also easier to maintain and this file doesn't seem to add any particular information that couldn't be found on the DO project page.

Moving issue to Needs review as an attempt to attract some attention and feedback on the suggested changes.

Thanks in advance!

dydave → created an issue.

Sounds great @ressa! 👍

Thanks a lot once again for keeping this feature on track 😅

Indeed, if you still find any textual element related with this feature that should be changed, we would surely be glad to do so.
Otherwise, let's keep the variable names as they are for now 👌

I have just updated the merge request to:

• Disable the 'toggle_shortcut' feature by default: Added a hook update to add the config variable value to module's config (a bit cleaner) and disabled the feature on install by default.
• Fix the expand button position for the disabled sticky behavior: Added the expand button CSS selector to the existing disable sticky CSS styles.
• Remove unused svg files, copied from the previous merge request and related with support for Gin. We can surely reuse these files in another merge request to fix compatibility with the Gin theme.

 
Updated the Functional Tests accordingly and they all seem to still pass 🟢

As a side note, the shortcut conflicts with a browser add-on I use.

Not sure what we could do for the shortcut though 😅
Would you have any suggestions of keys or ideas?

Other than that, I did another overall review of the code of the merge request and any potential conflict with other features, and at this point, we should be OK 👌

Back to Needs review for more testing, reviews and feedback on the latest changes.
Thanks in advance!🙂

Thanks a lot @ressa and @chhavi.sharma for your great help on this issue! 🙏

Following your confirmation at #8, I went ahead and merged the changes above at #9 👌

Another issue to scratch off our list, included in the next release! 🥳

Getting closer to the release date ... Let's see what else we could squeeze in 😅

Thanks again @ressa for suggesting these issues and @chhavi.sharma for your help with the code changes!

Other questions:
1 - What about existing sites?
With the current MR, when existing sites upgrade to this feature, it will be disabled by default (since the setting does not exist, has not been initialized), until the settings for is saved.
Should the feature be enabled or disabled after upgrade?

2 - Test with Sticky behavior disabled:
When hiding the toolbar, the icon stays fixed on screen, when the toolbar itself stays as the top of the page.
Do we want to make this behavior consistent by having the expand icon also stay at the top of the page (instead of following the scroll)?

Thanks again @ressa for the very clear and speedy reply!

OK, I've dealt with the simple changes, with the checkbox label and "Alt+p" (lower case) ✅

Concerning the variable name, this is a much bigger change 😅
enable_toggle_shortcut
Indeed, the keyword toggle is in all the names of the CSS/JS files, classes, functions and/or variable names in the JS code, etc... 😅

Besides, I "think" the keyword "toggle" in terms of code, in this particular context, should be "legit", in the sense, it actually toggles CSS classes in the body tag of the page, from one state (visible) to another (hidden) and vice versa.
This is what the JS function toggle actually does:
https://developer.mozilla.org/en-US/docs/Web/API/DOMTokenList/toggle

Maybe we could change it to something like toggle_display_shortcut or toggle_toolbar_shortcut?
Of if you think hide_show_shortcut could work better, then I would also be fine with that 👌

But in any case, if we were to make this change, we would better be 100% sure of the new name, since this is a pretty impactful change that could break quite a lot of things in this MR 😅

Moving back to Needs review for more feedback!
Thanks again! 🙂

Super nice @elimw! Great job! 🥳

Looks like everything requested above is now in the MR, with the added Unit Test 👌

Let's see if reviewers and maintainers could take another look at this issue.
Thanks again for the great help and work @elimw!

Indeed, great catch @ressa! 👍

I've just made the change to the MR and it is back to passing 🟢
Glad we have tests in place 🙂

Moving issue back to Needs review, for a final confirmation, before it gets merged 👌

Thanks again for all the great help!

dydave → made their first commit to this issue’s fork.

Thanks a lot @ressa as usual for the great help and feedback!

Thanks also Thomas (@thomas.frobieter) for the prompt and positive feedback on this updated merge request.

@ressa: No problem at all for all the suggested changes! 🙂

I have just moved the 'enable_toggle_shortcut' checkbox from the "Advanced" to the "Toolbar sticky behavior" ✅
It certainly makes sense, at least from a code perspective, since the setting is using the same CSS as the 'hide_on_scroll_down' sticky behavior.

I have also updated the checkbox label to: Enable keyboard shortcut (Alt + p) to show/hide the toolbar and couldn't find any more toggle keyword in the visible textual elements.

Should we also change toggle everywhere else? Or could we still at least keep it, as it is currently, for the variable names and such?
See for example, the name of the config: enable_toggle_shortcut

I think it "should" be fine to maybe keep it for variable names and in the code in general, as long as it is not visible by users and identifies clearly the feature for developers.

Otherwise, I've noticed we're using 'Alt + p' (lower case) in the form, but 'Alt +P' (upper case) in the JS code, see:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/146/di...
Are we fine keeping both? Or should we standardize with one of the two?

Feel free to send us more feedback, reviews and comments!
Thanks in advance! 🙂

Thank you very much @ressa for bringing this point up and all the kind words, once again, and Adrian (@adriancid) for granting me the necessary permissions to edit the project page, it's greatly appreciated! 🙏

I have checked and indeed, I now seem to be able to properly edit the project page ✅

I should be able to help handling any of the other documentation related issues 👌

The create issue page → now shows the updated message as suggested by @ressa above at #4 🥳

We should now be able to move forward with the other issues!
Thank you very much for your great help! 🙏

Thanks a lot everyone for the great work on this issue, with the CSS, JS and icons! 😍
It really looks great, thanks a lot!

Based on the work in the previous merge request, I created the new merge request MR !146 above at #61 which suggests a different approach for the implementation of this feature:

The shortcut keys were changed in this MR 😅
How about Alt + p? Could that work for you?
I have a plugin/extension in my browser locally that is using Ctrl + Alt + h already...
So I thought we could maybe try with "Alt + p" ?

For the setting, I thought we could do something similar to ✨ Access Admin Toolbar search field via keyboard shortcut alt + a Active and add a setting in the recently added Advanced section of the settings form.

Most of the work in MR !146 is around the CSS and JS logic:
Again, I thought we could maybe try to capitalize/build on top of what was done in ✨ Add Show on scroll-up / Always hide option to Disable sticky toolbar Active and re-use the display/hide animation and states/classes that were already defined.
The JS code was refactored to use Vanilla JS, fix ESLINT errors and implement a logic toggling CSS classes in the 'body' tag of the page, based on the local storage.
All the CSS styles and images for the toggle links were moved from the previous merge request.👌

Unfortunately, in this first draft, support for the Gin Theme had to be dropped: 😖
I noticed the 'hide_on_scroll_down' sticky behavior doesn't work very well with the Gin theme as well, so I thought maybe we could potentially address the issues at the same time.
Additionally, I "think" there is already quite a lot in this MR, so perhaps support for Gin for this feature should be part of a follow-up issue, maybe with a bit more generic approach? 😅

I would greatly appreciate to have your feedback and reviews on this new merge request, in particular, the shortcut key, the wording, labels, textual elements in general, the CSS/JS code and approach, etc...
Thanks in advance!🙂

Thanks a lot @ressa once again for your prompt and positive feedback! 🥳

Once again, following your reply, I went ahead and merged the changes above at #9 👍

Let's stay focused on the current tickets for now and maybe come back to CSS variables later, when we have more time, I guess 😅

Marking issue as Fixed for now:
That's one more included in the next release, great work!

We can still squeeze in more issues in this release, so feel free to let us know if you spot anything else, we would certainly be very grateful!🙏
Thanks again for all your great help!

Sounds great ! Thanks a lot @ressa! 🙏

Thanks for all the great feedback and encouragement in all the tickets, it definitely helps a lot! 🥳

The change suggested above at #17 has been added to merge request MR !145, which is very likely to get merged before the release 👌
See: https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/145/di...

Please let us know if you have other ideas or suggestions of improvements, it would definitely be greatly appreciated! 🙂
Thanks!

Thanks a lot @ressa once again for suggesting this change!

I checked quickly to see the color contrasts: https://webaim.org/resources/contrastchecker/

Currently: #003ECC in front of #FFFFFF
gives a ratio of: 8.19:1

I "think" it should be fine to update the default color of the search items links to something darker to increase the color contrast 👍

So I added a commit to your merge request above at #6, mostly to:

• Decrease slightly color contrast from 21:1 to 12.63:1
• Update search field placeholder label to include shortcut, as suggested in #3494646-17: Access Admin Toolbar search field via keyboard shortcut Alt + a →

 
I was a bit worried about adding a static value to override the link color, but it seems the module currently uses CSS static values everywhere 😅

Overall, I think the module should consider refactoring all static values in CSS files to use CSS variables instead, defined by the module or themes/core/defaults.
See for example:
https://git.drupalcode.org/project/klaro/-/blob/3.x/css/klaro-override.c...
But this is definitely out of scope and should be addressed in different tickets 😅

Let's focus on preparing this release and we'll get more technical debt issues fixed in the next one 👌

Thanks in advance for your feedback! 🙂

dydave → made their first commit to this issue’s fork.

Great catch @ressa!

But unfortunately, I don't have enough permissions to edit the project page 😭
I would need to be able to edit the project page to handle quite a few other documentation related tasks as well 😅

As for the message itself:
At the very least, the patching part should be removed, since officially: "we are only accepting merge requests".

What about replacing the current block with the following one?

Thank you very much for your interest in the Admin Toolbar → module.
We greatly appreciate your feedback, reporting, merge requests and tickets that could help improving the module.
For a good example of how to post an issue, feel free to take a look at Issue Summary Template standards.

Any comments, suggestions, testing, reporting, feedback, patches, ideas, features requests would be highly appreciated.
Thanks again for your time.

I might have to go back to Romain and ask for help on this one if we would really want to keep this moving along...
So I could get the necessary level of access to change these fields.

Any additional feedback, comments or suggestions would be greatly appreciated.
Thanks in advance!

 
Shall we plan for a release around the 21st of May 2025 (05/21/2025)?
(3 months after the last stable release)

We could certainly descope certain issues (postpone), in particular the one with ESLINT, which would probably need more help testing/reviewing and feedback.

Otherwise, I wouldn't see any other issues listed in the IS that could prevent from creating a release around the end of the month 👌

We could wait longer, if requested or necessary and would be glad to consider more issues to be added to the list 🙂

Any feedback, comments, or help on any of the pending issues would be greatly appreciated.
Thanks in advance! 🙏

Thanks a lot @ressa for the prompt and positive feedback! 🥳

Really happy I'm able to look at the module again and keep working with you on the next release 🤝

Following your confirmation above at #12, I did a last review of the merge request and since all the jobs and Tests seemed to be passing 🟢, I went ahead and merged the changes above at #13 🥳

Very good work in this issue, following-up with the recent changes that were made to the settings form and the added Tabs 👍
This is really helping make the module more presentable in the next release.

Marking issue as Fixed for now:
Feel free to create more issues if you see any further improvements that could be made to the form or anything we could have missed.
Otherwise, we'll make sure the form stays a bit maintained when adding more parameters or making changes in the future.

Thanks again to everyone for the great help testing and reviewing this issue! 🙏

The file LICENSE.TXT was removed from the code base at #3.

It should be automatically added to the next releases by Drupal.org packaging scripts.

Marking issue as Fixed for now.
Thanks!

dydave → created an issue.

Thanks a lot Charles (@Charles Belov), once again for taking the time to provide more details on the suggested changes.

I'm not seeing a color background to the focused item, I'm seeing the same gray background on both the Report and Structure, as shown in my screen print.

I've been testing with the DEV version of the module, could that explain the difference with the screenshot I attached above at #4, where you could see the background of the Focused item is different from the Active one?
Could you perhaps try testing with the latest 3.x DEV version of the module?

If there still is an issue with the latest version, we would certainly be glad to look further into your recommendations and try translating these into CSS/JS code changes.

Otherwise, I also wanted to point out some major work is currently being done to greatly improve accessibility for the module in issue: ✨ Tabbing order does not satisfy 508 accessibility requirements Active .
If you are interested in improved accessibility for the Admin Toolbar module, I strongly recommend you try testing the patch: I was very impressed 👍

Thanks again for your help on this issue!

@ressa! 🥳
Thanks a lot for your great help keeping this show on the road!
I've been very busy with work on projects lately and couldn't find the time to reply earlier 😅

OK, I've taken a quick look at the merge request and it looks great!👍

While we're at it, I thought we could go a bit further and added the following two changes:

• Add a small text at the top to describe a little bit what the module does and what can be configured in the settings form.
• Move the menu depth field in some kind of "default" settings fieldset, or menu/toolbar related.

I didn't really "like" to see the "Menu depth" field "floating" in the form like that...
Additionally, based on your comments above @ressa, I assumed we could probably hide the setting by default, since it is not "very" likely to be changed.

As for all the wording, labels, texts, etc... these are more suggestions, at this point:
Feel free to edit any added textual elements as you would see fit 🙂

Overall, these changes would help us pretty much stabilize/wrap up all the previous changes made to this form, so they could be properly packaged in the next stable release 👍

I would greatly appreciate any feedback and reviews on the changes to the form (collapsed/open?), the help text, or anything else we could have missed.

Thanks again everyone for the help moving this issue forward! 🙏

dydave → made their first commit to this issue’s fork.

Thanks everyone for the contribution!

Is this something we would really like to do?
As you can see this change is breaking Tests, since an important permission is added for existing module's routes.
See the permission used in the Tests: 'administer site configuration'
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...

This change could most likely break certain sites in the sense: certain roles might not have access to these routes anymore, after updating the module with this feature.
So it does not seem like a trivial feature to add... 😅

At the minimum, we would probably need to add a hook_update implementation to add the permission to any roles with the 'administer site configuration' permission, so the module's features stay the same on existing sites (after an update).

Additionally, there should most likely be ways to add permissions for these routes, for any particular project, with alter hooks or route subscribers, etc... So this patch should not be absolutely necessary to achieve the same results.

Overall, I'm not opposed to this feature, but a little worried of the impact it could have on the module compared to the benefits it could bring.

I'm leaving this in Needs review for now, to see if more feedback could be provided and whether more interest could be shown by users for this feature.

Thanks in advance for your comments and feedback! 🙂

Thanks Matt (@drupalmatts) for the reply 🙂

This is what the stable release on the project page currently looks like:
URL: https://www.drupal.org/project/login_switch →

Clicking on the DEV link of the release opens the following page:
https://www.drupal.org/project/login_switch/releases/3.x-dev →

Don't you find it confusing that the DEV release doesn't match the stable one?
I mean the versions supported are completely different ....
The dates as well are not consistent ....

What would other users and contributors think?

Thanks again for your help maintaining this great module.

Removed the hoverintent_functionality from the issue summary, since it was moved to admin_toolbar in issue 📌 Move the 'hoverintent_functionality' setting to admin_toolbar Active .

Thanks a lot everyone for the great help on this issue! 🙏

When I rebased the merge request, I didn't really pay attention and I overwrote the last commit from Tirupati (@tirupati_singh) 😅
So I re-applied the changes manually 👌
(sorry for overwriting your commit Tirupati... but the changes were not lost)

Overall, since these changes are really safe in terms of the main features of the module and all the jobs and tests were still passing 🟢, I went ahead and merged the MR above at #16. 🥳

This is going to be very practical for me when working on debugging and testing the module's settings forms 😅

We'll see later with a bit more time, if there is a better solution for the order of the module's menu links, but for now, this change shouldn't be "too" impacting (-1) 👌

Since all the work to be carried in this issue should have been completed at this point, marking issue as Fixed for now.

Feel free to let us know if you encounter any issues with any of the latest code changes, this ticket or the project in general, we would surely be glad to hear your feedback.
Thanks again everyone for the great help! 🙂

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! 🙂

Super nice Charles (@Charles Belov)! That's very helpful, thanks a lot!

OK, no problem, it seems you have well identified the issue 👍

Sorry, but I'm still trying to understand the problem here 😅
I have tried reproducing the steps you detailed above at #3 and here is a screenshot of the result :
Browse to the reports page and use the tab key until the focus is on "Structure"

It would seem the styles for identifying an active link and the ones for the focus state, would be slightly different: I'm seeing a different background color for the focus state and the active one is bold and underlined, as you can see on the screenshot above (the "Structure"/focused and "Reports"/active menu links) .

Could you please let me know what the problem could be exactly with the different styles: active and focused?

It would help greatly speeding up the resolution process if we could have a very clear understanding of the styles or elements to be fixed.

Expected result: the focus is clearly indicated and is well distinguished from the active tab, and both are well distinguished from other tabs (3:1 contrast)

Would you know how that would translate in terms of CSS?
Would you have any suggestions in terms of the changes for the color contrast?
Or any of the other styles really, whether bold or underlined...

Any suggestions, advice or recommendations would be greatly appreciated.
Thanks in advance!

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! 🙂

OK, what we "could" do, for now, is just move the Admin Toolbar menu link before the other two, so I've removed the weights for admin_toolbar_search and admin_toolbar_tools and added a very small weight difference to the admin_toolbar: -1, see commit:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/139/di...

I added the change as an extra commit so it could be reverted easily if you think it would not be reliable enough and we would still need to set a weight for other menu links.

I "think" it would still be fine to move forward with this change and come back later to other potential ordering issues with other menu items added by other modules.

Any feedback, comments or suggestions would be welcome.
Thanks in advance!

Thanks a lot @ressa for your help testing and reviewing this issue, it's greatly appreciated! 🙏

Glad to hear you find the idea of adding tabs could be a useful improvement 👍

I'm not sure exactly why the menu items would not be properly ordered by label....
Looks like by default, they are ordered by their level of dependency (?!) or something else (?!) ...
Maybe the keys of the menu links?!! I'm not really sure and didn't look further than that.

But indeed, I completely agree with you the order is currently wrong...
Looking for example at the way the config is exported, see in core.extension.yml:

  admin_toolbar: 0
  admin_toolbar_search: 0
  admin_toolbar_tools: 0

The modules seem to be correctly ordered....

Anyway, I made a quick amend to your last commit:
Increased the weights of the menu links, to add a bit more "space" between the links. Order stays the same.
Switched the Tools and Search tabs to be consistent with the order of menu links.

I'm fine with the merge request as it is, but just have one worry though about setting the weight of the menu links:
If other modules are adding links to the "User interface" menu, could there be some issues with the order the menu items would be displayed?
The links added by admin_toolbar would always be ordered first, wouldn't they?!

Maybe there could be some other way to approach this particular issue with the order of the menu items?

The changes for the local tasks shouldn't have any impact or conflict with other modules, though 👌

Thanks in advance for your help and feedback! 🙂

Could you please try with the latest version of the module?

I have just tested this with admin_toolbar_search:3.5.3 and confirmed the search text field had an associated label tag which is indeed visually-hidden, as suggested in the issue summary 🟢, see:

<div class="js-form-item form-item js-form-type-search form-type--search js-form-item- form-item-- form-item--no-label">

  <label for="admin-toolbar-search-input" class="form-item__label visually-hidden">Search</label>
  <input placeholder="Admin Toolbar quick search" accesskey="a" type="search" id="admin-toolbar-search-input" size="30" maxlength="128" class="form-search form-element form-element--type-search form-element--api-search ui-autocomplete-input" autocomplete="off">

</div>

 
Closing issue as outdated, for now.

Thanks again for your help and contributions.

Could you please try again with the latest version of the module?

If performance problems still appear, could you please create a new ticket → ?

Closing issue for now: Outdated.

Thank you all very much for your great help on this issue.