Portland, OR
Account created on 16 February 2009, almost 16 years ago
#

Merge Requests

Recent comments

🇺🇸United States joshuami Portland, OR

@lauriii, while the patch in #59 is focused on class and the style widget, the text-format-save blocking error can occur for any attribute that is defined by a module and in source editing. It could be an id or a data-attribute if there is a module that defines those attributes for the element in question.

The validation introduced in 3396628 🐛 Fix → native CKEditor 5 functionality and fix bug in SourceEditingRedundantTagsConstraintValidator that allowed it to slip by Fixed triggers on any "overlap". While the class issue is probably the most common, it's not the only scenario that can cause that error to throw and prevent the text format from being saved.

🇺🇸United States joshuami Portland, OR

I definitely like the idea of a rubric that could be used to grade the value of project contributions. @fjgarlin's three ideas for possible starting points are great, and while I like logarithmic for it's mathematical simplicity, I think it might miss the mark to focus to heavily on usage as the primary driver.

Usage statistics rely on unique sites that have the update manager module enabled if they run cron or manually trigger check for updates. The counts include non-production environments, including environments that may have been spun up for a test and then abandoned. The counts may not include production environments if the developers choose to disable update manager in production using something like config split. That makes usage statistics directionally accurate, but far from precise.

I could see a situation where a bad faith contributor spun up environments to boost usage numbers, but more likely is that we undercount usage on critical projects in the ecosystem. Case in point, Localgov is pretty essential to the growth of Drupal within government sites in the United Kingdom, but it shows no usage statistics at all. Similarly, GovCMS is an equivalent for Australian government websites and it shows only 12 installations.

Usage also misses the mark in highlighting relatively new projects that suddenly become key areas for contribution. The Drupal AI module comes to mind here as it only has 463 sites reporting usage, but is under active and rapid development.

As @kristiaanvandeneynde mentioned, Group and Commerce sites often have a significant number of total active editors and logged in users. I know of a Group site with well over 2,000 active editors as an example. It's a single installation, but represents a massive amount of total content and reach. Those sorts of sites are probably some of the most important to ongoing usage and support of Drupal.

Another counterpoint to usage, there are modules that are heavily used, but only releasing maintenance fixes. These heavily-used-and-stable modules don't need much contribution.

Likewise, there are heavily used modules that have that usage despite not having a stable release. You end up with thousands of sites running an alpha or beta version because the functionality is critical to the use case. It would be awesome to increase weight for contributions to help get these modules stable and get more of the ecosystem covered by the Security Team.

I agree that reviewing every single project to set a weight would be burdensome for the DA. That said, I don't think it is out of the realm of possibility for a working group to be formed that could present high impact projects to the DA for adjusting their weight. There are 9,355 modules currently claiming support for Drupal 10 that are not marked as "obsolete" or "no further development". (There are similarly 349 themes. I couldn't get the D10 support coverage for distributions easily, but there are only 202 distributions that are marked as "has a supported stable release"—so it is some number less than that 😉.)

A few steady contributors giving a little time to this sort of ranking could have a huge impact without as much strain on the DA. It would also provide an opportunity to highlight what type of support is needed for the project in question. Maybe it's a stable project that just needs better documentation. Maybe it is a project that needs a more active maintainer and becoming that maintainer would boost credits for the module in question. I can think of lots of helpful scenarios to improve the quality of a module and its impact on the ecosystem.

Great topic to bring up!

🇺🇸United States joshuami Portland, OR

......this is why I'm so afraid to touch this part of the logic 😆

Indeed! 😆

We rewrote much of the standard dashboard into an accessibility report that pulls in content based on an editor's group membership using views_custom_tables and some entity referencing from an entity ID relationship. Our end goal is to give editors a checklist of content to address—and maybe give them a cheery message when they've completed it all.

Now that we understand the logic a bit better, we've decided to have an administrator report that shows content marked and okay by editors that can be reviewed monthly or quarterly to address editor's over-reporting content as okay when it isn't. This particular team has a really strong editor mentorship approach built into their governance.

If you are interested, I could do a demo for you when I've finished up the approach.

🇺🇸United States joshuami Portland, OR

Ah... I see. We had not planned on allowing editors to mark OK for all as we kinda assume they will just clear results that are still an issue. (As I like to say to my teams, "editors are gonna edit." 😆)

I can work with the current model by having our admins review items that editors have dismissed and mark them as okay when they have been reviewed.

Thank you for the explanation!

🇺🇸United States joshuami Portland, OR

Now that Fix label token replacement for views entity reference arguments 🐛 Fix label token replacement for views entity reference arguments Fixed has landed in 10.3, this should probably be rewritten as a /src/Plugin/views/argument/SearchApiEntity.php handler instead of targeting just node. That might allow for the removal of /src/Plugin/views/argument/SearchApiTerm.

Adding a related issue that was probably incorrectly attributed to subpathauto: Views contextual filter replacement pattern doesn't use referenced entity's title available 🐛 Views contextual filter replacement pattern doesn't use referenced entity's title available Postponed .

🇺🇸United States joshuami Portland, OR

I should add that this is not a subpathauto issue and Fix label token replacement for views entity reference arguments 🐛 Fix label token replacement for views entity reference arguments Fixed fixes the issue for Taxonomy Terms and entity types—I would this it would fix the OG argument example that opened this issue. Search API views with a contextual argument are still not converted correctly. I wonder if we should open a separate issue in that queue and close this.

It feels kinda like https://git.drupalcode.org/project/search_api/-/blob/8.x-1.x/src/Plugin/... should be rewritten as SearchAPIEntity.php—as it appears you can pass a taxonomy term and get the correct label, but not any other entity type. 🤔

🇺🇸United States joshuami Portland, OR

I figured out a way to address this without any custom code, but it does require Twig Tweak and Metatag .

I appreciated the approach that @DamienMcKenna provided in comment #2, but it only addresses the title provided by the page_title.html.twig template and not the <title> element. I was able to get the text for the title of the contextual argument by passing it into Twig Tweaks drupal_field() function: {{ drupal_field('title', 'node', arguments.id) }} News.

This works for any entity type that has fields and where you can only get the ID from the contextual argument and not the title/name/label output—such as when you have a Search API Solr view that you are passing a contextual argument ID for an entity referenced field such as "parent group" or "tag".

I hope this helps anyone that might be tripped up on this issue and is looking for a way to set both the view title override and the metatag title element override.

🇺🇸United States joshuami Portland, OR

I'm setting this back to needs work based on @neclimdul's comment.

Ideally, a maintainer would want to apply the additional update to a new release that required 10.3 or greater. Given the module is currently at 2.0.0-beta2, I'm not completely sure what that release should be.

@smustgrave, I noticed you have a 3.0.x branch for Drupal 11 compatibility. Would it make sense to create a beta release for that branch with a requirement for "drupal/core": "^10.3 || ^11", that contains the new update in this MR?

🇺🇸United States joshuami Portland, OR

Thanks, @alexpott. Added a merge request with the repeated function. Let me know if I got the naming convention correct. I assumed it should be "update_10301" as the first update required after Drupal 10.3, but maybe that should be "update_10300". 🤷

🇺🇸United States joshuami Portland, OR

Quick note for anyone needing to run that command on an Acquia environment, I found that you need to escape the parenthesis when running the command through acli, acli remote:drush [application].[environment] -- php:eval '_scheduler_content_moderation_integration_add_fields\(\)' worked where application is the project code and environment is the CD, dev, stage, or prod.

It's a bit of a bummer to have to run it for every environment up the chain, but at least it unblocks the upgrade without persistent errors.

We really need another release that adds scheduler_content_moderation_integration_update_9004() calling the same _scheduler_content_moderation_integration_add_fields() function. Or is that too messy and we should just rename the 9001 update and move it to the 9004 position. 🤔

Any preferences from the maintainers? I'm happy to create the MR.

🇺🇸United States joshuami Portland, OR

@sanket.tale, I didn't see the issue on views with menu settings set by a view. It was in views off of an entity type were the the tabs were added via a mymodule.links.tasks.yml that defined the tabs using the menu api per https://www.drupal.org/docs/drupal-apis/menu-api/providing-module-define... .

This is similar to how the media module in core adds the views.view.media.yml to the system.admin_content base route:

entity.media.collection:
  title: Media
  route_name: entity.media.collection
  base_route: system.admin_content
  weight: 10

If you create the multiple views and add them via a custom module in mymodule.links.tasks.yml, I think you should be able to reproduce, but I haven't tried this on a view that has both primary and secondary menu tabs like /admin/content.

🇺🇸United States joshuami Portland, OR

Updating issue summary with a GIF to demo the issue.

🇺🇸United States joshuami Portland, OR

I've tested MR #10 and the dev branch with success as well. There are no tests in this module and the functionality is all there with multiple manual tests, so I guess this is RTBC. I tend to agree that getting a stable release would be ideal.

I do worry that the next release may break backwards compatibility with the contrib ckeditor module or possibly the paid CKEditor 4 version provided by CKSource.

I noticed that I cannot remove the composer dependency on drupal/ckeditor with the patch from this merge request. I don't see why not in the module's composer.json—which is a bit odd.

I also was curious about the ckeditor5_plugin_pack approach to CKEditor 5 templates. I guess the good news is that its implementation is very close to MR #10. The configuration of templates is nearly the same with a small difference in how the SVG is added via a field rather than as a file path.

The plugin widget has a neat little search feature, but I'm not a fan of the concatenated template descriptions. It does not show the full description in any way that I can find.

Until those clean up items get added to the ckeditor5_plugin_pack, I think there is still a place for this module, but afterwards, I would imagine most folks are going to settle on the module maintained by CKSource. 🤔

🇺🇸United States joshuami Portland, OR

This issue occurs with the s3fs module as well and I can confirm that the patch correctly handles the error for that contrib module as well. Updating the issue summary to reflect that a core fix would solve the issue for more than just stage file proxy.

Would something like this need tests? The patch basically shifts the error to the error logs rather than disrupting page render. That seems reasonable.

🇺🇸United States joshuami Portland, OR

@Falco010, there is a core patch for 'disable-refocus' => TRUE. I believe I've triggered that cursor refocus in more cases than just CKEditor5 as a part of using the media library on a entity reference field.

See #3415961 🐛 [drupalMedia] Using the Insert Media button causes the window to scroll to the bottom of the page Needs work .

Here is my attempt at the requested review from @mandclu. This issue is definitely a bit confusing to follow with both the patches and the merge request.

The latest patch in #146 applies cleanly to the latest release, 8.x-1.4. In my tests, it works well.

The merge request is against the 8.x-2.x dev branch. Which doesn't have a release and is not the default dev branch. I have not been able to get it to successfully install with composer.

The merge request seems to make some different choices than the latest patch other than the base branch.

  • The format of the README.md is a little different (minor).
  • The patch declares core_version_requirement: ^9.3 || ^10 || ^11 while the merge request shows core_version_requirement: ^8 || ^9 || ^10.
  • The patch includes an update hook.
  • The patch adds a permission called insert ckeditor templates. The merge request adds a permission called use text format advanced.
  • The patch adds some language management that the merge request does not.
  • The merge request adds an upcast and downcast in /js/ckeditor5_plugins/ckeditor_templates/src/ckeditorTemplatesCommand.js that adds the widget type around handles to add a new line before or after the inserted template.
  • The merge request includes code formatting changes in /src/Plugin/CKEditorPlugin/CkeditorTemplates.php.
  • The patch properly types attributes in /src/Entity/CKEditorTemplates.php but the merge request has a save() function in that same file that clears cache definitions when a template is saved.
  • There are a few spots in the patch that set the form fields as text_format where the merge request opts for textarea and string.

@mandclu (or another maintainer), should that merge-request be against the 8.x-1.x branch? And if so, how would we resolve which of the slightly different approaches should make it into the accepted merge request.

The sweet spot for me would likely be a new merge request against 8.x-1.x that is mostly the patch from #146 with the addtion of the type around widgets getting added to the templates if those work well. I've not been able to test them as I personally haven't found a way to get the merge request to apply.

Thoughts?

🇺🇸United States joshuami Portland, OR

That is a good question. I think just the CKEditor plugin would meet the need. Adjusting the media_embed filter for all possible configurations would get complicated quickly. And, there might be a need to support legacy content that used one of the disabled features.

A disallowed patterns filter would a pretty slick feature as well. It would be nice to strip empty elements like headings and paragraphs that editors always seem to be leaving behind. I'll have to think about writing that up as a separate issue. 😁

🇺🇸United States joshuami Portland, OR

That should be literally impossible 😳

I was equally confused by that result.

To make sure it wasn't some oddity in the moderately complex site on which I was working, I tested the patch on a site closer to the standard install. The only additions I made were to add two new display modes for images that were not used by the other default media entity types.

Before applying the patch, an embedded document showed only the "Default" view mode:

After applying the patch, an embedded document showed all enabled view modes even though "Half width" and "Narrow" were not configured display modes for documents.

🇺🇸United States joshuami Portland, OR

You're leaving me hanging there! 🤣 Where were you going with that? 😄

😆 At least you can tell it wasn't generated by AI.

That was a few days ago, so I can only guess at what I was going to say, but it would have been something like this. 🤔

It would be nice to be able to choose whether or not caption, link, display mode, and even alignment appear per each type of media. As a developer, I can put all sorts of things into a media entity display mode. Here are some examples of where I might want to prevent one of the default toolbar items.

Caption: I may choose to provide a caption rather than have CKEditor provide the caption functionality because I want to include license and attribution meta data in the caption.

Link: I might want to prevent editors from adding a link around an image to discourage the creation of gaudy image "buttons" that break from the style guide. I would definitely want to hide the link functionality from documents, audio, and video because it would break the functionality and lead to more support requests. (Sorry, that's two examples 😜)

Display mode: If there is only one display mode for a media type, I might want to hide that display mode to reduce the visual complexity for my editors.

Alignment: I might want to hide alignment from videos because I want my editors to always embed full-width videos and the align left and right forces a 50% width by default.

I can probably train editors to work around all of these scenarios, but if I could hide the feature I don't want them to use, they will be much happier and it lowers the chance of confusion.

🇺🇸United States joshuami Portland, OR

I ran across this issue with a document media type that is configured to show translations on some display modes via an entity views attach. (I know it sounds complicated, but it was a pretty simple solution for a complex translation need. 😆) If the document was embedded, and then immediately translated, the preview does not update for a bit. I didn't time it, but the 5 minutes mentioned above sounds about right.

So I tried the patch in #9. It did invalidate the cache for the preview upon applying the patch, but it didn't really fix the caching issue on subsequent tweaks to the display mode.

The patch did have an unintended consequence after applying it. It showed all display modes as an option even if the media type did not have that display mode configured. I really don't understand how setting the cachetag checksum could have had that affect, but I thought I would report it for whomever might pick this up.

🇺🇸United States joshuami Portland, OR

@jacobupal, I think you might be on to something, but the approach in patch #70 in CKEditor 5 support for Content Templates CKEditor 5 support for Content Templates Needs review creates a command called "ckeditorTemplates" in /js/ckeditor5_plugins/ckeditor_templates/src/ckeditorTemplatesCommand.js that is called in /js/ckeditor5_plugins/ckeditor_templates/src/ckeditorTemplatesEditing.js. That command is inserting a section around every template when embedded:

+          <section class="ckeditor-template-wrapper">
+            <div class="ckeditor-template-content">
+              ${htmlCode}
+            </div>
+          </section>

That section getting converted into a widgetEditable with the widgetTypeAround handles. I didn't test that patch in particular, but that's what I'm seeing in the code. It kinda looks like they were following the tutorial at https://ckeditor.com/docs/ckeditor5/latest/tutorials/widgets/implementin....

That said, it would be awesome if we could target any element + attribute combination to make it an editable block widget when in focus. A horizontal rule gets upcast into a widget, why can't we do that with other element + attribute combinations. It might fix some issues like the ability to edit a <cite> within a <blockquote>.

🇺🇸United States joshuami Portland, OR

@Wim Leers, thanks for asking the clarifying questions.

Audio might have a caption, but it would be rare.

Okay, "rare" may have been an overstatement. 😄

Originally, I was thinking of documents as not really something you would semantically mark up as a figure, but you might have a point. Reading through the HTML 5 definition of flow content (https://www.w3.org/TR/2011/WD-html5-author-20110809/content-models.html#...), I guess it would be a correct usage to wrap a document media link in a figure and provide a figcaption as the description.

I've certainly implemented a description field on documents and used the display mode template to output a description with an aria-describedby on the document. There wasn't a way to add a caption to a document in earlier releases of media library with CKEditor 4, so I hadn't really given it thought to using it.

Generally, a little more configurability for what appears in the drupalmedia toolbar would be helpful. There are so many use cases were the

Updated the issue summary to focus on the link button as that is the larger issue for creating patterns that will break media players and document download links.

🇺🇸United States joshuami Portland, OR

It's worth noting that nearly all of Media and Media Library's test coverage and development have been focused on images. The result is that new features for images tend to be added to all media types (whether they are appropriate or not). There are many more areas where adding or removing embed options would make other media types such as video, audio, documents, maps, charts (and more) significantly more usable and accessible. Overriding this experience was much easier in CKEditor 4 with the entity embed dialog (modal) as it allowed for Drupal form alters per media type. With CKEditor 5, this all has to be considered as part of the plugin architecture for the balloon toolbar that appears on each embedded media item.

🇺🇸United States joshuami Portland, OR

In a project using this patch, we just discovered an edge case that wasn't covered when scheduling media. It only occurs when creating new media, but the publish status sort weight changes back to 53 when the error is triggered before the media entity is saved. I can't reproduce this for content entities, so it may be an issue upstream in core media. I'll try an dig into this a little more as time permits.

🇺🇸United States joshuami Portland, OR

@mark_fullmer, can you point to the commit that introduced the bug for you? I was fairly certain it was introduced in 3396628 🐛 Fix → native CKEditor 5 functionality and fix bug in SourceEditingRedundantTagsConstraintValidator that allowed it to slip by Fixed , which added the validation that compared source editing to the currently enabled plugins—among some other changes related to lists.

I'm not able to reproduce the error in a site running 10.1.6 and CKEditor 5.

🇺🇸United States joshuami Portland, OR

I have been able to duplicate this issue on a site that used a combination of ckeditor_bs_grid and ckeditor_templates. Any template added to a grid where the template included divs—such as adding a Bootstrap card component—resulted in data loss on edit. For some reason, the duplicate ck-widget classes would trigger the removal of the div with col class immediately following the row.

It's an odd enough behavior that I can't quite track down the source. I'm adding a comment in case anyone else is seeing similar.

🇺🇸United States joshuami Portland, OR

@KlemenDev, this is not a duplicate and the issue summary is not fixed by 3410303 🐛 FilterHtml data loss when iframe and/or textarea is allowed Active . The problem is that you can't save a valid configuration for source editing after 10.2.

Unless we want to reopen issue #3396628 which I mention in comment 12 🐛 [10.2 regression] CKEditor 5 breaks when "Source"/Source editing button is added and "Manually editable HTML tags" are specified Needs review , we still have a validation issue which forces resolution of errors on the text format creating a state that will cause data loss upon editing existing content.

For any element that has a valid plugin, the current validations requires you remove the element and its classes from source editing. This is a problem, because not every plugin covers every attribute that may be needed in an existing site.

This is currently a hard blocker for upgrading a site from CKEditor 4, which allowed pretty much any source editing pattern that you desired, to CKEditor 5, which now requires new plugin for every possible pattern in source editing.

🇺🇸United States joshuami Portland, OR

@Wim Leers, I think #3396628 may be a separate issue.

What I was experiencing was definitely related to the changes in SourceEditingRedundantTagsConstraintValidator.php as part of #3396628. At https://git.drupalcode.org/project/drupal/-/commit/ec24a58c6dd7e7d3a1ead..., the validation is comparing elements enabled in source editing to all other enabled plugins. That check prevents someone from adding an attribute like class to anything that might be covered by another plugin that also enables aspects of that element.

The error message I was getting that prevented saving a valid config is The following @element_type(s) can optionally be supported by enabled plugins and should not be added to the Source Editing "Manually editable HTML tags" field: %overlapping_tags.

Here's an example:

We had a couple of CKEditor 4 plugins that we wrote to support adding phone numbers and email addresses with an enforced format and classes that allowed them to be editable and styled. That code required the following to be enabled in source editing: <span class="cke-phone cke-email">. (Note: this is a temporary need to avoid data loss until we can write a proper CKEditor 5 solution.

When we attempt to save the text format after the 10.2 upgrade, we get the error message:

The following attribute(s) can optionally be supported by enabled plugins and should not be added to the Source Editing "Manually editable HTML tags" field: Style (<span class="cke-phone cke-email">).

That is directly coming from $enabled_plugin_elements_optional, but the only other plugin enabled that affects span is the Language selector. Clearly, we are not going to get the ability to save those classes from a completely unrelated plugin, but the overlap filter is preventing span from being in source editing.

I propose we remove the overlap optional filtering so that source editing can truly allow the developer to say "I know I don't have another plugin for this so I want to allow it in source just to prevent data loss until I can write something way more complex and robust (possibly never 😆)".

To put this another way, source editing should allow us to define exceptions that are not covered by another plugin rather than forcing us to write a plugin from scratch.

🇺🇸United States joshuami Portland, OR

joshuami changed the visibility of the branch 3396628-tighten-sourceeditingredundanttagsconstraintvalidator to hidden.

🇺🇸United States joshuami Portland, OR

Added some clarification to the issue summary. The text filter won't cause data loss until saving the format after resolving all errors to allow saving.

🇺🇸United States joshuami Portland, OR

Hmm. I was adding <span lang dir> to source editing. Just adding <span> does work. Thanks for having me double check @ericgsmith.

I still think it might strip the declaration upon saving after an upgrade because of the new rules around plugin requirements, but at least you can sort of use the language selector again.

🇺🇸United States joshuami Portland, OR

Alright... another use case with data loss. The language selector no longer works with any core configuration.

If you try and add language to the toolbar, it gives you an error that you need another plugin to add <span>, but if you try to enable span via source editing, it throws an error that it is already enabled via language.

🇺🇸United States joshuami Portland, OR

Has anyone used the patch with Drupal 10.2? I'm curious whether the change introduced in 3396628 🐛 Fix → native CKEditor 5 functionality and fix bug in SourceEditingRedundantTagsConstraintValidator that allowed it to slip by Fixed impacts the ability to create and use a template with <div> and classes.

🇺🇸United States joshuami Portland, OR

It's still needed. Just needs a reroll. #3359796: Skip form displays that don't exist. added some lines above the function the patch was updating. I'll see if I can get a new patch posted today or tomorrow.

🇺🇸United States joshuami Portland, OR

While troubleshooting a new site with media entity download where path aliases were not working—I came across this issue. It seemed odd as several of our other D8+ (all D9 or D10 now) were working with the path alias instead of the /media/id/download. It turns out that we nearly always use subpathauto, which solves the issue of having a media alias path with media entity download quite well. Posting in case this unblocks anyone else looking for a solution in this space. 😅

🇺🇸United States joshuami Portland, OR

@kevinquillen, I think the only way to get it to work at the moment is to composer require drupal/simplesamlphp_auth:^4.0@dev. That worked for us, but I'm keen to see the full release sooner than later as I don't like pinning to a dev commit when I can avoid it.

🇺🇸United States joshuami Portland, OR

Oh, I didn't think about v4. I've been so focused on Radix 5 and Bootstrap 5 for the last couple of builds that I forgot what it used to look like. 😆

Interestingly, Bootstrap 4.6 does have root variables, but looking at https://git.drupalcode.org/project/radix/-/blob/8.x-4.x/src/kits/default..., I don't see the _root.scss getting called. I think that might be a miss in Radix 4. 🤔

In addition to documentation, it might be worth adding a CKEditor5 stylesheet to Radix and refactoring the way the dependencies are called in to allow the root variables.

I really think most themes (or at least generated subthemes) should come with a ckeditor5-stylesheet built in now. It would certainly make it easier for people to understand how that particular functionality works.

🇺🇸United States joshuami Portland, OR

@jwilson3, great write up! The only catch that I see is that the way the Radix subtheme pulls in the Bootstrap Sass files, the root.scss files would be prefixed with .ck-content, which can cause the variables in that file to no longer render. So all the color and size overrides that Bootstrap offers, or the variables that are overridden would not appear in CKEditor.

To get around this in one of my Radix themes, I restructured the Radix way of pulling in Sass quite a bit so that the subtheme's myradixsubtheme.style.scsss and ckeditor5.style.scss both pull in those root variables without the .ck-content prefix. So the ckeditor5.style.scss would look a little like this:

// ------------------------------------------------------------------------------------------
// Subtheme CSS overrides for CKEditor 5
//
// This stylesheet is applied to admin pages with a CKEditor 5 WYSIWYG editor. Use it to set
// or override styles of content within the editor.
// ------------------------------------------------------------------------------------------

// ------------------------------------------------------------------------------------------
// Import all the functions and variables.

// 1. Include functions first (so you can manipulate colors, SVGs, calc, etc)

@import '~bootstrap/scss/functions';

// 2. Include any default variable overrides here
@import 'base/variables';

// 3. Include remainder of required Bootstrap stylesheets
@import '~bootstrap/scss/variables';

// 4. Include any default map overrides here
// We don't have any in Westy

// 5. Include remainder of required parts
@import '~bootstrap/scss/maps';
@import '~bootstrap/scss/mixins';
@import '~bootstrap/scss/root';
@import '~bootstrap/scss/utilities';

// ------------------------------------------------------------------------------------------
// Preface all Subtheme styles with .ck-content to restrict them to the content within CKEditor
.ck-content {

  // 6. Optionally include any other parts as needed
  // We've chosen to comment out includes not in use.
  @import '~bootstrap/scss/reboot';
  @import '~bootstrap/scss/type';
  @import '~bootstrap/scss/images';
  @import '~bootstrap/scss/containers';
  @import '~bootstrap/scss/grid';
  @import '~bootstrap/scss/tables';
  @import '~bootstrap/scss/forms';
  @import '~bootstrap/scss/buttons';
  @import '~bootstrap/scss/transitions';
  @import '~bootstrap/scss/dropdown';
  @import '~bootstrap/scss/button-group';
  @import '~bootstrap/scss/nav';
  @import '~bootstrap/scss/navbar'; // Requires nav
  @import '~bootstrap/scss/card';
  @import '~bootstrap/scss/breadcrumb';
  @import '~bootstrap/scss/accordion';
  @import '~bootstrap/scss/pagination';
  @import '~bootstrap/scss/badge';
  @import '~bootstrap/scss/alert';
  // @import '~bootstrap/scss/progress';
  @import '~bootstrap/scss/list-group';
  @import '~bootstrap/scss/close';
  // @import '~bootstrap/scss/toasts';
  // @import '~bootstrap/scss/modal'; // Requires transitions
  // @import '~bootstrap/scss/tooltip';
  // @import '~bootstrap/scss/popover';
  // @import '~bootstrap/scss/carousel';
  // @import '~bootstrap/scss/spinners';
  // @import '~bootstrap/scss/offcanvas'; // Requires transitions
  // @import '~bootstrap/scss/placeholders';
  @import "~bootstrap/scss/helpers";

  // Subtheme Base
  // ------------------------------------------------------------------------------
  @import 'base/helpers';
  @import 'base/elements';
  @import 'base/typography';

  // Subtheme Layouts
  // -----------------------------------------------------------------------------
  @import "layout/page.header";
  @import "layout/page.content";
  @import "layout/page.footer";

  // Subthteme Components
  // -----------------------------------------------------------------------------
  @import "../components/nav/nav";
  @import "components/accordions";
  @import "components/alerts";
  @import "components/badges";
  @import "components/buttons";
  @import "components/cards";
  @import "components/callouts";
  @import "components/form";
  @import "components/item-list";
  @import "components/list";
  @import "components/navs";
  @import "components/office-hours";
  @import "components/pagination";
}

// 7. Optionally include utilities API last to generate classes based on the
// Sass map in `_utilities.scss`
@import '~bootstrap/scss/utilities/api';

My myradixsubtheme.style.scsss, looks the same as above, but without the prefix section. I bet there is a better way to do that so that I'm not maintaining two nearly identical *.style.scss files. 🤔

@ckng, if you use your subtheme's built CSS without any prefixes, and you use the Claro admin theme, you will likely see some conflicts between Bootstrap and Claro because they share a couple of classes. This can cause the form to have some weird layout. It also means that your font in Claro will match your frontend—that might not be an issue depending on your font choices.

🇺🇸United States joshuami Portland, OR

+1 on the RTBC. I also had the caching issue, and I *think* it is related to https://www.drupal.org/project/drupal/issues/3370828 🐛 Ensure that edge caches are busted on deployments for css/js aggregates Fixed .

As of Drupal 10.1.2, the cache tag on CSS and JS libraries are only going to get updated if there is a library version change. If the libraries file were updated to have a set version per release (or included in a patch for future release) it should make the cache tag parameter reset—whether for a local build with aggregation turned off or for a production environment with aggregation turned on.

I was able to get around this by including the patch in a build that also updated to Drupal 10.1.3—which has version changes that trigger the new cache tag.

🇺🇸United States joshuami Portland, OR

After a bit more research with some sites I'm trying to upgrade to D10 with CKEditor5, I think this may be more of a potential data-loss bug than a task.

If a site has used Media Library and embedded media into a list (ordered or unordered doesn't matter), those lists will be corrupted on the first edit of the entity after enabling CKEditor5.

While Document Lists (see my previous comment) in CKeditor5 allow paragraphs and images to be inserted into a list—retaining the nesting of those elements within the list item—you cannot do the same with a drupal-media element. I'm not immediately sure why as you can technically manually add divs. I think it might be because drupal-media elements are not explicitly allowed within CKEditor's DocumentList allowed HTML.

I'm not seeing the same issue with tables. You can have a drupal-media element with a table cell and it doesn't force that to break out of the table cell in the way that it forces the list to break into two list elements.

Can anyone else reproduce this? Do you agree it would be data loss since there is no path to editing the content that has this pattern?

🇺🇸United States joshuami Portland, OR

Just tested on a Drupal 10.1 site with the patch against a remote video and a document embed. In both cases, clicking the media item triggers the tooltip instead of triggering the interaction with the media item (playing the video or downloading the document).

Thanks for jumping in on this one @lauriii!

🇺🇸United States joshuami Portland, OR

I need to spend a bit of time figuring out how to actually create a merge request in an issue. Adding a patch for now.

🇺🇸United States joshuami Portland, OR

Adding some use cases to this issue.

  1. An editors uses media library to embed a document into an unordered or ordered list.
  2. An editor uses media library to embed an image into a list.

CKEditor 5 supports this per https://ckeditor.com/docs/ckeditor5/latest/features/lists/document-lists....

🇺🇸United States joshuami Portland, OR

@Wim Leers, do you think we could add 3283173 📌 Document and edge cases in \Drupal\ckeditor5\HTMLRestrictions::getTextContainerElementList Active to this list.

CKEditor 5 seems to support block level elements within a list item or table cell, but Drupal's implementation is stripping this valid syntax per HTML 5.

See https://ckeditor.com/docs/ckeditor5/latest/features/lists/document-lists....

Or am I thinking of this incorrectly, it's technically not blocked upstream, it's blocked in our implementation. 🤔

🇺🇸United States joshuami Portland, OR

Thanks, Wim. I posted a comment (and a comment on my comment). I appreciate the link to the specific CKEditor 5 change record .

Sheepishly, I have to admit that I'm not good at following change records—there are just so many of them.

I read release notes—though not all as I don't tend to read the release notes for a branch I'm not currently using. I like xjm's suggestion to include some reference to these known issues in release notes and to the Drupal 10 upgrade handbook .

Incidentally, I could only find that handbook by searching, it might make sense to include references to that upgrade process in the next few release notes as well. Upgrade instructions are more important information than linking to the release cycle. (Or at the very least the release cycle should link to those upgrade instructions.)

In the update contributed projects section of that handbook, it makes it sound like upgraded modules will just be compatible, but I've found several modules that have a version compatible with Drupal 9 or Drupal 10 but not both. I feel like we might need a bit of documentation to help module maintainers understand that it is best practice to have a release that is compatible with both major versions at each end of life transition to aid in the upgrade process.

Maybe we could add a bit to the Drupal 9 compatibility section of the add a composer.json file page in the creating modules handbook .

🇺🇸United States joshuami Portland, OR

Another thought that I touched on in Incorrect version of CKEditor being parsed from info.yml files 🐛 Incorrect version of CKEditor being parsed from info.yml files Active , it might be good to link to some documentation that suggests possible upgrade paths in the warning.

Resolving the Composer dependencies to get the contrib ckeditor module is place is not straightforward. At a minimum, a link to the change record , the composer issue 🐛 Incorrect version of CKEditor being parsed from info.yml files Active , and the deprecated and obsolete extensions page would be helpful.

From a release standpoint, it is even possible to get this sort of warning included in Drupal 9 at this point? Or is it only possible for us to alert the developers after they've tried to upgrade to 10 or run Upgrade Status checks?

🇺🇸United States joshuami Portland, OR

+1 to adding some sort of notification to status report that would show for both Drupal 9.5 and Drupal 10 installations.

The change record by itself is not adequate to alert less experienced devs that they have a blocker to upgrading to Drupal 10 when using core's ckeditor module versus the contrib module.

My only recommendation would be to display the information as a warning rather than an error. I know that the clients I've helped establish CI/CD practice have tests that fail if "no errors found" is not in the status page. Could this be an error in Drupal 10 and a warning in Drupal 9? It would allow continued development not related to CKeditor and Core upgrades without breaking typical site-specific tests.

🇺🇸United States joshuami Portland, OR

I'm trying to help a couple clients through their CKEditor 5/Drupal 10 upgrades and have come to the conclusion there are two paths:

Option 1: While on Drupal 9, upgrade to CKeditor 5, then upgrade to Drupal 10.

Option 2: Upgrade to Drupal 10 using the contrib ckeditor module, then upgrade to CKEditor 5 at a later date.

Either option is going to require some custom work to replace functionality from CKEditor 4 contrib modules (ckeditor_acr 1.0.1, ckedtior_config 8.x-3.1), some custom CKEditor 4 widgets (for inserting a phone number, email address, columns, accordions), and some code that relied on the MediaEmbedDialogue form and the ability to edit a media item inline and change attributes.

Of the two approaches, I would prefer option 2. Upgrading core to Drupal 10 is a larger risk. The smaller risk of not having our CKEditor 4 functionality replicated is not ideal—replicating the functionality is a fair bit of work—but at least it would *just* be CKEditor.

I was just about to post that there does not seem to be a valid way to add the contrib ckeditor to the latest Drupal 9 release, but then I happened on a successful order of operations.

Running composer remove drupal/ckeditor_acr or composer remove drupal/ckeditor_config gives me an error like ckeditor_acr is not required in your composer.json and has not been removed.

If I manually remove ckeditor_acr and ckeditor_config from my composer.json, and run composer update --lock, the files for those two modules are removed. When I then try to composer require drupal/ckeditor:'^1.0', it adds the dependency to the composer.json file, but not to the composer.lock file. A new composer install gives the error:

- Required package "drupal/ckeditor" is not present in the lock file.
This usually happens when composer files are incorrectly merged or the composer.json file is manually edited.
Read more about correctly resolving merge conflicts https://getcomposer.org/doc/articles/resolving-merge-conflicts.md
and prefer using the "require" command over editing the composer.json file directly https://getcomposer.org/doc/03-cli.md#require-r

It turns out that I also needed, to remove the following:

  "provide": {
    "drupal/ckeditor": "*"
  },

This snippet was recommended in 3309026 to stop ckeditor_config's requirement of drupal/ckeditor from downloading the contrib module unexpectedly. I was able to require, enable, and reconfigure ckeditor_acr and ckeditor_config after successfully switching to the CKEditor 4 contrib module.

If anyone has a simpler upgrade path, please share. If anyone thinks this comment belongs in another queue, let me know.

🇺🇸United States joshuami Portland, OR

The improvements to images embedded via CKeditor 5 are impressive, but all other media types suffer a little bit from an upgrade to CKeditor 5.

More use cases for linked media within CKeditor 5 are embedding documents, videos, and audio that are all interactive. I'm not sure the media module is the correct place to address whether media is clickable within the text editor. Right now the touch target for showing the contextual toolbar on a media item is a thin line around the image, document, video, etc. If you miss that line and click the media, you get one of two experiences:

Unlinked Media

If the media is an image without a configured linked display, you get the contextual media toolbar.

This is pretty much what an editor would expect.

Linked Media

If the media is a remote video, clicking on the media will play the video.

If the media is a document, clicking on the media will download the document.

For any media type other than image, the loss of an "edit media" button that has a decent touch target is a huge loss. The loss of an embed dialog that allows you to configure and possibly replace the media seems to also be the result of only looking at media management from an image perspective and not considering the embed complexities for other media types.

It would be a better experience, if a user could interact with a preview of the media entity where the entire embedded item is a trigger for the contextual toolbar. This would be similar to the link contextual toolbar which opens when you click the link rather than taking you to the link destination.

Updating the issue summary with the additional use cases.

🇺🇸United States joshuami Portland, OR

For my client's use case, this is fixed in Drupal 10 with #2807629. If you need it for D9, use the patch from comment 104.

🇺🇸United States joshuami Portland, OR

I can verify this patch does indeed fix the action to use the correct permission. The tests are failing because the test doesn't expect the permission in ViewsBulkEditModifyEntityValuesTest.

Production build 0.71.5 2024