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: 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 have started a bit of code review, but overall the changes looks good! I'm wondering if this could be a solution as well for
π
zindex issue between admin toolbar and ckeditor 5
Active
(?!) 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). There could very well be something specific with my local environment.... 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? Additional feedback, testing, reviews and comments would be greatly appreciated. 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. Great job @megakeegman! π₯³ Thanks a lot for your time, patience and great help with all the testing! π I made a few minor changes to the MR just to keep a few lines from the reverted commit. Great catch, once again and thanks a lot for raising this one so shortly after the release so at least this major problem could be fixed for other users π There are still a couple more issues to go, but this should get released in an upcoming 3.6.1 patch version very soon (I'm hoping next week max / if possible this week π). oh ... btw: requalifying as a bug report π I don't really see any follow-up issue to this: The code from this MR should just get removed (the deprecated function) when support for versions below 10.2 or 10.3 is dropped anyway... so an overall clean-up should be done at that point. Therefore, marking issue as Fixed for now. Feel free to let us know if you would encounter any other issues or would have any suggestions or questions on the module, we would surely be glad to help. Thanks @megakeegman! Reviewing and testing locally right now. Thanks a lot @megakeegman for the MR! Were you able to test the changes on your setup? Otherwise, the tests and jobs are all passing π’, so when we could get your confirmation, we should be able to merge the MR ASAP π Thanks in advance! Re #9: No problem @megakeegman! I "think" we're probably going to want to keep the changes in the neon file, but I'm not sure exactly. I guess the most difficult part is probably the testing, as you mentioned at #6. Feel free to let us know if you have more questions or how we could help, we would be glad to reply as soon as possible.
Anyway, I guess the most important takeaway of this message is that it is slowly becoming increasingly difficult to update a Drupal 9 site that has contrib modules installed.
Totally agree @megakeegman: This is mostly due to the fact that Gitlab CI does not run build pipelines for 9.x anymore (by default). Another improvement could be to add the necessary configuration options to the Gitlab CI file of the module to test 9.x as well. Thanks @megakeegman for the prompt and very helpful feedback!
If I understand correctly, DeprecationHelper class was introduced in Drupal 10.2,
raaaaa... Great catch! π
Looking at the issue that introduced this piece of code again: We're using a method/class introduced in 10.2 It doesn't make any sense π
OK, so, indeed, we probably have two choices at this point: 2 - Move forward and drop support for versions lower than 10.2 or 10.3 so we could provide a minor release with a sliding window allowing to upgrade core to 10.3, then further upgrade the module and core to D11, something like that. If option 1 could work for you and you think it could provide an acceptable solution for the time being, then could you please help us create the corresponding revert merge request and maybe try testing it on your setup? Please let us know if you have any other ideas, suggestions or questions on this particular issue or the module in general, we would surely try answering as soon as possible. Thanks a lot Alberto (@avpaderno) for taking the time to look at this. π Thanks @megakeegman for raising this issue. π Could we potentially requalify this as a Task instead of a Bug report? Ideally, we would like to keep the bugs count focused on the actual bugs/problems/issues of the releases that are currently supported, in order to give users a more accurate vision of the current level of stability of the module. Feel free to let us know if you would have any objections, concerns or questions this particular comment or the project in general, we would surely be glad to help. Thanks a lot @dgwolf for the prompt and positive feedback! No problem at all π Since Kris (@kriboogh), the maintainer of the admin_toolbar_content module merged the patch and released a new version of the module, we should be able to consider this issue Fixed for now. Thanks a lot to Kris for moving on this so quickly: I certainly didn't expect his reply to be so quick π
Feel free to let us know if you encounter any other issues or would have any questions on the module in general, we would certainly be glad to help. Wow Kris (@kriboogh), thanks a lot for the very quick feedback and follow-up on this! π₯³ Since the module is marked Minimally maintained, I was certainly not expecting this fix to be merged so quickly. Thank you very much for the prompt, positive feedback and for creating a release right away. Thanks also for granting us (@dgwolf and I) the credits on the commit and this issue.
dydave β
made their first commit to this issueβs fork. @dgwolf: This issue was also reported in the
Admin Toolbar Content β
module, see: Could you please try testing again with the suggested patch for Could we maybe try fixing this issue in the
Admin Toolbar Content β
module instead? Otherwise, as a "personal" advice: Therefore, if you have a choice, I would recommend to try moving away from the Admin Toolbar Content module to another solution, since the module doesn't necessarily seem to be very well maintained. Since a solution was provided in this comment, moving issue to Needs review for now, as an attempt to attract more attention, reviews, testing and reporting feedback. Feel free to let us know if you have any questions on this issue or the changes suggested in the merge request, we would certainly be glad to help. Quick follow-up on this issue: Created the initial merge request MR !37 above at #2 which : Converts the After testing the changes locally, the error disappeared and the expected menu items seemed to display properly under Content, for example: this patch should fix the fatal error.π Moving issue to Needs review for now, as an attempt to attract more attention, reviews, testing and reporting feedback. Feel free to let us know if you have any questions on this issue or the changes suggested in the merge request, we would certainly be glad to help.
dydave β
created an issue. Created initial merge request MR !156 above at #13 based on latest patch at #12. The MR in related core issue would also require some help updating and testing. Could we requalify this issue as Feature request? Any feedback, comments or suggestions would be greatly appreciated. @alex.skrypnyk: Sorry again for the additional work and noise on the updates. A fix was committed in the DEV branch in issue
#3527315-8: in admin_toolbar_toolbar_alter(), Trying to access array offset on value of type null β
and should be released in a patch version hopefully soon, after a few other "urgent" issues have been addressed (as many as possible). We will try to be more careful with these types of error cases in the future. Thanks Mathew (@minoroffense) for suggesting this feature! I thought it would be worth mentioning the contrib module
Accessible Menu β
in the issue summary, since it provides a very quick and concrete way to immediately test the library, for anyone interested in exploring what an integration with Drupal could look like. Thanks in advance for your feedback and comments! Thanks a lot Michael (@justcaldwell), that's very helpful! We didn't really carry any real testing with MacOS when working on these keyboard shortcuts and since we knew there would be issues, we added a configuration option to disable them, if needed, for the time being. There is already a follow-up issue for this scope of work, see
β¨
Allow shortcut key to be configurable
Active
:
Allowing users to configure shortcut, either through the interface or at the very least with some kind of override in code.
Is this something we could trying moving to the Feature request instead and group our development efforts? Otherwise, concerning the merge request and the use of We already tried this implementation and encountered the following issue
#3494646-5: Access Admin Toolbar search field via keyboard shortcut Alt + a β
:
4 - But most important: Changed the condition So it felt like the Good thing we have different types of keyboards, regional settings, languages, etc... π
So it definitely seems there is more complexity to try matching a specific keyboard combination. The current shortcuts are going to work fine in many cases (default) and probably not in other ones with conflicts for various reasons (browser plugin shortcut, language, keyboard type, etc....), but as long as they could be disabled, for the time being, it should not be a problem with the module. Any feedback, comments or advice would certainly be greatly appreciated. Thanks a lot Peter (@pwolanin) for taking the time to look at this and give us some advice. When we first encountered this issue in
#3440852-26: Make un-hover delay configurable β
, I suggested to @ressa to test with the update hook, which fixed it, but we didn't anticipate on other types of issues. We will try to stick to this approach in the future if there are more configuration changes with update hooks, to avoid as much as possible these types of issues. For the time being, I went ahead and merged the changes above at #8. This will most likely be released as a patch release, along with several other issues, if possible, in a 3.6.1 corrective version, for example. Marking issue as Fixed for now. Feel free to let us know if you spot anything else or if we missed anything, we would certainly be glad to help. Thanks Peter (@pwolanin)! Any chance this could be related to
π
Warning: Trying to access array offset on null in admin_toolbar_toolbar_alter()
Active
? Thanks in advance!
dydave β
changed the visibility of the branch 2.1.x to hidden.
dydave β
changed the visibility of the branch 3314213-add-open-by to hidden. Re #12: Created merge request MR !10 targeting 2.1.x above at #19 based on the work from previous merge request by @aaronpinero. I have not had the time to review the code more than that... See a few screenshots of the results below: CKEditor: Configuration of a details section to be open or closed by default: CKEditor: View source, see the markup generated in the background: Text editor configuration form in the admin backend: At this point, it looks like all the points that were requested above should have been addressed in this merge request. Keeping this issue in Needs review for now, as an attempt to attract more attention, get more feedback and reviews. Feel free to let us know if you have any questions or concerns on any aspects of this comment, the merge request or this issue in general, we would surely be glad to help.
dydave β
changed the visibility of the branch 3.x to hidden. Thanks a lot everyone for the great work on this issue, all the contributed patches and comments! π Looks like there is already a lot of history in this issue, going back to 2017 π
Let's try summing up the current status: Currently, this issue contains two different implementations to achieve a similar goal: Improve the way dropdown menus with many items are currently displayed, so they could be visible. 1 - Multi-column menu dropdown: MR !150 Patch MR!150 displays long lists of items in dropdown menus broken down into multiple columns, so they could be displayed horizontally and fit the viewport, see a screenshot of the result: 2 - Scrolling menu dropdown: MR !151 Patch MR !151 displays long lists of items in dropdown menus with a fixed height and a scrollbar, to be displayed when scrolling up or down the menu dropdown, see a screenshot of the result: At this point, I have not started any kind of code review or careful testing of any of these merge requests, but only very superficial testing locally and they both seemed to work very well π OK, first of all, we need to clarify what implementation should be chosen in this issue: What solution should be implemented exactly? Solution 1 or 2 or something else? Keep in mind, it is always possible to make any of these solutions configurable through the backend, for example, to enable/disable or select one method or another (select list, checkbox, radios, etc...). But if we would like to make any kind of serious progress here, we should start by having a clear vision of the results that would be expected by the changes in this issue. What would be your suggestions on this? Otherwise from a coding standpoint, MR !150 is much less intrusive than MR !151 which changes the HTML structure and classes of the menu with a twig template override.... Keeping this issue in Needs review for now, as an attempt to attract more attention, get more feedback, reviews of the new merge requests and replies to the questions in this comment. Feel free to let us know if you have any questions or concerns on any aspects of this comment, the merge request or this issue in general, we would surely be glad to help. This is great stuff Mathew (@minoroffense)! π Thanks a lot for sharing this! π Recently, I have been searching around, looking for a good Vanilla JS alternative to the current custom scripts based on the hoverIntent JQuery plugin.... But up until now, I couldn't find anything that could look like an exploitable solution. Additionally, I have been looking at
β¨
Tabbing order does not satisfy 508 accessibility requirements
Active
, which seems to basically implement with custom code exactly what the accessible_menu library does π So essentially, if I understand this right: we "could" replace entirely (almost) all the JS and CSS that is currently used by the module to: Thanks a lot everyone for the great work on this issue! Quick follow-up on #5: I've created an initial merge request above at #13 for the 2.1.x branch, based on the patch above at #9: Understandable, thanks a lot Benjamin (@mlncn)! I've create a follow-up issue to address all the validation errors on which Wylbur (@wylbur) has been working, see: and moved all his work to MR !7 π Any chance we could consider this particular issue Fixed, get credits assigned and move to the other issue? Any feedback, comments or questions would be greatly appreciated. Quick follow-up on this issue: 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:
dydave β
created an issue. Super good catch @ressa! π₯³ Thanks a lot for the very clear ticket: 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! π
Thanks a lot Thom (@thomwilhelm)! Looks like we have the same understanding of the complaints brought up in this issue:
this doesn't seem related with Admin Toolbar specifically, but rather with the way site administrators maintain and deploy their configuration on their sites.
Sure @ressa, no problem at all π Of course we're going to release "patch" versions of the 3.6 branch if there are any urgent issues to fix π I have created this issue mostly for
β¨
Tabbing order does not satisfy 508 accessibility requirements
Active
which is a very big ticket with a large amount of changes .... so I thought that could potentially require another new minor version. But in any case, I wouldn't mind at all changing the target version, the title of this issue, or anything in the coming scope of work π I just created this issue really to try to get a list of priorities organized for the coming months. Feel free to make any changes, suggestions or modifications to this issue or the suggested scope of work, we would be very grateful for your help, as usual π I mean ... Alex, this doesn't seem related with Admin Toolbar specifically, but rather with the way site administrators maintain and deploy their configuration on their sites. This seems like a very generic problem, which is most likely the case with all other contrib modules and core π
As far as the module is concerned, we "should" be able to consider everything was done in a very standard way. Closing in favor of
π
Warning: Trying to access array offset on null in admin_toolbar_toolbar_alter()
Active
Thanks everyone! The way we usually do this in our projects: Update the code base locally: This is always the standard procedure that we follow for any core or contrib upgrade and ensures any module is properly updated with its configuration inline before it gets imported. I mean .... if it's like that Alex (@alexgreyhead) we are never going to be able to make any changes to configuration objects in general π
We have to be able to update configurations in our projects, otherwise we don't do anything anymore π
I'm not a big fan of getting this patch applied as it adds "unnecessary" code in my sense.... @dgwolf: It seems you are using the contrib module
Admin Toolbar Content β
(see Sorry, but I didn't have time to test this myself, but it would be worth looking into: Just make sure you have the latest versions installed in general, for this module as well... maybe this could help fixing the issue in your case. Otherwise, I'll try taking a look a bit later when I have more time. Thanks in advance! Thanks @nojj for the prompt feedback and glad it worked!
I thought it would be bad practice, to require a specific version as this would not be updated to e.g. V 3.5.4
Not really no.... it's OK to use a fixed version, we call this "pinned", since in some cases your site might be running a bit behind certain versions. Thanks again for the feedback! Fantastic! Great catch Guillaume (@guilhom)! I have just added the following paragraph to the 3.6.0 release notes:
Important: Support for older versions of the
Project Browser β
module has been dropped: Please upgrade to the latest version or at least the 2.x branch. Hope this helps others not to go through the same types of errors. Thanks again for your interest in the Admin Toolbar module. Thanks a lot @star-szr for your help answering these "support" requests. Closing this in favor of
π
Warning: Trying to access array offset on null in admin_toolbar_toolbar_alter()
Active
. Cheers! I don't understand ..... Site administrators are not running database updates after updating their sites anymore? It has always been very standard to run DB updates after a site upgrade, whatever is being updated, whether core or contrib... Running DB updates should update the necessary configuration values, see: which should prevent this type of errors from occurring. OK, no problem, thanks @nojj Could you maybe try the following command? Thanks in advance! Any chance you could upgrade the project browser module in your project? Thanks in advance! Thanks a lot @ressa for catching this so quickly, before we got a storm of angry messages π
I've set the 3.4.x release unsupported instead, so we only show the last two releases supported on the project page: Let me know if you catch anything else, I will be monitoring the issues closely and try to answer as soon as possible for urgent requests π Marking issue as Fixed for now. Thanks again for your great help @ressa! π Thanks @ressa! Sorry for the noise with the emails: I've made the change immediately on the administer releases page. Could you please double check again and see if it is better now? Thanks in advance!
dydave β
created an issue. That's it @ressa! π₯³ The new
3.6.0 release β
is out! π€© This is probably when we really start receiving feedback on how we've managed breaking the module on sites π
π Otherwise, I tried writing a bit of documentation on the release, with a short summary on the maintenance tasks and bug fixes, but a big highlight on the added features π I thought it would be good to let everybody know about the new keyboard shortcuts and the pretty cool slide in/out sticky behavior. I've also added a paragraph on some of the BC breaking code changes... Next time, in such cases, I will probably issue a change record, it will probably be easier π
25 issues with 19 contributors over 3 months!! It's enormous !! ππ₯³ Really happy we could work together on this @ressa and this release would certainly not have been possible without all your great help, guidance and cooperation π Thanks again so much for your help working with other contributors and I, through rounds of changes in merge requests, your reviews, advice, discussions, issue summary updates, etc... Very happy this release is now behind us, so we are now able to start looking at the next sprint/release, with already a bunch of tickets lined up in the issue queue, in particular: accessibility improvements, ESLINT fixes (let's get this in ASAP in DEVπ), compatibility with Gin, more configuration settings, etc... Lastly, I'd would like to thank once again very much all other contributors with whom I had the great chance and pleasure to interact and work on the issues included in this release. I definitely learnt a lot and met great people with whom to share the same passion for Drupal and Open Source development π€ Marking issue as Fixed for now. Feel free to let us know if you have any questions or concerns on any aspects of this ticket, the latest
3.6.0 release β
or the project in general, we would certainly be glad to help.
dydave β
made their first commit to this issueβs fork. Great job Tanner (@tanner.svg)! Great catch! The patch works great! The changes in the merge request MR !6 remove the code that was added in issue
#3111124: Force inclusion of drupal.collapse shim β
, corresponding to commit: See file history: The code was added 5 years ago, probably to support older versions of Internet Explorer that could maybe not be supported anymore. Bumping issue to Major, as this is definitely a significant performance improvement for the module. Moving to RTBC, as I would personally be in favor of dropping support for these older browser versions, if this could result in a significant performance improvement. Feel free to let us know if you would have any questions on this issue or the merge request, we would surely be glad to help. It seems development has moved to the 2.1.x branch now, therefore: Created the new merge request MR !8 above at #11 based on the previous merge request but rebased for branch 2.1.x. Updated the previous merge request to be hidden. This MR could use a round of textual/content review and overall testing. Any feedback, comments, reviews or suggestions would be greatly appreciated.
dydave β
changed the visibility of the branch 3336825-hookhelp-is-missing to hidden.
dydave β
made their first commit to this issueβs fork. Closing in favor of
π
Automated Drupal 11 compatibility fixes for ckeditor_details
Needs review
. Closing in favor of
π
Automated Drupal 11 compatibility fixes for ckeditor_details
Needs review
. Martin (@mandclu), could this issue be closed or fixed now? Thanks in advance! Quick follow-up on this issue: Created the merge request MR !7 above at #2 which adds an initial Added the bare minimum configuration to enable automated build pipelines with all validation jobs: This is left completely at the appreciation and preference of the project maintainers: feel free to adjust the file as you would see fit. The merge request build came back with errors for all jobs, but fixing them should be considered out-of-scope in this issue. Bumping issue to Major as this task should be very straight forward and unblock the validation of all pending merge requests. Moving issue to Needs review as an attempt to attract the attention of project's maintainer and collect more feedback.
dydave β
created an issue.
dydave β
made their first commit to this issueβs fork. Yes indeed Charles (@charles belov): Otherwise, your feedback on this particular patch is very helpful and constructive as usual, thanks a lot! No problem: We could consider changing the font color to a darker #000000.
[added] Actually, thinking about it, that's one more reason to use the browser's default focus indicator rather than coming up with your own.
So essentially, we would be reverting or resetting all styles for the focus of the menu items? Once again, all your suggestions and advice are more than appreciated! π Thanks so much Charles (@charles belov) for the feedback once again!
Please note my comment on Focus indication clashes with active tab indication. There is code in patch 138 which conflicts with the patch 149 for that issue.
Yes indeed: these are different issues currently under development, so they are very likely to have conflicts and there is not much we could do to fix that. But you shouldn't be applying both patches at the same time to test both issues, there are great chances it won't work. If we take out the problem with the styles of the focus: What would be your general feedback on this patch: In particular, all the work that was put into changing the tabbed navigation and the display, etc... ? There are a lot of things that could be tested in this issue, putting aside any other known issues that could be addressed separately. Could we perhaps try focusing on what we already have in this patch? I personally think it is a great improvement: Please let us know if you encounter more issues while testing or if you would have any questions or concerns on any aspects of this issue, we would surely be glad to help! @charles belov: Was your comment at #9 meant for this issue or rather
β¨
Tabbing order does not satisfy 508 accessibility requirements
Active
? It's a bit confusing... Thank you so much Charles (@Charles Belov) for taking the time to test again this issue and for all the advice and suggested changes. Thanks a lot for translating this issue into actionable technical changes. Based on your suggestions above at #7, I created the initial merge request MR !149 above at #8 with the following changes: Could you please try testing these changes and let us know if we missed or misunderstood anything in your last reply?
- Active is bold and underlined. I do find it odd that it's shown as a link (underlined), since it's active and I can't tab to it. That might be a separate issue.
OK, no problem π Moving issue to Needs review for now, as an attempt to collect more comments, reviews and feedback on MR !149. Feel free to let us know if you encounter any issues while testing the patch or have more suggested changes, we would surely be glad to help.
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 β
I've tested this locally and it seems to work pretty well!
Great job!
Great find with Drupal.displace(); π
(Same issue, with the CKEditor toolbar / a problem of offset calculation)
No additional change required and it seemed to work fine for me with Claro on D10.4.
Not sure exactly, but from my initial round of tests, the patch seems to fix the issue.
In particular with the Disabled behavior, if possible.
Thanks in advance!
I did some testing locally as well and since all the tests and jobs of the merge request were still passing π’, I went ahead and merged the changes above at #20 π₯³
You were right: given the current supported versions, this is definitely a regression, so it should be counted as such in module's stats.
Thanks in advance!
This should be merged within the next hour.
Did it fix the issue?
Thanks again very much!
If you create a revert merge request, the jobs should run anyway and we should be able to revert or make any other changes to the MR, as required to fix the PHPSTAN errors.
Thanks in advance!
In fact, only the current two supported majors are really tested, currently 10 and 11.
But that would probably be a "nice-to-have" at this point (?!)
π
Automated Drupal 11 compatibility fixes for admin_toolbar
Active
It doesn't make sense really π
https://www.drupal.org/node/3379306 β
to support versions lower than 10.2???!!
1 - Revert the corresponding change so we still support version lower than 10.2
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/136/di...
which would be the most straight forward, immediate solution, just creating a revert merge request for:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/136
So provide support for 10.3 and 11
This would probably take much more time and involve a lot code changes, mostly moving on to new APIs, service declarations, removing a lot of BC code, etc....
This could definitely be released in an upcoming patch version 3.6.1, whereas dropping a version would probably require a new minor (a bit more complicated/documentation, etc...).
Thanks in advance for your feedback and help.
Thanks everyone for the great work on this issue!
Cheers!
Thanks in advance for your understanding and help! π
Thanks a lot for confirming the patch worked as expected, fixed the issue and we're very happy the changes worked fine.π
Thanks in advance!
Cheers!
π
Compatibility with Admin Toolbar 3.6.0
Active
where a patch was submitted in issue's merge request MR !37.admin_toolbar_content?
Do you think we could consider Admin Toolbar is working fine and that the issue is rather with Admin Toolbar Content?
It would seem the module
Admin Toolbar Content β
is Minimally maintained with No further development, a rather limited user base (about 500 reported installs) and maintainers are now directing users to
Navigation Extra β
as a potential replacement.
Thanks in advance!__construct method to create and all the existing code or logic was kept pretty much "as-is".
Thanks in advance!
Since the Tests seem to be failing β, the MR was set in Draft for now, since more work would be needed to integrate with the patch required for Drupal Core.
As it shouldn't register as an existing "bug" in the module since the feature is not completely integrated with Drupal Core yet.
Thanks in advance!
Sorry again for the inconvenience and thanks in advance for your understanding. π
which aims at making these shortcuts more flexible, with the ability to select different key combinations, see some elements of discussion in the following issue:
#3494646-6: Access Admin Toolbar search field via keyboard shortcut Alt + a β
The idea behind this feature would be to take a more generic approach to solving any issues with conflicting keyboard key combinations (whatever language, system, OS, etc...).event.code === 'KeyA' instead of event.key === 'a':event.code === 'KeyA' to event.key === 'a'.
When I tested the initial code, the feature didn't work with my keyboard (French azerty) π
Turns out on my keyboard the KeyA corresponds to the key Q π
(Q <==> A on French keyboards, the key positions are switched)event.code property couldn't be reliable enough and a different property should be used instead.
Printing the event object allowed me to check which other properties or keys could be used and I found event.key: 'a'.
It certainly allows us to cover more potential use cases π
Therefore, we thought we should keep it simple in this first version and try to extend this feature in priority, in the next version.
Until we could
β¨
Allow shortcut key to be configurable
Active
, so that a wider range of specific keyboard setups or combinations could be supported.
Thanks in advance!
Thanks in advance!
But I've done some basic tests locally and it seemed to work pretty well π
Great job @aaronpinero! π₯³
Thanks in advance! π
Based on patch #59 which is the latest version of the very first implementation of this feature, when the ticket was initially created, so it has quite a lot of reviews and testing feedback.
Based on patch #55 which is a more recent approach of the feature with fewer exchanges, reviews or feedback, but seems like a very valid approach as well.
Great job everyone! π₯³
It is also possible to break down this issue into multiple smaller issues with different types of implementations.
So we would probably need to rework some of the code implementations once we have a better idea of the expected result.
Thanks in advance! π
On paper, this looks like an amazing idea and exactly for what I had been looking for the past two months π
Now, I would have to give the
Accessible Menu β
module and libraries a bit of testing to see how (and if) this could potentially work.
But, would you have any recommendations on how this could work?
Is my understanding correct or could I be missing something?
Would you like to take an initial stab at the implementation of this feature and create an initial merge request?
Any suggestions, advice or recommendations would be greatly appreciated.
Feel free to let us know if you have any questions or concerns on this comment or the module in general, we would certainly be glad to help.
Thanks in advance! π
I have tested a bit the changes myself locally and they seemed to work pretty well, great job! π
One thought though:
Could this be something we would like to be able to configure? Enable/disable, for example, or maybe select a tag (from a dropdown list)?
Or do you it be fine to just switch over to use the <p> tag?
Therefore, at this point, moving back issue to Needs review, as an attempt to collect more feedback, comments and reviews.
Feel free to let us know if you have any questions, suggestions or concerns on any aspects of the merge request or this issue in general, we would surely be glad to help.
Thanks in advance! π
π
Gitlab CI: Fix validation errors reported by jobs CSPELL, PHPCS and PHPSTAN
Active
Maybe this ticket's merge request MR !6 could be closed now, since it's not going to go any further anyway?
Thanks in advance! π
Since all the jobs are now passing π’, moving issue to Needs review.
Please note: The changes to require jobs and broaden Tests coverage (GitLab CI config file) are completely at the appreciation and preference of the maintainers.
Feel free to adjust as you would see fit π
Whatever works best for you!
Any feedback, comments or reviews would be greatly appreciated.
Thanks in advance!
#3304810-72: Toggle away Admin Toolbar completely β
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 π
But I thought this would be straight forward enough not to require a specific ticket to request help for testing and/or plan for big changes.
Also, we "could" start considering dropping support for version below 10.3 in the next minor, since by the month of August we should be about a year away from D10 EOL....
Thanks in advance!
composer update -W (for example with a global update)
Update the configuration:
drush cr
drush updb
Export the configuration changes:
drush cex
Commit/push the configuration changes:
git add xxxx
git commit
git push
(drush deploy, for example)admin_toolbar_content in the stack trace in the issue summary), which "could" be the problem here ...
https://git.drupalcode.org/project/admin_toolbar_content/-/blob/2.0.x/sr...
Indeed, in your following updates you might have to change the version constraint again to upgrade the module.
But you can probably deal with that a bit later, once you have a bit more time to figure out what could be breaking on your site with 3.6.0.
I had completely forgotten to add Project Browser to the Breaking changes π
If the Project Browser 1.x module is installed in your project and you upgrade Admin Toolbar, without upgrade Project Browser, your site will crash.
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...composer require 'drupal/admin_toolbar:3.5.3'
3.5.3 (3.5.x) and 3.6.0 (3.6.x)
We've managed taking the number of active tickets from more than 80 down under 30 \o/ a much more manageable base of active tickets, more realistic and easier to maintain.
We've greatly tightened the code base with improved Tests and stricter validation jobs allowing a much more efficient validation of changes submitted by contributors.
Many of the features in this new release are based on your ideas, functional designs and suggestions.
It definitely greatly contributed to making this module much better! π
I've also started working on
π
zindex issue between admin toolbar and ckeditor 5
Active
π
and made good progress already π€
We could already start cloning this ticket to: [Meta] Roadplan for Admin Toolbar 3.7π
and start sticking tickets on there.
Thanks in advance!
Thanks a lot!
https://git.drupalcode.org/project/ckeditor_details/-/commit/6d2cc1d71c4...
https://git.drupalcode.org/project/ckeditor_details/-/commits/2.1.x/cked...
I didn't check in detail which versions of Internet Explorer could be impacted, but I would assume, going forward, in particular with D11 versions, support for older versions of Internet Explorer could probably be dropped.
Thanks in advance!
Thanks in advance!
It's been more than a year without activity..gitlab-ci.yml file to automate builds on GitlabCI.
cspell, eslint, phpcs, phpstan and stylelint.
Left all the variables and custom jobs configuration code commented, so it could be enabled in the future (uncommented).
Fixing validation jobs should be addressed in separate tickets.
Thanks in advance!
Both patches should not be applied at the same time, see my comment at:
#3286466-64: Tabbing order does not satisfy 508 accessibility requirements β
Do you think that could be a better option overall?
Thanks in advance!
Ideally, we would like to consider these issues separately, or potentially group one in the other...
I would personally say the module is much better with this patch than without.
Thanks in advance! π
Let's try to maybe focus on this color contrast change first, then we could always take a closer look at the active menu items styles.
Thanks in advance!