🇺🇸United States @justcaldwell

Yet another call to Drupal.displace() did the trick for me on the "Enabled" setting and it's still working for me with the other settings. Will be curious to see what others find. Back to NR for testing.

Thanks @ressa, I appreciate it!

Actually, the "Disabled" option is working as expected for me with the changes in the MR:

→

BUT I did test the standard "Enabled" setting after seeing your response, and it's not working for me there.

→

Best guess, we may need to add a call to Drupal.displace() in admin_toolbar.js to ensure the offset is correctly calculated on load (?). So, yeah, back to the drawing board.

Regarding #3: out of the box, Bookmarks definitely do not result in a visual front-end indicator. As @bkosborne says, you would have to add some styling to accomplish that.

Per the introductory github issue, Bookmark is a CKEditor core plugin to support html anchors and provide a UI to create/edit them. They chose the name Bookmark as "... it’s a much more recognizable name for content creators. Its name and iconography are used in current popular content creation systems (Word, Google Docs)."

Hoping that Drupal CMS and Drupal Core add the default Anchor button to CKEditor 5

If that happens, I think that would be the Bookmark button, as Bookmarks are CKEditor's implementation of anchors.

As someone who has invested some time into the anchor_link issue queue, I think there are some issues with the underlying js plugin, and most projects would be best-served using Bookmark — certainly any new projects. So, respectfully, I think deprecation is worth contemplating.

At a minimum, you might consider mentioning and linking to the Plugin Pack → as an alternative. There's also CKEditor5 Bookmark → , which I created before Plugin Pack added the feature. (I maintain it now just for folks who don't need any other features from the Plugin Pack.)

Adding related issues and back to NR.

Thanks or pointing this out @ressa — below is a quick recording of the issue you describe in #39.

CKEditor toolbar with the incorrect offset when admin_toolbar is hidden.
→

Note: To replicate this, the ckeditor edit area must be focused while scrolling the entire page.

Partial solution

Wrapping the call to Drupal.displace() in a timeout mitigates the issue, but only when scrolling down. We still get the wrong offset when we scroll up and the toolbar reappears. See below (there's also an obvious z-index problem below, but that is a separate issue 🐛 zindex issue between admin toolbar and ckeditor 5 Active ):

CKEditor toolbar has correct offset when scrolling down, but not when scrolling up.
→

Core patch needed

This is due to 🐛 CKEditor5 sticky panel top offset does not get recalculated after calling Drupal.displace() Active . With the [small] change to core suggested in that issue, our CKEditor toolbar problems are mostly fixed:

→

As you can see, there's still some "jankiness" around the transitions and it's possible to leave the ckeditor toolbar hanging if you scroll just a few pixels (less than 10). There is another issue to 📌 Improve show/hide transition Active .

As previously mentioned, there are z-index problems, but I think that's a separate concern and have proposed a fix 🐛 zindex issue between admin toolbar and ckeditor 5 Active in #34026402. Getting that change committed would definitely help mitigate some of the visual jank here.

My latest change to MR160 defers the call to Drupal.displace(). There's no harm in committing that change, but it only gets us half-way there. We also need the change to core in 🐛 CKEditor5 sticky panel top offset does not get recalculated after calling Drupal.displace() Active .

justcaldwell → changed the visibility of the branch 3526123-thead-element-hangs to hidden.

I just installed and enabled ckeditor_indentblock on a new site I'm working on (Drupal 10.4.8) .

I don't get an error on the status page; BUT, looking at ckeditor_indentblock.install, it certainly seems like the module is still checking for the lib.

Opened an MR with the mitigation proposed in #16. Static patch attached.

IMO, this is not an issue with admin_toolbar, and I wouldn't try to solve it here. The problem also occurs if you're just using the core toolbar module without admin_toolbar.

The issue stems from devel's dumper using such a high z-index (99999). Actually, it's upstream from devel, as the (default) dumper functionality comes from Symfony var-dumper. I assume there's a reason for the high z-index, but maybe an issue should be opened in one or both of those projects.

If you wanted to conditionally raise the toolbar z-index in the presence of a dump, you could use something like:

body:has(.sf-dump) .toolbar-oriented .toolbar-bar {
  z-index: 100000;
}

Note: This might need other classes in addition to .sf-dump.

That would still have the same conflicts with other modules mentioned above, but only when there's a dump on the page. Maybe it's worthwhile if devel is only temporarily enabled (?). As I said, I don't think this is amin_toolbar's problem to solve.

I think a couple things are getting conflated, neither of which is really an admin_toolbar issue.

1) z-index:
The overlap issue also appears if you're just using the core toolbar. See 🐛 The CK Editor toolbar is overlapping with the Admin toolbar Active . Hopefully there will be a core fix at some point. In the meantime, we could mitigate the problem — @amit.mall was on the right track in #2. I would propose something like:

body:has(.ck.ck-editor) .toolbar-oriented .toolbar-bar {
  z-index: calc(var(--ck-z-panel) + 1);
}

2) Top offset:
When the editor area is focused, ckeditor is supposed to make the toolbar 'sticky' (with an appropriate offset), but 🐛 CKEditor5 sticky panel top offset does not get recalculated after calling Drupal.displace() Active . There might be something we could do to mitigate this, too, but I would argue any attempt should be handled in a separate issue.

Regarding item 2 from #13:

table tags now will all have the table class added to them

It might be worth calling that out in the release note. If a theme uses the .table class to apply styles, CKEditor might now apply table styles where that wasn't previously the case. The CKE release notes don't mention the added class.

For example: in our Bootstrap-based theme, the .table class is used to opt-in to Bootstrap's table styles, and we already add the class to all tables (via a filter) in our "Basic HTML" text format. But, we also provide a more permissive text format where advanced contributors can omit the class if they need specific table styling. Unless I'm misunderstanding something, after this change CKEditor will essentially force those styles in all cases.

Back to NR for testing.

Thanks for testing @erutan!

1) Great catch. I neglected to add back the code to suppress the keystroke when I addressed the issue in #10. Should be fixed now.

2) opt+p is working for me. Can you confirm that the 'Hide or show the toolbar with shortcut (Alt + p)' setting is enabled on the Admin Toolbar Settings page at /admin/config/user-interface/admin-toolbar?

3) I agree that the shortcut "tip" text could be improved — it shouldn't even be there if the shortcut has been disabled — but that's probably out of scope for this issue and could be handled in ✨ Allow shortcut key to be configurable Active .

Ahh, the refactored conditional was too greedy and capturing keyboard input even when altKey wasn't true. Grouping the 'key' and 'keyCode' conditions did the trick.

Score one for unit tests! 🙌

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

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

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

justcaldwell → created an issue.

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

justcaldwell → created an issue.

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

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

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

To move this issue along, I suggest we:

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

Hi @dydave -- thanks for the feedback! I'd be glad to help test ✨ Allow shortcut key to be configurable Active once it gets underway.

Until that happens, obviously it's up you you and the other maintainers, but I think there's potential value in trying to resolve this bug as it affects a potentially substantial set of users (i.e. MacOS).

Thanks for sharing your experience on a French keyboard -- it definitely expanded my thinking on testing for this. I configured some additional keyboard layouts on my machine for testing, and I was able to replicate the issue you described with the event.code approach.

Using W3C's Keyboard Event viewer, I tested the current versions of Chrome, Firefox and Safari with keyboard layouts U.S (standard qwerty), French (azerty), German, Spanish, U.S. Dvorak. In all cases, event.keyCode (even though it's deprecated) seems the most reliable property, and is consistent for a (65) and p (80). This is reinforced by the first table on the MDN docs for keyCode.

Given all that, I think the following would be a more fault tolerant approach, and should resolve the bug for MacOS users (and possibly others):

if (event.altKey && event.key === 'a' || event.keyCode === 65) ...

and

if (event.altKey && event.key === 'p' || event.keyCode === 80) ...

I'll update the MR with those changes. Thanks!

Thanks for the review/testing @tirupati_singh!

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

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

In the latest changes:

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

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

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

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

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

Just noting that this change broke prior functionality with little/no notice. In the past a value of zero resulted in creating links for all entities with no need to designate a max number.

In our case, the previous value of '0' was forced to '1' so all but the first of each bundle link disappeared unexpectedly. At a minimum, seems like there should have been an update that set 'invalid' values to the module default of 20.

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

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

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

Adding static patch for testing.

Small update to the MR. I realized we should prevent transmission of the keypress for Alt + p as well, in case the cursor is somewhere that can receive keyboard input.

Setting back to Needs work

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

MR is up.

justcaldwell → created an issue.

Adding a static patch based on MR 22.

Agreed, this would be a nice enhancement!

Media Library Edit → also allows editing media items in a modal (via a form widget). That module's code might offer some ideas for implementation.

MR 13 allows any valid `$text-container` element (e.g. headers) to be indented.

It also updates the CSS to remove the element targets (p, ol, ul). List element are indented by nesting a new list container, so the class is not used in that case anyway. I also changed the units from `em` to `rem` to ensure consistent indents where font-sizes might vary (again, e.g. headings).

Static patch attached.

This would be a small but nice UX improvement.

The project page lists "Text filter for syntax highlighting" as one of the features.

Maybe it does mean we could make that a recommendation for this particular feature need.

Makes sense to me. That's definitely how we're using it.

I think the two fill different needs. This module (greatly) improves the user experience when using ckeditor's view/edit source feature, with the ckeditor wysiwyg still intended to be the primary editing mode.

CodeMirror Editor is a dedicated editor for code and would be associated with it's own text format.

We use CodeMirror Editor → to edit code-only blocks. Works great for our needs.

My mistake -- I should've used .once() instead of .on(). MR updated.

Thanks for testing!

Marked MR as ready. Given that the previous code remains, this should still work with previous versions.

Attaching a static patch.

I opened an MR with an update that's working for us. I've only done initial testing, and I'm not in a position to easily test with ckeditor < 44.0.0 -- hence the 'draft' MR status.

Please test/review if you're able.

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

Another +1 for RTBC: Drupal 10.4.1, Field Group 8.x.-3.6

Thanks for the info on wrappers. And I just wanted to say, Custom Field really is an excellent contribution — thanks for your work!

That works -- merged to dev. Thanks for contributing!

Okay, the patch did resolve the warnings. Thanks for the fast turnaround!

I also see the new config item after actually editing/saving the formatter settings per your instructions in the other issue 🐛 Errors when displaying Entity Reference field as a block in block layout Active .

We also use fences, so I'm curious to see if/how these new wrappers might interact with that module.

Hello! Just here to confirm that we're seeing the wrapper/offset warnings described in #4 after updating to 3.1.0. Re-saving manage display settings didn't help.

I haven't had time to investigate further. We've reverted to 3.0.x for now.

justcaldwell → created an issue.

Confirming that the CKEditor 5 Bookmark plugin does not support bookmarks/anchors on links. It my testing it also appears to strip id attributes from existing links.

If you're on Drupal 10.4 or 11.1, you can test for yourself with CKEditor 5 Bookmark → .

justcaldwell → created an issue.

Glad to help.

Drupal ^10.4 || ^11.1 now ship version 44.0.0 of ckeditor5 and the Bookmark plugin. I created CKEditor 5 Bookmark → to enable the plugin for use/testing.

Actually, as of Year Only 9.1.1, that's true for any supported version of Drupal. The image above shows the Drupal 10.2+ UI.

Hi @fvd! What version of Drupal are you using? On Drupal 10.2+, it is under the "Date and time" field type.

Updated static patch attached.

Updated the MR with a second post-update hook to run after invalid permissions are removed. So the solution now:

1. Implements logic to associate appropriate menu config dependencies with Menu Admin per Menu permissions.
2. Removes any stale/invalid Menu Admin per Menu permissions (i.e. for menus that have been deleted).
3. Re-calculates dependencies and updates roles that have existing Menu Admin per Menu permissions (in light of #1).

Back to NR.

Hmm. Looks like the update hook should also cause an update of any role with any menu_admin_per_menu permission, so the dependencies for existing permissions are applied. Back to NW. :\

Posting a static patch. Also bumping to Major as this can render the Permissions page unusable unless/until you do something like the suggested workaround in #5.

Post update hook added. Ready for review/testing.

Great, thanks @mahyarsbt!

I see the MR was approved, but it seems like it's not actually merged to 1.0.x?

justcaldwell → changed the visibility of the branch 3457584-remove-keyvaluestore to hidden.

Until someone provides an MR for that feature, I recommend the approach in MR 8 for folks who need a resolution to the issue in the meantime. To that end, I'm attaching a static patch based on the MR.

Updated IS

The update hook from a patch for the same issue in entityqueue → looks almost cut-and-paste-able.

MR 20 adds the required dependencies. Ideally, we'd also provide an update hook to clean up any stale invalid permissions hanging around (hence the draft MR). I'll try to take a pass that hook soonish, if no one beats me to it ;).

Just bumped into this again. Updated Steps to Reproduce to more reliably invoke the error.

MR 8 is an alternative solution that makes fewer changes to the module, retains the KeyValueStore approach and the role specific form/route handling. It just discards any stored settings after each use — settings are re-loaded/updated as usual on submitting the filter. I kept the store as expirable in case something unpredictable occurs after the form is submitted, but reduced the time to 5 minutes.

Either approach ensures the permissions page is immediately usable if the user returns to it after attempting to load too many values (or within 5 minutes if something else went wrong).

Hi @nsciacca. Thanks for starting the MR! Yeah, I ran into the same issue this morning. I added the fix to the MR, as well as some additional cleanup of KeyValueStore-related code.

Also, PermissionsRoleSpecificForm.php depends on saveFilterSettings(), so that needed to be addressed. Since a permission filter has been added to Drupal's core form, I don't think it's necessary for filter_perms to override the role-specific form any longer — so I removed it and the route subscriber.

We'll have to see how the module owner feels about all that 🙂. Setting to NR.

@cyu: Some initial testing of this approach worked for me. If we $form_state->setRebuild() in the submit handler, the submitted values will be still be available on subsequent loads, so $filter can just draw from those values and drop the KeyValueStore entirely. The submit handler would become:

public function submitFormFilter(array &$form, FormStateInterface $form_state) {
    $form_state->setRebuild();
}

Then $filter becomes:

$filter = [
    'roles' => $form_state->getValue('roles', []),
    'modules' => $form_state->getValue('modules', []),
];

See: FormState::$rebuild and FormState::setRebuild

The form 1) would no longer "remember" the previous filter if you navigate away from the permissions page AND 2) the render will still fail if too many permissions/modules are selected, BUT the page will be immediately usable if you navigate away and come back to it.

The solution for #2 is user training, unless we add some generic warning text about selecting too many permissions/modules.

Hi @nsciacca - I think remembering the filter settings between submissions is essential to the way the module functions.

@cyu - regarding your question in #5:

Out of curiosity, what kind of numbers for roles/permissions and memory limits are causing this problem?

We have 865 permissions and 54 roles (yikes!).

I just tried 'All modules / All permissions' again on my local with a memory limit = 1024M and it looks like the form actually begins to render for me (after quite a long time). It's the browser (Chrome in this case) that chokes on the results and ends up giving the 'Oh snap, something when wrong' message after partially rendering the page.

On our actual web servers, the memory limit on admin routes is 512M, and the page exhausts the memory and fails quite a bit sooner with no attempt to render.

Noting that the latest release of CKEditor 5 (44.0.0) now includes the Bookmark plugin — CKEditor's native UI for creating anchor links.

Additionally, work on link UI enhancements to ease linking to "on page" anchors/bookmarks is being tracked in this github issue. See the second video on that page for a demo.

I think that modules should ensure compatibility with popular admin themes, so it's probably more likely that this becomes part of (or sub-module of) Moderation Sidebar. That's probably no going to happen, so we'll just settle for the status quo 🙂.

Added 'Administration Tools' -- seemed more accurate than 'Developer Tools', which I removed.

LGTM and mo issues found in manual testing against D10/11.

Merged to 1.x-dev. Thanks for contributing.

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

justcaldwell → created an issue.

justcaldwell → created an issue.

Merged to 2.x, release to follow.

Re #3: Thanks, but the patch only addresses comment #2, not the full issue as described in the summary.

Released in 2.0.0-alpha9.

Released in 2.0.0-alpha9

The signature for the __construct() method of ConfigFromBase has changed in Drupal 11 such that we'll need to drop support for Drupal 9. See https://www.drupal.org/node/3404140 → .

justcaldwell → created an issue.

This issue and MR is 'sub-optimal' at best! Closing.

Merged to dev. New release to follow soon.

Just looking at the examples from the core ckeditor5 module, your example seems close, but maybe it should be something more like:

my_plugin:
  ckeditor5:
    plugins: [ htmlSupport.GeneralHtmlSupport ]
    config:
      htmlSupport:
        allow:
          - name: ~
            classes: [my, whitelisted, classes]
  drupal:
    label: Whitelisted elements
    library: core/ckeditor5.htmlSupport
    elements:
      - <* class="my whitelisted classes">

I'm working on 📌 Add support for new field selection UI Active , and it makes sense to go ahead and fix this in those changes. Closing and adding credit there. Thanks!

Makes sense to fix ✨ Allow underscores in CSS classnames/style key field. Active while we're making these changes.

Released in 1.0.4

Fixed in dev. New release soon.

justcaldwell → created an issue.

Thanks for posting this @gravelpot! Confirming same.

The offending commit was reverted in https://github.com/robrichards/xmlseclibs/releases/tag/3.1.3 -- updating to that release resolves the problem for us.