[drupalMedia] Support alignment on <drupal-media>

Wow! 3 people on the same merge request — loving the collaboration here! :D

@lauriii opened a blocking issue: #3261061 — and I think I just unblocked you there: #3261061-3: Allow filter condition to depend on multiple filters.

needs a rebase,

If I understood correctly the code was a copy/paste from ckeditor imagestyle plugin. Code looks very similar so I don't think there are problems with it (it seems to works as intended). Got at couple of small questions

Looks good to me now, thanks :)

Left a bunch of questions, and found one significant bug.

The bug I found wasn't an actual bug, but I did uncover a weakness in investigating that: we have no upgrade path test coverage proving that <drupal-media … data-align> actually works. And we should do that as part of this issue, to ensure that we do not have to touch upon this again.

So created #3261712: Expand SmartDefaultSettingsTest to also test a format + editor with media embedding to establish test coverage of the state in HEAD, this MR will be able to remove the data-align from the expected generated configuration for ckeditor5_sourceEditing.

Yep, and as expected and intended, this failure occurred thanks to #3261712: Expand SmartDefaultSettingsTest to also test a format + editor with media embedding:

Testing Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest
.....F..... 11 / 11 (100%)

Time: 00:14.780, Memory: 4.00 MB

There was 1 failure:

1) Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest::test with data set "basic_html with media_embed added => needed => supported through sourceEditing (3 upgrade messages)"

The test coverage now proves that this solves a need! 🥳

Wanted to RTBC this, but then realized the JS hadn't gotten a thorough review yet. So did that just now, and ended up with a range of questions 🤓

No more remarks. Huge leap forward, and preparing us well for the future 👍

Epic work here :D

Not sure if the patch itself needs any changes yet, but the issue summary does not seem indicative of the solution provided. Setting to NW for visibility on that, but I'll still go ahead and look at the patch.

Something I ran into while testing that does not need to be addressed in this issue, but should be mentioned in case other people run into it.

If GHS has <drupal-media data-align> configured, but Align Images isn't enabled - if it is later enabled, the allowed HTML tags will add unwanted allowed values of 0 and 1 instead of simply allowing the attribute

This will be fixed when we land #3228334: Refactor HTMLRestrictionsUtilities to a HtmlRestrictions value object

1. +++ b/core/modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/drupalelementstyle.js
   @@ -0,0 +1,52 @@
   + * This plugin is internal because it was originally written for adding
   + * `data-align` support to `<drupal-media>`, without tightly coupling it to
   + * `<drupal-media>`. The intent is to make this plugin a starting point for
   + * adding `data-align` support to other elements, because the `FilterAlign`
   + * filter plugin PHP code also does not limit itself to a specific HTML element.
   + * Nor is this plugin limited to just `data-align`, because other filters
   + * plugins on the server side will also need to provide a great authoring
   + * experience. Even supporting `data-align` just requires configuration to be
   + * specified. The intent for this CKEditor 5 plugin is hence to make it simple
   + * for more (PHP) filters to provide a good authoring experience without the
   + * need for additional JavaScript code.
   

   This gets pretty confusing, could it get another pass? Some of the context might not be necessary - perhaps some of it could be summarized as "This is marked @internal because although it is currently part of drupalMedia, the intent is to make it available to other elements to provide the following benefits:
   - benefit 1
   - benefit 2
   etc."

2. +++ b/core/modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/drupalelementstyle.js
   @@ -0,0 +1,52 @@
   + * To be able to change element styles in the UI, the model element needs to
   + * have a toolbar where the element style buttons can be displayed.
   

   Could this be rephrased as something like "This provides a toolbar making it possible to add element styles via the UI"?

3. +++ b/core/modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/insertdrupalmedia.js
   @@ -55,6 +55,25 @@ export default class InsertDrupalMediaCommand extends Command {
   +      for (const style of elementStyleEditing.normalizedStyles) {
   +        if (
   +          attributes[style.attributeName] &&
   +          style.attributeValue === attributes[style.attributeName]
   +        ) {
   +          modelAttributes.drupalElementStyle = style.name;
   +          break;
   

   This is where it clicks for me that a model's drupalElementStyle is a single string value. The following may not be a concern since elementStyle is @internal, but just in case: Assuming I'm even understanding this correctly, are we limiting ourselves by introducing this as a single value property. It seems likely there could be needs to map many arbitrary attribute values to classes added on downcast. It seems like this could even be the foundation of reproducing StylesCombo functionality. I'm still reading through all this so this may become apparent later.

Still reviewing - adding what I have so far since I'll be AFK for a bit.

Have to pause again. Leaving what I have so far (pretty much surface level stuff) and will continue looking at it soon.

1. +++ b/core/modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/drupalelementstyle/drupalelementstylecommand.js
   @@ -0,0 +1,104 @@
   + * @return {null|module:engine/model/element~Element}
   

   Needs @return description

2. +++ b/core/modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/drupalelementstyle/drupalelementstylecommand.js
   @@ -0,0 +1,104 @@
   +    super(editor);
   

   Needs the full doc or an @inheritDoc

3. +++ b/core/modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/drupalelementstyle/drupalelementstylecommand.js
   @@ -0,0 +1,104 @@
   +   * @param {Object} options
   

   Needs param description

4. +++ b/core/modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/drupalelementstyle/drupalelementstyleui.js
   @@ -0,0 +1,267 @@
   +const getDropdownButtonTitle = (dropdownTitle, buttonTitle) => {
   +  return (dropdownTitle ? `${dropdownTitle}: ` : '') + buttonTitle;
   +};
   +
   +/**
   + * Gets the UI Component name.
   + *
   + * @param {string} name
   + *   The name of the component.
   + * @return {string}
   + *   The UI component name.
   + */
   +function getUIComponentName(name) {
   +  return `drupalElementStyle:${name}`;
   +}
   

   These two functions that begin with 'get', which create new strings, are used very close to uses of CK5 get(), which returns an existing property within the editor object. It's not a huge concern, but if there's alternate phrasing that makes sense and overlaps less with nearby methods we don't have naming rights for, lets do it.

5. +++ b/core/modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/drupalelementstyle/drupalelementstyleui.js
   @@ -0,0 +1,267 @@
   +     *             defaultItem: 'drupalElementStyle:alignCenter'
   

   Is there syntax for adding a @see to the element style definition within media_mediaAlign in ckeditor5.ckeditor5.yml?

6. +++ b/core/modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/drupalelementstyle/drupalelementstyleui.js
   @@ -0,0 +1,267 @@
   +/**
   + * Returns the first argument it receives.
   + *
   + * @param {*} value
   + * @return {*}
   + */
   

   Needs description for @param and @return

#21: I've noticed that occasionally but was never able to figure out what exactly caused that to happen — I'm glad to see you did! :D

Leaving this Needs review for @bnjmnm.

I added a few additional feedback items. They're largely docs related as the code itself seems sound. Once those items are addressed I think another reviewer could RTBC back to a candidate for me committing.

I see that @lauriii has thoroughly addressed all of @bnjmnm's feedback. So, moving back to RTBC.

Patches to facilitate committing.

Added to 10.0.x and 9.4.x, will see how amenable 9.3.x is next.

Cherry-picked to 9.3.x as CKEditor 5 is experimental, this is non disruptive, and it's a feature we want to get people using ASAP.