Make un-hover delay configurable

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

Made an initial MR for this - we could always add an update hook to make sure this setting exists, ive coded defensively for it.

This is awesome @aaron.ferris, thanks!

It works perfectly, and bumping "Hover Intent Timeout (ms)" up to 500 ms or even 750 ms allows for non-precise mousing, without having to start all over with a multi-level traversal, due to slipping out of the menus accidentally. I'll set to RTBC, and if more code is on the way, it can always be reverted to "Needs work".

PS. Setting timeout to 0 ms and speed-running Drupal menus, traversing to the deepest level of all menus could be a new sports discipline :-)

No problem, glad it’s working well for you.

I can confirm that this does indeed work well. In addition, by moving the hoverintent to a drupal behavior instead of $(document).ready it fixes another issue I've been seeing on some of my sites where hoverintent failed to work at all.

Thanks a lot everyone for the great work on this issue, it's greatly appreciated!

I've rebased/fixed a few conflicts that were blocking MR!99 and it should be good now.

I haven't tested the feature yet, but after a very quick review of the MR, it looks good overall, great job! 🙂

I think there are a few more changes I'd like to add to the MR, in particular, I think the whole hoverintent feature should probably be moved out from the admin_toolbar_tools module to the admin_toolbar.
This feature "should" work without the admin_toolbar_tools module, so the config variable hoverintent_functionality should be move out to admin_toolbar, impacting the schema, the settings forms and probably the install files of the module (to update module's configuration on all sites).
I'll then add a form state to the added hover_intent_timeout config field to hide or show if the functionality is enabled or disabled.

Lastly, we'll mostly likely need to add Functional tests for this:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/tests/src/Fu...

More work is needed on this, so I'm setting this to Needs work and will circle back on this in the next few days: I'm going to need a few hours to get this properly fixed.

Bringing this one back up to the top of the list.
The good news is that the changes at this point seem pretty straight forward, therefore there are great chances this feature could be merged in the coming days/weeks.

At which point the MR will be set back to Needs review and we will need you help testing and reviewing again.
Thanks again!

We'll see, but I'll most likely split this into a separate ticket to get the hoverintent_functionality moved out from admin_toolbar_tools to admin_toolbar and come back to this issue right after to add the state on the field added by the MR.

More coming in the next few days.
Cheers!

Any chance anybody here could help testing the merge request in parent issue 📌 Move the 'hoverintent_functionality' setting to admin_toolbar Active ?
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/132

Once the other MR is in, we should be able to get this issue merged in quickly.

Any feedback, comments, questions or reviews would be greatly appreciated! 🙏
Thanks in advance!

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 @dydave, I think that looks great! Everything is pretty clear, about how it works.

I left a few comments, but only about cases. The project maintainers of the plugin use the "hoverIntent" project page, even when starting a sentence, perhaps we should as well? I have left comments about instances added or altered in this commit, but there are a few more ... maybe they should be updated as well, while we're at it?

$ grep -R "HoverIntent" .
./config/schema/admin_toolbar.schema.yml:      label: 'Enable HoverIntent'
./src/Form/AdminToolbarSettingsForm.php:    // Enable the HoverIntent plugin behavior.
./src/Form/AdminToolbarSettingsForm.php:      '#title' => $this->t('Enable HoverIntent'),
./tests/src/Functional/AdminToolbarSettingsFormTest.php:    // Check the HoverIntent functionality is enabled.
./tests/src/Functional/AdminToolbarSettingsFormTest.php:    // Check the HoverIntent functionality is not enabled.

And for the Admin Toolbar users who don't what hoverIntent is, we could link to the project page, perhaps in the description text?

Now

Provides a smoother user experience, where only menu items which are paused over are expanded, to avoid accidental activations.
Disable to use module's default basic JavaScript behavior.

Better?

Provides a smoother user experience, where only menu items which are paused over are expanded, to avoid accidental activations.
Disable hoverIntent to use module's default basic JavaScript behavior.

Actually, maybe a drop-down for delay in milliseconds could work better? Also, I think 1500 ms might be enough for most users, to not make the list too long ... what do you think?

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

Beautiful work @dydave, thanks! I agree that this looks much better, and works just like I hoped for. I can set the timeout to a value, which works well for me.

It could even be considered to set the default hoverIntent timeout to 500 ms? This would make it more forgiving for new users, who need to traverse many levels down. It's frustrating for them, to have to start over, if they mistakenly "skate" outside a menu item more than 250 ms, and lose the position, and have to start all over ... Expert Admin Toolbar users, who like it really snappy, can always reduce it to 250 ms ... what do you think?

I think the new Settings form and the section wrappers look great, and the field labels and descriptions as well!

About "hoverIntent sensitivity" and "hoverIntent interval" I have a difficult time wrapping my head around what they actually do, or why I would want to change them, even after reading the hoverIntent project page ... This just to say, that the descriptions in the MR are fine, since my problem is a more fundamental lack of understanding of those two concepts :)

Also, I did try changing their values to something else, but I think the defaults of 6 pixels and 100 ms work well. Thinking more about it, I feel that these last two values "sensitivity" and "interval" are less important than "timeout", and could even be not shown by default, but placed under an "Advanced settings" fieldset, collapsed by default?

PS. I found a last lingering "Hoverintent" here:
$ grep -R "Hoverintent" .
./tests/src/Functional/AdminToolbarSettingsFormTest.php: // Test Hoverintent and Sticky behavior.

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!

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?

Awesome work @dydave, perfect!

The "Advanced" section does look perfect now, and the new description text is really good ... But I do appreciate your openness to remove the two settings. I can only say, that I can't see that I would ever feel the need to change the sensitivity or interval values ... so maybe we should remove them?

PS. Great that tests with even big_pipe installed now work!

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.

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

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

Yes @dydave, I also know very well how the idea of adding a cool feature can lead you down some roads, only to turn out that it may not be very useful after all ... though it was interesting building it. But if it's not adding value or quality to the final result, it's time to Kill Your Darlings 🙂

I agree that it would be too bad to lose all the great code, so great call making a fresh MR to preserve it, in case we might want to resurrect these, or some other features, and make them available as settings via the GUI, or maybe just re-use the code in some other way.

The new MR works perfectly! I agree that the user interface now looks great, and is very simple and intuitively understandable.

A last detail, is that I get this error, if I start with the current dev-version, install the Admin Toolbar module, apply the MR here, and then visit the settings page, and I added a comment about it:

Error message
Warning: Trying to access array offset on null in Drupal\admin_toolbar\Form\AdminToolbarSettingsForm->buildForm() (line 135 of modules/contrib/admin_toolbar/src/Form/AdminToolbarSettingsForm.php).

Also, I can trigger this with the same method, but am not sure how to fix it:

Error message
Warning: Trying to access array offset on null in admin_toolbar_toolbar_alter() (line 38 of modules/contrib/admin_toolbar/admin_toolbar.module).

When these last two details are fixed, it looks ready to me 🙂

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 !

Ah yes, you're right! Thanks for taking the time to carefully explain the correct method, and steps required to test the MR. I totally missed the crucial first step, as you describe, to start out from version 3.5.3 -- that was the missing piece of the puzzle.

I tried again with the method you describe, and everything worked, just as expected -- no errors or warnings, just a perfect, brand new, beautiful user interface!

It was really great to get this cleared up for me, and we can disregard my last comments, the MR is RTBC!

PS. And no problem at all, that we tried some new stuff out, but that was it was scrapped in the end. It's very much a good thing, as I see it: To experiment, and see if a new feature brings something new to the table -- each of the features could have been fantastic additions. In this case maybe not so much -- but at least some great code was created for potential later re-use, recycling is good 🙂

dydave → closed merge request !99

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 all, and to you @dydave for persevering with my general concept!

Awesome! 🎉 Thanks @aaron.ferris for kickstarting the solution, and @dydave for carrying the task to completion so tenaciously!

I'll surely share any future ideas with the community -- I actually once had another idea, which got added (shortcut to search). But I since found a better, more universally supported solution, and I'll add it to the version 3.6 Meta issue, and we can see if it's possible. Thanks!