<thead> element hangs, when Toolbar is hidden

Super good catch @ressa! 🥳

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

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

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

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

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

Sounds fantastic, thanks as always for a fast and positive response @dydave! And this is such a small detail, that it can easily be put on the 3.7 Meta list, and handled when it's convenient, so I have added it there.

And yes, it does seem like all the thorough and diligent testing (and let's not forget you writing tests in the first place!) is paying off, and the only bump so far for the users, were the missing database update, or in combination with other contrib modules -- it's pretty impressive 🙂

Ps. I was translating Admin Toolbar, when I discovered a detail about strings, where "toolbar" and "sticky" is reversed in one place, and shown as a translatable string:

https://localize.drupal.org/translate/languages/da/translate?project=adm...

$ grep -iR "toolbar sticky" .
./js/admin_toolbar.sticky_behavior.js: * Admin Toolbar sticky behavior JS functions.
./src/Form/AdminToolbarSettingsForm.php:      '#title' => $this->t('Toolbar sticky behavior'),
./src/Form/AdminToolbarSettingsForm.php:      '#prefix' => $this->t("By default, the Admin Toolbar sticky behavior is <em>enabled</em> so it stays at the top of the browser window when scrolling up or down.<br/>Select <em>Disabled</em> to disable Admin Toolbar's sticky behavior so it stays at the top of the page when scrolling up/down and does not follow the browser window."),
./admin_toolbar.libraries.yml:# Disable toolbar sticky behavior.

$ grep -iR "sticky toolbar" .
./config/schema/admin_toolbar.schema.yml:      label: 'Sticky toolbar behavior'

Suggestion: Use "Toolbar sticky behavior" in admin_toolbar.schema.yml.

It's such a small thing, so maybe fixing it here as well is easiest? 🙂

Totally! Thanks a lot @ressa.

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

I'm noticing something similar with views tables that have 'sticky headers' enabled. Inspecting it shows it seems to have the correct offset in and of itself:

table.sticky-header thead {
    position: sticky;
    z-index: 500;
    top: var(--drupal-displace-offset-top, 0);
}

It appears properly aligned below the menu bar when it exists, but doesn't pop back up to the top when the admin menu hides itself, which is a bit odd as it just floats in the middle of the table. It reseats itself to the top of the table properly when scrolling up. I can create a new issue, but this sounds like it has the same root cause.

Hi @dydave and @ressa, I've followed the steps to replicate the issue and can confirm that the issue persists. When choosing the option "Disabled, show on scroll-up" under Toolbar sticky behavior setting of Admin Toolbar Settings, on scrolling down and up the attribute "--drupal-displace-offset-top" is not getting restored respectively, which is triggering the issue for all the tables that have "Enable Drupal style "sticky" table headers" property enabled as @erutan commented on #6. I'm attaching the screenshot for reference.

My bad, forgot to attach the attachments. Attaching here. I'm also looking into the issue.

CSS-based Solution

[data-toolbar="disabled"].toolbar-fixed {
  --drupal-displace-offset-top: 0px;
}

[data-toolbar="disabled"].toolbar-fixed.toolbar-tray-open {
  --drupal-displace-offset-top: 79px; /* Adjust as needed */
}

@dydave, I've fixed the sticky table header design issue that occurred when the option "Disabled, show on scroll-up" is selected. Please review the MR and let me know if any further changes are needed. I've attached screenshots of the fix for your reference.

Thanks!

@abhishek@kumar, I've tried the CSS shared in comment #10, but it didn't work as expected. The attribute [data-toolbar="disabled"] is not present in the DOM during testing on Drupal 10.4.7.

Let me know if there's an alternate approach or if this behavior is version-specific.

The CSS in #10 didn't fix the issue in either my admin or default/site theme. Tested in Safari & Chrome.

The patch from the MR by @tirupati_singh works for me, thanks! It doesn't seem like it'd cause any other issues, the existing disabled and enabled for admin toolbar sticky still work as expected.

Waiting for a 3.7 minor release doesn't make sense as this is a pretty obvious/annoying visual bug IMO. :)

Setting back to Needs work

As indicated in comments #11 and #12, MR 152 only address the issue when using the "Disabled, show on scroll-up" setting. The problem persists if you're using "Enabled" or "Disabled"

We need to toggle the data-offset-top attribute on the toolbar-bar and any active toolbar-tray so Drupal.displace() will recalculate offsets → correctly. Updated the MR to do that.

This is working for me if toolbar sticky behavior is set to "Enable" or "Disabled, show on scroll-up". It does not fix the "Disabled" case yet.

I think we probably need a new js behavior the "Disabled" case that also unsets data-offset-top attributes. So, still "Needs work" but testing/review of the latest changes appreciated!

Thanks everyone for the work on this!

Re #15:
I've tested this locally and it seems to work pretty well!
Great job!

I have started a bit of code review, but overall the changes looks good!
Great find with Drupal.displace(); 👍

I'm wondering if this could be a solution as well for 🐛 zindex issue between admin toolbar and ckeditor 5 Active (?!)
(Same issue, with the CKEditor toolbar / a problem of offset calculation)

One thing though @justcaldwell:

It does not fix the "Disabled" case yet.

It worked for me locally, with the same patch applied, on the same page that was causing issues for the sticky behavior (/admin/content).
No additional change required and it seemed to work fine for me with Claro on D10.4.

There could very well be something specific with my local environment....
Not sure exactly, but from my initial round of tests, the patch seems to fix the issue.

Switching this back to Needs review, as an attempt to attract some attention on the current patch so we could get more testing feedback and reviews.

@ressa: Do you think you could give the patch a round of tests?
In particular with the Disabled behavior, if possible.

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

Hi all, I’ve retested the MR I raised for the fixes after the latest changes were applied. I patched it onto another local environment and verified that the changes are working as expected. The sticky behavior of the admin toolbar now responds correctly to the option selected under "Toolbar sticky behavior" configuration.

@dydave — while testing, I encountered a new issue when the "Hide or show the toolbar with shortcut (Alt + p)" option is enabled. The shortcut is not working in my setup, and I’m seeing a console error related to the following code:

toolbarElement.querySelector('#toolbar-bar').insertAdjacentHTML(
          // The toolbar tab item is inserted as the last item in the toolbar.
          'beforeend',
          Drupal.theme.adminToolbarToggleCollapse(),
        );

This error appears in the JS file:
/admin_toolbar/js/admin_toolbar.toggle_shortcut.js at line 66.

Upon inspecting the DOM, I noticed that the element with ID "toolbar-bar" is not present, which might be causing the issue and preventing the shortcut from functioning.

Could you please confirm if I’m missing any specific configuration needed for this feature to work?

I've attached a screenshot of the console error for reference.

Thanks!

Thanks @tirupati_singh!

I've checked and the CSS ID #toolbar-bar seems to be used in many CSS files in Core...
So it should be safe to assume the module should also be able to use it...

I tested again the patch just now and I'm unable to reproduce the issue.

Could you please maybe try on a fresh D10/11 install?

Otherwise, additional information on how the problem could be reproduced would be very helpful.

Thanks in advance!

Hello,

I have tested the latest merge request, and it is working as expected.
Also, there are no console errors when enabling the "Hide or show the toolbar with shortcut (Alt + P)" button and it is working as expected for me too.

However, during testing, I observed that while the sticky toolbar functionality is working, the transition does not appear smooth. It may benefit from some refinement to improve the user experience. While scrolling upwards the toolbar sticky has some delay.

Attaching the video for reference.

Thanks everyone for looking at this and providing solutions, it's great to see so much activity!

The latest patch works well for "Disabled, show on scroll-up", but but the table header still hangs 80 pixels too low for the "Disabled" option ...

Disabled, show on scroll-up: Works, table header slides away, and reappears
Disabled: Table header hangs 80 pixels too low

Tested in Drupal 11.1.7 with Claro, DDEV PHP 8.3.21, Debian 12 Firefox and Chromium browsers.

@jatingupta40: Thanks for the feedback, maybe you can check how "Disabled" looks as well?
@tirupati_singh: Thanks! Did you test both options?

And I agree, the transition is slightly janky, but it may be difficult to get right?

Thanks a lot @ressa!

OK, I'll have to do more testing locally with the Disabled behavior, as @justcaldwell mentioned above at #15.

Looks like we might have to add some more code to the MR, as suggested, to fix the offset of the table header.

I'll try testing this more carefully a bit later when I have some time.

Thanks again very much!

Thanks for the quick follow-up, @dydave!

I went ahead and tested the module on a fresh Drupal 11 installation, and I can confirm that the issue I previously encountered (mentioned in comment #17 🐛 element hangs, when Toolbar is hidden Active ) is no longer present. The shortcut toggle (Alt + P) is working as expected, and there are no console errors related to the #toolbar-bar element.

It seems the issue might have been due to some customization or configuration in my earlier setup. I’ll keep an eye out and report back if I’m able to consistently reproduce it in a specific scenario.

Thanks again!

@ressa

@tirupati_singh: Thanks! Did you test both options?

Yes, I tested all three options under the "Toolbar sticky behavior" configuration, including "Enabled", "Disabled, show on scroll-up", and "Disabled". Everything was working correctly in my setup.

I specifically checked using the Claro theme with sticky table headers enabled, and didn’t notice the table header hanging issue with the "Disabled" option.

Happy to retest or provide more details if it helps narrow down the issue!

Thanks for verifying @tirupati_singh! That's interesting ... perhaps you can share your browser version, in case that plays a role?

I tested on Chrome Version 137.0.7151.55 (Official Build) (64-bit) and Drupal 10.4.7v. It seems to be working fine on this version, but I’ll keep an eye on it and let you know if anything changes.

Let me know if you need any other details!

Perfect, thanks @tirupati_singh :)

I just discovered the same thing I originally saw with "Disabled, show on scroll-up", but which is fixed in the current MR:

With "Disabled", if I hover over the too low placed <thead> element and inspect it, it jumps into place at the top. Note: The dev-tool (F12) cannot be opened, then it won't jump to the top :)

Indeed, I have done a bit more testing and this issue is going to need some more work 😅

I was able to reproduce the different cases mentioned above and think we should be able to fix them. 👌
But the current MR is most likely going to need more refinements which will probably take some time.

Therefore, for the time being, I went ahead and created the 3.6.1 → patch release of the module with urgent bug fixes, so it gives us more time and less pressure to work on this issue a bit more carefully.

Once all cases have been covered and tested, we should be able to get this issue released in another patch version.

Let's take a bit more time for testing/reviewing the MR and hopefully we should be able to come up with a proper solution for all the cases. 🤞

Thanks in advance!

That's a good call @dydave, I agree 100% -- those issue are more urgent, and this issue can easily wait, since it's a minor problem. So really great to get 3.6.1 released to fix bugs, thanks!

Sounds good that you were able to recreate the scenarios here, and addressing this issue (and its tasks) in its own time is a good plan.

In the latest changes:

1 - I removed the data-offset-top attribute toggle logic I previously added. Turns out Drupal.displace() is smart enough to deal with the hidden toolbar CSS, so long as it's deferred long enough for the transitions to finish.

2 - I added the disable_sticky JS behavior that does remove the data-offset-top attributes when stickyness is "Disabled". In this case, we don't want the toolbar used in calculating offsets at all. Interacting with the toolbar in various ways can cause the offsets to be recalculated, so we have to listen to drupalViewportOffsetChange. I hesitated to use jQuery in the new behavior, but since the Toolbar module still uses jQuery to dispatch that event, we can only use jQuery to listen for it, at least as I understand it.

Looks like the Drupal 11 version of the Toolbar module is still using jQuery, so it should be compatible.

The changes resolve the hanging <thead> issue for me across all configurations. I tested in clean install of 10.4.7 with the latest versions of Chrome, Safari and Firefox on a Mac. More testing/review is welcome!

Regarding the (lack of) transition smoothness, my vote would be to handle that in a separate issue -- particularly if we find that the <thead> issue has been addressed.

Hi all, I’ve re-reviewed the latest changes related to the disabled sticky behavior fixes. The issue with the sticky table header persisting when "Toolbar sticky behavior" is set to "Disabled" has now been resolved. All configuration options under "Toolbar sticky behavior" are functioning correctly.

The remaining transition property enhancements for the toolbar can be addressed separately, as suggested by @justcaldwell in the last comment.

I’m keeping the issue status as Needs Review to gather feedback from @ressa and @dydave. If everything looks good, we can move it to RTBC.

Additionally, I’ve updated the MR to include the missing disabled_sticky library dependency required for Drupal.displace().

Please review the latest changes and let me know if any further updates are needed.
Thanks!

Thanks for the review/testing @tirupati_singh!

I'm not sure we need any of the core/drupal.displace dependencies. All these behavior libraries already depend on admin_toolbar/toolbar.tree, which, in turn, depends on the core Toolbar module's toolbar/toolbar library.

toolbar/toolbar depends on core/drupal.displace and a number of other core libs that we're using in admin_toolbar (once, jquery, drupal, etc.). So check my thinking here, but I think those additional dependency declarations can be removed from admin_toolbar.libraries.yml.

As indicated in comments #11 and #12, MR 152 only address the issue when using the "Disabled, show on scroll-up" setting. The problem persists if you're using "Enabled" or "Disabled"

I wonder what the difference was between me & @dydave #16 vs @justcaldwell since he was also apparently using 10.4.7 back then. @ressa being on D11 makes the change in behaviour more understandable if there's been an underlying structural issue that has changed as @tirupati_singh is also working off of 10.4.x

FWIW Comment 12 had nothing to do with the MR, it was talking about a CSS only solution presented in comment 10. Comment 11 also never specified the issue was still present in disabled and enabled.

Splitting off the jankyness when the toolbar hides and shows repeatedly as shown in the video in #19 makes sense to me vs blocking this issue with it.

Tested with the latest diff and things are unchanged on my end (so no regressions in my environment at least) in 10.4.8 and 3.6.1. :)

Thanks for the updates! With the latest patch, it works well for both settings, "Disabled, show on scroll-up" as well as "Disabled".
Tested in Drupal 11.1.7 with Claro, DDEV PHP 8.3.21, Debian 12, Firefox and Chromium browsers.

The jank feels like it's less than before (I think) and not really an issue as I see it, so could definitely be handled in a follow up issue.

About - core/drupal.displace, it works well also without this for me ... so maybe not needed?

Additionally, I’ve updated the MR to include the missing disabled_sticky library dependency required for Drupal.displace().

Should Status be changed to "Postponed" until the last questions are figured out, and a (more or less) final, intended version of the code is ready for review?

The current, de facto approach for the admin_toolbar module is to not explicitly declare a dependency if it's been declared somewhere up the dependency chain. For example, once is used in almost all the JS files/behaviors, but does not appear as an explicit dependency anywhere in admin_toolbar.libraries.yml. That example argues for removing core/drupal.displace as an explicit dependency.

BUT, there are examples in core that redeclare dependencies. e.g.: core/drupal depends on core/drupalSettings. There are numerous examples of other core libs in core.libraries.yml that explicitly declare dependencies on both drupal and drupalSettings (e.g., active-link, autocomplete, batch, etc.). I can't find any best practice documentation on the subject, so we probably have to rely on core's example.

That, then, would mean core/drupal.displace stays, and that we actually need to declare some additional dependencies based on usage in each behavior.

To move this issue along, I suggest we:

1. Stick with the module's de facto approach for the purposes of this issue, and remove core/drupal.displace.
2. Decide on whether or not to explicitly declare all dependencies and make the resulting changes in (yet) another issue. 😊

Opened 📌 Improve show/hide transition Active for the transition smoothness issue.

I added MR 160 based on an alternative branch that removes explicit core/drupal.displace dependencies. I also created a new issue regarding dependencies.

If #35 seems reasonable, a path forward is to merge 160 and continue discussion in 📌 Determine and implement JS dependency approach Active .

I kept MR 152 around, too, as I think it would be harmless to merge that one instead. Both resolve the issue at hand.

Thanks @justcaldwell for creating the dependencies issue, it would be great to get that cleared up. I agree with your proposal #1:

Stick with the module's de facto approach for the purposes of this issue, and remove core/drupal.displace.

MR 160 still works well for me, for all combinations.

@dydave mentioned:

I'm wondering if this could be a solution as well for 🐛 zindex issue between admin toolbar and ckeditor 5 Active (?!)

(Same issue, with the CKEditor toolbar / a problem of offset calculation)

I did previously notice that the CKEditor toolbar would occasionally also get the 80px offset, but could not reliably re-create it in the CKEditor ...

But I tried again, and this time I could reproduce it reliably. So we probably need to make sure that it works as well for CKEditor ... So I updated the Issue Summary, and am adding the related CKEditor issue, in case these two issues need to be coordinated. Thanks!