🇫🇷France @dydave

Quick follow-up on this issue:

All the changes detailed in the issue summary have been implemented and described in the merge request MR !176 above at #2.

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

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

dydave → created an issue.

Hi Joachim (@joachim)!

Thanks a lot for the great work on this issue and for keeping the suggested code changes maintained.🙏

Just wanted to bring to your attention related issue 📌 Automated Tests: Add Functional tests for classes ExtraLinks and SearchLinks Needs review which references this issue several times 🙂

It would be super helpful if you could please try to see how the changes in the merge request could integrate with the 'ExtraLinks' tests added in the related tests issue.
I would definitely be in favor of working on this issue and getting this change potentially integrated, as soon as we could have the 'ExtraLinks' and 'SearchLinks' automated tests in place.

Overall, I think this ticket is more of a refactoring task than a new feature, so with the tests in place, we should be able to move forward a bit quicker with these changes.

Could we also refactor the following links/labels? 'Add {entity type}' links under 'Structure' ('admin/structure')

'Add contact form'
https://git.drupalcode.org/project/admin_toolbar/-/blob/af36beae3b23cab0...
https://git.drupalcode.org/project/admin_toolbar/-/blob/af36beae3b23cab0...

'Add media type' ==> I "think" you've already done this one ✅
https://git.drupalcode.org/project/admin_toolbar/-/blob/af36beae3b23cab0...
https://git.drupalcode.org/project/admin_toolbar/-/blob/af36beae3b23cab0...

'Add content type'
https://git.drupalcode.org/project/admin_toolbar/-/blob/af36beae3b23cab0...
https://git.drupalcode.org/project/admin_toolbar/-/blob/af36beae3b23cab0...

'Add vocabulary'
https://git.drupalcode.org/project/admin_toolbar/-/blob/af36beae3b23cab0...
https://git.drupalcode.org/project/admin_toolbar/-/blob/af36beae3b23cab0...

'Add role'
https://git.drupalcode.org/project/admin_toolbar/-/blob/af36beae3b23cab0...
https://git.drupalcode.org/project/admin_toolbar/-/blob/af36beae3b23cab0...

'Add view'
https://git.drupalcode.org/project/admin_toolbar/-/blob/af36beae3b23cab0...
https://git.drupalcode.org/project/admin_toolbar/-/blob/af36beae3b23cab0...

Note that Block content and Comment types currently do not have an 'Add {entity type}' link added by the ExtraLinks class.... which should ideally also be standardized: the same behavior across all entity types would be easier to understand for anyone. 👌

It would be great if we could find a more generic way to approach the 'Add {entity type}' links added by the module under 'Structure' 👍
Then we should be able to come back to the 'Add {entity type} content' links, under the 'Content' menu.

Any feedback, comments or reviews on related issue or this ticket would be greatly appreciated.
Thanks in advance!

Thank you so much @ressa! 🙏

Sorry I didn't reply to your other comments in other tickets, but I wanted to focus on really pushing out in priority all the Automated Tests on which I have been working for months! 😅

Some issues really took a lot of time, in particular:
📌 Automated Tests: Add Functional tests for classes ExtraLinks and SearchLinks Needs review 😅
Some test files are very long with lots of cases because a lot of things are being tested (more than 300 assertions).

There is still more code that could be further tested, in particular, all the Javascript code.... But for now, this should give us a much more robust, complete and stronger base for testing the module with almost 100% of the PHP code being covered, on which we should be a able to keep building. 🥳

Re #14:
Sorry I was not aware you were not familiar with automated tests 😅
Thanks a lot for contributing this documentation: we should be able to direct users who would wish to contribute to testing these issues in case they are not familiar with the required setup 👌
 

Overall the tickets at #16 have almost 0 impact on existing code files, only test classes and files should be impacted.
📌 Remove unused Tools icon for Drupal 8 Active is just some suggested clean-up task and 📌 Automated Tests: Add Functional tests for classes ExtraLinks and SearchLinks Needs review changes slightly extra links order in the admin menu (for roles and menus)... so overall, it should be very safe to move forward with these changes.

A lot of the technical documentation in these tickets or in the code files is initially meant for me 😅
juts to remember some of the technical choices, implementations or problems identified along the way. 👍

With these tests in place, we should feel much of confident moving forward with code refactoring or certain tickets with big changes, such as 📌 GitlabCI: Fix ESLINT validation errors Active .
 

In any case, feel free to let me know at any point if you have any questions or concerns on any aspects of these tickets or related merge requests, I would definitely be very happy to help. 🙂
Thanks in advance!

Super nice @ressa, as usual! Thanks a lot for your help!

OK, here is a big batch of tickets ready for testing 🥳

• 📌 Remove unused Tools icon for Drupal 8 Active
• 📌 Automated Tests: Add Functional tests for classes ExtraLinks and SearchLinks Needs review
• 📌 PHPUnit: Improve tests for admin_toolbar_tools Active
• 📌 Automated tests: Simplify admin_toolbar_search FunctionalJavascript tests Active

 
This should allow the tests coverage of the module to be greatly increased so we should then be able to start taking on refactoring tickets 👌

Let me know if you spot anything, have any questions or suggestions, as usual, your feedback is always very helpful! 🙏

Thanks again for all your great help with all the tickets! 🙂

Quick follow-up on this issue:

All the changes detailed in the issue summary have been implemented and described in the merge request MR !175 above at #2.

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

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

dydave → created an issue.

Long overdue follow-up on this issue 😅
But since I've been working on fixing module's tests, I thought we might as well get this one done 👍

All the changes detailed in the issue summary have been implemented and described in the merge request MR !174 above at #4.
Added the necessary logic to change form settings values so the expected results could be tested. 👌

• Very basic check of an extra link ('All menus') added for 'menu_ui' to confirm the 'max_bundle_number' was properly updated.
• Test the display of the 'Local Tasks' tab and links when the 'show_local_tasks' setting is enabled, tests:
  \Drupal\admin_toolbar_tools\AdminToolbarToolsHelper::buildLocalTasksToolbar()

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

Overall, this merge request should complete the tests coverage for the admin_toolbar_tools module, along with related 📌 Automated Tests: Add Functional tests for classes ExtraLinks and SearchLinks Needs review . 🥳

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

Quick follow-up on this issue:

All the changes detailed in the issue summary have been implemented and described in the merge request MR !173 above at #2.

Mostly: added two new Functional tests files:

• admin_toolbar_tools/tests/src/Functional/AdminToolbarToolsExtraLinksCustomTest.php
• admin_toolbar_tools/tests/src/Functional/AdminToolbarToolsExtraLinksTest.php

with a generic method for testing all supported entity types through a centralized test matrix containing all the necessary information for each entity type to be tested defined in added test trait:
admin_toolbar_tools/tests/src/Traits/AdminToolbarToolsEntityCreationTrait.php

Entity type bundles currently tested:

• Block content type (block_content)
• Comment type (comment)
• Contact form (contact)
• Media type (media)
• Menu (menu)
• Content type (node)
• Taxonomy vocabulary (taxonomy)
• User role (user)
• View (views)

 
The idea behind the test matrix is to try keeping everything related to the entity types in one place, making it easier to maintain and update as needed, for example, adding or removing an entity type to be tested, or testing different 'field_ui' operations, etc...
See new trait: Drupal\Tests\admin_toolbar_tools\Traits\AdminToolbarToolsEntityCreationTrait
https://git.drupalcode.org/issue/admin_toolbar-3550604/-/blob/3550604-au...

The important thing to know is that the entity bundles are now dynamically generated with mostly two parameters controlling the tests, see in trait method 'setUp':
\Drupal\Tests\admin_toolbar_tools\Traits\AdminToolbarToolsEntityCreationTrait::setUp():
- The variable '$max_bundle_number' controls the configuration setting 'max_bundle_number' for the tests.
- The variable '$entity_bundle_ids_count' controls the number of entity type bundles to generate for the test. The greater its value and the longer the tests will take to run, since there will be more bundles to create for each entity type. Keep this value above the 'max_bundle_number' to fully test the expected behaviour of the module.
 

Mostly, the complexity of these tests stems from the heterogeneity of the code and logic in the ExtraLinks class... It's quite a mess actually 😅... with lots of exceptions and cases.

For example, the 'max_bundle_number' config variable currently only works with links added under 'admin/structure', but not with the ones added under 'admin/content' or not with config entities, such as Views or User roles... which doesn't really make sense ?!
In other words, if you limit the menu to 10 bundle items and have 15 content types, the 15 links will still be displayed under the 'Content' menu...
I had assumed this feature was initially meant to limit the number of bundle links displayed in the menu... 😅

I think we will definitely need to come back on this feature at some point and see if there could be any other potential solutions.... Or at the very least, try to fix/standardize a bit this feature.

Refactoring some code, for example ✨ generalize creation of 'add ENTITY' and 'add BUNDLE' links Needs review , would also help having fewer exceptions in tests and thus fewer properties in the test matrix.

The 'Add {entity_type}' links would also need to be standardized: Some entity types do not currently have an add link, such as Block content or Comment types ==> Part of the refactoring work 👌

Bonus: +1000 for TDD: Test Driven Development! \o/ 🥳

Thanks to the tests, a few "bugs" could be identified and fixed along the way:

\Drupal\admin_toolbar_tools\Plugin\Derivative\ExtraLinks:
- Standardized the display order of the 'Add entity type' and 'All types' links, which was not the same for menus.
Now all entity types have the same order: first 'All types', then 'Add entity type'.
- Standardized the display order of the extra links operations for User roles: 'Edit permissions', 'Delete'. The 'Delete' link is the last of the list in general.

\Drupal\admin_toolbar_tools\AdminToolbarToolsHelper:
- Added support for rebuilding extra links for 'user_role' and 'view' types when an item is created, updated or deleted.

\Drupal\admin_toolbar_search\SearchLinks:
- Added missing 'field_ui' operation link 'Manage permissions', copied and adapted from class 'ExtraLinks'.
- Standardized entity type label for extra links added for 'menu_ui': Replaced 'Menus' with 'Menu'.

In general, all the links are tested with url, link text, position and CSS classes, wherever possible.

The test class \Drupal\Tests\admin_toolbar\Functional\AdminToolbarToolsSortTest was removed in favor of the new \Drupal\Tests\admin_toolbar_tools\Functional\AdminToolbarToolsExtraLinksTest which does the same checks:
- Update and delete entity type bundles: Ensure the extra links are rebuilt when entity bundles are updated or deleted.
- Test the display order of the links.

Some weird interactions with the contact module's 'personal' default contact form: Since it does not have an 'Edit' route, it just doesn't display in the menu, even though it has other operations links... Maybe to be investigated in a different issue 😅
Therefore, for the time being, certain weights or label prefixes were used to control the display order of the entity type bundles created for the tests, see:
https://git.drupalcode.org/issue/admin_toolbar-3550604/-/blob/3550604-au...
 

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

Overall, this merge request would allow the module to greatly increase its Functional tests coverage to almost 100% 🥳
Which would be a great step forward allowing us to process more changes safely and more particularly, work on refactoring modules admin_toolbar_tools and admin_toolbar_search.
Not to mention the great help with the maintenance in general, with core version updates, deprecated functions, etc...
This initial base of tests would definitely be a great improvement! 👍

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

dydave → created an issue.

Quick follow-up on this issue:

All the changes detailed in the issue summary have been implemented and described in the merge request MR !172 above at #2.

Since all the tests and jobs still seem to be passing 🟢 (in particular stylelint), moving issue to Needs review as an attempt to get more testing feedback and reviews.

Overall, this merge request is really just a "routine" maintenance task trying to clean-up some old code and unused CSS rules.

For the time being, I would personally recommend moving forward with this merge request and removing any code that would not be effectively used in the module, for the core versions currently supported (YAGNI!).

Less code means fewer automated tests, a slightly lighter/faster CSS and module, and of course less maintenance work 🙂

Otherwise, if anybody has a better idea on which logo files could be used, we would be glad to hear it 👌

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

dydave → created an issue.

Thanks a lot @ressa!

Please keep the same testing protocol for all other tickets, it's super helpful! 🙏

After receiving your confirmation, I added a quick commit to ignore PHPSTAN errors that were unrelated to these changes 👌

 ------ ----------------------------------------------------------------------- 
  Line   admin_toolbar.module                                                   
 ------ ----------------------------------------------------------------------- 
  99     Property Drupal\Core\Menu\MenuLinkTreeElement::$options (array<string  
         >) does not accept array<array<string, list<string>>|string>.          
         🪪  assign.propertyType                                                
  100    Property Drupal\Core\Menu\MenuLinkTreeElement::$options (array<string  
         >) does not accept array<array<string, list<string>|string>|string>.   
         🪪  assign.propertyType                                                
 ------ ----------------------------------------------------------------------- 
 [ERROR] Found 2 errors   

Not sure why these errors are popping up now 😅.... Probably because there were some recent updates with the PHP or PHPSTAN versions or standards with the newer supported Core versions tested on Gitlab CI.
But since we were already ignoring the offsetAssign.dimType error above in the code, we might as well just ignore these assign.propertyType ones for now.
Note that this method introduces other errors for previous major core version (10.5.x):

 ------ ----------------------------------------------------------------------- 
  Line   admin_toolbar.module                                                   
 ------ ----------------------------------------------------------------------- 
  100    No error with identifier assign.propertyType is reported on line 100.  
  102    No error with identifier assign.propertyType is reported on line 102.  
 ------ ----------------------------------------------------------------------- 
 [ERROR] Found 2 errors    

So we're probably going to have to look into this a bit closer in a separate ticket 😖
Try to find a better way to conditionally support different PHPSTAN versions 😅

As a follow up, should the remaining assertMenuHasHref be switched to use this new solution?

Thanks a lot @ressa for being very thorough and taking the time to carefully review the code changes! 🙏

All the affected files are in the admin_toolbar_search module's tests and they are going to be completely revamped in the next tickets and merge requests.
So let's keep them as is for now 👌
 

For now, I went ahead and merged the MR above at #5.
Let's keep moving with the module's tests coverage, then more refactoring and tickets in the issue queue. 👍

More Automated Tests tickets and MRs coming up! 🥳
Thanks again very much for your great help @ressa! 🙏

Super nice @ressa! 🙏

Thanks a lot for the thorough review, it's greatly appreciated!

Following your confirmation, I went ahead and merged the merge request above at #6.

Let's keep moving, with the rest of the tickets for the tests... I've still got a few more big ones coming up. 👍
There is still quite a lot of work left. 😅

Then we'll finally be able to start makin changes and taking some more refactoring tickets, in particular for admin_toolbar_search. 😅

I'm going to get back to you on other tickets very shortly and add a few more ones related with Tests, for which I would also need your help👌
Thanks again very much for the prompt reply and great help @ressa! 🙂

More tests refactoring coming up!

Adding another issue ready for review to the list:
📌 Tests: Add trait AdminToolbarHelperTestTrait Needs review

Hey @ressa! 🙂
Whenever you get a chance, I would greatly appreciate if you could please take a quick look at these two Tests related issues.
They should be pretty much ready to go, so it would be great if they could get merged soon. 👌

Thanks in advance for your help and feedback! 🙏

Quick follow-up on this issue:

All the changes detailed in the issue summary have been implemented and described in the merge request MR !171 above at #2.

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

Overall, this merge request is really just a "routine" maintenance task trying to refactor and improve some old test code so it could be used by new test classes.

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

dydave → created an issue.

Super nice Tim (@tim-diels)!! 🥳

Thanks for your help and the credit on the issue! 🙏
Cheers!

Thanks everyone for the great help on this issue!

Quick update of the merge request, as an attempt to keep this moving along with testing and reviews:

• Reverted the last commit, which sort of broke the coding standards of the file.
• Fixed the remaining PHPCS and PHPSTAN errors manually.

I haven't really had the time to test this myself manually though, with the Project Browser module installed and all...

So testing, comments and feedback would be greatly appreciated!

Back to Needs review!

Thanks in advance!

Quick follow-up on this issue:

Since the changes required to fix the development pipelines were really minor, I went ahead and merged the changes directly above at #3 📌 Fix broken stylelint and phpstan jobs Active , so other merge requests could be rebased and tested properly.

Marking issue Fixed for now.

Thanks!

dydave → created an issue.

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

Super nice Peter (@jepster_)!! 🥳

Thanks for your reactivity and the credit on the issue! 🙏
Cheers!

Thanks Peter (@jepster_) for the prompt and positive follow-up on this issue, it's greatly appreciated.

Just a quick reply to let you know the merge request MR!47 was just updated with the changes suggested in the patch #3 🐛 Improving the verification of the user field in the AccessStorage service Active .

Leaving in Active for now, at the appreciation of the module maintainers.

Feel free to review and let us know if you have any comments, suggestions or questions, we would certainly be happy to help.
Thanks in advance!

Re-rolled patch from MR !47, for stable 3.1.37.

Uploading static patch file.

Thanks a lot Johannes (@ammaletu) for the positive feedback, it's greatly appreciated!

This will certainly help others with the same type of requirements and should be very helpful to us as an example when documenting related issue 💬 Change the deriver class of a menu link Active .

Glad this could work for you 🙂
Thanks again for reporting back and documenting your changes! 🙏

Thanks Tom (@tomtech) for granting us credit on this!

Cheers!

Super nice of you @ressa, as always! 🙏

I was able to make a bit of progress on the refactoring of the admin_toolbar_search module, as I mentioned previously and have created the first issue: 📌 Automated tests: Merge several Functional test classes Active .

This is mostly some code clean-up: trying to optimize a bit the files in the code base.
The only impacted files are modules' Functional tests, but it could be good if you could just take a quick look at the MR and ticket to see if everything checks out or if I could have missed anything.

Next on the list should be module's FunctionalJavascript tests 👍
Let's keep consolidating the Tests coverage before proceeding with refactoring modules' files 😅

Once again, let me know if you spot anything or would have any suggestions, your feedback if always very helpful 🙂
Thanks in advance!

Quick follow-up on this issue:

All the changes detailed in the issue summary have been implemented and described in the merge request MR !168 above at #2.

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

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

dydave → created an issue.

Thanks a lot everyone for raising this issue and for the initial work on the merge request, it's greatly appreciated.

I think a few things would need to be clarified here :

The access to the routes added by the admin_toolbar_tools module can be controlled with the permission 'administer site configuration', see:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...

However, there is currently nothing allowing to really control access to all the links added by the module in class ExtraLinks, see:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...
For example the links Add link, as mentioned in the IS:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...

The solution suggested in the MR is probably worth considering and and testing a bit more carefully.

But I'm wondering whether we could just add a check for the permission at the top of the method adding the extra links and just exit if the access condition is not met.
It would make sense to prevent any access to admin_toolbar_tools extra links, if the user does not have access to the toolbar and thus the admin_toolbar features, something like:
In https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...

  public function getDerivativeDefinitions($base_plugin_definition) {
    $links = [];

    // Only users with 'use toolbar' permission will see links.
    if (!$this->currentUser->hasPermission('use toolbar')) {
      return $links;
    }

Would we be hiding too many links ? Could there be any links we would still want to display on the index page ?
Could this make sense or correspond to the problem described in the IS ?

I'm not completely clear right now on the links we would like to keep and the ones that should be hidden.
But if the changes suggested in this comment could help fixing this issue, then it would probably be preferable to go down this road.

Once the necessary changes will have been clarified, we should be adjust the PHPUnit Tests accordingly, so the MR goes back to green.

I'm switching this back to Needs work as an attempt to get more information on the links that should be hidden and reviews or tests of the implementation suggested in this comment.

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

OK, no problem at all 👌

Let's keep releasing more regularly and we'll just postpone the next scheduled release.

This will take off some pressure and give us a bit more time to get some more issues included in the next planned release 👍

Therefore, I went ahead and created the new admin_toolbar-3.6.2 → stable release 🥳

Feel free to let us know if you spot anything else with recent core versions or some change records we may have missed, we would certainly be happy to take a look.
Thanks everyone once again for the great help keeping the module well maintained! 🙏

Thanks Marcelo (@marcelovani)!

can you please cut a new release?

Is this super urgent, or could it wait a few weeks, around the 20th of August ? (Planned date for the next release)

Thanks in advance!

Thanks Benjin (@benjifisher) for letting us know.

We'll wait to see what happens in the related issue before moving forward with the suggested changes.

Thanks again!

Thanks a lot @benjifisher for raising this issue and getting the work started on this!

Just added a quick comment to the merge request, could you please take a quick look when you have some time and let us know what you think?

Thanks in advance!

Thanks a lot @ressa, as usual for your promt and very positive feedback! 🙏

I'm really glad this idea made sense to you too and the changes worked as expected with your tests!

Honestly, I was a bit surprised to see this had not been done earlier, since it actually required very little code changes with few impacted files. 😅
So quite easy overall, but with a significant impact.

Following your confirmation at #4, I went ahead and merged the changes at #5 🥳

I'm quite happy we could move forward quicker with this task, since it unblocks some of the work I have been doing recently, trying to clean-up and refactor the Admin Toolbar Search module.

I should be following up shortly with another issue for the Tests and the module file, then another one for the JS code. 👍

I thought it would be better to break down these refactoring changes into smaller/related changes with specific issues and merge requests, so it would be easier to revert or roll-back in the future.

Once again, thanks a lot for your great help @ressa reviewing and testing these issues as always.
Make sure to let me know if you spot anything specific that would need to be addressed quickly, I would surely be very happy to help. 🙂

Thanks a lot @ressa and sorry for the late reply 😅

I have recently focused on a bit of refactoring for the Admin Toolbar Search module and would greatly appreciate some help, if you get a chance 🙏

I have added the following ticket to the list:
📌 Admin Toolbar Search: Remove dependency on Admin Toolbar Tools Active
which has a merge request, ready to be reviewed at anytime.
I've also tried to document a bit the changes, in terms of technical or user interface impacts.

I will most likely break down more "refactoring" into multiple issues, mostly around tests, the module file and the JS, for which your help would also be definitely warmly welcome 🙂

In any case, feel free to let us know if you have any feedback, suggestions or other ideas on the added issues, it would be a great help as always.
Thanks in advance!

Quick follow-up on this issue:

All the changes detailed in the issue summary have been implemented and described in the merge request MR !165 above at #2.

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

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

dydave → created an issue.

Removed the doc comment block of method:
Drupal\image_link_formatter\Plugin\Field\FieldFormatter\ImageLinkFormatter::viewElements
and replaced with an {@inheritDoc} block to directly inherit from its parent:
Drupal\image\Plugin\Field\FieldFormatter\ImageFormatter::viewElements

Fewer overridden doc comment blocks means less code and less maintenance for us.

The changes were merged above at #4 and the dev pipelines are back to passing 🟢 \o/
https://git.drupalcode.org/project/image_link_formatter/-/pipelines/551669

Marking issue as Fixed for now.

Thanks!

dydave → created an issue.

Crediting my colleague @tYb for his help fixing the version compare for the install routes.

Thanks a lot Sylvain (@johnatas) for the prompt and positive feedback, it's greatly appreciated! 🙏

Following your confirmation, I did a few more manual tests locally and since all the jobs and automated tests for the merge request were passing 🟢 , I went ahead and merged the changes above at #25 🐛 D11.2: update.theme_update and update.module_update no more exist Active 🥳
Great job everyone! 👍

As a follow-up to this issue, I created feature request ✨ Provide extra links for the Automatic Updates module Active as an attempt to provide an "equivalent" to these extra links, which should most likely be supported by the Automatic Updates → contrib module from now on.

At this point, since I don't see anything else outstanding, this issue could probably be considered as Fixed for now.

Feel free to let us know if we missed anything or if you would encounter any issues with any of the latest code changes, we would surely be glad to hear your feedback.
Thanks again everyone for the great work and help on this issue! 🙂

dydave → created an issue.

OK, I've tested this with D10.3.14 and the routes seem to be displaying in the same order before and after patch ✅
See screenshot below:

Upgraded to 10.4.8 and the install routes disappeared ✅

I didn't get any crash on the Edit admin menu page, since the existence of the routes is checked as well.

Therefore, I added an additional commit to update the version comparison for the install routes for 10.4, corresponding to change record The feature to install a new extension from a URL via the Drupal UI has been removed → .

I've tested the changes locally myself with D10.3, D10.4, D11.2.2 and they seemed to work as expected 👌

At this point, the merge request should be pretty much ready to get merged, but let's get some more testing feedback and reviews, if possible, before effectively merging it.
Thanks in advance!

The change suggested above at #7 was added to issue 🐛 Uncaught TypeError: toolbarElement.querySelector(...) is null Active and merged in with MR !163.

It can be scratched off our list 👌

OK, so I've checked these with a colleague and it would seem the install routes were dropped in D10.4 according to the change record The feature to install a new extension from a URL via the Drupal UI has been removed → .

I understand better now, why I could not find the routes when testing with D10.4, D10.5 and D11.

So we should also probably update this compared version in the merge request as well.

I'll do a bit more tests to see if my latest commit changed the order of display of the links.

Thanks a lot Stephen,

Indeed, I've been wondering the same thing here: #3532010-21: D11.2: update.theme_update and update.module_update routes no more exist → and found yesterday the change record:
The feature to install a new extension from a URL via the Drupal UI has been removed →
in which the routes have been removed, introduced in 10.4.x and 11.x.
In other words, we should be comparing with 10.4 and not 11.

We will most likely make this correction in the related issue.

Thanks again for reporting back.

I can also confirm seeing these deprecation messages in current build pipelines, see:
https://git.drupalcode.org/issue/admin_toolbar-3532958/-/jobs/5862610#L129

    1 test triggered 1 deprecation:
    
    1) /builds/issue/admin_toolbar-3532958/vendor/symfony/deprecation-contracts/function.php:25
    Since symfony/validator 7.3: Passing an array of options to configure the "Drupal\Core\Validation\Plugin\Validation\Constraint\NotNullConstraint" constraint is deprecated, use named arguments instead.
    
    Triggered by:
    
    * Drupal\Tests\admin_toolbar_search\FunctionalJavascript\AdminToolbarToolsSearchTest::testToolbarSearch (34 times)
      /builds/issue/admin_toolbar-3532958/admin_toolbar_search/tests/src/FunctionalJavascript/AdminToolbarToolsSearchTest.php:112
    

 
Would you have any advice on how these deprecation warnings could be addressed?

The error message is not very clear: would there be any code changes expected on our end?
See the line of code reported above:
https://git.drupalcode.org/issue/admin_toolbar-3532958/-/blob/3532958-35...

What should we change to fix these warnings in the code reported as deprecated?

Any pointers, related issues or recommendations would be greatly appreciated.
Thanks in advance!

@gugalamaciek:
Any chance you could help us roll-in your patch from #30 in issue's merge request MR !6200?

Apparently the merge request has some conflicts and would probably need to be rebased locally manually.

Thanks in advance!

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

OK, I thought while we were at it, we might as well check for toolbarElement:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/js/admin_too...

So I wrapped the whole block in a if statement, just like you did for the other ones.

Tested locally with the same trick mentioned above at #4 and it seemed to work fine: Fixed the error as well if the ID toolbar-administration is changed:

        document.getElementById('toolbar-administration').id = 'testing123a';

Same results as above.

Added an extra commit from: #3527462-10: Import of strings skipped due to malformed HTML tag
→ , see: #3525938-7: [Meta] Roadplan for Admin Toolbar 3.7 → .
And the .eslintignore file from 📌 GitlabCI: Fix ESLINT validation errors Active .

After fixing a Stylelint validation error, the tests on the Merge request came back green 🟢

Therefore, I went ahead and merged the changes above at #5 🥳

Since I don't see any follow-up to this issue at this point, it could probably be marked as Fixed for now.

Feel free to let us know if you have any comments, suggestions or concerns on any aspects of the committed changes or this ticket in general, we would surely be glad to help.
Thanks again very much Julian (@anybody)!

Thanks a lot Julian (@anybody) for raising this issue and contributing the necessary code changes to fix it 🙏

The resulting error is documented in the issue summary, but I would personally be quite curious to know how you were able to trigger it.

Could you please provide a bit more information on the context or setup and update the following section of the issue summary?

Steps to reproduce

I "guess" the IDs could have been changed in the theme, or with some other potential overrides, but I would have thought the dependency to the Toolbar module should ensure the IDs should be present in the HTML markup of the page (?!) 🤔

Otherwise, I have reviewed and tested locally the changes in the merge request and they seem to work fine, so great job! 👍

However, since I was unable to reproduce the error in the first place, I'm not sure whether the patch really fixes the issue 😅

Therefore, to force the error, I added a line of JS to change the HTML ID of the element:
For testing, added locally here:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/js/admin_too...

        document.getElementById('toolbar-bar').id = 'testing123a';

On 3.x got the following error:

drupal.js?v=11.2.0:64 Uncaught TypeError: Cannot read properties of null (reading 'insertAdjacentHTML')
    at admin_toolbar.toggle_shortcut.js?sywaq9:70:53
    at Array.forEach (<anonymous>)
    at Object.attach (admin_toolbar.toggle_shortcut.js?sywaq9:21:62)
    at drupal.js?v=11.2.0:166:24
    at Array.forEach (<anonymous>)
    at Drupal.attachBehaviors (drupal.js?v=11.2.0:162:34)
    at big_pipe.js?v=11.2.0:152:10
    at big_pipe.js?v=11.2.0:183:3

Tested on branch: 3532249-uncaught-typeerror from the MR and the error disappeared.
The toggle button is not displayed and the error seems to have been fixed.

We don't have any tests in place for this, so I don't see any more related changes that should be added to this MR.

Therefore, I'm going to add a quick unrelated commit to this merge request and get it merged ASAP 👌

Any feedback, comments or questions would be greatly appreciated!

Thanks again very much for your help Julian (@anybody)! 🙂

Not sure why though .... But I'm not seeing the update.module_install and update.theme_install links on D10.5...

I wanted to check whether my last refactoring commit changed the display order of the menu items, but I was unable to find the links Install new module or Install new theme added by the admin_toolbar module....

Would anyone have any feedback on that ?

Otherwise, I'll take a bit more time later to look into this.

Thanks in advance!

Thanks a lot everyone for reporting this issue and all the great help working on the code changes and tests.

Sorry for the late reply... But it's been a rough couple of weeks work-wise 😅

The changes from the merge request seem very reasonable and accurate to fix this bug.

I have just added a commit to refactor a bit the calls to version compare.
I have tested the changes locally on a D11.2 and a D10.5 sites and they seemed to work as expected 👌

I would greatly appreciate if you could please help testing and reviewing the merge request.

We don't currently have tests for this part of the module, so once we're good with these changes, we should be able to get them merged 👍

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

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

Just ran into this issue:

Downloaded the latest version of slick.js here:
https://github.com/kenwheeler/slick/blob/master/slick/slick.js

Which was modified last week and fixed the problem.

Thanks everyone for the great help and feedback!

Thanks a lot everyone for the great advice!

For those upgrading the module, updating the library constraint for the RC in project's composer.json fixed the issue for us as well:

"npm-asset/select2": "^4.1@RC"

Followed by a composer update (-W) command, or simply:
composer require npm-asset/select2:^4.1@RC

Make sure the minimum-stability property allows 'RC' versions though.

Is there anything we could do to make the update more automated? Perhaps in module's composer.json file?

Any feedback, suggestions, comments or reviews would be greatly appreciated.
Thanks again for the great help on this issue.

Re-rolled patch from #27 for D11.2.

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

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

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

Re-rolled previous MR for D11 and created new merge request MR !12527 above at #27.

The changes in this issue have not been switched to Needs review once in the past three years 😅
So let's give it a try!

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

dydave → changed the visibility of the branch 3212602-revisions-tab-show-revision-state to hidden.

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

dydave → created an issue.

Quick follow-up on this issue:
As suggested at #3, created the new merge request MR !10183 for D11 above at #5.

I was brought to this issue from a related ticket for the Admin Toolbar module:
🐛 zindex issue between admin toolbar and ckeditor 5 Active

I have tested this locally with Admin Toolbar and couldn't really see a difference with or without the patch.
In other words, calling Drupal.displace(); (in the console) seems to have the same result in both cases: The CKEditor Toolbar seems to get the correct offset calculated.

I have not tested with the steps suggested above in the IS, so I'm unable to really say whether this patch fixes the issue initially reported.

Setting this issue to Needs review anyway, as an attempt to get more reviews, comments or feedback.
Thanks in advance!

dydave → changed the visibility of the branch 3487446-ckeditor5-sticky-panel to hidden.

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

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

Back to Needs review after getting moved to RTBC at #21 to prevent MR from getting merged abruptly:

Please give us a little bit of time to review and test the changes. 🙂
But your feedback above and confirmation the changes worked as expected, were well received. 👌

Additionally, we'd like to squeeze in a few minor changes (wording, texts and such) in the next MR to get merged, so the opportunity to add another commit would be greatly appreciated by maintainers.

Any additional feedback, testing and comments would be welcome.
Thanks everyone for the great help on this issue! 🙏

Thanks Sandip (@sandip) for the recent feedback on this issue!

So i think if we cant increase z-index of #toolbar-bar so can we apply the same approach to its parent div which is #toolbar-administration ?
Like we can use position: relative and z-index around 99999.

Could you please try translating these suggestions into actual changes in the following branch?
https://git.drupalcode.org/issue/admin_toolbar-3335841/-/tree/3335841-fi...

Then try testing a bit with different types of settings for the Admin Toolbar module, browsing on various pages of the site.

That would definitely be very helpful 🙂
 

Otherwise, I personally think this issue is probably different from the one with the CKEditor5 Toolbar ( 🐛 zindex issue between admin toolbar and ckeditor 5 Active ).

But, at this point, it is still unclear whether a CSS or JS solution should be preferred.

It could still be worth exploring potential JS solutions as well:
For example, adding/removing CSS classes, altering tags styles, etc... on initialization, when the devel module is enabled and/or a Kint or Symfony vardump is found on page.

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

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

Thanks @neptune-dc for the recent feedback on this issue!

I took a closer look a few weeks back and was actually able to trace this down to the initialization of the view port offset of the CKEditor5 Toolbar....

In other words:
With the latest stable, for example 3.6.1, if you try reproducing the problem below:

Then, try resizing the window (without refreshing, just a small change of the window size):
If you test again, you should see the toolbar displays at the expected position right underneath the Admin Toolbar.

Therefore, it somewhere boils down to the initialization of some JS settings variables and/or to a "resize" event triggered.

See potentially related issue 🐛 element hangs, when Toolbar is hidden Active and Drupal.displace();

Maybe approaches with JS instead of CSS should be further explored.
It is likely this issue could be fixed with a few lines of JS code (?!)

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

Small change to add in one of the next MRs to be merged:
#3527462-10: Import of strings skipped due to malformed HTML tag
→

A single lingering untranslated string was found in the interface, which is "milliseconds" here:

$ grep -rinoE '.{0,10}millis.{0,10}' .
./src/Form/AdminToolbarSettingsForm.php:136:ffix' => 'milliseconds',

It just needs to get wrapped in t(), and all is well:

'#field_suffix' => $this->t('milliseconds'),

See source code:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.6.1/src/Form/A...

Super good catch @ressa! As usual! 🙏

Indeed, it appears we missed this string 😅
milliseconds
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.6.1/src/Form/A...

Let's try to add this change to the next merge request that could get merged 👌

Great job on the translations! 🥳
Thanks a lot for the feedback as well on the localization work, I hope this could serve as documentation for others to find their way through the localization system on Drupal.org.
I've followed your link myself and submitted more than 20 French translations suggestions 👍
We've got a bit of work, but looks like we're not the worst 😅

Thanks again for all the great help keeping the module well maintained @ressa!

benjifisher → credited dydave → .

Thanks a lot Michael (@justcaldwell) and @erutan for the great work on this issue! 🙏

I tested the changes locally once again and the keyboard shorcuts worked fine, on my PC laptop, just as they did before.
I couldn't test the changes for MacOS myself, but trust that you have been testing them yourselves successfully.

Since the tests and jobs all seemed to be passing 🟢, I went ahead and merged the changes above at #17. 🥳

Let's settle for this patch for now and as mentioned above, try improving this feature in related ✨ Allow shortcut key to be configurable Active .

I've added your comments and suggestions above at #13 (point 3) and #15 (point 2) to the related issue so we could hopefully circle back on these comments and try to include these changes in the next merge requests.

Since we've covered the follow-up tasks for this issue, at this point, it should most likely be considered as Fixed for now.

Feel free to let us know if you have any questions or concerns on any aspects of the latest code changes or this ticket in general, we would surely be glad to help.
Thanks again both for your great help! 🙂

So nice @ressa, as usual, of you to keep the ticket well documented based on the work from other issues 🙏

Adding here a few additional helpful comments and suggestions:

#3527344-12: New keyboard shortcuts do not work on MacOS → :

3) More advanced mac users will know that alt = option / ⌥, but ideally it'd show "⌥ + a" instead of "alt + a". If this would be trivial to change based on parsing user agent strings that'd be useful. More of a minor/optional QoL issue.

#3527344-13: New keyboard shortcuts do not work on MacOS → :
Issue summary:

Side note: When the search shortcut is disabled in the modules settings, the search input placeholder text still includes "(Alt+a)". Probably needs it's own issue.

I agree that the shortcut "tip" text could be improved — it shouldn't even be there if the shortcut has been disabled

#3527344-15: New keyboard shortcuts do not work on MacOS → : Concerning the form settings for the hide/show toolbar shortcut:

it is a little odd it's tucked at the bottom of "Toolbar sticky behavior", since it's only tangentially related to that. Either the header should be something like "Toolbar visibility" or it should be in it's own section. It also wouldn't be terrible if the keyboard shortcuts were just in their own section instead of below what they're related to.

Let's see how we could organize and include these changes in the next merge requests.

Thanks again everyone for all the great help!

Great job Michael (@justcaldwell) and @erutan on this! 🥳
Thanks a lot! 🙏

The changes look good at this point and consistent with my comment above at #10.

Moving this back to Needs review to prevent the MR from getting merged abruptly.
(But we're clear: the issue was marked RTBC by @erutan above at #15 👍)

I will review and most likely merge this within the next 24 hours.

In the meantime, feel free to let us know if you see anything else that we could have missed or would have any questions, we would be surely be glad to help.
Thanks in advance! 🙂

Requalifying as Feature request since this is not actually preventing the proper use of the module.
It should maybe not be considered as a problem currently, but rather an improvement.

Additionally, the changes from merge request MR !53 don't seem to work anymore:

I didn't manage getting the chevron icons displayed locally by adding the piece of CSS to my code locally.

Thus moving issue to Needs work.

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

Thanks a lot Michael (@justcaldwell), once again for the very helpful and constructive reply!

Thank you very much for documenting and testing very carefully the changes, with various keyboard layouts and doc links, it will certainly be very useful when we try to approach the related feature request.

No problem at all for getting this change added before, as well 🙂

We would definitely want to provide support for MacOS users, especially if this could be achieved with very few changes 👌

OK, concerning the MR:

Based on your tests and comments above at #7, I would expect the changes to the checks for the keys would work on your setup and they do as well on mine, so great job! 👍

One thing though: The FunctionJavascript Tests for the Admin Toolbar Search seemed to fail with the MR. 🔴
See pipelines: https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/154/pi...

So I cloned the branch and tried debugging the build and identified the problem was caused by:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/154/di...

Removing this particular line seemed to fix the Tests and the MR came back green 🟢

Going back to #4:
Do we really need to the calls to:

            // Prevent transmitting keypress.
            event.preventDefault();

I only removed the one from the Admin Toolbar Search keyboard shortcut, but I "think" this was probably one of the main reasons you added these calls initially (to prevent the key press from being recorded by the input field)....

So I'm not sure exactly how to approach this problem, at this point, would you have any ideas or advice?

Could we maybe add a test or check for MacOS users?

Or maybe we would really need this call, probably revert my commit and try to fix the broken Tests instead and find out the reason they could be failing?

Feel free to trigger more Tests and make more changes to the debugging branch, it is very practical for triggering and debugging broken builds:
https://git.drupalcode.org/issue/admin_toolbar-3527344/-/tree/3527344-ne...

Based on your feedback we should be able to move this issue forward and hopefully get a stable solution very soon 🤞
Thanks in advance!

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

dydave → changed the visibility of the branch 3527344-new-keyboard-shortcuts-debug to hidden.

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!

Nice work Tirupati (@tirupati_singh)! 👍

Thanks a lot for taking the time to understand the issue, test it, fix it and make some suggestions on potential improvements, it's super appreciated! 🙏

I definitely appreciate knowing I'm not the only one in the project following the different on-going issues.

Overall, I think you broadened a bit the scope of the issue, where we had probably not taken into account testing the vertical toolbar. 😅

I agree as well on restoring some background color contrast, but in this initial version of the merge request, I thought we could test resetting the focus styles first, see if it works: I would like to confirm this is what Charles actually meant by resetting the focus styles.

Then, discuss again with @Charles Belov and @mgifford so they could give us more recommendations on the changes, in particular, for the colors (back/fore-ground).

I would "guess" it would be a combination of the first commit (with the changes to the colors) and the following ones with the reset of the focus styles, but at least everything is in separate commits in the MR 👌
 

But at this point, we are mostly waiting on @Charles Belov feedback since he has the ability to test precisely the focus styles, the display in general and has been giving us great advice on this issue.

Any additional feedback, comments or suggestions on the merge request or this ticket in general would be greatly appreciated.
Thanks in advance!

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!

Super nice! As always!

Thanks a lot @ressa! 🙏

Following your confirmation, I went ahead and merged the changes above at #8 🥳

That's one more issue to be release in the upcoming 3.6.1 patch version 👍

Let's keep working on the other few issues so we could create a new stable release.
Thanks again very much @ressa for the great help! 🙂

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!

Thanks a lot Charles (@Charles Belov) and Mike (@mgifford) and sorry for the late reply, but I have been quite busy lately with the 3.6.0 release and mostly fixing/answering more urgent requests.

Based on your suggestions at #14:

Not to override the browser's focus indicator

I made a few changes to the patch/merge request to:

• Revert the changes to the color or any focus styles on the links.
• Revert links styles to use User Agent's (UA) default indicator.

I have tested this locally, moving the focus with my keyboard tab key and the indicator would seem to display as an outline box around the links, in my Chrome desktop browser.

Once again, we would greatly appreciate if you could please help us take a look at the updated patch and give us your reviews and feedback.

Do you think we should still keep some styles with the colors? (background/foreground)
Or just the outline styles could be enough?

Lastly, I would like to attract your attention on another related issue that was recently created:
✨ Use the Accessible Menu library to manage the menu structure Active
which gave me a lot to think about on the direction the module should be following in terms of accessibility support in general.

In short, the issue suggests switching module over to using the Accessible Menu CSS/JS library:
https://accessible-menu.dev/

which would be a major improvement in terms of accessibility, mostly covering ✨ Tabbing order does not satisfy 508 accessibility requirements Active and probably other issues.

We have only just started investigating that issue as well, but we would be very happy to hear what you think about this library and idea.

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

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

The changes suggested in the patch in the issue summary have been added to the merge request MR !159 👌

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 solution, 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. But if getting this patch committed could help with your projects, we would certainly be glad to do so 🙂

We would greatly appreciate if you could please try testing merge request MR !159 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!

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

Thanks a lot @ressa, once again for the all the help testing and reporting in issues! 🙏

Sorry for the late reply on this, but I've been handling other issues in the queue as well 😅

Thanks a lot for catching this issue, documenting it so well and for providing the solution in the merge request.
I have done a search in the code base and could not find any other occurrences for <br/> after applying the patch.

I have added a quick commit with very minor text edits:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/155/di...
Mostly to roll-in the changes from:
#3526123-4: element hangs, when Toolbar is hidden → and #3304810-72: Toggle away Admin Toolbar completely →

Would you see any more textual elements that should be adjusted/improved, before releasing a 3.6.1 patch version?

Otherwise, whenever we could have your confirmation on the latest text changes, the MR should be good to be merged 👍

Over to you: Needs review!

Thanks very much in advance! 🙂

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!

Thanks @heddn:

The config has moved, see 📌 Move the 'hoverintent_functionality' setting to admin_toolbar Active .

Moved out from admin_toolbar_tools to admin_toolbar.

Could running DB updates help fixing the issue?

Affected version: 3.6.0.