Media Library: Default value for data-align attribute should not be center

Completely agree with this one, the default value should be left to none. Even better if we could declare it from field settings.

The center alignment was suggested as a sensible default in #2994699: Create a CKEditor plugin to select and embed a media item from the Media Library #15

Center alignment is not a sensible default - IMO. Setting in field settings as per #4 would be awesome.

patch #2 is working fine for all themes(Bartik, Claro and Seven).

Please ignore comment #7.
patch #2 working fine in drupal 8.8.6

Not sure if empty string would be an appropriate value for data align property, maybe we remove that attribute by default.

I think we will need an issue summary update to explain the rationale for this change. Why is center alignment bad?

Why is center alignment bad?

We most often translate "left" into float: left;, "right" into float: right; and "center" into margin: auto. With "none", we take the item as-is, and have it flow normally based on its size and the text alignment of the container. So, at least for us, "center" is a concious decision on how the item should be displayed, thus it shouldn't be the default.

Also, why would there be a "None" setting, if we consoder "center" as the "default" (i.e. unchanged) state?

I agree with @ckaotik in #12.

Patch at #2 applies and works perfectly to Drupal 9.0.7.

It would be a bonus if the default value could be set in the UI, but setting the default to none works perfectly for my use.

I'm a new contributor to Drupal. I am reviewing the documentation page for writing an issue summary for an existing issue using this issue as an example. Feel free to add comments and edit. Thank you so much!

Wholeheartedly agree with @ckaotik in #12. IMO Drupal shouldn't be biased by default when None is an option.

Hopefully this patch gets accepted. Even better if the default could be set in the UI, like the view mode can be set as a default. And even better than that, if it could be configured to present the choices *before* inserting the media, rather than making my users re-edit the media every time. I wish I were in a position to be a contributor instead of just a complainer :)

+1 for this change here as well. No default is a more sensible default than center for the reasons mentioned.

I also agree that the center alignment is problematic. I think this is supposed to be against the latest branch for feature requests so am changing the version to 9.2.x.

Patch applies and works as expected.

Per #11 the issue could use an issue summary update - also some screenshots for before/after (the ones in #7 appear to be for a different issue).

Updated the issue summary.

To quote #4:

Completely agree with this one, the default value should be left to none. Even better if we could declare it from field settings.

And #6:

Center alignment is not a sensible default - IMO. Setting in field settings as per #4 would be awesome.

I should make clear that it is not going to be possible to configure this per field. Media embed settings are stored in the text format, not the field. So it will have to be configurable by text format. Will that be acceptable? Also, I assume we're talking about configuring which alignment will be the default choice in a given format, rather than forcing every embed in that format to use a particular alignment.

If that sounds correct, let's open a follow-up issue to add that configurability before we return this issue to RTBC.

Being able to set a default alignment in the text format would be perfect. Failing that, I agree that an alignment of none is a better default than center.

The patch in #2 works in Drupal 8.9.13.

The patch in #2 will print `data-align=""`, I think this can be omitted.

@lauriii asked me to take a look at this. The default seems to have been introduced in #2994699-15: Create a CKEditor plugin to select and embed a media item from the Media Library, but I think we might have overlooked the implications of this.

I agree with #12 that doing nothing seems to be a better default. This is also how regular image alignment works in the editor. It makes sense for media to behave the same as images as much as possible.

+1 for changing the default.

This looks good to me. Tests are green. RTBC!

Rerolled for 9.5

Fixed fail test for 9.5 patch

As the patch in #37 was not applicable in 10.1.x. Fixed it.
Please review.

Removed an empty line for patch #37 so now patches for 9.5.x and 10.1.x are the same.

Because it are re-rolls. back to RTBC

So:
#40 is for 9.5.X
#39 is for 10.1.X

please don't attach tests for a different version....
It will change the issue status...

This is tagged "Needs screenshots", but I don't think it does. It's just about the default option, and does not change the presentation of any of the options.

And I, personally, click the button from "center" to "left" 80% of the time, and to "right" 20% of the time. +1 from me.

It could maybe use a small change record since this is a small user interaction change.

Draft CR: https://www.drupal.org/node/3328698

Manually tested, confirmed it works as advertised, without altering existing content or the options available.

Committed to 10.1.x. I did not backport it as it's a minor user interface/interaction change, and therefore minor-only.

Thanks everyone for working on this!

Published the CR.

This affects both CKEditor 4 and 5 👍

… but this introduced an authoring experience regression. CKEditor 5 will continue to show the image as centered even when it actually is not.

Before this commit

After this commit

👆 Note how there is still a left margin present, which visually suggests the embedded media is centered, even when it isn't. Only if you look at the filtered result (at /node/<NID>) will you see that it's indeed just flowing with the text.

Root cause

.ck .drupal-media {
  position: relative;
  display: table;
  clear: both;
  min-width: 50px;
  margin: 0.9em auto;
  text-align: center;
}

in core/modules/ckeditor5/css/drupalmedia.css needs to be updated to match the behavior, and we need to verify with manual testing that the different alignment choices work correctly.

Comment #50 is correct that this commit introduced a regression in the CKEditor interface. I've removed the margin and text-align code as per Wim Leers suggestion and it does fix the issue he pointed out. First screenshot shows after the last commit was made.

This screenshot shows the results of the code change to .ck .drupal-media demonstrating the image is left-aligned and visually appears left-aligned.

The screenshot below shows that the text-aligning to center is still working and there are no regressions.

I should note that there are margins left and right of 40px that make it look like when the image is left-aligned or right-aligned, that the image is not flush with the editing window. These margins come from the user-agent style sheet. Should we override this behavior?

I have verified manually that the CSS change proposed does cause the default alignment of a media item to appear left-justified, and that center and right-justification behave as expected in the context of CKEditor 5's editing window.

As for the question about the remaining margin, visible in the second screenshot above, it's tricky: that is coming from the figure element of the drupal-media downcasted version of the element, and would not usually be present when the media item is rendered on the page. So, the margin is potentially misleading, but it's also impossible to predict whether the frontend theme will add a similar margin for the media item or not. I think the presence of the margin doesn't cause confusion about alignment, which was the original problem. So I think it should be left as-is, and sites can add CKEditor-specific CSS if they want to change it.

Fixing attribution.

I was confused to see this pop up again -- in general, when we hotfix things, the issue should either be reverted or a followup filed. We shouldn't reopen fixed issues without a revert.

Anyway, committed the hotfix to 10.1.x. Thanks!