Move the 'hoverintent_functionality' setting to admin_toolbar

Thanks @dydave, always a great idea to optimize the code structure :)

I have tested the patch, but the hover stops working ... I need to click (or right-click) to expand a child item. The code itself, I have not looked at.

Also, I don't know what hoverintent is about ... so perhaps this sentence could be expanded, and briefly describe what it does?

Check it if you want to enable the hoverintent feature, to ...

... similar to how sticky is described (explanation emphasized by me):

Disable Admin Toolbar's sticky behavior so it stays at the top of the page when scrolling.

Thanks you so much @ressa !!

Did you try flushing the cache ?!

Very strange, my colleagues gave this a round of tests and reviews and it was working ...

Also, watchout, you need to run db updates after applying the patch or switching to the branch.

Also, I don't know what hoverintent is about ... so perhaps this sentence could be expanded, and briefly describe what it does?

Totally agree with you!
I didn't want to change this so far, since it's going to break all translations ....

But since we're going for a new minor 3.6.0 and are already introducing BC breaking changes (see: #3514075-5: unsafe usage of new static() → ) and this change in particular in this MR:
Got me very worried and I made a lot of research to see how to handle changing a config or renaming it ....
That's also potentially BC breaking ...

So we're going for a new minor, definitely !

I'll create tonight the META issue: Road to 3.6.0, as you recommended previously.... Now the release plan is clear to me !
Let's go and break thinks 😅

I'll get the wording updated for this setting, definitely, based on your suggestions...

But please if you could: Try forcing a bit more the tests to work on your ENV locally ...

Everything seemed to work for my colleagues.

Stay tuned for rounds of changes from tonight and over the weekend !

Thanks again so much!

Also, make sure you have the correct settings enabled on the config forms in the backend after running the DB updates, if you're updating from a stable or branch 3.x ....

Code :
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/132/di...

I've tested this a bit, switching from 3.x to this feature branch 3516995-move-the-hoverintentfunctionality and run drush updb, and everything seemed to work fine ...

But I'd like to get more testing feedback on this, if possible, as well ....

This is a big change @ressa for refactoring and clean-up.... we're going to feel much better once this is IN 😅

RE: The field description and label:

What does this field do?

Technical explanation:

There are 2 scripts to handle the hover menu animation to unfold the menu items:
1 - Default: is custom JS produced by and for the admin_toolbar module:
Standalone script: https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/js/admin_too...
Which doesn't require any external library to work ... just the usual Core libraries.

But this animation is a bit "dry" in the sense, the JS doesn't provide any feature on the time delay to hide the menu, to show, etc...
It's very basic and simple, the display and hide are just instantaneous.

2 - HoverIntent: is the JQuery HoverIntent plugin:
We've got a small piece of JS in the admin_toolbar module that builds on top of the JQuery plugin:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/js/admin_too...
See how it call the JS function hoverIntent.
See recent issue: 📌 update hoverIntent Active
Plugin URL: https://github.com/briancherne/jquery-hoverIntent

This is much more flexible, with a lot of parameters, see the demos here:
https://briancherne.github.io/jquery-hoverIntent/

Thus allowing to do nice things such as requested in child issue ✨ Make un-hover delay configurable Active
But a lot more parameters "could" potentially be configurable.

I think it's very nice to have this feature in the module, but it needs to be a bit refactored currently, see my explanation in the IS.

Plus the wording of the field is not clear at all ....

So if you have any suggestions, they would surely be very welcome! 🙂

I definitely need help moving this ASAP please .... this is a big move that's going to unblock a lot of other tickets I've got lined up for a lot of improvements and code refactoring/clean-up.
As a heads up: We're going to move to Vanilla JS entirely and replace this JQuery plugin with its Vanilla JS equivalent: 🤫
https://github.com/svivian/sv-hover-intent-js

See the code example on that page for some of the parameters that "could" be configurable "out-of-the-box" 🥳
(without "too" much work)

This is coming tonight ... during my contrib time!

Please help!! 🙏

I've also got an answer line-up on the scroll issue, as well so I could get back to you on this ....
Just need a bit of time to put down everything in the ticket 😅

RE #7 for the checkbox field label:

Bear in mind this is going to show/unfold the other config fields which will be added in the child issue to make the "hoverIntent" parameters configurable ... so one or more fields will be added and visible there when the checkbox will be checked.

I've also changed the default behavior on install to use module's default JS standalone script.
In other words, currently, the default is JQuery hoverIntent and after this MR is merged, the default will be the simple standalone JS script.

The advanced hoverIntent animation will have to be selected by users specifically.

The reason for that is that the default shouldn't rely on an external JS plugin dependency.
If the script is not there or the dependency is not met for some reason....

By default, the module should be able to work fine without anything else (ie. its own script).

You're welcome @dydave! Sounds good with the changes, a spring cleaning is sometimes needed :)

Here's what I do to set it up in Drupal 11 (Drush and Composer commands prepended with ddev via alias):

mkdir drupal && cd drupal
cd drupal
ddev config --project-type=drupal --docroot=web
composer create drupal/recommended-project
composer require drush/drush
drush site:install -y
composer require drupal/admin_toolbar:3.x-dev@dev
cd web/modules/contrib/admin_toolbar/
wget https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/132.diff
patch -p1 < 132.diff 
drush install admin_toolbar admin_toolbar_tools admin_toolbar_search

Do I need to do more steps? Could it be because I am on Drupal 11?

Looks good @ressa!

Yes! It looks accurate .... not sure about D11 ... You're right... I haven't tested that ...

Try playing with the form options at admin/config/user-interface/admin-toolbar Check and uncheck, flush cache (just in case), and test the display of the menu... see if it works ....

I'll give a round of tests again on a fresh D11 install as well...

If cache flushes need to be added, that's part of the changes to be added to the MR...

Try doing a bit more tests and report back ... I'll do the same this afternoon quickly... and ask some colleagues as well.

There's still some work needed indeed, with the checkbox label and description also.

So any feedback at all would be a great help!
Thanks in advance!

Thanks for explaining the thoughts and plans ahead for this in previous comments, and introducing JQuery HoverIntent (which I'll check it out later) They sound like great improvements!

I tried in Drupal 10 and Drupal 11 as well (both fresh installs) by checking out the Git branch:

git clone https://git.drupalcode.org/project/admin_toolbar
cd admin_toolbar/
git fetch "https://git.drupalcode.org/issue/admin_toolbar-3516995.git" '3516995-move-the-hoverintentfunctionality'
git checkout -b 'admin_toolbar-3516995-3516995-move-the-hoverintentfunctionality' FETCH_HEAD

... but I get the same result in both places, I tried checking and unchecking "Enable/Disable the hoverintent functionality", flushing cache, etc. but hovering does nothing. There are no errors in the Console.

It tried both Firefox and Chromium in Debian 12, if it makes any difference. But DDEV should make it irrelevant, I would think ...

Thanks a lot @ressa!!

I'm going to give this a clean round of tests later today with a fresh D10 and D11 sites.

I'll definitely get back to you on this as soon as I have a bit of time.... I'm gonna be stuck in clients meetings this afternoon 😅 (joy!! 😅)

But I'll circle back on this ticket as soon as I am free 👌

Cheers! 🙂

Thanks @dydave, sounds good! Feel free to share any tips later on, how I can possibly do some simple debugging, or things to look out for -- then I can also look at this later today, since I'll also be away for now.

I cracked the riddle: It's BigPipe! For some reason, hovering over "Extend" showed child menus all of a sudden, and looking at the source code, the only difference was a few BigPipe mentions. After uninstalling BigPipe everything works.

I think I understand why the hoverIntent plug-in cannot be used by default ... but it is a bit sad, since with the default settings, the menus will then fly out, when a user pass over the menu items, on the road to their destination. But perhaps that's a necessary situation? Maybe in the future, the "smooth" user experience can be made the default, somehow? I do know that some contrib modules, like Leaflet → and Collapsiblock 🐛 Update readme to reflect JS library download no longer required Fixed , simply bundle the JS library inside the module, to avoid loading external resource, and the module "just works". Is this not possible for Admin Toolbar as well?

Thanks a lot @ressa!

Super helpful on all points! 🥳

I cracked the riddle: It's BigPipe!

Great job! That's super helpful and indeed, I generally disable big_pipe since it currently sort of breaks the collapsible on devel:dpm with Symfony vardumper.
So that's totally on point and I'm going to look into this ASAP.

I think I understand why the hoverIntent plug-in cannot be used by default ...

First of all, I'm completely open to discussion and making different choices.
This has not been completely settled yet.

But, Technically speaking, I was brought to think this approach would be preferable because:

If we're looking at moving the hoverIntent plugin (whether Vanilla JS or JQuery) out of the code base, to be added as a dependency required in the composer.json file, we have to assume there might be cases where the dependency might not be met.

See #3513588-16: Update the jquery-hoverIntent JS library → :

The admin_toolbar module is so popular that we need to support installation from a tarball, for example, people using the ZIP file to install the module manually and not composer, see for example:
https://www.drupal.org/project/admin_toolbar/releases/3.5.3 → (Download zip)

In this case, they would not be able to have the JS file downloaded at the same time and therefore the module would be broken.

In other words: Not all users may be using composer to install the module and therefore might not have installed the hoverIntent plugin. In such cases, they would have to do it manually and place the file at a specific location, see for example the Colorbox module installation steps:
https://git.drupalcode.org/project/colorbox/-/blob/2.1.x/README.md?ref_t...

Many other JS or JQuery related modules will have such kind of logic and also special messages displayed on the Status report page.
Additionally, any types of settings related with the missing libraries would be disabled to fallback on a default or even disabled state/files.

This is why I would "personally" suggest that we set the default behavior to the standalone admin_toolbar default vanilla JS behavior, without any external hoverIntent plugin.
 

But I'm open to anything really if you have better ideas in terms of UX: Let's try thinking a bit further:

If we set the hoverIntent library as the default, with the checkbox ticked by default, as it is currently on the Settings page, we could add an extra check upon display to check whether the file exists, in which case we would attach the plugin library, otherwise we would just fallback on the default..... something like that?!

In other words: if the plugin file doesn't exist, the default file would always be loaded.
+ Warning/Info message on the Status report page
+ Info message on Settings form + field disabled

I do know that some contrib modules, like Leaflet and Collapsiblock, simply bundle the JS library inside the module, to avoid loading external resource, and the module "just works". Is this not possible for Admin Toolbar as well?

Sure, no problem, that's also another option:
We should just not do this 😅 Moving the plugin out of the code base, see:

I have reviewed your changes and thought a little bit more about it and we're not going to be able to move the JS plugin out of the code base.

This is definitely not as straight forward as I would have thought....

I'm definitely open to suggestions @ressa 🙏
 

For the time being: We're going to keep the same logic as currently: hoverIntent plugin enabled by default (checked).
IF later we find a good way to move the plugin out of the code base and decide it is a better choice than keeping the file, then we will change the default value in the corresponding merge request 👌

I'm reverting this change for now!

I'm getting back to work on the MR... this will need more time to get reviewed and tested properly 👍
Thanks a lot!

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!

Awesome @dydave, it looks and feels great! As a regular Admin Toolbar user, I can just download and install with the simplest of methods (when it's released) which is ideal, and not have to add an external library:

composer require drupal/admin_toolbar
drush install admin_toolbar admin_toolbar_tools admin_toolbar_search

About the description, it is a difficult concept to explain in a few words ... but I found a great article Timing Guidelines for Exposing Hidden Content, got some inspiration, and tried to add a suggestion. What do you think?

PS. So great that the BigPipe thing got fixed! Though strange that it worked for your colleague, but maybe they disabled BigPipe as well? ... but shouldn't the tests fail? After my own experience of halting progress by using a "tampered" environment ✨ Drupal admin_toolbar disable sticky/fixed state. Is there any option to do this? Active , I now attempt to use a freshly installed plain vanilla Drupal 11 for patch testing :)

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

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!

Thanks for your thorough explanations of code, concepts and your deliberations in such detail, it's really great and helpful! And great that the refactoring of code can consolidate the hover-related code in a single place, for easier future maintenance.

About tests, I would also not be surprised if some test suites don't include BigPipe, due to potentially unforeseen issues ... So it's awesome that we're catching bugs, and oddities along the way, and getting better and more resilient testing in place, as a side effect.

And yes, eventually the config form for hoverIntent will have to be revisited, new features added and good wording found for labels and descriptions -- I am looking forward to getting started with this task, it will yield even more nice improvements :)

I have tried the new patch, and it works perfectly.

About the placement and enabling of hoverIntent, I never gave it another thought, since I always enable all the modules, i.e. the "main" Admin Toolbar, (admin_toolbar) module as well as both sub-modules Tools (admin_toolbar_tools) and Search (admin_toolbar_search) -- and sub-module Tools gave me hoverIntent.

I agree that moving hoverIntent from the sub-module Tools to the "main" Admin Toolbar module makes sense, which is your option #2.

About enabling hoverIntent or not for users who don't already have sub-module Tools enabled ... Seen from the perspective of "Who would not want this enabled, and what would be the reason?", my assumption is that the majority of users would want this feature, and I can't see strong reasons for not wanting this -- so I would vote for enabling hoverIntent by default, for all users.

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 !

This looks perfect @dydave, great work!

Like you, I also tried the new code, going from an existing Admin Toolbar 3.5.3 with hoverIntent disabled, and it stayed disabled after the database update. A fresh install of only Admin Toolbar gives the user hoverIntent enabled, as we agreed is the best default situation. Yes, a slight performance increase might be the result, but that's probably a tiny performance hit 🙂

And I agree, we have been around a lot of edges, and it looks good to go 🎉 Fantastic efforts here from you again, I am very grateful for that, and your openness to my suggestions. Have a nice day!

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

So great to complete this, thanks @dydave! It's great that the code got cleaned up, and refreshed around the edges. Let's move on to the next task of adding extra config parameters for HoverIntent!

Automatically closed - issue fixed for 2 weeks with no activity.