Add Show on scroll-up / Always hide option to Disable sticky toolbar

pls review

Nice, thanks @darkdim, that works perfectly!

I got an idea, to display the options like Development settings (/admin/config/development/settings) in a container, so I updated the MR. I could set it the issue RTBC, but someone else should probably do that.

I have added a title to the container. Pls review

Thanks for the update, I do think I prefer a fieldset and title around the sticky_options_wrapper, since it adds both 1. a title, 2. a border around the radio buttons, and 3. an indentation, to show it's a sub-element (which I added with "07686fc8 - Show options as fieldset"):

    $form['sticky_options_wrapper'] = [
      '#type' => 'fieldset',
      '#title' => $this->t('Disable sticky toolbar behavior'),

... over this, which only adds a title, and no indentation:

    $form['sticky_options_wrapper']['sticky_behavior'] = [
      '#type' => 'radios',
      '#title' => $this->t('Disable sticky toolbar'),

Right now, both are set, and it should probably only be one of them ... so I'll remove title under radio.

Sorry, about this, I shouldn't have updated the MR, but left a code review instead.

Slide or fade-in, when scrolling up

I thought about the way the Admin Toolbar re-appears, when you scroll up. And though it currently works well, I do think it would be nicer if the menu showed itself more gradually, like the way it slides the menu into view from above at https://www.1xinternet.de/en

It could also be a fade-in, where it's always there, but just transparent, and scrolling up makes it visible, if that's easier to code?

Do you think this would be possible? If it's difficult, the way it works now is all right, but I do think it would be the optimal solution with a more fluid transition, and I added this in the the Issue Summary:

It would be nice if the transition was smooth, i.e. the menu slides down from the top, when you scroll up.

Updated Issue Summary's "Proposed resolution".

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

I've taken a closer look at the initial MR and wanted to suggest a few changes, so I've created the new merge request MR!123 above at #11, with a few comments about some of the differences with the previous MR.

Could you please try giving this patch a round of tests and reviews?

The disable sticky behavior is still in DEV, in the sense it has not yet been published in a stable release, therefore, we "should" still be able to change the property/configuration, without necessarily requiring all the hook_updates/install code.
Therefore, removing/merging the 'Disable sticky' field into a different field integrating a more generic logic (moving from bool to string), shouldn't be a problem if that seems like a better/simpler choice for the form UI.

Re #8:
@ressa, I think it "should" be possible to do this with custom CSS, or JS in your theme.
I think the 'FadeIn' animation you mentioned could be achieved with Vanilla CSS transition animations on opacity, for example. Probably the same for the 'SlideIn' animation, with a position property.

We could always explore this further, but I wouldn't really see how custom animations could be integrated to the admin_toolbar module itself.
Ideally, this is something that could perhaps sit in a separate module, such as Animate CSS → , which seems to offer very comprehensive sets of animations that could be implemented with CSS classes (fade, bounce, slide, glide, etc...).
I'm not opposed to the idea or feature, I just think it would be very complicated to match the level of flexibility or customization you could obtain with a more specific library.
We could for example add a documentation example CSS file with different CSS rules implementing various animations, fade, slide, etc... Users could then copy/paste the rules in their themes and change all the parameters by themselves (speed, animation, levels, etc...).
Any other options... (?)

In any case, we would certainly be glad to hear your feedback on this new merge request and some of the suggested changes.
Thanks in advance!

@TODO: More tests should be added for the 'hide_on_scroll_down' option.

Example of 'FadeIn' animation on scroll-up with CSS only:

/* Admin Toolbar's sticky behavior to hide on scroll-down. */
body.sticky-toolbar-hidden.toolbar-fixed .toolbar-oriented .toolbar-bar {
  opacity: 0;
  /* Hide immediately */
  transition: none;
}

/* Admin Toolbar default visible state. */
.toolbar-fixed .toolbar-oriented .toolbar-bar {
  opacity: 1;
  /* Simple fade in animation on the 'opacity' property. */
  transition: opacity 400ms ease-in;
  will-change: opacity;
}

Paste this code in your theme CSS or the file from the merge request css/admin_toolbar.sticky_behavior.css for testing purpose and you should be able to see a similar animation to the site you indicated above.

Hope that helps!

This is awesome @dydave, thanks for carrying on the great work by @darkdim!

* Great new interface
I like the new user interface, having the options as a radio list, while clearly signalling the current (default) sticky behaviour on installation, and the options.

* Wording
The wording could be this, to signal that both option 2 and 3 are disabled, but different behaviours, by adding "Disabled: " to the last option?

• Enabled
• Disabled
• Disabled: Hide on scroll-down, show on scroll-up

* Fade-in
I added the fade-in CSS in css/admin_toolbar.sticky_behavior.css and it works great, thanks! Just a detail, perhaps the fade-in could be a little snappier, like 200ms?

Thanks @ressa for your help testing and once again for the very positive and constructive feedback 🙏

No problem for the changes to the wording, I'll make the changes to the MR a bit later 👌

For the Fade-in and animations, what's your take on this ?!
Do you see this as part of the module, or as I suggested earlier (#12)?
Keep it simple (bare minimum/no animation) and allow users to override the corresponding CSS rules with their own animations, based on documented CSS suggestions, for example.

To change the animation above (feel free to play with it @ressa):
See the CSS line above transition: opacity 400ms ease-in;
It is possible to modify this value to 200ms to make it a bit quicker + change potentially the type of transition, ease-in, ease-out, ease-in-out, or use multiple transitions, etc...
For more details, see: https://developer.mozilla.org/en-US/docs/Web/CSS/transition

See:

Just a detail, perhaps the fade-in could be a little snappier, like 200ms?

This is what I'm afraid about 😅
If everyone starts asking for different types of animations, delays, effects, etc.... It's not going to be viable for us 😅
That's why, I would be personally be more inclined to try to keep this type of feature out of the module and left at the charge of the site developers to refine and pick the exact type of display they would like manually by changing the CSS themselves.

I'm not against or opposed to anything, I'm just worried of the number of requests from users to have this type of feature adapted to their needs 😅
We could include this type of effect/transition in the module as an example that developers could override, but I don't see any type of customization of these types of transitions going into the module itself.... sounds like too much work 😅

Any feedback, advice, comment or suggestions would be very appreciated.
Thanks again very much for all your help @ressa!

Thank you @dydave for as always a fast and thorough answer!

Just to be clear: I don't think or suggest that any of these transitions or times should be configurable -- we just set some sane defaults, and allow the user to override them, if they so desire.

Also, approaching this purely from an "aesthetical" user experience POV, I think the fade-in CSS should be included, since with the current method, the reappearance seems to me too "abrupt". A gradual appearance is more pleasant. Don't you think, putting on the "regular-user"-hat?

Since I believe the majority would prefer fade-in, we should document how to disable the fade-in, for the minority, who don't want this. And about the speed, let's just set it to 200ms, and again -- if someone likes another speed or method, they should feel free to override.

It all sounds great to me @ressa!!

All good with all your points above 👍
I think we're on the same page in terms of the fancy bells and whistle animations 😅

we just set some sane defaults, and allow the user to override them, if they so desire.

Let's do it !

Feel free to keep posting reviews ... I'll address them later today, maybe in 5 hours or so....

Thanks again! 🙂

Yes, I agree, let's keep it simple and user friendly :)
I don't think I have more additions, these two were my only thoughts of changes ... thanks!

Thanks @ressa!

The merge request MR!123 has been updated with the suggested changes and seems to still be passing all tests 🟢

• Updated 'hide_on_scroll_down' radio field option label.
• Updated sticky behavior CSS file to support fadeIn animation by default.
• Added PHPUnit tests to check for the 'hide_on_scroll_down' libraries on page.

 
See the resulting Admin Toolbar Settings form:

At this point, this MR should pretty much be good to go, thus moving back to Needs review 👍

Additional comments, reviews and testing feedback would be greatly appreciated! 🙂
Thanks in advance!

ressa → changed the visibility of the branch 3509584-add-show-on to hidden.

Awesome work @dydave, and even getting the tests done as well -- and it looks great!

A last minor thing, is an occasional flicker, if I use the mouse wheel to scroll down, or press Down key, or Page Down key a few times -- I guess it's because the pixel value difference hits 1 pixel, detecting this as the user scrolling up, triggering a change of state -- even though as a user, this was not the action ...

Perhaps it's possible to make the scroll up/down detection less sensitive, and only react as the user scrolling up, if the change was more than 1 pixel and less than the movement when scrolling up or down in the window? It looks like a Down key press moves the screen around 55 pixels.

So perhaps the difference threshold could be set to something between 1 and 55 pixels, like 10 pixels?

PS. It can be a bit of struggle to trigger the occasional "false" fade-in ... I was tempted to set the issue to RTBC, and if you can't replicate it, we can set it to RTBC, and sort it out in a follow up issue. What do you think?

Re #21: I'll look a bit closer into that and do a bit more testing to see if there is anything we could do to improve the JS interactions, in particular try testing the solution you suggested 👍

Thanks again @ressa! 🙂

@ressa: I've done a bit more testing, but I'm not really seeing the issue with the scroll or key/up down:
It all looks pretty smooth to me 😅

I "think" I can understand what you mean, in terms of giving a bit of flexibility to the change on window, but I'm not sure I'm really able to replicate this with default settings (browser, drupal install, theme, modules, etc...).

I was tempted to set the issue to RTBC, and if you can't replicate it, we can set it to RTBC, and sort it out in a follow up issue. What do you think?

Indeed: I would tend to think the current patch/behavior "should" be stable enough to be included in a release, thus, setting it back to Needs review, hoping to get more reviews and feedback 🙂

Why not create a follow-up issue, if you'd like, for the behavior mentioned at #21, but maybe let's see if we could get users feedback, experiencing the same type of behavior (?)

It would be great if we could get a bit more testing feedback and reviews on this issue before it gets merged 🙏
Thanks again for your great help!

Thanks for checking, and yes -- it can be a chore to replicate :) ... I can always create a follow up issue, if it gets too much.

I have found a way of replicating the movement artificially for easier development, running scrollBy in Console, which scrolls the document up 2 pixels: window.scrollBy(0, -2);

If (when) a 10 pixel threshold for triggering a fade-in is implemented, this 2 pixel movement up shouldn't then make the admin toolbar reappear.

I am attaching a screen-capture of a real mouse scroll-wheel induced false positive.

From a functionality perspective I think it's RTBC, but comments on the code are welcome!

Thanks a lot @ressa, once again for the very clear description and video, it's greatly appreciated 🙂

No problem, we're going to look into this and see if we could find a proper solution 👍

I think I'm going to have to ask some help from other colleagues more specialized in front-end/JS development to see if they would have any advice on the way we could implement a threshold for the scroll animation.

Let's sit on this issue for a bit:
We've got as far as we could for now and we still have some time before the next release anyway.
I'd like to get at least 4 or 5 other tickets in before we release the next stable.... so we probably still have a few more weeks in front of us 👌
(see the recently updated tickets in Needs review, quite a lot of issues are moving🥳)

I'll take a little bit of time in the coming days to speak with some colleagues and try to put together something concrete that could be tested more carefully.
 

In the meantime, feel free to let us know know if you have any questions, concerns or comments on this particular issue, we would certainly be very happy to hear your feedback.
Thanks in advance!

You're welcome @dydave! I am very grateful for the thorough explanation and road plan, and that you will check in with your colleagues, it sounds very good :)

Great to hear that there are many issue in the pipeline! Perhaps it's time to consider a "[META] Plan for Admin Toolbar release 3.6" and list the issues there, prioritized under headers like for example "Important/low effort" and "Nice to have"? That way, us regular users can know which issues to focus on, for patch testing, and similar tasks.

Thanks @ressa!

I told my colleague Laurent about this issue, this morning and funny thing, he said he knew exactly what you meant 😆

He told me he would look at the code when he has some time and get back to me on this, if there is anything we could do 👌

Great idea for the META issue, we could create this next week maybe 👍

Hopefully... more to report back soon 🤞

Heh, nice to get this behaviour confirmed, and it sounds great if he can take a look at it, thanks! Perhaps we need a new word for this?

Back Scroll: When a mouse scroll wheel bounces slightly back in the opposite direction, after scrolling up or down.

After the recent changes, it looks like the MR needs a re-roll ... I tried to look for alternative solutions for the "show on scroll up" challenge and the article Hide header on scroll down, show on scroll up has an interesting approach, which is not pixel-oriented, and that perhaps could work here as well?

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

Perfect @dydave, thank you for yet another very fast solution, again!

I am glad that the example was useful and could be used as inspiration, nice solution you made 🙂 After testing with the new MR, I no longer see the dreaded Back Scroll effect. The Admin Toolbar menu is only ever shown again, when I intentionally scroll up. So 10 pixels seems like a fairly safe and sane setting to go for.

I only have a single suggestion, to get the nice "sliding in from above" effect as seen elsewhere. What do you think about that? Thanks!

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!

It looks good @dydave, thanks! Great catch with fixing the JS problem with the focus key to navigate menu elements. I am glad to hear that you like the change to the slide-effect. I did notice that a slight shade was still visible at the top, but setting it at 220% is perfect, to hide it completely 🙂

About support for Vertical mode, I think your suggestion of dealing with that in another issue is the right approach. I think, if someone needs that, they should feel to create it an issue, and we can take it from there.

So this looks good to go, since everything looks fine from my perspective, thanks!

dydave → closed merge request !118

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!

It's so great to get this feature included in the module! 🎉

Thank you as well @dydave for the great co-operation, and also very timely and productive code updates, great suggestions for improvements in functionality and user interface, as well as openness to my suggestions, I very much appreciate it!

And yes, let's add follow up issues, if there seems to be a need for that 🙂

Your plan about including some more issues for the next release is a great idea, and I agree. So I went ahead and created a Meta issue, where we can perhaps gather the relevant issues, to more easily keep track of where an extra effort is needed? Thank you!

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

Yes @dydave, I agree we make a good team -- the Admin Toolbar is getting some serious attention 🙂
And it sounds great that the Meta issue is useful, onward towards more improvements!