Constrain the width of aligned images, media, blockquotes etc to a max of 75%

Looks okay to me. Will manually test it, but otherwise we should add some A/B screenshots to the issue summary and get this in.

Per #2801307-110: [META] Support WYSIWYG embedding of media entities:

Note that we need to handle this not only for images, but for any media. But that's a mere/simple selector problem, so I'm not worried. :)

1. +++ b/core/modules/media/css/plugins/drupalmedia/ckeditor.drupalmedia.css
   @@ -20,3 +20,20 @@ drupal-media {
   +.cke_widget_drupalmedia.align-right,
   

   This is already a selector that works for all embedded media, yay! 👍

2. +++ b/core/modules/media/css/plugins/drupalmedia/ckeditor.drupalmedia.css
   @@ -20,3 +20,20 @@ drupal-media {
   + * When aligning left or right, there should be something beside the embed, so
   + * allow room for that in the CKEditor.
   

   I think this language needs refining. Something like the updated issue title would be better.

Updated the IS, we have screenshots, patch looks good. I think we are done here. Nice improvement!

HEAD changed, causing the patch to no longer apply. Rebased.

Would be great if we could get a usability expert to look at this issue. I'm not sure if this will actually improve the UX or have the opposite effect. If the max-width doesn't exist on the actual page, this will further divide the look and feel of content in CKEditor from what is on an actual page. If we set a 75% max-width in CKEditor, I'd expect that to exist on the actual page as well. I don't know what is the right solution to fixing this, but I'd assume it would be something similar to what we have for CKEditor uploaded images - users can decide what's the width of the element.

Moving back to needs review for input from a usability expert.

Yeah, the front-end/back-end mismatch points are definitely valid. The CKEditor iframe is smaller than the page content, so it's not a true WYSIWYG experience anyway, which is why it felt ok to get away with it, and definitely preferable to not being able to edit the text at all (or functionally so).

I did originally suggest the ability to add a width attribute in the dialog at first (as you can do with images), but that got shot down, I guess because it's not transferable between media types? There was talk of instead adding the ability to select image styles (#2061377: [drupalImage] Optionally apply image style to images uploaded in CKEditor 5), but that's a much bigger lift. (And also is fraught with a lot of "pogo-sticking" UX problems unless it comes with a visual preview of some kind, which is a DRAGON style lift. ;))

However, somehow, Image images don't let you do this. The width is constrained to the largest word, so you can't physically get into an uneditable situation:

So if the max-width: 75% is contentious, I guess we could just copy/paste whatever is happening over there.

I'm wondering if we could just make the max-width configurable in percentages or pixels? Or do we actually need a layout builder like tool in the CKEditor that allows adjusting more complex layout rules? Anyway, this feels like different issue than choosing image style.

We also talked about that (like a 25%/50%/75% selector), but that *also* seemed like a much bigger lift than hard-coding something for now (to prevent you from being unable to edit your content at all) and making it configurable later in a follow-up.

If the max-width doesn't exist on the actual page

I think we should add it to the actual page as well. Any reason not to add it to a library attached by MediaEmbed?

We also talked about that (like a 25%/50%/75% selector)

Right. See #2801307-106: [META] Support WYSIWYG embedding of media entities. Let's open an issue for that if there isn't one already.

25%/50%/75% selector...Let's open an issue for that

By the way, when configuring a 2 column section in Layout Builder, the choices are: 25%, 33%, 50%, 67%, 75%, so maybe in that new issue, we want to expand to that same list for consistency (and cause 1/3 and 2/3 are nice layouts).

This seems like a nice improvement generally. I like the fact that the CKEditor and public-facing site renders the same way.

1. +++ b/core/modules/media/css/filter.media_embed.css
   @@ -18,3 +18,20 @@
   + * Constrain the width of embedded media to a max of 75% when left or right
   + * aligned.
   
   +++ b/core/modules/media/css/plugins/drupalmedia/ckeditor.drupalmedia.css
   @@ -30,3 +30,20 @@ drupal-media {
   + * Constrain the width of embedded media to a max of 75% of the text editor's
   + * viewport.
   

   We should explain relation between these styles.

2. +++ b/core/modules/media/css/filter.media_embed.css
   @@ -18,3 +18,20 @@
   +.media.align-right,
   +.media.align-left {
   

   Does this actually have to be specific to media? Wouldn't it make sense to apply this rule more globally?

3. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
   index f3486bcc8b..78ba7c76a6 100644
   --- a/core/themes/stable/css/media/filter.media_embed.css
   
   --- a/core/themes/stable/css/media/filter.media_embed.css
   +++ b/core/themes/stable/css/media/filter.media_embed.css
   

   1. I'm not sure if we can actually do this in stable theme since this will affect how a lot of themes render media. What if someone expects the previous behavior to be the correct behavior 🤷‍♂️ We should open at least a change record for this.

   2. This doesn't feel like this belongs to Stable. This seems more like something that should be in Classy. However, concern 1 is applicable to Classy as well.

1. #9:
   

   this will further divide the look and feel of content in CKEditor from what is on an actual page.

   The goal never was nor is to have a perfect preview. That's impossible anyway. It's an approximation. If the preview itself is getting in the way of text editor usability, which is the case here, then making the preview "less pixel perfect" is perfectly reasonable IMO.

2. #9:
   

   it would be something similar to what we have for CKEditor uploaded images - users can decide what's the width of the element.

   We definitely cannot do that — that's a problem with the existing CKEditor + Drupal 8 + image uploading integration actually: the in-place resizing ability gives the user the illusion of control and precision, yet the place where it will actually be viewed by end users is completely different.

3. #14: but a percentage max-width is relative to the container element. On the actual page, that element may have margins that the CKEditor <iframe> body does not have. So I'm concerned about restricting it to 75% also on the front end (via the filter), rather than only in CKEditor <iframe> instances.
4. #17: that does look odd — thanks for opening a new issue, reviewed that.
5. #18 + #19: I bet you can also do this with Drupal <=8.7's image uploads plus data-align. We need to verify that, it impacts how we solve this. #25 did investigate this 🥳
6. #25 contains changes from another issue.

Updating issue title per changed direction. The issue summary still needs an update.

I like the direction #25 is going to.

I'm still concerned about the backwards compatibility of this change. I'm wondering if we should just ship this as part of the modules and remove it in Stable. AFAIK, we haven't provided Stable overrides for CKEditor CSS files. This would mean that themes extending Stable wouldn't receive this change in the frontend but could still benefit from the CKEditor improvement. This would lead into inconsistency but maybe we could recommend themes extending stable to copy this CSS file to their theme.

1. #26.1 I understand that the CKEditor cannot have a perfect preview. Setting this type of rules change the way the content gets rendered. If we make this type of deviations, in my opinion, the preview would no longer be even a close approximation. It would be particularly confusing in scenarios where the CKEditor is narrower, but the media is still rendered more nicely in the preview than on the page.
2. #26.2 Should we open issue to try to come up with a better solution?

3. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/DrupalImage.php
   --- /dev/null
   +++ b/core/modules/filter/css/filter.align.css
   

   👍 We should ship this in the module since this is needed for the module functionality.

4. +++ b/core/modules/media/css/plugins/drupalmedia/ckeditor.drupalmedia.css
   @@ -30,3 +30,20 @@ drupal-media {
   +.cke_widget_drupalmedia.align-right .align-right,
   

   Would be awesome if we could fix the JavaScript so that we don't need these CSS overrides.

5. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/DrupalImage.php
   @@ -85,4 +125,14 @@ public function validateImageUploadSettings(array $element, FormStateInterface $
   +  public function getCssFiles(Editor $editor) {
   +    return [
   +      $this->moduleExtensionList->getPath('filter') . '/css/filter.align.css',
   +    ];
   +  }
   
   +++ b/core/modules/media/src/Plugin/CKEditorPlugin/DrupalMedia.php
   @@ -121,6 +121,7 @@ public function isEnabled(Editor $editor) {
   +      $this->moduleExtensionList->getPath('filter') . '/css/filter.align.css',
   

   🤔We should probably open a follow-up to discuss if we should provide Stable override for CKEditor CSS files.

+++ b/core/modules/ckeditor/tests/src/Kernel/CKEditorTest.php
--- /dev/null
+++ b/core/profiles/demo_umami/themes/umami/css/components/filter/filter.align.css

Why are we duplicating these rules to Umami? Umami is extending Classy so it should receive these styles from there.

I think we should create a change record and then ask for input from a release manager on whether they think this would be too disruptive.

+++ b/core/themes/classy/css/components/filter.align.css
@@ -0,0 +1,52 @@
+ * Within the WYSIWYG, if the alignment class is applied within a widget,
+ * unset the rules.
...
+.cke_widget_wrapper.align-right .align-right {
...
+.cke_widget_wrapper.align-left .align-left {
...
+.cke_widget_wrapper.align-right .align-right {

This still seems like a bug to me. Would it be possible to solve it so that this wouldn't be necessary?

Discussed this issue with @Wim Leers, @oknate, and @phenaproxima. We focused on discussing where to ship this improvement.
The current solution fixes the bug in Classy because, for example, Umami renders some media so that text becomes hard to read. The bug fix is made of layout level CSS which we don't generally ship as part of Stable theme or modules, and therefore we felt that Classy was a natural place to ship this.

This goes against what @Wim Leers was recommending in his role as subsystem maintainer of the Text Editor and CKEditor modules. He was recommending this to be fixed only in CKEditor because CKEditor preview is only an approximation of how the actual page will render. He also wanted to fix this for sites running themes not extending Classy.

We came with a compromise to ship the max-width in CKEditor globally so that all themes will receive this improvement. Besides this, we want to ship the same styles for frontend themes extending Classy as a default value. We don’t want to ship the default frontend styles in Stable or modules since they are supposed to be unopinionated.

We also agreed that #36 could be moved to a follow-up, since it's a pre-existing issue with the media embed markup. Since the media embed has conceptually only one element that should be aligned, the class should be only set on the wrapper. The latest patch already includes a workaround for this, but it has some potential side effects.

I quickly talked to @lauriii to hash out the implementation details. Here's what we'll do:

• We will add a new library to the Filter module, called filter/align. (This was previously implemented in #25.)
• Since this is a new library, it will also be copied into Stable.
• The DrupalImage and DrupalMedia CKEditor plugins will always attach this library, by referencing its CSS file directly. (The one that ships with filter.module, that is.)
• Whether the library is attached on the front end is going to vary by theme. The Stable theme will not do anything. Classy, on the other hand, will need to implement hook_element_info_alter() and give the processed_text element a new pre-render function (ideally at the beginning of the pre-render stack) which attaches the filter/align library to the element if the format being used has filter_align enabled.

1. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/DrupalImage.php
   @@ -16,7 +20,43 @@
   +   * @param string $plugin_id
   +   *   The plugin_id for the plugin instance.
   

   Nit: This can just be "The plugin ID".

2. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/DrupalImage.php
   @@ -16,7 +20,43 @@
   +   * @param \Drupal\Core\Extension\ModuleExtensionList $extension_list_module
   +   *   The module extension list.
   

   Total nitpick: Can the variable be named $module_extension_list? It's just jarring to see its grammar shift around in the same line.

3. +++ b/core/modules/filter/css/filter.align.css
   @@ -0,0 +1,52 @@
   +/**
   + * Add space between embed and surrounding element (such as a paragraph).
   + */
   

   "Embed" is presuming that we're embedding something. I think this should be "Add space between the aligned element and whatever might surround it (such as a paragraph)."

4. +++ b/core/modules/filter/css/filter.align.css
   @@ -0,0 +1,52 @@
   +/**
   + * Within the WYSIWYG, if the alignment class is applied within a widget,
   + * unset the rules.
   + */
   +.cke_widget_wrapper.align-left .align-left,
   +.cke_widget_wrapper.align-right .align-right {
   +  max-width: unset;
   +}
   +
   +.cke_widget_wrapper.align-left .align-left {
   +  margin-right: unset;
   +}
   +
   +.cke_widget_wrapper.align-right .align-right {
   +  margin-left: unset;
   +}
   

   As @oknate pointed out, this can have undesirable edge-case side effects if there are descendant elements which have alignment. I think we might want to open a follow-up, at the very least, and reference it here with a @todo.

   Whether we want to fix it here, now, is up to @lauriii.

5. +++ b/core/modules/filter/css/filter.align.css
   @@ -0,0 +1,52 @@
   +/**
   + * Set the margin on figure element to override user agent style sheet.
   + */
   +figure.align-right {
   +  margin: 0 0 1.2em 1em;
   +}
   +
   +figure.align-left {
   +  margin: 0 1em 1.2em 0;
   +}
   

   This feels like it could use a @see here...are we accounting for this kind of thing (i.e., default margins on a figure element) anywhere else in core?

6. +++ b/core/profiles/demo_umami/themes/umami/css/components/filter/filter.align.css
   @@ -0,0 +1,18 @@
   +/**
   + * @file
   + * Align filter: default styling for displaying aligned images, media, etc.
   + */
   

   Wouldn't we also want to copy the rest of the styles from the original filter.align.css into this style sheet as well? Unless I'm misunderstanding how libraries-extend works (which I probably am).

7. +++ b/core/profiles/demo_umami/themes/umami/css/components/filter/filter.align.css
   @@ -0,0 +1,18 @@
   + * filter.align.css in classy.  We're adding a bit of top margin to align
   

   Supernit: 'classy' should be 'Classy'.

8. +++ b/core/themes/classy/classy.theme
   @@ -0,0 +1,40 @@
   +  if (array_key_exists('text_format', $info)) {
   +    $info['text_format']['#pre_render'][] = 'classy_pre_render_text_format';
   +  }
   

   I'm not sure this is what we want...we want to apply the library any time processed text with filter_align is rendered out in any context. The text_format element produces a CKEditor instance, which will bring in the appropriate CSS by way of the CKEditor plugins we're using. All of this is why we need to alter the processed_text element instead. This means we are going to need test coverage, I think.

9. +++ b/core/themes/classy/classy.theme
   @@ -0,0 +1,40 @@
   +function classy_pre_render_text_format($element) {
   +
   +  $format = \Drupal::entityTypeManager()
   

   There's an extra blank line here, and $element should be type hinted.

+++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/DrupalImage.php
@@ -85,4 +125,14 @@ public function validateImageUploadSettings(array $element, FormStateInterface $
+  /**
+   * {@inheritdoc}
+   *
+   */
+  public function getCssFiles(Editor $editor) {
+    return [
+      $this->moduleExtensionList->getPath('filter') . '/css/filter.align.css',
+    ];
+  }

I didn't even consider this before, but @oknate reminded me that hook_ckeditor_css_alter() exists, and IMHO it is what we should use to add this CSS (assuming the format associated with the editor has filter_align enabled). That way, alignment stuff can be used without needing to add it to specific plugins; I think that will be both cleaner and more flexible for non-core code.

But that said, I think we need to really test four overall things here:

1. When CKEditor is loaded for an input format that uses filter_align, the CSS should always be present. (We can determine this by checking for the presence of filter.align.css, rather than any specific rules.)
2. But, if that format doesn't have filter_align, it shouldn't be.
3. Conversely, when formatted text is rendered out (using the processed_text render element), the filter_align CSS should be added as well, if the format is using it...and only if we're using Classy as the front-end theme, or anything derived from it.
4. Which means that, if the front-end theme is not Classy-based, then the CSS should not be added.

This is blocked on #3072279: Stop generating align-* classes for preview in DrupalMedia CKEditor plugin, but that issue just became unblocked. So, decreasing the postponement counter.

Center align doesn't work, which I believe is a pre-existing condition and is out of scope. Should we try to fix this here?

😨 Reproduced, this indeed exists in HEAD!

This is caused by

@media all and (min-width: 560px) {
  .node .field--type-image {
    float: left; /* LTR */
    margin: 0 1em 0 0; /* LTR */
  }
  [dir="rtl"] .node .field--type-image {
    float: right;
    margin: 0 0 0 1em;
  }
  .node .field--type-image + .field--type-image {
    clear: both;
  }
}

in core/themes/bartik/css/components/field.css. How the hell did we miss this before?

Let's open a separate issue for that 🙏

1. +++ b/core/modules/filter/css/filter.align.css
   @@ -0,0 +1,44 @@
   +/**
   + * Constrain the width of aligned elements to a max of 75% when left or right
   + * aligned.
   + */
   +.align-right,
   +.align-left {
   +  max-width: 75%;
   +}
   

   🤔🤔🤔 I'm still concerned why we're doing this on the front end too, instead of just inside CKEditor instances. The issue summary does not explain the reasoning. Nor does the CSS file. The Needs issue summary update tag is already present, so I can't add that.

2. +++ b/core/modules/filter/filter.module
   @@ -14,6 +14,7 @@
   +use Drupal\editor\Entity\Editor;
   

   🤓 Unused, should be removed.

3. +++ b/core/profiles/demo_umami/tests/src/Functional/DemoUmamiProfileTest.php
   @@ -239,4 +240,26 @@ protected function drupalLoginWithPassword(AccountInterface $account, $password)
   +   * Tests that umami\align library is added conditionally.
   
   +++ b/core/tests/Drupal/FunctionalTests/Theme/ClassyTest.php
   @@ -35,4 +36,45 @@ public function testRegressionMissingMessagesCss() {
   +   * Tests that classy\align library is added conditionally.
   

   🤓 The backslash should be a forward slash.

4. +++ b/core/profiles/demo_umami/tests/src/Functional/DemoUmamiProfileTest.php
   @@ -239,4 +240,26 @@ protected function drupalLoginWithPassword(AccountInterface $account, $password)
   +  }
    }
   

   🤓 Missing blank line between these closing braces.

5. +++ b/core/profiles/demo_umami/themes/umami/css/components/ckeditor.css
   @@ -0,0 +1,9 @@
   +img {
   +  max-width: 100%;
   +  height: auto;
   +}
   
   +++ b/core/profiles/demo_umami/themes/umami/umami.info.yml
   @@ -32,3 +33,6 @@ regions:
   +ckeditor_stylesheets:
   +  - css/ckeditor.css
   

   🤔 I'm surprised we need custom styling for Umami. This IMHO needs documentation. Why do we need this for Umami, but not for any other theme?

6. +++ b/core/profiles/demo_umami/themes/umami/css/components/filter/filter.align.css
   @@ -0,0 +1,39 @@
   + * Set the margin on figure element to override user agent style sheet and
   + * filter.align.css in Classy.  We're adding a bit of top margin to align
   
   +++ b/core/profiles/demo_umami/themes/umami/umami.info.yml
   @@ -12,6 +12,7 @@ libraries:
   +  classy/align: umami/align
   

   This is a contradiction: the CSS cannot be overriding Classy CSS if Classy CSS is not being loaded at all.

7. +++ b/core/profiles/demo_umami/themes/umami/umami.libraries.yml
   @@ -50,6 +50,11 @@ global:
   +      css/components/filter/filter.align.css: { weight: -10 }
   
   +++ b/core/themes/classy/classy.libraries.yml
   @@ -97,3 +97,9 @@ user:
   +      css/components/filter.align.css: { weight: -10 }
   

   🤔 Why -10?

8. +++ b/core/profiles/demo_umami/themes/umami/umami.libraries.yml
   @@ -124,3 +129,4 @@ demo-umami-tour:
   +
   

   🤓 Unnecessary change.

9. +++ b/core/themes/bartik/css/components/field.css
   @@ -48,6 +48,17 @@
   +   * If the image is align left, undo the above rule.
   

   🤓 s/align/aligned/

10. +++ b/core/themes/bartik/css/components/field.css
    @@ -48,6 +48,17 @@
    +   * See core/modules/filter/css/filter.align.css
    

    🤓 s/See/@see/

11. +++ b/core/themes/bartik/css/components/field.css
    @@ -48,6 +48,17 @@
    +  .node .media.align-left .field--type-image {
    +    float: none;
    +    margin-right: 0px;
    

    🤔 Are we sure this is the correct approach? Can't we instead make the rule this is overriding a bit more strict?

12. +++ b/core/themes/bartik/css/components/field.css
    @@ -48,6 +48,17 @@
    +  }
    +
    +
       [dir="rtl"] .node .field--type-image {
    

    🤓 Two blank lines, should be one.

1. #75.5: Agreed! 😊 But often easier said than done.
2. #75: 👍
3. My concern about doing this on the front end too still stands. I believe we should do this only in CKEditor iframe instances. I'm fine adding CSS to Classy for this too, but I do believe we should not try to closely match them — the CKEditor iframe viewport is always going to be much narrower, so there's no point IMHO. Assigning to @lauriii in his role as front end framework manager for making a decision :)

4. +++ b/core/modules/filter/css/filter.align.css
   @@ -0,0 +1,44 @@
   + * @code
   + *   margin-inline-start: 40px;
   + *   margin-inline-end: 40px;
   + * @endcode
   

   What is this doing here? According to https://developer.mozilla.org/en-US/docs/Web/CSS/margin-inline-start, these are experimental CSS properties. But surprisingly widely supported.

   But then there's no further explanation as to why this is listed here. Do we want to use this in the future?

1. +++ b/core/themes/classy/src/ProcessedText.php
   @@ -0,0 +1,50 @@
   +/**
   + * Class to hold trusted callback for processed text.
   + */
   +class ProcessedText implements TrustedCallbackInterface {
   

   Maybe worth asking @lauriii, but: should this class be marked @internal? It's doing things that are totally specific to Classy.

2. +++ b/core/themes/classy/src/ProcessedText.php
   @@ -0,0 +1,50 @@
   +    $format = \Drupal::entityTypeManager()
   +      ->getStorage('filter_format')
   +      ->load($element['#format']);
   

   Nit: This can be using FilterFormat::load().

Apart from these minor things, the code looks good.

I'm removing the "needs usability review" tag, because I don't think this needs usability review. It's not really introducing any new patterns, or modifying existing ones -- it's trying to prevent a usability problem. I added the tag back #10, but a lot has happened since then which I think makes the tag irrelevant.

That only leaves my concern in #84.3. It's already assigned to @lauriii for that. Tagging Needs subsystem maintainer review to stress that even more.

I'm fine adding CSS to Classy for this too, but I do believe we should not try to closely match them — the CKEditor iframe viewport is always going to be much narrower, so there's no point IMHO.

For example, just testing Umami and Claro, it looks like CKEditor is rendered much wider than the actual page. Content can be also rendered in different sizes and formats - not all content is rendered as a full page. Even if this is the case for the Node entity type, CKEditor can be used for other entity types. Serving a max-width as part of the alignment everywhere makes sense because in both use cases, not having a max-width could lead to poor UX.

Also, making this change only in CKEditor would mean that the layout behaves differently depending on whether you're viewing the content on the frontend or in CKEditor. I understand that CKEditor cannot replicate the page 100% and it doesn't try to achieve that, but this type of layout rules are pretty visible. I think it's great if we can have consistency. Classy is a theme with sensible defaults and since we think this improves UX in some use cases without any real downsides, I don't really understand why we wouldn't make this change?

Content can be also rendered in different sizes and formats - not all content is rendered as a full page. Even if this is the case for the Node entity type, CKEditor can be used for other entity types.

Right … and that to me is a reason to not do theme-specific width restrictions. Because CKEditor <iframe> instances only know they're operating on a text field. So there is no way for a theme to provide different width restrictions for e.g. a block body field vs a node body field vs a product description field.

Serving a max-width as part of the alignment everywhere makes sense because in both use cases, not having a max-width could lead to poor UX.

Sure, but that's not my concern. Let me repeat #84.3: I'm fine adding CSS to Classy for this too, but I do believe we should not try to closely match them.

Classy is a theme with sensible defaults and since we think this improves UX in some use cases without any real downsides, I don't really understand why we wouldn't make this change?

Again, just like I said in #84.3: I'm fine with Classy also shipping with some width restrictions. But it's important to stress that there's no 1:1 relationship, no promise of an exact match. My concern here is with establishing an expectation. And since the current patch adds the exact same CSS for both filter.module and classy.theme, that is a concern for me.

+++ b/core/themes/classy/classy.theme
@@ -0,0 +1,17 @@
+function classy_element_info_alter(array &$info) {
+  if (array_key_exists('processed_text', $info)) {
+    $info['processed_text']['#pre_render'][] = [ProcessedText::class, 'preRender'];
+  }
+}

+++ b/core/themes/classy/src/ProcessedText.php
@@ -0,0 +1,49 @@
+  public static function preRender(array $element) {
...
+    if ($filter_align && $filter_align->status) {
+      $element['#attached']['library'][] = 'classy/align';
+    }

If we're going to do this, we need clear documentation for why this cannot use libraries-override to override filter/align. This is very complex.

(I didn't point this out before because I wasn't sure whether the new alignment CSS in Classy would stay. It sounds like it will now, so pointing it out now.)

To clarify #90: I see three possible solutions:

1. No classy-specific CSS.
2. classy-specific CSS that does not match filter's CSS + document explicitly why.
3. classy-specific CSS that matches filter's CSS + document even more explicitly why.

(In order of my personal preference — obviously we're not going with the first option based on @lauriii's feedback 🤓.)

In chatting with @lauriii and @phenaproxima, I decided to apply the patch and test it manually. I spotted another problem.

1. +++ b/core/themes/stable/stable.info.yml
   @@ -117,6 +117,10 @@ libraries-override:
   +  filter/align:
   +    css:
   +      component:
   +        css/filter.align.css: css/filter/filter.align.css
   

   This does not work (the stable override is never used), because …

2. +++ b/core/modules/filter/filter.libraries.yml
   @@ -31,3 +31,9 @@ caption:
   +align:
   +  version: VERSION
   +  css:
   +    component:
   +      css/filter.align.css: {}
   

   This is never used!

3. +++ b/core/modules/ckeditor/ckeditor.module
   @@ -60,12 +60,21 @@ function ckeditor_ckeditor_css_alter(array &$css, Editor $editor) {
   +  if ($filters->has('filter_align') && $filters->get('filter_align')->status) {
   +    $css[] = drupal_get_path('module', 'filter') . '/css/filter.align.css';
   +  }
   

   That is never used because core/modules/filter/css/filter.align.css is only ever loaded here, via CKEditor's <iframe>-only stylesheets loading, not through Drupal asset library loading.

4. Note that 1+2 could be addressed by having FilterAlign attach the filter/align library, which would also remove the need for the pretty complex hack in Classy to be able to attach CSS. #31 stopped doing that.

   @oknate did that in response to my #26.3, where I raised concern we'd do exactly the same on the front end as what we do in CKEditor instances. But … we're doing that anyway in the current patch, so what's the point of this convoluted alternative way of loading CSS? 🤷‍♂️

   @lauriii has said we should also be loading alignment CSS for Classy. Fine. So then let's let Classy do a libraries-override for filter/align?

I know we've gone back-and-forth a lot here. My primary concern is expectations for the content author. My secondary concern is grokkability of what a front end developer sees going on in Classy. The points in this review hopefully bring us back on track to solve the last few concrete concerns before we can ship this. 🤞

Ok, just a little history lesson here, to maybe help with arriving at agreement on the plan of attack.

This issue was originally filed, because if you insert a media into CKEditor, and it's of a certain size, it makes your text reflow like this:

...now you basically can't update your text, because it's illegible.

(I was not able to reproduce this problem with Image images, either because I did something wrong, or because images do some other funky-special CSS thing, or whatever.)

That's it. That's the bug.

To fix it, we needed to either a) allow people to resize their media in the WYSIWYG editor, or b) *force* a size such that this "illegible text smooshing" situation can't happen.

For a), we already committed #3074608: Optionally allow choosing a view mode for embedded media in EditorMediaDialog, so there is now a workaround, albeit one quite advanced. We also discussed offering drag handles for parity with images, but this caused Wim to flip a table while guzzling the blood of an unstructured glomp of text. ;) We also discussed a width percentage selector, captured at #3081303: Allow media embed width constraint to be configurable. This would actually be a superior approach to this, but we tabled it since it sounded like it was going to be a can of worms.

Since we didn't know if the two viable alternatives under a) would come together in time, we opened *this* issue to hard-code a sensible default in CKEditor media embeds, in theory a "one line patch" (lol) to satisfy the problem. Clearly, this has not borne out to be the case... and we have now escalated from "can of worms" to like... "basket of spiders" territory. 😂

So if #3081303: Allow media embed width constraint to be configurable looks like it will be easier to resolve all the concerns here (which it might be, since the percentage chosen would be the same on front-end + back-end, thus negating inconsistency concerns), then let's maybe do that instead. But still only in a "should have" capacity, since there is now a workaround in 8.8.x for the original bug that was identified.

(I worried that these two—view modes and width percentages—might be incompatible, but Alex pointed out they are not, since you can still take a "thumbnail" media size and make it 50% of that.)

I'm also just as happy to "won't fix" this one, and leave #3081303: Allow media embed width constraint to be configurable around for "if we ever get around to it." Or if this one is 99.9% of the way there, finish it off.

Basically, whatever causes the least amount of angst, and allows us to hyper focus efforts on must-haves for this initiative, +1.

We also discussed offering drag handles for parity with images, but this caused Wim to flip a table while guzzling the blood of an unstructured glomp of text. ;)

Perhaps part that, but far more importantly: A) it'd only work for image media, B) how would we pass that width and height in pixels on to the view mode, and then on to the field formatter, and then on to the image style, C) it would not work at all with responsive images.

we have now escalated from "can of worms" to like... "basket of spiders" territory. 😂

😂

(I worried that these two—view modes and width percentages—might be incompatible, but Alex pointed out they are not, since you can still take a "thumbnail" media size and make it 50% of that.)

+1

I think this has been close for a while, but those last few things are important for long-term maintainability. And conveying the nuances of those impacts is where I've been unsuccessful at convincing and/or explaining it to others.

So let's turn this around by letting me roll a counterproposal patch that is still 90% the same as the latest patch. Then I can hear from @oknate, @phenaproxima, @lauriii, @effulgentsia and @webchick where that is falling short.

Stay tuned 😊

So let's turn this around by letting me roll a counterproposal patch that is still 90% the same as the latest patch. Then I can hear from @oknate, @phenaproxima, @lauriii, @effulgentsia and @webchick where that is falling short.

Did that.

To aid reviewers, here's the rationale for each of the changes in #96's interdiff:

1. +++ b/core/modules/ckeditor/ckeditor.module
   @@ -72,6 +72,7 @@ function ckeditor_ckeditor_css_alter(array &$css, Editor $editor) {
   +  // @see core/modules/filter/filter.libraries.yml
   

   This stresses the relationship between the "load additional CSS files into the CKEditor <iframe> instance" logic and the asset loading logic in all other situations.

2. +++ b/core/modules/filter/src/Plugin/Filter/FilterAlign.php
   @@ -40,7 +40,12 @@ public function process($text, $langcode) {
   -      $result->setProcessedText(Html::serialize($dom));
   +      $result->setProcessedText(Html::serialize($dom))
   +        ->addAttachments([
   +          'library' => [
   +            'filter/align',
   +          ],
   +        ]);
   

   This ensures that the asset library is actually used, which means that stable's override of this also actually works.

3. +++ b/core/tests/Drupal/FunctionalTests/Theme/ClassyTest.php
   @@ -57,7 +55,7 @@ public function testFilterAlignCss() {
   -        'value' => 'Highly illogical.',
   +        'value' => '<blockquote data-align="center">Highly illogical.</blockquote>',
   

   Now that we're loading the asset library in the alignment filter, the asset library is also only loaded when it's needed. So, we need to actually trigger the use of the alignment filter.

4. +++ b/core/themes/classy/classy.info.yml
   @@ -26,5 +26,8 @@ libraries-extend:
   +libraries-override:
   +  filter/align: classy/align
   
   +++ /dev/null
   @@ -1,17 +0,0 @@
   -function classy_element_info_alter(array &$info) {
   -  if (array_key_exists('processed_text', $info)) {
   -    $info['processed_text']['#pre_render'][] = [ProcessedText::class, 'preRender'];
   -  }
   
   +++ b/core/themes/classy/css/components/filter.align.css
   @@ -1,6 +1,6 @@
   diff --git a/core/themes/classy/src/ProcessedText.php b/core/themes/classy/src/ProcessedText.php
   

   Rather than doing something complex to always load Classy's override of the (previously unused …) filter/align asset library, we can just use the existing libraries-override infrastructure instead.

5. +++ b/core/themes/classy/css/components/filter.align.css
   @@ -1,6 +1,6 @@
   - * Align filter: default styling for displaying aligned images, media, etc.
   + * Override of core/modules/filter/css/filter.align.css.
   

   I still don't really see the value of having Classy override this if the CSS is 100% the same, but if we're going to do this, then this is what I think we need to change to avoid confusion about why we're doing this at all: document that it is an override.

+++ b/core/modules/filter/src/Plugin/Filter/FilterAlign.php
@@ -40,7 +40,12 @@ public function process($text, $langcode) {
-      $result->setProcessedText(Html::serialize($dom));
+      $result->setProcessedText(Html::serialize($dom))
+        ->addAttachments([
+          'library' => [
+            'filter/align',
+          ],
+        ]);

requires some test assertions to be updated. Did that.

1) Drupal\Tests\block\Functional\BlockCacheTest::testCachePerRole
Drupal\Core\Config\UnmetDependenciesException: Configuration objects provided by block_test have unmet dependencies: block.block.test_block (classy)

These failures do not make sense!

Oh but wait …

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -387,16 +387,18 @@ protected function setUp() {
-    $container = $this->initKernel(\Drupal::request());
+    $this->container = $container = $this->initKernel(\Drupal::request());
 
     // Initialize and override certain configurations.
     $this->initConfig($container);
 
-    $this->installDefaultThemeFromClassProperty($container);
-
     // Collect modules to install.
     $this->installModulesFromClassProperty($container);
 
+    // Install the default theme using the new container at this point
+    // $container can be stale as module installation will rebuild it.
+    $this->installDefaultThemeFromClassProperty($this->container);

+++ b/core/tests/Drupal/TestSite/Commands/TestSiteInstallCommand.php
@@ -198,10 +198,12 @@ protected function installDrupal() {
     $this->doInstall();
     $this->initSettings();
-    $container = $this->initKernel(\Drupal::request());
+    $this->container = $container = $this->initKernel(\Drupal::request());
     $this->initConfig($container);
-    $this->installDefaultThemeFromClassProperty($container);
     $this->installModulesFromClassProperty($container);
+    // Install the default theme using the new container at this point
+    // $container can be stale as module installation will rebuild it.
+    $this->installDefaultThemeFromClassProperty($this->container);
     $this->rebuildAll();

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -564,10 +564,12 @@ public function installDrupal() {
     $this->initSettings();
-    $container = $this->initKernel(\Drupal::request());
+    $this->container = $container = $this->initKernel(\Drupal::request());
     $this->initConfig($container);
-    $this->installDefaultThemeFromClassProperty($container);
     $this->installModulesFromClassProperty($container);
+    // Install the default theme using the new container at this point
+    // $container can be stale as module installation will rebuild it.
+    $this->installDefaultThemeFromClassProperty($this->container);
     $this->rebuildAll();
   }

ARGH! I accidentally included these in #99. Juggling many patches. Sorry about that 🤦‍♂️

Alright, now I'm curious about @phenaproxima's, @oknate's and @lauriii's feedback :)

#108: I'm interested in hearing how/where this results in a different experience than #88, and especially where you think it's inferior. I'm glad to hear you're okay with it, but I'd love to read more nuance. :)

Bump. This is the last remaining should-have of #2801307: [META] Support WYSIWYG embedding of media entities. Let's get this done :)

I haven't closely read every comment in here, but I see no explanation of this in the summary, nor on a quick skim of recent-ish comments, so I'll ask here:

+++ b/core/tests/Drupal/FunctionalTests/Theme/ClassyTest.php
@@ -27,4 +28,43 @@ public function testRegressionMissingMessagesCss() {
+   * Tests that the classy/align library override of filter/align is loaded.

+++ b/core/themes/classy/classy.info.yml
@@ -26,5 +26,8 @@ libraries-extend:
+libraries-override:
+  filter/align: classy/align
+

+++ b/core/themes/classy/classy.libraries.yml
@@ -26,6 +26,12 @@ base:
+align:
+  version: VERSION
+  css:
+    component:
+      css/components/filter.align.css: {}
+

+++ b/core/themes/classy/css/components/filter.align.css
@@ -0,0 +1,25 @@
+/**
+ * @file
+ * Override of core/modules/filter/css/filter.align.css.
+ */

+++ b/core/themes/stable/css/filter/filter.align.css
@@ -0,0 +1,25 @@
+/**
+ * @file
+ * Align filter: default styling for displaying aligned images, media, etc.
+ */

+++ b/core/themes/stable/stable.info.yml
@@ -117,6 +117,10 @@ libraries-override:
+  filter/align:
+    css:
+      component:
+        css/filter.align.css: css/filter/filter.align.css

What gives? ;)

A) I thought we weren't allowed to touch Stable and Classy except for critical bugs.

B) The "overrides" are completely identical to the filter module's upstream CSS. We're not overriding anything here. We're just jumping through a lot of hoops, apparently for nothing.

Can we either rip all this complication out of the patch, or document why we need it (either in the issue summary, the code, or both)?

Otherwise, big +1 to getting this in. It is indeed an important fix for a potentially common usability problem.

Thanks!
-Derek

@seanB, @WimLeers, @oknate, @phenaproxima, @webchick @lauriii, @dww - Media in 8.8.0 ROCKS! Thank you so much for all of the time and energy that went into making this happen! I was just curious where things stand re: the status of this issue. As you probably know, the contrib Media module allows for the setting of the horizontal and vertical space inside the image property dialogue (screenshot below), but based on this thread I understand the complexities of providing this type of functionality in core. Is the patch in #104 the proposed solution? The video with the patch in #108 seems great to me.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.