🇫🇷France @dydave

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.

Thanks @nitrocad for contributing this code and raising this issue, it's greatly appreciated! 🙂

If you don’t allow a user / role to view any admin menu item, the user will see the Management button with empty submenu.

How do you test that? 😅
Could you please provide some of the permissions or roles, created/used for testing this issue?

Could you please also provide a screenshot if possible?

If possible, could you please also try with the latest version of the module, either stable or dev?

Anything that could help reproducing the issue locally, would allow us to test and reviewer quicker the suggested changes and confirm whether the code could really be needed, thus moving this issue forward. 👍

Feel free to let us know if you have any questions, advice or suggestions on this particular issue or the project in general, we would certainly be glad to help.
Thanks in advance!

Thanks again everyone for the help testing and debugging this issue, it's greatly appreciated! 🙂

I have done a bit more testing on this issue with:

• admin_toolbar:3.5.3 →
• toolbar_menu:3.1.0 →
• drupal/core:10.4.6 →

 
and everything seemed to work fine 🟢, as expected: The menu configured with Toolbar Menu displays just the same as the administration menu under the "Manage" Toolbar item, see the screenshot below:

Could you please try testing again with the latest versions of the modules and let us know if the problem is still occurring?
In which case, we would definitely appreciate some more context, if possible, to try to reproduce the issue, for example, maybe there could be some conflicting themes or other modules that could prevent the dropdown menus from displaying properly.

Currently, we are unable to reproduce the issue, thus, closing ticket as cannot reproduce (anymore).

If you encounter more issues with the toolbar_menu and admin_toolbar modules, feel free to re-open this issue and provide more information so we could try to help.
Otherwise, feel free to create a new ticket → with more specific information on the problem.

Thanks again everyone for the help on this issue! 🙏

Thanks a lot everyone for all the work and contributions in this issue, it's greatly appreciated! 🙏

I have taken a closer look at the issue summary and the comment from Jenny (@jennypanighetti) above at #3, and it would seem the problem described can still be reproduced with the latest version of the module.

Comment #3 also suggests we should clarify the use of:
Maximum number of bundle sub-menus to display
In particular, what is considered "bundle sub-menu" and where their display should be limited.

I personally agree with Jenny (@jennypanighetti) and think the behavior should be:
1 - Consistent across all menu items where bundles are used (for example "Content types" and "Add content" should have the same limit enforced) and
2 - For all entity types as well as config entities (Views, for example).

Note the point (1) above would be closer to fixing a bug and (2) closer to a feature request.

Additionally, the label and description texts of the corresponding configuration field, would also need to be updated to be clearer and align with the implementation chosen for the setting.

The requirements for this feature should be clarified before going further into any implementation.
We would need to go back through the tickets in which this feature was initially added and try to understand what the initial requirement was.

If the initial requirement was:

In the first place we added the limitation for UI reason and we did not think that the "Content > Add Content" sub links cause any trouble.

For UI or performance reasons (maybe): trying to set a limit to the number of menu link items the module could create and most importantly, display in its dropdown sub-menus.
If there are too many bundles or menu items in a sub-menu, then, there might not be enough room on the screen to display all the items and they would overflow out of the view port (not visible).

Perhaps there could be different approaches or more generic ones that could potentially provide simpler solutions to the initial problem.
Maybe setting a limit in general to all sub-menus at a certain depth could also work?!
Something like: "Maximum number of menu links to display in sub-menus"

Or something very specific like what was done in merge request MR !30 and patches above, allowing a very granular configuration of the maximum number of menu items per entity types, but with lots of fields.

Or maybe a completely different approach, fixed with a UI solution, see:
✨ Make the menu dropdown as scrollable. Needs review

We would greatly appreciate to have your opinion and feedback on this comment with the suggested logic and the steps to try moving this ticket forward.

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

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

Thanks a lot @nterbogt for the very clear and great answer, it is very helpful! 🙏

Allow other modules to alter/modify the menu links added by admin_toolbar (or any of its sub-modules).

This seems to be a rather frequent/recurring request and requirement, so we should probably create a specific documentation page to explain and describe the approach suggested at #2.
Information about other potential methods of extension of the Extralinks class and why it was made final could also be explained in the same documentation, with a specific section, see for example:

• Documentation on how to Extend Smart Trim → contrib module.
• #3514075-8: unsafe usage of new static() →
• 📌 GitLab CI - PHPStan - "Unsafe usage of new static()" Fixed

 
Ideally, a paragraph should also be added on project's page to direct users to this ticket and the documentation page:
"Extending Admin Toolbar" > "Overriding menu links added by Admin Toolbar Tools"
The same changes would probably also need to be added to module's README.md file or in sub-module admin_toolbar_tools

Re-qualifying ticket as an active Task which should hopefully result in the creation of a new documentation page for the module.

Feel free to let us know if you have any suggestions, comments or questions on this particular comment, or the issue in general, we would surely be glad to help.
Thanks in advance! 🙂

Thanks a lot Matt (@mglaman) for creating this issue and taking the time to look in module's current code, it's greatly appreciated! 🙏

If the initial requirement is to be able to alter the menu links added by the module, could you please try the approach suggested in the following issue?
#3488920-2: Change the deriver class of a menu link →

It would seem an implementation of hook_menu_links_discovered_alter could allow modifying any of the menu links added by the admin_toolbar_tools module.

But the solution suggested in the IS is also interesting though:

If we're calling \Drupal\admin_toolbar_tools\Plugin\Derivative\ExtraLinks::routeExists, actually return the route and use its title.

Sure why not, but

This might be difficult if it uses a title callback, though.

got me a bit worried... probably some more work here 😅

I would greatly appreciate your opinion on this feature and feedback on the solution suggested above, in particular, whether it would really be worth spending more time on fixing this issue in the admin_toolbar_tools module.

For the time being, closing this issue as Won't fix, since it seems there is already a proper solution provided by Drupal Core.
Feel free to re-open the issue at any time if you consider these changes would still be needed, we would be glad to take another look.

Thanks in advance! 🙂

Could we please check whether this issue is still occurring on the latest versions?

Thanks in advance!

Thanks a lot everyone for raising this issue and the code contributions, it's greatly appreciated!

It would seem the initial problem:

Enable the admin_toolbar_tools submodule. It generates a bunch of links, but you can't change them.

could already be achieved by using the approach detailed in comment #3488920-2: Change the deriver class of a menu link → , with an implementation of hook_menu_links_discovered_alter.

Could you please try the suggested approach and let us know if this could work?

For the time being, closing this issue as Won't fix, since it seems there is already a proper solution provided by Drupal Core.

Feel free to re-open the issue if you consider these changes would still be needed, we would be glad to take another look.

Please let us know if you have any questions on this comment, this issue or the project in general, we would surely be glad to help.
Thanks again for your interest in the module and all your contributions! 🙂

The behavior described in the issue summary is actually controlled by the core Toolbar module.

Altering the current behavior could have other/unexpected consequences (ie. break things 😅).

Besides, it would appear the behavior requested is already provided by the Toolbar module:

1. Click on the manage Toolbar item to close/collapse the Tray toolbar-item-administration-tray.
2. Navigate to a different page by clicking a link on the page.
3. When the next page loads, the tray is collapsed by default.

 
The default tray selected on a page load, is stored in user's browser local storage (Drupal.toolbar.activeTabID), changed with user's selection (clicking on toolbar items).
 
Additionally, since this feature request didn't receive any further comments for almost two years, it seems it didn't manage attracting much interest from other users of the module.
 
Therefore, closing issue as Won't fix, for now.
 
Thanks in advance!

Thanks a lot Michael (@mstrelan) for your help on this issue, it's greatly appreciated! 🙂

The changes suggested at #6 have been added to the merge request MR !97 👌

To be honest, I did not test this directly myself, since I'm not really using the admin_toolbar_links_access_filter module anymore...

I think it was marked deprecated and supposed to be removed when future releases drop support for core versions below 10.3.
So personally, I wouldn't really be interested in working on this, but I would not be opposed to getting this added to the module.

If you could please help testing and reviewing the changes in the MR and if you think they could provide an acceptable compromise to Jakob's (@japerry) comment above at #4, then I would be glad to get these changes merged in.

Again, since we're looking at removing this module, we wouldn't be bothering with Tests, docs and such.

We would greatly appreciate if you could please try testing merge request MR !97 and give us your feedback or reviews.

Feel free to let me know if you spot anything or if I missed anything else in the comments above, I would certainly be glad to help.
Thanks 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 @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!🙂

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

Thanks @ressa!
No worries, we'll take a quick look at that issue.

Any chance you could take a look at the following one?
✨ Add local tasks (tabs) for Admin Toolbar Settings forms Active

It should be an easy one, since there isn't any impact on module's code or logic.
Just added Tabs on modules config forms.
But we would still appreciate some feedback on the feature in general and the labels of the tabs, if possible 🙂

Added at the top of the list of the IS, since it is accessible to any contributor (not necessarily tech-savvy), with any level or experience.

Thanks in advance!

Thanks a lot Tirupati (@tirupati_singh) for the detailed review, it's greatly appreciated! 🙂

I have just updated the merge request with the following changes:

• Updated the Admin Toolbar Settings local task label, as suggested, from Settings to Toolbar settings.
• Added PHPUNIT Functional Tests to check whether the local tasks are displayed correctly.

 
I'm still wondering if it is a good idea to add tabs like that... 🤔
 
I mean, personally, I find these tabs super useful to easily switch between the forms while testing and developing on the module.
So with your positive feedback and confirmation the changes worked fine, I think we're probably going to include this new feature in the next 3.6.0 release 👌
 
In this case, we need to try to get a bit more feedback on the local tasks labels and this feature in general, so I'm going to add this issue to the 📌 [Meta] Roadplan for Admin Toolbar 3.6 Active , as an attempt to attract more attention from contributors and hopefully get more comments on this issue.
 
Feel free to give the MR another round of review and let us know if you spot anything else.
Thanks in advance!

Quick follow-up on this issue:

Copied over the same Tests class from the admin_toolbar_tools module and made a few adjustments to match with the admin_toolbar_search module.

This is a very basic initial Tests class, with the bare minimum:

• Browse to admin/config/user-interface/admin-toolbar-search-settings
• Submit the form
• Check the settings were saved with a success message.

Since all the jobs and tests were still passing 🟢, I went ahead and merged the changes above at #4.

This class should allow us to detect any issues with the schema or any other form related code and be used as the basis for adding more checks and automated tests for the admin_toolbar_search module.

Marking issue as Fixed for now.

Thanks in advance!

Thanks a lot @andypost! 🙏

Any chance you could also take a look at the RTBC issues and the Gitlab CI one ?

• 📌 Fix the errors/warnings reported by Php_CodeSniffer Needs review
• 📌 Add Queue UI integration to 3.x Needs review
• 📌 Automated testing: Configure GitLab CI Needs review

 
They should be quick wins really 🙂
 
Thanks in advance!

dydave → created an issue.

Quick follow-up on this issue:

Created initial merge request MR !139 above at #2 which adds local tasks (tabs) for Admin Toolbar Settings forms.

Moving issue to Needs review, as an attempt to get some feedback on this new Feature.

This MR "could" need some Tests for the tabs in a new test class for the admin_toolbar_search Settings form.
We could use an xpath expression for example to check for the tabs on the page (?!).

Any testing/reporting feedback, questions or suggestions would be greatly appreciated.
Thanks in advance!

dydave → created an issue.

Setting this back to Needs review, since this could still be a temporary solution before this issue could really be properly addressed in ✨ Tabbing order does not satisfy 508 accessibility requirements Active .

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

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

It is a big ticket, with quite a lot of exchanges and code changes involved, so I've been trying to get a better understanding of what could be expected in this issue 😅

Thanks Benji (@benjifisher) for summing up the current status at #53:

I rebased the previous merge request (MR !58) locally, fixed all the conflicts, cloned the branch and pushed to a new feature branch, with a "standard" name.
I then added a quick commit to fix CSpell, PHPCS and StyleLint validation errors, mostly with the --fix flag for automated fixes.

I created the new merge request MR! 138 above at #57 based on the work from the previous one, which would seem to pass all validation jobs and tests 🟢
 

While I was running build pipelines for the MR, I had a bit of time to test it locally and I personally really love it! 😍
Great job Camille (@camilledavis)! It looks great! 🤩
Thanks a lot! 🙏

I like very much the way the chevrons display for the top level menu items, the way they toggle when navigating with the keyboard.
I tested navigating with the keyboard "Tab" key, through the top level links, then hit the "Space" key on the chevrons to get the sub-menu expanded... this is great!!
Currently you have to go through each menu item of a sub-menu before being able to go to the next top level one... 😅

Keyboard navigation and UI are greatly improved by this merge request! 👍

See below a few screenshots of the major changes introduced in this merge request:

Navigation with mouse over, with keyboard accessibility enabled:

Focus/keyboard navigation with "Tab" and "Space" keys:

Disabled keyboard accessibility: Module's current behavior:

How do we move this feature forward?

From a coding perspective, this seems like a major refactoring and rewrite of the module's CSS and JS files:

• css/admin.toolbar.css
• js/admin_toolbar.js

This is where most of the impact of these changes would be focused and unfortunately, front-end development is not my strongest suit 😅
I have no clue how to tell whether all these changes would be "legit", respect any kind of standards, best practices, etc....

It looks like these files have completely changed and I'm not sure how I could review or validate them...
I'm not sure exactly how we could proceed with this major change, in the sense: how could we ensure we would not be breaking some of module's current front-end features?!

Otherwise, from a personal testing experience: it felt great 👍 and everything seemed to work very well.
I have not tested responsive support, mobile support, rtl support, different browsers, etc...
I have only done very limited front end testing.

I will have to spend much more time reviewing and testing the JS file along with the CSS.
 

IF we were to decide to move forward with these changes and this major refactoring, we would still have the following tasks:

• Convert the current code in admin_toolbar.js to full Vanilla JS, without JQuery.
• Fix all ESLINT validation errors for file admin_toolbar.js.
• Change name of added config to enable_keyboard_accessibility: true.
• Update added form field label/description and wording.
• Add basic Functional Tests coverage in existing Test class AdminToolbarSettingsFormTest.

Since all the jobs and tests are still passing 🟢, moving issue back to Needs review as an attempt to collect more reviews, testing and feedback.

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

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

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

Thanks @andypost!

Any chance you could help us create the new branch and clarify what would be needed to get this issue moving?

It is currently still blocking the upgrade for projects.

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

Thanks a lot @ressa for getting this going! 🙏

Ideally, I would like to get the following issues merged before creating the next release:

• #3487246-16: Warn site owners about the removal of Admin Toolbar Links Access Filter → :
  No impact at all on the module, so very safe. Additional reviews of the wording, text/message would be appreciated.
• 🐛 Some mandatory parameters are missing ("source") to generate a URL for route Active :
  Pretty much ready to go: It has received feedback from several users, but more testing and reviews would certainly be welcome.
• 📌 GitlabCI: Fix ESLINT validation errors Active :
  This is a big one.... The impact is high and should maybe be split into two tickets, one for admin_toolbar.js and one for admin_toolbar_search.js.
  It has already received quite a lot of testing though from @andresalvarez and myself. But more testing is definitely required.

 
Overall, the most sensitive issue is the one related with the work on ESLINT and the impacted major JS files.
The problem is that currently the module doesn't have FunctionalJavascript Tests for admin_toolbar...
It does seem to have FunctionalJavascript Tests for admin_toolbar_search though, see:
https://git.drupalcode.org/project/admin_toolbar/-/tree/3.x/admin_toolba...
But I'm not sure how reliable they could be and the types of issues that may arise with certain browser compatibility issues, etc...

On the side, I've started working in a local branch on converting admin_toolbar.js to full Vanilla JS, so I was hoping I could combine this work with the ESLINT fixes in a ticket, which would only really leave testing the admin_toolbar_search.js (JQuery).

But if we find it too risky for this release and hard to test properly, we could also consider moving the ESLINT fixes to the next release and tag a new stable straight away 👌
 

Otherwise if we receive positive feedback and reviews on these issues and the corresponding merge requests, we should definitely be able to tag the new 3.6.0 stable release.
Thanks in advance! 🙂

Fantastic news @ressa! 🥳

Thanks a lot for testing carefully the upgrade process from stable 👍

Following your confirmation, I gave the MR a quick round of manual testing locally and a final review.
Since everything looked good 🟢, I went ahead and merged the changes above at #30 🥳

Another nice feature has landed... after doing a bit of exploration and getting a better understanding of the overall hoverIntent features 😅

Feel free to create new tickets if you encounter any issues with any of the latest CSS/JS changes and the newly added features.
If you have more ideas, like this one or the slide-on-scroll one: please share them with us 🙂

Marking this issue as Fixed, for now.

Let's keep moving forward with the next issues on the 📌 [Meta] Roadplan for Admin Toolbar 3.6 Active 👌

Thanks again to @aaron.ferris, @redeight and @ressa for the great help! 🙏

Thanks @ressa!

Looks like we're making a good team! 🙏
Really glad we found each other on this module 😅

Thanks a lot for the META ticket!

I'll get it updated as we go, definitely! 🥳

Right right right !!! @ressa, you need to run DB updates!! 😅

Thanks a lot BTW for the very nice feedback, once again! 🙏

Indeed:
We can't move from DEV to this MR like that ... because the update hook was changed between the previous change in 📌 Move the 'hoverintent_functionality' setting to admin_toolbar Active and this issue....

In other words, the update hook in the DEV (3.x) is not correct anymore for this merge request.

To avoid having the warning, you would have to download the DEV version, patch it, then install it.

Otherwise, to fully test the update hook from this MR, you would have to go back to 3.5.3 (stable), install the module (drush en admin_toolbar), then upgrade the module to DEV (3.x), patch it, then run drush updb (DB updates) to make sure the correct config objects are properly created.

Let me know if that helps.
As far as I can see, this shouldn't be a problem with the code, since the update hooks that were added in other tickets can still be changed since they have not yet been released as stable (still considered as DEV).
Back to Needs review.

Looking forward to your feedback!
Thanks in advance !

Great job @mparker17 and @spiderman! 🥳

Thanks a lot for the great documentation in this ticket and the very clean approach taken in the merge request to deprecate previous API signatures. It's definitely a great example and source of inspiration to me in terms of steps you have taken to drop support for certain versions 👍

Glad to see the project moving forward!
Thanks again for all the great work to keep the module well maintained! 🙏

No activity on issue for more than a year.

Could you please try updating to the latest stable version and test if the problem still occurs?
Maybe this issue was fixed in other interfering modules or themes?

Closing issue as Outdated, for now.

Thanks everyone for all the work!

OK, back to the original version, clean and simple, with:

• Single open fieldset
• Single checkbox enable/disable
• Single config field with select dropdown: Timeout / displayed only when hoverIntent enabled

 
Created the new merge request MR!137 above at #23 so the other more complete version could be kept on the side for reference.
 
Back to the previous, simple version, which I personally find much better and easier to understand 😅
Additionally, this is probably a big enough step for this feature at this point. We could certainly come back to the changes from the previous MR with the interval and sensitivity settings at a later stage, if any interest is shown for this feature 👌

Since all the jobs and tests still seem to be passing 🟢, moving issue back to Needs review as an attempt to collect more reviews and testing feedback.

Thanks very much in advance! 🙂

dydave → changed the visibility of the branch 3440852-make-un-hover-delay to hidden.

Agreed! Thanks @ressa!

I think I got carried away above at #17 and had not realized the sensitivity and interval settings would not be very useful or as difficult to use or understand as they would seem to be 😅

After all, I thought about it again and came down to: if we are not really going to use these settings, we should not be bearing the weight of their maintenance, support and testing.

I'm going to create a new merge request through, since I'd like to keep the changes from MR!99 in the repo somewhere, even if the stale feature branch gets deleted in the future. There are a lot of labels/descriptions, schema, settings definitions, etc... that took some time to write and "could" maybe be of interest to some users.

There are quite a lot of changes to revert, so it's probably going to take a little bit of time.

Thanks a lot @ressa!! 🥳

This is another big one 😅

Another very nice improvement on top of the disable sticky behavior added previously in ✨ Drupal admin_toolbar disable sticky/fixed state. Is there any option to do this? Active .

After fixing a very minor ESLINT validation error in one of the JS files, by adding a line return, since all the tests and jobs were still passing 🟢, I went ahead and merged the changes above at #36.

If more adjustments need to be made to the form, the doc or CSS we should always be able to address different issues and tickets as they come. 😅

Marking issue as Fixed for now.

Once again, I'm very grateful for your help through this process @ressa, testing/reporting back, reviewing, providing advice on the best or most simple options/solutions in a very timely and constructive manner 🙏
This feature could never have moved at this pace without your great help 🙂

Let's get a few more important tickets in before we could release this great new feature in a stable 🥳
Thanks again!

After doing a bit more testing, I wouldn't really be opposed to removing completely from the form the sensitivity and interval parameters....
Do you think we should just get rid of these settings?

Thanks @ressa!

I think I made all the changes you suggested above at #18, but mostly:

• Changed 'Timeout' default value to '500' and
• Moved 'sensitivity' and 'interval' to an 'Advanced' section collapsed by default.

 
I added a bit of description text to the advanced collapsible section to sort of warn users:
They should know what they are doing if they change these settings 😅
 
Otherwise all the tests still seemed to be passing 🟢 and my tests locally with big_pipe as well 👌
 
Let me know if you could spot anything else 🙂
Thanks again for the great help!

Thanks a lot @ressa for the great help with the CSS, reviews, testing and advice! 🙏

I've tested the CSS changes suggested above at #33 and they seemed to work very well 🥳

I've rolled them in the merge request, in the latest commit, see:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/123/di...

Mostly, I increased slightly from 200% to 220% since we could still see the small shadow of the menu at the top of the window (not sure if that was intended or not?! 😅).
I've done some tests as well with other modules adding toolbar items to make it wrap to the next line and with 220% it still seemed to work.
I've done a quick check and found the computed height in my browser was 40px, so 220% would be 88px, which "should" be enough to cover the height of the toolbar and its tray to make it exit the screen completely.

I've done a bit of testing locally and found the animation is pretty cool and works very well 🥳

I've added a small change to fix another JS problem I found after refactoring some of the hoverIntent and hover JS code in:
https://git.drupalcode.org/project/admin_toolbar/-/commit/4cbcb8d2df2a1b...
This issue would appear when using the focus key to navigate menu elements, then hovering the menu again: expanded menu items would still stay displayed.
 

I like the "sliding in from the top" effect, but did have to limit to only Horizontal mode, since it breaks Vertical mode ... Could this work?

I think it's completely fine: A quick search for vertical in module's code base brought up a single occurence:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/css/admin.to...

.toolbar .toolbar-tray-vertical li.open > ul.toolbar-menu.clearfix {
  display: block;
}

Which would seem to indicate none of the logic of the admin_toolbar module, whether hoverintent or sticky behaviors would apply to the navigation of the toolbar when it switches to vertical mode.

We could maybe create another issue to try addressing support for the vertical toolbar with a more generic approach, since at this point, it seems to be non-existent in the module anyway 😅.

Back to Needs review for more testing and feedback on the latest changes! 🙂
Thanks again!

See the Admin Toolbar module for the solution: 🐛 Some mandatory parameters are missing ("source") to generate a URL for route Active .

Thanks again @ressa! 🙏

I've taken a closer look at the example you sent above at #30 and it certainly helped 👍

I tried to add the variable delta to define a minimum distance in pixels to record a scroll and change the visibility state, see:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/123/di...

          // The delta is the minimum amount of scroll in pixels before the
          // toolbar is hidden or shown. This is to prevent the toolbar from
          // flickering when the user scrolls up and down quickly.
          const delta = 10;

The comparison is based on the abs() function, so it goes both ways: up and down:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/123/di...

            // Ensure user scrolled more than delta. The abs() is used to
            // enforce the delta distance in pixels in both directions: up/down.
            if (Math.abs(lastScrollTop - scrollTop) <= delta) return;

 
In this change, the delta was set to a value of 10 pixels, as suggested at #25.

I have tested manually and couldn't really see a difference with a low value (which is good), but with a value around 80 I had to scroll twice (mouse wheel) in the same direction before triggering the change of visibility, which seemed to be working as expected.

Lastly, I tested with the commands you provided above as well at #25 and they seemed to work as well 🥳

// Nothing happens.
window.scrollBy(0, -9);
// Toolbar is visible.
window.scrollBy(0, -11); 

 
Since the jobs and tests all seem to still be passing 🟢, moving issue back to Needs review as an attempt to get more feedback and reviews on the latest changes.
 
Any feedback, reviews or comments would be greatly appreciated.
Thanks in advance! 🙂

Thanks a lot @ressa, once again for all the advice and help on this big feature 🙏

I've just pushed a big round of updates which should include all your comments above, plus all the points from #14:
✅ Fixed all your comments above, thanks a lot!
✅ Greatly improved the form display by moving around form elements and wrapping all hoverIntent configuration related fields into fieldset and details (collapsible) wrappers. Added field descriptions, suffixes, prefixes, etc... to provide more information for non-tech savvy users and make the form overall a bit more user friendly.
✅ Added support for hoverIntent settings: 'interval', 'sensitivity' and 'timeout'.
✅ Added basic Function Tests coverage by checking the configured hoverIntent settings values are properly added to the 'drupalSettings' JS variable of the page.
✅ Refactored hoverIntent config update hook ==> TO BE TESTED (manually), if possible.

I've done a few tests with the form and personally found it much easier to understand with these changes 👍
See a screenshot below:

 
I would appreciate some feedback on the changes to the Settings form, the section wrappers (fieldset/details), field labels/descriptions, the added fields sensitivity and interval, etc...
 

Since all the tests and jobs are still passing 🟢, moving issue back to Needs review as an attempt to get more feedback and reviews on the latest changes.

Feel free to let me know if you catch anything else and your overall feeling on the latest changes, I would certainly be open to making more adjustments where needed 👌
Thanks in advance! 🙂

Thanks a lot @ressa!
I've got this issue on my list and had planned on updating the issue very soon...

Thanks for the link and advice!

Thanks a lot everyone for your great help on this issue! 🥳

Thank you very much for making the steps very clear in the IS:
Indeed, I was able to reproduce the issue and make a bit of digging around the impacted piece of code:

It seems the piece of code removed in MR!77 stems from an initial request in:
Issue #2991056: Always hide active menu item's navigation on mobile → ,
with commit:
https://git.drupalcode.org/project/admin_toolbar/-/commit/c1e705ff67b6d5...

which would seem to have "broken" module's previous JS behavior and led to:
Issue #3172430: Hide the navigation menu on mobile properly →
with commit:
https://git.drupalcode.org/project/admin_toolbar/-/commit/6dd6516
 

We can't be sure whether this change is going to break the expected behavior on some sites, but since this piece of code is currently causing an issue it is probably safe to assume, these changes should be reverted for now.

Perhaps the way this feature was initially introduced in the module should have taken a different approach?!
Maybe it could have been added as a form setting or in a sub-module, for example.

Personally, I don't think it would be a good idea anyway to have a different behavior between larger and smaller devices.
Additionally, if this was the case, we would probably want to alter JS variables set in the Local Storage by the Core Toolbar module, instead of changing the DOM elements of the page.

Again, we wouldn't be closed or opposed to discussing the reverted feature again, if anybody would still be interested in working on this feature.

Since the jobs and tests were passing 🟢, I went ahead and merged the changes above at #10.

Marking issue as Fixed, for now.

Thanks again everyone for the great work and contributions! 🙏

This task is a consequence of the api change introduced in Drupal Core 10.2, described in:
Change record: _drupal_flush_css_js() is deprecated and replaced by \Drupal::service('asset.query_string')->reset() → .

Based on the rector patches above at #2, the code of flushJsCss was refactored with backwardsCompatibleCall to support deprecated function _drupal_flush_css_js for versions below core:10.2.

PHPSTAN: Ignored error message:

Unable to resolve the template type Deprecated in call to method static method Drupal\Component\Utility\DeprecationHelper::backwardsCompatibleCall()

Since all the tests and jobs were still passing 🟢, I went ahead and merged the changes above at #7.

Marking issue as Fixed, for now.
Thanks!

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

Closing as a duplicate of 📌 Automated Drupal 11 compatibility fixes for admin_toolbar Active .

Fantastic! Thanks a lot Derek (@spiderman) for the kind and prompt reply, once again! 🙏

Alright, alright! I'll just back out if you don't mind!! 😅

I can see the module is in super great hands! You've got a great plan! 👌

So I'll just go back to fixing my stack of tickets and modules!! 😅
(I've got so much to do ...)

Feel free to ping me directly at anytime if you need anything, I would be happy to help the best I could 🙂
Thanks again for all the great work and keeping this module well maintained!
Cheers!

I'm seeing lots of other modules doing that (dropping version below 10.3), see for example :
https://www.drupal.org/project/commerce_stripe →

@mparker17 let's go?!

Is there anything else you could see that could prevent this issue from moving forward?

Personally, I don't feel concerned at all with this issue and change since all of the project I'm currently working on, or managing are all above 10.3 with recent versions of PHP (8.3.19 security patch).

If a customer came to us with a Core version lower than 10.3 (9.x or 10.x), I would recommend upgrading core and modules + consequently upgrade this module based on the different versions (major/minor), which doesn't really sound like a problem to me.

What does the M of @mparker17 stand for?! 🙂

Just my personal humble opinion.
Cheers !

Thanks @makbay!

I didn't see this: 📌 Deprecate in favor of the CKEditor 5 Plugin Pack RTBC

Please don't use this module anymore it should be marked deprecated.

Maintainers have agreed to move developments to contrib module CKEditor 5 Plugin Pack → , which now provides the same features.

This issue is most likely never going to get fixed and so are all other issues in the project's tracker.

This module should be considered as Abandoned.

Thanks @mparker17 for the prompt and kind reply, once again, it's greatly appreciated.

Sorry I didn't read carefully your previous comment (lack of time and finished work late😅).....

RE: #23

Note that, to my surprise, I found the new additions to the menu link data gets duplicated, given the current production code in the merge request and/or the example code added in structure_sync.api.php in this merge request.

Thanks a lot for raising this point.
🟢 I checked and indeed, this is the intended behavior: The API document demonstrates how various attributes and keys could be modified for a menu link item. In other words, the hooks don't only allow to modify the values in the options array, but also any potential key or attribute that users would like to add to a menu link item.

Do you think this could be misunderstood, unclear or misleading for some users?
These are "just" basic examples without necessarily any specific purpose in mind to illustrate how the parameters could be altered.

Thank you very much for taking this into account in the tests: it wasn't absolutely necessary, but it gives a good example of how the hooks could be implemented with more generic parameters.

Thanks a lot for getting this over the line @mparker17! 🙏

The tests look very great! 👍

I've done an initial read/review of the added MenuLinkExtraConfigTest Test class and more particularly its testExportAndImportWithCustomData Test case and it seems to do everything mentioned above at #20.

✅ The test module with all the new alter hooks structure_sync_hook_test is properly enabled.
✅ Test export_alter: Set a value for a menu link options' key 'structure_sync_hook_test_EXPORTKEY', export it and check the value was exported properly.
✅ Test update_alter: Update the value of the menu link options' key 'structure_sync_hook_test_EXPORTKEY' in the exported config data, import the modified data with the test value and check it was merged properly with the existing menu options.

The tests seem to be passing 🟢 which is great!

Not sure if you wanted to test the import_alter hook though, I "think" this one would need maybe destroying a link, which could be re-imported/re-created with the correct options (?!).
But that would just be an extra nice to have 😅

I hope I didn't misunderstand some of the code and that this review will be useful, since I mostly just read it...

I left another small comment in the MR, but overall it looks very good!
Thanks again very much for the great work! 🙂

Seems there are some activity around this topic and issues, see 📌 New Toolbar Roadmap: Path to Beta & Stable Active .

OK, yes, thanks @mparker17 and @spiderman!

Dropping support for versions below 10.3 should be fine as well, just like it's suggested in the MR:
https://git.drupalcode.org/project/structure_sync/-/merge_requests/37/di...

I made the suggestion above at #4 since it would be the most immediate solution to get module's development unblocked .... Not excluding the possibility to further drop support, in another minor version... So essentially both would be possible successively.

But if you'd like to make a bigger version drop right now, that should be fine as well.

However, not making any decision is blocking the development of the project and preventing project's tickets from moving forward.

Thanks for finding this!

I've done a bit of testing and I "think" I was able to reproduce with the steps clearly detailed in the IS.

I need to do a bit of digging though to see where and why this code was initially added.

Needs a bit more time testing, but bringing this back to the top of the stack.
Thanks!

Following the recent resolution of parent issue 📌 Move the 'hoverintent_functionality' setting to admin_toolbar Active , thanks to @ressa's great help, we should now be able to take another look at this issue 👌

I rebased MR!99 and made a few adjustments to the merge request, see:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/99/dif...

But mostly:

Updated config name, field label, description and added basic states to hide field if hoverIntent is disabled.

The MR should be ready to be tested at this stage, but I think some work would still be needed:

• Probably tidying up a bit the Settings form, adding a fieldset, with a description text, moving a bit fields around, etc...
• Do we really want to set a step for the configurable integers (timeout, for example)? What about a max?
• Would we want to add more HoverIntent configurable settings, see HoverIntent Common Options:
  https://briancherne.github.io/jquery-hoverIntent/
  such as interval or sensitivity?
• Automated tests would need to be added to check the settings variables are correctly added to the JS of the page.

 
Any other changes that anyone could see?

Otherwise, I tested locally MR!99 and it works very well: increasing the timeout slows a bit down the interactions with the menu, which could be very practical for certain users 👍
(I tested 750 and 1000)

Since the tests and jobs all seem to be passing 🟢, moving this to Needs review as an attempt to received more feedback to the points above, in particular some help with the field wording, descriptions, options to implement, etc...

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

Thanks a lot @ressa for the great help on this issue! 🥳

This is definitely a big one with a lot of code clean-up, in particular for the JS files with a rewrite of the hoverIntent feature with standard Drupal JS API and as little JQuery as possible, while fixing all ESLINT errors in these files.

After your confirmation, I sneaked in a last commit to fix a few minor ESLINT validation errors, see:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/132/di...

Minor changes with indentation and line breaks.

I gave the module a last round of manual testing locally, with big_pipe enabled and since all the tests were still passing 🟢 I went ahead and merged the changes above at #25 ! 🥳

Let's get to the fun part now 😆
We should now be able to add extra config parameters for HoverIntent: ✨ Make un-hover delay configurable Active

Thanks again very much for all the great help testing/reporting and super constructive advice on this issue @ressa! 🙏

Alright then @ressa! 🥳

Looks like we're all good then ! 👌

I've made the necessary code changes in the last commit, all jobs and tests are still passing 🟢
See commit: https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/132/di...

Everything described in your comment above at #22 is now implemented in the merge request, in other words :

Users upgrading from the previous stable to the next will only have the HoverIntent feature disabled, if they specifically selected it to be disabled on the admin_toolbar_tools form, before running the upgrade.
All other users, with only the admin_toolbar module enabled (admin_toolbar_tools disabled) or admin_toolbar + admin_toolbar_tools enabled (Enabled HoverIntent), will have the HoverIntent feature enabled.

I have just tested the upgrade with only admin_toobar enabled and after the last commit, it is working as expected:
HoverIntent enabled, after running DB updates.

Fresh installs will have it enabled by default.

I "think" we've really given all the different aspects a lot of thoughts and documented the different choices thoroughly in this issue 😅

Back to Needs review so you could take another look 👍

Thanks again so much for the great help !

OK, I think I understood this feature better yesterday:

In 3.5.3 and initially, I "think" what was initially planned for this feature is that the HoverIntent functionality would only be available if users enabled the admin_toolbar_tools module, see:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...
If the admin_toolbar_tools module is not installed, then its config setting (hoverintent_functionality) doesn't have any value and therefore loads by default the Basic hover JS behavior.

So we would be modifying the initial logic by moving the setting out of the admin_toolbar_tools module, which should allow users to enable the HoverIntent setting just with the admin_toolbar module enabled.

Which I "personally" think is an improvement for the module, especially given your feedback on the smoothness and flexibility of the plugin.

The way this feature was initially implemented is quite confusing, because part of the logic went into admin_toobar and part went into admin_toolbar_tools.....
I "think" the way this feature was integrated to the module was to consider the HoverIntent as an "Extra" functionality, added by enabling the sub-module admin_toolbar_tools.

We have different options:
1 - Keep the current behavior and move the related logic from admin_toolbar to admin_toolbar tools (JS library, display logic in the admin_toolbar.module file), to have everything in one place.
2 - Change the existing behavior to allow admin_toolbar to fully control this setting (move the setting over to admin_toolbar).

Given the amount of tests and usage of this feature by users, we "should" be able to consider it should be stable enough to be moved to the "global" configuration of the module, and thus:
My preference would go to the option 2, which is currently implemented in the merge request: It would make sense to allow users to enable this feature without necessarily installing the admin_toolbar_tools module.

One thing would remain though if we wanted to be fully ISO with the previous logic: we would have to disable hoverintent by default, for users having only the admin_toolbar module installed after running DB updates.
In other words, users currently only running admin_toolbar and not admin_toolbar_tools, are using the standard Hover default behavior.
So after running the hook_update from this MR, the enable_hoverintent setting should be disabled and users would have to explicitly change the value in the form to select the hoverIntent behavior.
That is ... only if we really want to be ISO with the current behavior.... this is also open to discussion and we could just go ahead and enable hoverintent by default for these users as well.

That's the only comment I had left on the MR, concerning the code here:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/132/di...

Otherwise, new users installing admin_toolbar, would be using the HoverIntent plugin straight away (default setting, same as admin_toolbar_tools).

But it was important for me to fully clarify the change for this feature.

Thanks in advance for your feedback and comments!

Super helpful feedback @ressa, once again! 🥳

Looks like we're doing some much needed clean-up here and I also don't really understand how the module could have worked with big_pipe before, without the correct dependencies to core JS libraries 😅.

I think some of the clean-up work we've been doing and enforcing certain job validations is starting to pay off 👍

For example, raising the PHPSTAN validation level allowed me to catch a wrong test I was doing in the added code of the hook_update function, which I was able to modify in the last commit.

I've given a round of tests as well, locally, for the update function and it would seem to be working fine/as expected:
Moving the old config value to the new config.

About the description, it is a difficult concept to explain in a few words

The description you suggested is much clearer, thank you very much for taking the time to make it easier to understand for non tech-savvy users, that's a very difficult exercise 🙏
I've made the change to the merge request and the field should now display properly with the new cleaner description.

Let's keep in mind, this field might further evolve soon, when we start adding more hoverIntent config parameters, in particular in child issue: ✨ Make un-hover delay configurable Active
So we should still be able to make more refinements to the field labels/descriptions in the future.

Lastly, concerning big_pipe:
I'm also very happy we could clean-up all this part properly:
All the JS files related with this feature: admin_toolbar.hover.js and admin_toolbar.hoverintent.js
were moved to Vanilla JS as much as possible, with a single JQuery call to the hoverIntent plugin,
fixed all ESLINT validation errors as well and
wrapped in Drupal behaviors with calls to Core once, to ensure their code would only be executed once.

This started prompting errors and problems when enabling big_pipe (calls to once), which allowed identifying the libraries could be loaded in the wrong order due to missing dependencies.

Concerning the Tests: I'm not sure, but I would tend to "think" PHPUNIT Tests don't necessarily run or enable the big_pipe module by default. Or maybe there could be other reasons as well...
But this one is definitely a module that needs to be enabled when doing manual testing: I'll pay much more attention in the future.

Feel free to give the MR another round of review and let me know if you spot anything else 👌
Otherwise, I've done quite a bit of testing myself as well and think the changes should be pretty much ready to go.

Thanks again for all the great help! 🙏

Quick follow-up on this issue:

Starting from level 3, I gradually raised the level and each time fixed all the errors prompted by PHPSTAN, until reaching level 6.

Fixed all validation errors, mostly:

• Updated Doc comment blocks return types.
• Updated Doc comment blocks param types.
• Added inline @var types definitions.
• Changed 4 lines of code, to add elvis operators and reorder chained calls.
• Caught a wrong variable passed in assert function in a Test class.

 
Added project's phpstan.neon file to raise validation level to 6 and ignore certain error messages.
 

Very few lines of code with very minor changes were impacted in the merge request.

Since the Tests of the MR all passed 🟢, I went ahead and merged the changes above at #3.

The configuration change was made on Gitlab CI by adding the file: phpstan.neon, forcing rule level to 6.

This should be helpful keeping module's code properly maintained with the evolution of the different Core versions, APIs and PHP versions.

PHPSTAN validation is currently not required to merge.

Marking issue as Fixed for now.

Thanks!

dydave → created an issue.

Thanks a lot @mparker17 for all the nice and prompt replies, it's greatly appreciated! 🙏

Do you feel comfortable writing the first draft of the test? If not, I can try, and pass it to you for refinement!

I'm very sorry, but frankly, I don't think I would have the time right now:
I'm working on improving the Admin Toolbar → module these days and there's quite a lot to do actually 😅
In particular updating some of the JS libraries.

I've got plenty of tickets, MRs and support requests to answer already 👌

Not to mention several D11 compatible tickets I'm still trying to push 😅

But I would be happy to review or test any code produced 👍

Thanks again very much for keeping this module well maintained!

OK @ressa:

• Reverted the default value to be: HoverIntent Enabled (checked) / Exactly as it is currently in 3.5.3.
• Fixed the big_pipe issue by changing the JS libraries dependencies.
• Updated the enable_hoverintent checkbox label and description to be a little bit clearer, but feel free to make suggestions:
  

  ✅ Enable HoverIntent
  Provides enhanced control over the mouse movements and interactions with the menu items of the Admin Toolbar.
  Disable to use module's default basic javascript behavior.

Sorry for the scope creep again, above at #16:
Changing the default value along with the way the plugin file is installed is definitely out-of-scope in this issue 😅

Tests are still passing 🟢 Back to Needs review 🤞

Feel free to let me know @ressa if you catch anything else, I would be glad to look into it as soon as possible.
Thanks again very much for the great help!

Additionally: Automated testing on Gitlab CI allows running PHPUNIT Tests and PHPSTAN code validation for the 3 currently supported major core versions.
See the changes suggested in issue 📌 Gitlab CI: Broaden Tests coverage and require jobs to pass Active , with MR!47:
Build pipelines: https://git.drupalcode.org/issue/structure_sync-3517495/-/pipelines/465983

See the different jobs for the different types of supported versions.

This ensures a better automated testing coverage and can help reporting many deprecation errors so maintainers can make module's code evolve with the different Core version API changes.

In other words: with all the Gitlab CI optin variables enabled, we should be able to provide a pretty good and standard support coverage of the different versions required by Core.

Quick follow-up on this issue:

Created initial merge request MR!47 above at #2 which should enable complete Tests coverage and code validation on Gitlab CI.

These changes should allow the module to exploit and benefit from all of the current features and configuration options provided by the Drupal templates for Gitlab CI, available at this moment.

The MR currently fails on PHPCS validation job, see parent issue 📌 Fix PHPCS validation job errors Active .
So, once the changes from the other MR are merged, this MR should pass all jobs and Tests correctly.

Since all Tests are passing 🟢, moving issue to Needs review:
https://git.drupalcode.org/issue/structure_sync-3517495/-/pipelines/465983

The configuration in this MR is really only a suggestion, it is left at the maintainers appreciation to comment/uncomment/remove any sections added in the changes.

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

When the following core version will come out, same thing:

Create a new major branch and drop support for the requirements of the the oldest version.
Supporting 3 major core versions in a module major version gives a large widow for users to upgrade core and other contrib modules in different steps and flexible order.

This is what I understood maintaining modules and reading the following post:
Drupal module semantic versioning for Drupal core support

Could you please help us make the suggested changes so we could keep moving forward with other issues and update pending merge requests to point to the 3.x branch?

Thanks in advance for your feedback!

Thanks @mparker17!

I would recommend doing the following changes:

1 - Create the new major development branch: 3.x
2 - Drop support for all core versions below 9.5
3 - Drop support for all PHP versions below 8.0.0

The requirements would then become:

core_version_requirement: ^9.5 || ^10 || ^11
php: 8.0

Changes would also have to be made to the composer.json file, see:
https://git.drupalcode.org/project/structure_sync/-/blob/2.x/composer.js...

Let me know if you would need any help putting together a corresponding merge request, we would surely be glad to help!
Thanks in advance!

Quick follow-up on this issue:

Created initial MR!46 above at #2 which fixes all PHPCS errors.

I just executed locally automated fixes with phpcbf and all the errors were fixed.
Most of the errors where just a missing nullable operator on some of the variables Type declarations, see the merge request for the detailed changes.

Since all Tests are now passing 🟢, moving to Needs review and bumping to Major: This issue is preventing other merge requests from being properly tested.
Additionally, we are unable to know whether pending merge requests could be introducing Coding Standards errors.

We would greatly appreciate if a maintainer with write access could take a look at the merge request and let us know if any more work would be needed.

Thanks in advance!

dydave → created an issue.

dydave → created an issue.