[regression] Restore styling for embedded media edit button in Seven theme

Not sure what else is needed here. I suppose that this is bringing in more styles than we'd ideally like, so we could split out the CSS into multiple files, but that seems out of scope here.

I too ran into the issue for #4. It seems every theme should have a copy of media-library.css for the ckeditor styles, even if the media library is only used in the admin theme. Should we add it to bartik and umami for now to fix this?

It sucks, but I guess that is what we have to deal with. I really think we should consider adding more styles back into the module. It is a separate issue, but I personally think overriding existing styles is the lesser evil compared to the amount of hooks and CSS a theme now has to copy to make the library look good. Apparently even in the admin theme. I don't expect anyone to understand this, since even I really don't know how to explain this to people.

So I just had a quick call with phenaproxima and lauriii about this. The real issue here is indeed that admin themes can't provide styles for the ckeditor. For that we can do the following things:

1. Provide a ckeditor stylesheet in the media library module (basically add back the missing stylesheet mentioned in #3092779: media_library.theme.css is being called despite it no longer existing) to provide the seven styles for the buttons. The drawback is other admin themes will also get the seven styles for the media library without being able to override it. It can however be overridden from frontend themes (same as now).
2. Remove the buttons in CKEditor for now. Users can still edit the alt text etc by right clicking the media item. This will make media items show less consistent, but will provide a consistent experience for different admin themes.
3. Solve the issue of allowing admin themes to add CSS to CKEditor. Not sure if we can actually solve this in this release though. That seems unlikely.

Options 1 or 2 are most feasible for the short term, but they both have serious drawbacks. We probably need a product manager to make the final call on this.

That still leaves my other concern about the overall issue that themes need to copy a lot of hooks, templates and CSS to their own themes to make the media library look good. For that we can and should provide a handbook page and a contrib module for the short term to make it as easy as possible.

Probably not eligible for 8.8, but still created #3092828: Allow admin themes to specify ckeditor_admin_stylesheets in their info file for admin components in ckeditor. as a possible solution.

As mentioned before we had a talk with lauriii and phenaproxima about this, and fixing it in the module or classy is the easiest solution for now, but probably problematic from a theming perspective. The BC implications of doing it in the module or in classy might come back to bite us in the ass.
My preference would be to solve it properly via #3092828: Allow admin themes to specify ckeditor_admin_stylesheets in their info file for admin components in ckeditor., so Seven and Claro can provide their own styles for admin elements within CKEditor. But ultimately it is a PM decision.

Should we at least ship with some styles as part of the core themes?

Should we at least ship with some styles as part of the core themes?

+1 for this as well.

Sounds like we should create a revision of #15 which has the changes moved to Claro, Seven, Bartik, and Umami.

By process of elimination, we're left with the second patch from #15.

Option 1: CSS should be provided by admin themes, Seven and Claro
Unfortunately, admin themes cannot specify stylesheets for CKEditor.
hook_ckeditor_css_alter() cannot be implemented by a theme.
We could implement hook_ckeditor_css_alter() in Media, but then there's no point of storing the stylesheets in the various admin themes. It'd be housed with Media. At that point, we're basically at Option 3, below.

Adding it to Bartik and Umami isn't useful as those are demo themes. They're unlikely to be used on a production site, even as a base theme.

Option 2: CSS should be added to classy
With Classy moving to contrib, #3050389: [META] Remove dependency to Classy from core themes, this seems like a nonstarter.

Option 3: CSS should be provided by the Media module.
This is what we're left with. And I don't see this as a bad option. Modules provide their own CSS all of the time.

Whereas a user is able to click on the icon, this patch adds pointer behavior to indicate an interaction to the user.

  border-radius: 20px;
  background-size: 13px;
  text-shadow: none;
  font-size: 0;
+ cursor: pointer;
}

drupal-media .media-library-item__edit {
  right: 10px;

Verified and tested by applying the patch on Drupal 8.9 and 9.1.It looks good to me.Can be moved to RTBC.

Steps to test -
1. Go to the admin site.
2. Go to admin/appearance
3. In the Administration theme , select Claro/Seven.
4. Go to admin/modules , enable Media, Media Library.
5. Go to admin/config/content/formats
6. In the text editor , from the Available buttons add the media type in the Active toolbar.
7. In the Enabled filters , enable these filters Align images and Embed media and click on save configuration.
8. Go to Article page.

Before Patch 9.1 -

Before Patch 8.9 -

After_Patch_9.1_Seven -

After_Patch 9.1 Claro -

After Patch 8.9 Seven -

After Patch 8.9 Claro -

@priyanka.sahni - Thanks for your thorough review.

Because this is a bug (and not a new feature), I'm setting the version back to 8.9.x-dev.

Per #25, moving to RTBC.

Setting back to RTBC. Test failure was an intermittent branch test failure and is unrelated to this patch.

Setting back to RTBC. Test failure was an intermittent branch test failure and is unrelated to this patch.

@Chris Burge it looks like you've rtbc'd your own work here - sorry if I've misunderstood that.

But also the solution in #22 appears to directly go against the consensus built by #20. As @lauriii is the front end framework manager we need his +1 that the solution in #22 is okay.

@alexpott - I did jump the gun. @priyanka.sahni reviewed and stated RTBC (w/o changing the status). I should have requested she move the status herself.

Re the consensus, see #21. Admin themes cannot specify stylesheets for CKEditor, so Seven and Claro are out. Umami is a demo theme, and fixing this only in Bartik doesn't help anyone. So.. patch #22 uses the Media module to provide the CSS. (If admin themes could provide CKEditor stylesheets, that would, of course, be a game changer.)

Adding NorthAmerica2021 and Easy Out of the Box tags for visbility.

DrupalCon NA is April 12-16 with a focus on EOOTB on Wednesday, April 14.
Thanks

Does this impact CKEditor 5? With CKeditor 4 moved to contrib should this issue be moved to contrib

Re-rolled #22 for Drupal 9.5.

EDIT: This didn't work. I ended up copy-pasting the styles into a CSS file defined as ckeditor_stylesheets.

Re-rolled Patch #41 with CKEditor 4 in Contrib

EDIT: Looks like I left an incorrect filepath to an image in this patch. Need to adjust that.

Fixed the icon path.

#43 is not applying for me for some reason, attaching a new patch that should be identical to #43.

There is a patch in #44, however, @larowlan's questions in #40 still need to be answered, which is why the status for this issue is Postponed and not Needs Review.

Does this impact CKEditor 5?
With CKeditor 4 moved to contrib should this issue be moved to contrib?

This came up again in Bugsmash today. It has been over a year since this was set to PMNMI and no further information has been provided.

The seven theme no longer exists in core and I can't see that CSS file either that the patch is changing. Therefore I'm marking this as Closed (outdated).

Please feel free to open a new issue with steps to reproduce this bug from a fresh drupal installation.