Move some representational classes in Media Library to Classy and others to Seven and/or Claro, as appropriate

This should fix all instances of '#type' => 'container' in AddFormBase.

Oops! Good point as always, @lauriii :)

This should fix that problem. It also addresses the rest of the '#type' => 'container' instances of this snafu that I found in this module. No interdiff because it would be bigger than the patch.

Addressed most of the instances of '#type' => 'html_tag'. I know I'm creating a lot of templates here, but the plan here is to overshoot the mark if necessary, fix the tests, then dial things back.

Fixed every input element, container, and HTML tag in MediaLibraryWiget.

OK -- I think this completely fixes AddFormBase, FileUploadForm, OEmbedForm, and MediaLibraryUiBuilder. It also removes a lot of templates; I was able to move them to preprocess functions in Classy, which honestly seems like a better place for them, and cuts down the size of the patch considerably.

I think this moves or otherwise abstracts away just about opinionated CSS class that can be moved or abstracted away, including in media_library.module. Not sure if I've overreached the scope, but as I previously said, I'm just firing everything I have at this, more or less, and we can refactor later.

This is definitely still buggy and will definitely fail tests, but it's a start.

Here's a little self-review, and a "guide" of sorts to this patch for the next person who looks at it.

1. +++ b/core/modules/media_library/media_library.module
   @@ -144,9 +144,7 @@ function media_library_preprocess_media(&$variables) {
   -    $variables['metadata_attributes'] = new Attribute();
   

   I shouldn't have moved this line into Classy. It needs to be moved back.

2. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -157,23 +157,24 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +            // This needs to be focus-able by an AJAX response.
   +            // @see ::updateFormCallback()
   +            'js-media-library-add-form__added-media',
   

   This is new class, but updateFormCallback() was previously targeting the representational one.

3. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -187,14 +188,11 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +        '#type' => 'container',
   +        '#theme_wrappers' => [
   +          'container__media_library_add_form_description',
            ],
   +        'description' => $this->formatPlural(count($added_media), 'The media item has been created but has not yet been saved. Fill in any required fields and save to add it to the media library.', 'The media items have been created but have not yet been saved. Fill in any required fields and save to add them to the media library.'),
   

   This doesn't actually work; it's a temporary hedge. There's doesn't seem to be any way except #prefix and #suffix to wrap the message in a P tag, so this might just need its own template, sadly.

4. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -628,13 +627,13 @@ public function updateFormCallback(array &$form, FormStateInterface $form_state)
   -        $response->addCommand(new InvokeCommand(".media-library-add-form__media[data-media-library-added-delta=$delta_to_focus]", 'focus'));
   +        $response->addCommand(new InvokeCommand("[data-media-library-added-delta=$delta_to_focus]", 'focus'));
   

   The .media-library-add-form__media part of the modified selector was redundant anyway. Nothing else gets the data-media-library-added-delta attribute.

5. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -106,6 +106,15 @@ public static function create(ContainerInterface $container) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function buildForm(array $form, FormStateInterface $form_state) {
   +    $form = parent::buildForm($form, $form_state);
   +    $form['#theme_wrappers'] = ['form__media_library_add_form_upload'];
   +    return $form;
   +  }
   

   This isn't so nice. I'd rather give FileUploadForm and OEmbedForm unique form IDs and implement hook_form_FORM_ID_alter in Classy.

   But, since they share some opinionated classes with AddFormBase, what might be better is for AddFormBase to implement BaseFormIdInterface as well. This might result in a change record, but it should be completely backwards-compatible. And besides, Media Library is still experimental :)

6. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -338,25 +340,23 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +        '#type' => 'container',
   +        '#theme_wrappers' => [
   +          'container__media_library_widget_empty_text',
            ],
   +        'text' => $this->t('No media items are selected.'),
   

   This also doesn't work and might need its own template. Which totally sucks, but unless we're allowed to use #prefix and #suffix to inject opinionated classes, I'm not sure if we have another option.

7. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -338,25 +340,23 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +        '#type' => 'button',
   +        '#theme_wrappers' => [
   +          'input__submit__media_library_widget_toggle_weight',
   +        ],
   

   This is also messed up. There is still no template in core that can just freaking slap some content into a button tag. (WTF!) But I'm not entirely sure how to fix this yet. I would rather not create a whole template just for this...

8. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
   @@ -47,9 +47,7 @@ public function render(ResultRow $values) {
   -    $form['#attributes'] = [
   -      'class' => ['media-library-views-form', 'js-media-library-views-form'],
   -    ];
   +    $form['#attributes']['class'] = ['js-media-library-views-form'];
   

   In manual testing, I established that the media-library-views-form class is being applied by Classy. So it's not needed here. :)

9. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
   @@ -107,10 +105,7 @@ public function viewsForm(array &$form, FormStateInterface $form_state) {
   -    $form['actions']['submit']['#attributes'] = [
   -      'class' => ['media-library-select'],
   -      'data-disable-refocus' => 'true',
   -    ];
   +    $form['actions']['submit']['#attributes']['data-disable-refocus'] = 'true';
   

   Same thing here.

Addressing #13:
1. ✅ Moved $variables['metadata_attributes'] = new Attribute(); back.
2. 👍
3.This seems to work OK, it’s just the text needs to be wrapped in markup element.
4. 👍
5. Still @todo
6. This seems to work OK, it’s just the text needs to be wrapped in markup element.
7.This seems to work OK. There was a bug in twig. I think to remove the attribute, it needs to be like this: {% attributes.removeAttribute('name') %}
8. 👍
9 .👍

Thanks, @oknate! Those changes look good to me, and indeed they do fix some problems I was having. :)

In this patch, I put BaseFormIdInterface on AddFormBase and its children. This makes the required modifications substantially cleaner.

As for the "No media items selected" text in the field widget, I wasn't able to get that into an opinionated P tag any way except #prefix and #suffix. IMHO this should be acceptable, since it is still overridable -- just not easily by themes. Custom modules can do it in a jif, though, with hook_field_widget_WIDGET_TYPE_alter(). (That hook really should run for themes, too, but that's not in scope here.) That allowed me to get rid of another template. We're down to only one new template, and maybe we'll find a way to eliminate that too.

At this point, I'd like @lauriii to validate the approach we're taking here and point out any glaring flaws we may have missed. If he's good with what we're doing here, we can go about fixing the tests.

One major thing still outstanding: I am encountering an ugly bug when adding new media in the modal. Each thing has two "remove" buttons -- one styled correctly, the other wildly messed up. I can't figure out why this is happening.

This patch fixes the ugly bug (turned out to be me using #theme where I should have used #theme_wrappers). It also removes virtually all the #theme_wrappers overrides from AddFormBase and its children in favor of three hook_form_alter() implementations in Classy, which is much cleaner and less complicated overall, IMHO.

The field widget and MediaLibraryUiBuilder remain the more "troublesome" ones, because there is no way for the theme to influence their output.

Managed to remove a couple more #theme_[wrappers] overrides in FileUploadForm and OEmbedForm.

Here's a little brain dump, primarily directed at @lauriii, about the current state of this patch.

• I expect #18 to pass tests. If it does, that's frankly amazing: less work for us! 🥳
• I personally am a big fan of using hook_form_alter to allow Classy to influence AddFormBase. Although @lauriii tells me it is "discouraged", it is in my opinion the cleanest and most straightforward option for overriding the add form's markup/styling, by far. Adding pseudo-templates everywhere (container__media_library_add_form_blahblahblah) is cumbersome, significantly less discoverable, and will be a worse experience for developers and themers. Sure, render arrays are somewhat unreliable, but that's a fundamental problem with render arrays. They're what we're stuck with, so IMHO let's alter them in whatever way is least painful.
• From my perspective, the problem with hook_form_alter is that doing it properly necessitates adding BaseFormIdInterface to AddFormBase. Well, it doesn't exactly necessitate it...but if we don't do that, then anything wanting to alter the add forms we ship in core (FileUploadForm and OEmbedForm) will need to alter the same form ID, and detect which class is being used to generate the form ($form_object instanceof Foo). And that just sucks. So, to me, adding BaseFormIdInterface and changing the form IDs of FileUploadForm and OEmbedForm is a small BC break in a currently experimental module...but it's a BC break worth making. Otherwise, we're permanently saddled with a confusing, non-standard form alter mechanism that sucks.
• The hardest parts of Media Library to "frontend-ize" are the field widget, and the output of MediaLibraryUiBuilder. The field widget is tough because themes cannot implement hook_field_widget_form_alter() -- if they could, we'd just do a similar thing as with AddFormBase, and be done with it. As it is, we can get by with pseudo-theme hooks (as implemented in #18). I'm really not too keen on the idea of the widget having a template of its own; that seems complex and heavy, in exchange for little gain and increased fragility.
• As far as MediaLibraryUiBuilder goes, I'm debating whether it would be worthwhile for us to add a new theme hook/template for that (called media_library in code, and media-library.html.twig in themes). It seems a bit "heavy", and it would allow themes to maybe impose more opinions than we'd like; after all, Media Library is meant to be opinionated and not terribly extensible. But it might also be the "right" way to do this. I'll defer this decision to committers.
• If we don't want to move some of the more questionable things in #18 -- like the form_alter hooks -- into Classy, @lauriii proposed potentially moving them into Seven for the purposes of getting Media Library stable. I'm fine with that; the main goal here is to get all styling-related CSS classes out of Media Library, and into a theme; I don't much care which theme it is. :) Seven is a fine place to put them for now, to be honest, because all of our current styling is targeted for Seven anyway, and that's where it will be moved when we mark the module stable.

Another brain dump. I didn't understand why we needed the one template, and couldn't just use a preprocess hook. @phenaproxima explained this to me on slack.

Me: "I don’t understand why we need that one template."
"does this not work?"

function classy_preprocess_input__submit__media_library_widget_toggle_weight(array &$variables) {
 unset($variables[‘attributes’][‘name’]);
}

phenaproxima:

Because of the markup we need: <button>STUFF</button>. There is no template which provides that.

Regarding this:

I personally am a big fan of using hook_form_alter to allow Classy to influence AddFormBase. Although @lauriii tells me it is "discouraged",

If it is discouraged, is there a way to use it but mitigate the downsides? Maybe adding comments that this should only be used for adding classes and minor markup changes, and that if not used carefully, it could break Media Library functionality?

I have manually tested #18 and everything looks good visually and functionally. I think the code changes make sense too. I think this is ready, AFAIK.

Issue summary update to encompass the larger scope other than the one file.

Found a way to remove the awkward input__submit__media_library_toggle_weight pseudo-template and the hard-coded prefixing/suffixing around the widget's empty text. The upshot is that I had to exchange the removed template for a different, but more useful, pseudo-template wrapping the entire media library widget, and go back to using '#type' => 'html_tag' for the weight toggle button.

I think that's okay in this case, though, because:

1. These are sane defaults. The empty text is markup-free, and the weight toggle is a <button> element -- which, IIRC, it needs to be for accessibility reasons -- so, in the absence of a <button>STUFF</button> template, I would argue that this is a legitimate, functional use of '#type' => 'html_tag'.
2. They can still be modified or replaced in a preprocess function, as is now demonstrated by classy_preprocess_fieldset__media_library_widget().

The only reason I have a new fieldset--media-library-widget.html.twig template in this file is because both of these things are put into the fieldset prefix, which is wrapped by a <span> tag, and because I am a good HTML citizen, I didn't want to put a P inside a SPAN.

Nice job, MediaLibraryTest! #25 caught a legitimate problem -- Drupal.behaviors.MediaLibraryWidgetToggleWeight is not tolerant of the surrounding markup being changed. Should be fixed in this patch.

Tagging for front-end framework manager review.

Something for committers to consider, since this is technically blocking Media Library from being marked stable...

Even though the approach we're taking (so far) is probably a bit on the hacky side (pseudo-templates everywhere...), we might consider this as a short term fix for the immediate problem -- namely, that Media Library is outputting render arrays peppered with representational CSS classes -- then file follow-up issues, possibly to be resolved by beta (or in another minor feature release, if possible), to add dedicated templates and increase the overall themeability.

Potentially, something like #3065677: Create a media_library form element could be a long-term solution.

I’m not a very good themer, I guess I can’t really say to much about it. All approaches seem a bit yuck to me. I don’t see a good way around it though? And I understand why we need it. Apparently it is really hard to make Drupal output exactly what you want. That doesn’t seem to be something we can fix in media library 😞

You're not wrong. This is why I'm pushing for us to use hook_form_alter() in this patch -- through it, I was able to remove a lot of the pseudo-theme hooks.

I'm starting to think that having MediaLibraryUiBuilder::buildUi() output through a new '#theme' => 'media_library' theme hook would be a good idea (and not that hard to implement). Another option would be to essentially implement #3065677: Create a media_library form element now, which would probably increase the themeability too.

The field widget remains the real challenge. I don't know if there is a pre-existing pattern in core for making a field widget completely themeable. I took at look at FileWidget, for example -- it outputs a #managed_file element, which is themeable to an extent, but not totally customizable without writing a custom module to implement hook_field_widget_form_alter().

This is kind of why I really want to discuss with @lauriii before going further. How themeable do we really need to be, in order to be on par with the rest of core?

I decided to proceed with making the output of MediaLibraryUiBuilder::buildUi() more themeable. To that end, I added a new theme hook (media_library) and simple template. This allowed me to merge a couple of preprocess functions in Classy, except for one which cannot be removed due to the way MediaLibraryUiBuilder responds to AJAX requests (it intentionally doesn't return all the "chrome" of the UI when switching between media types, thus necessitating a pseudo-theme hook to target the content of the UI, rather than the whole thing).

This will fix the tests.

Moved all the Classy stuff in #34 to Seven, after discussion with @seanB, @alexpott, and @lauriii. The latter will follow up in this issue to summarize. I expect this patch to fail tests enormously; MediaLibraryTest will need to be changed so that it no longer uses CSS classes which are found, as of this patch, only in Seven. (It will also have to test everything against both Seven and Classy.)

Whoops! Sorry, #35 accidentally included an unrelated change.

This issue is a potential contrib blocker for making it easy for themes to support Media Library.

Perhaps a good next step here is to propose which CSS class additions belong in Seven and do not belong in Classy, and why?

Lucky 42.

This won't pass all tests yet, but it will fix several of the failures.

I spent some time pairing with @lauriii today on this. It seems that we're going to go piece by piece and figure out what should be in Seven, what should be in Classy, and what should be converted to a template/theme hook of its own. I put todos in the patch to record our decisions.

Be forewarned: the final version of this patch will be long and complex due to the sheer number of small changes required to MediaLibraryTest.

OK, this should fix the rest of the broken tests.

The good news: this will almost certainly fix the tests, and clear the way for us to implement the todos added in #42.

The bad news: I had to do a not-insignificant amount of clean-up and refactoring in MediaLibraryTest to make it only use functional selectors, rather than the ones that are coming from Classy/Seven. The upshot is that this not only makes the test a little easier to understand, but should also help make it easier to run the tests against both Classy and Seven later on. Unfortunately, that necessitated a lot of small changes which increased the size of this patch to well over 100 KB. 🤪

Another "self-review", with notes to help the next set of eyes make sense of my changes.

1. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -58,7 +58,7 @@
   -            .parent()
   +            .closest('.js-media-library-widget')
   

   This is a preemptive bug fix. If the structure of the field widget is changed (e.g., by a template), the weight toggling can fail, which is a critical accessibility problem.

2. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -157,23 +158,25 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -      $form['#attributes']['class'][] = 'media-library-add-form--with-input';
   -
   +      $form['#attributes']['data-input'] = 'true';
   

   The with-input and without-input classes were being used by tests, but they are now added by the theme and were never functional in nature anyway. Although this is purely for testing, switching to a data attribute gets around that. (Ideally we would just remove these assertions from MediaLibraryTest, to be honest, but I thought it was better not to remove coverage.)

3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
   @@ -222,7 +222,7 @@ public function testButton() {
   -    $assert_session->elementExists('css', 'button.media-library-select.button.button--primary')->click();
   +    $assert_session->elementExists('css', '.ui-dialog-buttonpane')->pressButton('Insert selected');
   

   We can no longer rely on these presentational classes.

4. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/EmbeddedFormWidgetTest.php
   @@ -122,8 +122,8 @@ public function testInsertionAndReselection($widget) {
   -    $this->assertTrue($assert_session->waitForElementVisible('xpath', $first_item_locator));
   -    $assert_session->elementExists('css', '.media-library-item__remove')->click();
   +    $this->assertTrue($first_item = $assert_session->waitForElementVisible('xpath', $first_item_locator));
   +    $first_item->pressButton('Remove');
   

   .media-library-item__remove is a presentational theme class that has no js- prefixed equivalent (and does not need one), so a fair number of the changes to MediaLibraryTest are about accessing the "Remove" button in a CSS-agnostic way.

5. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -2,6 +2,7 @@
   +use Behat\Mink\Element\NodeElement;
   

   This can be deleted, it's cruft from refactoring.

6. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -439,7 +440,6 @@ public function testWidget() {
        $this->openMediaLibraryForField('field_unlimited_media');
   -    $this->waitForText('Add or select media');
   

   I moved the waitForText() call into openMediaLibraryForField(). In core, there is no
   circumstance under which that text will fail to appear if the media library has opened successfully. I thought this was a nice way to increase the test's assertiveness while decreasing its verbosity.

7. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -479,7 +479,7 @@ public function testWidget() {
   -    $this->openMediaLibraryForField('field_single_media_type', '.media-library-wrapper');
   +    $this->openMediaLibraryForField('field_single_media_type', '#media-library-wrapper');
   

   .media-library-wrapper is presentational and added by the theme now. #media-library-wrapper, on the other hand, is functional and used as an AJAX target.

8. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -494,7 +494,7 @@ public function testWidget() {
   +    $links = $this->getTypesMenu()->findAll('css', 'a');
   

   getTypesMenu() was added to abstract away the CSS selector used to get to the menu (it was previously a presentational class).

9. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -560,50 +560,55 @@ public function testWidget() {
   -    $assert_session->elementExists('css', '.ui-dialog-buttonpane')->pressButton('Insert selected');
   -    $this->assertNotEmpty($assert_session->waitForText('Added one media item.'));
   +    $this->insertSelected('Added one media item.');
   

   This pattern ($assert_session->elementExists()->pressButton(), followed by the waiting for an announcement) was repeated all over the place. I made it its own method for clarity and reduced verbosity.

10. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -560,50 +560,55 @@ public function testWidget() {
    -    // Assert the weight field can be focused via a mouse click.
    -    $assert_session->elementExists('named', ['button', 'Show media item weights'])->click();
    -    $assert_session->elementExists('css', '#field_twin_media-media-library-wrapper .media-library-item__weight')->click();
    -    $assert_session->elementExists('css', '#field_twin_media-media-library-wrapper .js-media-library-widget-toggle-weight')->click();
    +
    +    // Assert that we can toggle the visibility of the weight inputs.
    +    $wrapper = $assert_session->elementExists('css', '.field--name-field-twin-media');
    +    $wrapper->pressButton('Show media item weights');
    +    $assert_session->fieldExists('Weight', $wrapper)->click();
    +    $wrapper->pressButton('Hide media item weights');
    

    This is important accessibility test coverage and definitely should not rely on presentational CSS selectors.

11. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -560,50 +560,55 @@ public function testWidget() {
    -    $this->waitForElementsCount('css', '.field--name-field-twin-media .media-library-item', 2);
    +    $this->waitForElementsCount('css', '.field--name-field-twin-media [data-media-library-item-delta]', 2);
    

    .media-library-item is presentational. [data-media-library-item-delta] is functional and added by the widget for accessibility reasons. :)

12. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -560,50 +560,55 @@ public function testWidget() {
    -    $assert_session->elementExists('css', '.media-library-item__remove')->click();
    -    $this->assertNotEmpty($assert_session->waitForText('Removed Dog.'));
    -    $this->assertElementExistsAfterWait('css', '.media-library-item__remove')->click();
    -    $this->assertNotEmpty($assert_session->waitForText('Removed Dog.'));
    -    $assert_session->assertNoElementAfterWait('css', '.media-library-item__remove');
    +    $wrapper->pressButton('Remove');
    +    $this->waitForText('Removed Dog.');
    +    $wrapper->pressButton('Remove');
    +    $this->waitForText('Removed Dog.');
    +    $result = $wrapper->waitFor(10, function ($wrapper) {
    +      /** @var \Behat\Mink\Element\NodeElement $wrapper */
    +      return $wrapper->findButton('Remove') == NULL;
    +    });
    +    $this->assertTrue($result);
    

    This removes the reliance on presentational classes, and increases the precision of the assertions/interactions.

13. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -560,50 +560,55 @@ public function testWidget() {
    -    $assert_session->elementExists('css', '.media-library-selected-count');
    -    $assert_session->elementTextContains('css', '.media-library-selected-count', '0 of 2 items selected');
    -    $assert_session->elementAttributeContains('css', '.media-library-selected-count', 'role', 'status');
    -    $assert_session->elementAttributeContains('css', '.media-library-selected-count', 'aria-live', 'polite');
    -    $assert_session->elementAttributeContains('css', '.media-library-selected-count', 'aria-atomic', 'true');
    +    $selected_count = $assert_session->elementExists('css', '.media-library-selected-count');
    +    $this->assertSame('0 of 2 items selected', $selected_count->getText());
    +    $this->assertSame('status', $selected_count->getAttribute('role'));
    +    $this->assertSame('polite', $selected_count->getAttribute('aria-live'));
    +    $this->assertSame('true', $selected_count->getAttribute('aria-atomic'));
    

    Again, .media-library-selected-count was repeated all over the place. There was no need for that, so I removed it.

14. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1203,37 +1185,28 @@ public function testWidgetUpload() {
    -    // Assert the focus is shifted to the added media items.
    -    $this->assertJsCondition('jQuery(".media-library-add-form__added-media").is(":focus")');
    -    // Assert the media item fields are shown and the vertical tabs are no
    -    // longer shown.
    -    $assert_session->elementExists('css', '.media-library-add-form__fields');
    -    $assert_session->elementNotExists('css', '.media-library-menu');
    +    $this->waitForFieldExists('Alternative text');
    +    $this->assertMediaAdded();
    

    The "added media" area has a lot of presentational classes in it. To abstract those away, I created assertMediaAdded() and assertNoMediaAdded().

15. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1563,15 +1523,14 @@ public function testWidgetUploadAdvancedUi() {
    -    $selection_area = $this->assertElementExistsAfterWait('css', '.media-library-add-form__selected-media');
    -    $assert_session->elementExists('css', 'summary', $selection_area)->click();
    +    $selection_area = $this->getSelectionArea();
    

    Same idea here -- getSelectionArea() is abstracting a presentational selector away.

16. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -1872,33 +1808,25 @@ public function testWidgetOEmbed() {
    -    $this->assertJsCondition('jQuery(".media-library-add-form__added-media").is(":focus")');
    -    // Assert the media item fields are shown and the vertical tabs are no
    -    // longer shown.
    -    $assert_session->elementExists('css', '.media-library-add-form__fields');
    -    $assert_session->elementNotExists('css', '.media-library-menu');
    +    $this->assertMediaAdded();
    

    assertMediaAdded() asserts that the media types menu doesn't appear. When new media is being added, it never should. So this increases the assertiveness and "correctness" of the test.

17. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -2228,21 +2141,35 @@ protected function assertElementExistsAfterWait($selector, $locator, $timeout =
       protected function switchToMediaType($type) {
    -    $page = $this->getSession()->getPage();
    -    $lowercase_type = strtolower($type);
    +    $link = $this->assertSession()
    +      ->elementExists('named', ['link', "Type $type"], $this->getTypesMenu());
     
    -    if (!is_null($page->find('css', ".media-library-menu-type-$lowercase_type .active-tab"))) {
    -      // There is nothing to do as the type is already active.
    +    if ($link->hasClass('active')) {
           return;
         }
    +    $link->click();
    +
    +    $result = $link->waitFor(10, function ($link) {
    +      /** @var \Behat\Mink\Element\NodeElement $link */
    +      return $link->hasClass('active');
    +    });
    +    $this->assertTrue($result);
     
    -    $page->clickLink($type);
    -    $this->assertElementExistsAfterWait('css', ".media-library-menu-type-$lowercase_type .active-tab");
    -    $this->assertElementExistsAfterWait('css', "[data-drupal-media-type='type_$lowercase_type']");
    

    Again, the idea here was to remove the reliance on CSS selectors in favor of functional markup.

18. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -2359,9 +2286,69 @@ protected function pressSaveButton($expect_errors = FALSE) {
    +  /**
    +   * Asserts a media item was addd, but not yet saved.
    

    Whoops, typo.

I paired with @lauriii on this for a couple of hours today. We were able to make a lot of headway. I'm most proud of turning the list of added media generated by AddFormBase into a true item list -- that's more semantically correct and certainly more themeable.

Changes to MediaLibraryWidget continue to be challenging. The problem is that the AJAX operations (like removing things, updating the widget, and so forth) tend to be very sensitive to the relative positions of various elements in the render array, so anything which reorganizes the render array for theming purposes tends to compromise it.

Therefore, some things are more themeable now; other things will continue to be in hook_form_alter() and preprocess functions. Exactly which mechanisms we use, and were, are decisions @lauriii needs to make one by one.

@lauriii has promised to review this patch tomorrow (his time), ignoring the changes to MediaLibraryTest (which are not functional changes; just making it less sensitive to particular selectors). I ran MediaLibraryTest locally and it passes handily for me.

And, hey! I was able to do a thing @lauriii had asked for, which was create a new media_library_item template for use in both the widget, to display selected items, and in the "Additional selected media" area in the advanced UI. Mission accomplished!

This passed tests locally.

+++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
@@ -365,21 +366,34 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+      // Ideally, this entire element would be themeable through a dedicated
+      // template. Unfortunately, the only way to do that while maintaining
+      // the proper form structure is via a #pre_render callback, which would
+      // necessarily change the form structure. And that, sadly, must be "just
+      // so" in order for various AJAX operations, like removing media items
+      // from the selection, to work correctly.
+      // @todo See if this can be done in $ISSUE_URL.
+      // @see ::removeItem()
+      // @see ::openMediaLibrary()
+      // @see ::updateWidget()

This is out of date and needs to be removed. :)

A little more work here...I removed the outdated comment, and also found a way to make the "Add media" button in the widget more theme-friendly. By moving it into the fieldset's suffix with a #pre_render callback, I was able to remove another preprocess function. If we add a preprocess function in media_library.module, we could potentially expose it in the template as its own variable, which would be cool. I think things are shaping up!

This passed tests locally, too. 🤞

I'm wondering what should we do with the existing classes if we did this? Maybe we could just leave them intact in existing installations and remove them from the default configuration? We could recommend users who care about this to reinstall Media Library.

+1 for this approach. Update paths are a complex and potentially dangerous pain in the ass, especially where Views is concerned, and we already have too many of them in Media Library. For existing sites, AFAICT the worst case scenario of the proposed approach is that they'll have a few additional, harmlessly inert classes scattered around, which they can easily remove in Views UI (or uninstall/reinstall, as you say).

function seven_views_pre_render(&$element) {
$element->field['edit_media']->options['alter']['link_class'] = 'media-library-item__edit';
}

Nice approach! But we need to append to rather than overwrite the existing link_class. We probably also want to make the append logic smart enough to prevent duplicates.

Update paths are a complex and potentially dangerous pain in the ass, especially where Views is concerned, and we already have too many of them in Media Library.

I don't think that should be the deciding factor. But, I do think that a good argument for not removing these classes from people's existing ML View is to preserve BC for a site that has a custom theme that's already styling these classes.

We could recommend users who care about [cleaning up their Media Library View] to reinstall Media Library

I don't think that this is the best recommendation, because doing so has other side effects. For example, if you install ML, and then add Media fields to content types, they default to the ML widget. If you then uninstall and reinstall ML, all those widgets become generic autocomplete ER widgets instead. But, I think we can recommend that for someone who wants to reset their ML View to the one provided by the ML codebase, that they can use /admin/config/development/configuration/single/import to do that and just paste in the YML from media_library/config/install/views.view.media_library.yml. Let's include that in the eventual CR that's written for this.

I paired with @lauriii again and we went through the view, moving classes into either media_library_views_pre_render() or seven_views_pre_render() as appropriate. This necessitated another big round of changes to MediaLibraryTest, which is now refactored quite a bit, to be honest, but I think it still has the same coverage as before, and with increased assertiveness and flexibility.

This passed all tests locally except for \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest::testAdministrationPage(), but that frequently has random failures on my machine, and I don't think we did anything that will break it on testbot. But, we'll see.

No interdiff because, as with many iterations of this patch, it would be unjustifiably large and confusing.

Some things from my walk-through with @effulgentsia:

1. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -445,7 +477,7 @@ protected function buildSelectedItemElement(MediaInterface $media, array $form,
   -      'rendered_entity' => $this->viewBuilder->view($media, 'media_library'),
   +      '#content' => $this->viewBuilder->view($media, 'media_library'),
   

   This isn't bueno. The media_library_item template should be a theme wrapper in order to avoid this.

2. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -123,14 +123,12 @@ public function buildUi(MediaLibraryState $state = NULL) {
   -        'menu' => $this->buildMediaTypeMenu($state),
   -        'content' => $this->buildLibraryContent($state),
   +        '#menu' => $this->buildMediaTypeMenu($state),
   +        '#content' => $this->buildLibraryContent($state),
   

   This should be using a render element in its theme hook, rather than two property/render arrays.

3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -439,8 +439,9 @@ public function testWidget() {
   +    // @todo: This should only be done in openMediaLibraryForField() when
   +    // testing in Seven, since only Seven has styles to add the checkbox.
   +    // $this->assertFalse($assert_session->elementExists('css', '.media-library-select-all')->isVisible());
   

   Let's add a new class extending MediaLibraryTest, which runs all of the same tests in the Seven theme (and includes this assertion). That way we can have our Classy cake and eat Seven too.

4. +++ b/core/themes/seven/seven.theme
   @@ -186,3 +188,184 @@ function seven_form_media_form_alter(&$form, FormStateInterface $form_state) {
   +/**
   + * Implements hook_preprocess_views_view_fields().
   + *
   + * This targets each rendered media item in the grid display of the media
   + * library's modal dialog.
   + */
   +function seven_preprocess_views_view_fields__media_library(array &$variables) {
   +  // Add classes to media rendered entity field so it can be targeted for
   +  // styling. Adding this class in a template is very difficult to do.
   +  if (isset($variables['fields']['rendered_entity']->wrapper_attributes)) {
   +    $variables['fields']['rendered_entity']->wrapper_attributes->addClass('media-library-item__click-to-select-trigger');
   +  }
   +}
   

   Can this be moved into seven_views_pre_render()? And more to the point, should we?

This should fix those dastardly test failures.

Made the changes @effulgenstia requested (#1-3) in #58. This passed tests locally!

@lauriii did a walkthrough of this patch, looking at every presentational class and determining where each is added, and by what. He even made a beautiful spreadsheet with his findings! 😍 🙏

This latest patch fixes the last bits of feedback from that round of review:

• Renamed the media_library theme hook to media_library_wrapper, for more accurate naming.
• Removed an addition of the media-library-item__remove class from MediaLibraryWidget; it's already added by Seven, which is as it should be.
• Deleted container--media-library-widget-item.html.twig, which is a defunct template from an earlier iteration of the patch.
• Moved details--media-library-add-form-selected-media.html.twig from Classy to Seven, and added Seven-specific seven-details classes to it.
• Added styling fixes to account for the fact that the "added media" list in AddFormBase is now a true item list.
• Restored presentational classes (in Seven) for the preview and fields of each added media item. The classes are Seven-specific and present in HEAD, but were accidentally removed at some point in this whole kerfuffle.

To make it easier to commit this monstrous patch, @effulgentsia asked me to move the JavaScript bugfixes, and as many changes to MediaLibraryTest as possible, to another issue. So I opened #3088681: Make MediaLibraryTest less dependent on representational CSS selectors for that -- once we commit it and reroll this, we should shave a good 85 KB off this patch. Which this patch could sorely use.

Rerolled on top of #3088681: Make MediaLibraryTest less dependent on representational CSS selectors. I haven't yet addressed @lauriii's feedback; I wanted to get that damn reroll out of the way. Phew!

Addressing #63:

1. 👎@effulgentsia had requested this so that we wouldn't be passing sub-render arrays as properties of a parent render array. Leaving as-is.
2. ✅
3. ✅Decided to fix it here. It's not worth a follow-up.
4. ✅How's this look?
5. ✅
6. ✅Filed #3088856: Make it possible/easier to apply CSS classes to links in the links template about this and linked it in the code.
7. ✅Okay, gave it a shot. What do you think?
8. ✅Agreed. Fixed.
9. ✅
10. ✅
11. ✅

Also fixing a dumb mistake in #64 which will cause tests to fail.

Re #65.1, right, here's more places related to that...

1. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -123,14 +123,12 @@ public function buildUi(MediaLibraryState $state = NULL) {
   -        'menu' => $this->buildMediaTypeMenu($state),
   -        'content' => $this->buildLibraryContent($state),
   +        '#menu' => $this->buildMediaTypeMenu($state),
   +        '#content' => $this->buildLibraryContent($state),
   

   Render arrays should not be assigned to #properties, so this change moves in the wrong direction.

2. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -416,12 +447,14 @@ protected function buildCurrentSelectionArea(array $form, FormStateInterface $fo
   +      '#theme_wrappers' => [
   +        'media_library_item__small',
   +      ],
   +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -365,21 +367,26 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +        '#theme_wrappers' => [
   +          'media_library_item__widget',
   +        ],
   

   These can be #theme rather than #theme_wrappers. Because the media library item is its own render element that doesn't have or need a separate #theme function other than this one.

3. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -366,6 +351,54 @@ protected function buildEntityFormElement(MediaInterface $media, array $form, Fo
   +    $element['#content_prefix']['select'] = $element['select'];
   +    unset($element['select']);
   +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -558,6 +560,54 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    $element['#content_suffix']['weight'] = $element['weight'];
   +    $element['#content_suffix']['target_id'] = $element['target_id'];
   +    $element['#content_prefix']['remove_button'] = $element['remove_button'];
   +    unset($element['weight'], $element['target_id'], $element['remove_button']);
   

   Converting media_library_item from a #theme_wrapper to a #theme would allow these assignments of a render array to a #property to be removed.

I like those suggestions! This patch implements them. It allowed me to remove a couple of pre-renders, as promised, and also revert changes to MediaLibraryAddFormTest, which is good. I think this reduces complexity and increases flexibility, so I'm all for it! Plus, it slims down the size of the patch :)

+++ b/core/modules/media_library/js/media_library.view.es6.js
@@ -21,7 +21,7 @@
-              .closest('.media-library-view')
+              .closest('.js-media-library-view')
+++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
@@ -829,13 +820,13 @@ public function testWidgetViews() {
-    $assert_session->elementTextContains('css', '.view-media-library .pager__item.is-active', 'Page 1');
+    $assert_session->elementTextContains('css', '.js-media-library-view .pager__item.is-active', 'Page 1');
...
-    $this->waitForElementTextContains('.view-media-library .pager__item.is-active', 'Page 2');
+    $this->waitForElementTextContains('.js-media-library-view .pager__item.is-active', 'Page 2');
...
-    $this->waitForElementTextContains('.view-media-library .pager__item.is-active', 'Page 1');
+    $this->waitForElementTextContains('.js-media-library-view .pager__item.is-active', 'Page 1');

Looks like this didn't make it into #3088681: Make MediaLibraryTest less dependent on representational CSS selectors. Can we open a new issue for it?

+++ b/core/modules/media_library/src/Form/AddFormBase.php
@@ -629,13 +637,13 @@ public function updateFormCallback(array &$form, FormStateInterface $form_state)
-        $response->addCommand(new InvokeCommand(".media-library-add-form__media[data-media-library-added-delta=$delta_to_focus]", 'focus'));
+        $response->addCommand(new InvokeCommand("[data-media-library-added-delta=$delta_to_focus]", 'focus'));
...
-      $response->addCommand(new InvokeCommand('.media-library-add-form__added-media', 'focus'));
+      $response->addCommand(new InvokeCommand('.js-media-library-add-form-added-media', 'focus'));

Same for this. Can be part of the same issue as #69.

diff --git a/core/modules/media_library/tests/src/FunctionalJavascript/ContentModerationTest.php b/core/modules/media_library/tests/src/FunctionalJavascript/ContentModerationTest.php
...
diff --git a/core/modules/media_library/tests/src/FunctionalJavascript/TranslationsTest.php b/core/modules/media_library/tests/src/FunctionalJavascript/TranslationsTest.php

Same for this. Can also be part of the same issue as #69.

1. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -993,13 +982,11 @@ public function testWidgetUpload() {
   -    $assert_session->elementExists('css', '.media-library-add-form--without-input');
   ...
   -    $assert_session->elementNotExists('css', '.media-library-add-form--without-input');
   

   Why are we removing this test coverage?

2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -2025,9 +1993,11 @@ public function testFieldUiIntegration() {
   -    $page->pressButton('Save field settings');
   +    while ($button = $page->findButton('Save field settings')) {
   +      $button->press();
   +    }
        $assert_session->pageTextNotContains('Undefined index: target_bundles');
   -    $page->checkField('settings[handler_settings][target_bundles][type_one]');
   +    $this->waitForFieldExists('settings[handler_settings][target_bundles][type_one]')->check();
   

   Is this related to this issue? If so, how?

+++ b/core/themes/classy/templates/media-library/links--media-library-menu.html.twig
+++ b/core/themes/classy/templates/media-library/media--media-library.html.twig
+++ b/core/themes/seven/media-library/details--media-library-add-form-selected-media.html.twig
+++ b/core/themes/seven/media-library/fieldset--media-library-widget.html.twig

These are pretty involved templates, and we're only overriding the default in a trivial way (adding a class or two). I think for these, it would be better to follow a pattern of using extends. E.g.,

{% extends "links.html.twig" %}
{% set attributes = attributes.addClass('media-library-menu') %}

Note that I'm aware that there's feedback in #2387069-104: {% extends "foo.html.twig" %} in Twig templates does not respect theme inheritance to not do stuff like that, but there hasn't yet been an agreed upon resolution to that, and in the meantime, core already does it in places like field--text.html.twig, and IMO, that's better than duplicating and maintaining duplicates of non-trivial Twig code and markup.

1. --- a/core/modules/media_library/templates/media--media-library.html.twig
   +++ b/core/modules/media_library/templates/media--media-library.html.twig
   

   We don't have a copy of this in Stable. Should we? And if so, should we copy this one, or the one prior to these changes?

2. +++ b/core/modules/media_library/templates/media-library-item.html.twig
   +++ b/core/modules/media_library/templates/media-library-wrapper.html.twig
   

   Do we need to add copies of these in Stable as well?

+++ b/core/themes/classy/templates/media-library/container--media-library-content.html.twig
+++ b/core/themes/classy/templates/media-library/container--media-library-widget-selection.html.twig

Because these are containers, should we also add the has_parent ? ['js-form-wrapper', 'form-wrapper'] classes too, for consistency with Classy's container.html.twig?

NW for addressing #69 - #75, whether that means fixing the patch, or commenting as to why the feedback is incorrect.

Self-assigning to deal with all that.

Addressing #72:

1. That was removed since those classes are only being added by Seven's form_alter implementations. However, now that we have MediaLibrarySevenTest, we can easily re-add that coverage. So that's what I did :)
2. Good catch. It is not related; that's detritus from me trying to rein in a test which never random-fails on testbot, as far as I could tell, but constantly random-fails on my local machine. It's not needed for this patch.

Addressing #73:

I was able to change this in two of the templates. The two I couldn't change -- fieldset--media-library-widget.html.twig and media--media-library.html.twig -- are hard-coding new classes into the markup, not just using local variables to add classes, so I don't think they can extend a base template.

Addressing #74:

1. I think this is a pre-existing template, so I'm not sure if we can or should add it to Stable here. Maybe in the main "stable-maker" patch? 🤷‍♂️
2. These are new templates, so they do belong in Stable, but maybe that should also be done in the main stable-maker too. This is probably a question for the front-end framework manager.

I'm leaving these as-is for now.

Addressing #75:

I decided to leave these as-is for now. has_parent isn't really super relevant to theming these things; we're using container for convenience here, because we need a DIV, with attributes, wrapping other elements. Because of where they're being used, these specific templates will always have a parent, so any has_parent stuff is probably just a confusing and borderline-useless appendage in this case.

That said, I don't feel very strongly about this, so I'm fine with restoring has_parent and its related classes if wanted.

Opened #3089168: Remove or reduce representational CSS classes in all Media Library tests to deal with #69 through #71.

Whoops, sorry! Accidentally removed some stuff from MediaLibraryTest::testFieldUiIntegration() that should still be there.

Rerolled on top of the changes in #3089168-7: Remove or reduce representational CSS classes in all Media Library tests, shaving another 20 KB off the size of the patch. Yeah!

Discussed #75 with @effulgentsia. We agreed that our specialized container templates should probably not deviate too much from the default template for containers, so kicking this back to "needs work" to restore the has_parent flag, and related classes, to those templates.

Also waiting on #3089168: Remove or reduce representational CSS classes in all Media Library tests to reduce noise in this issue.

to reduce noise in this issue.

Mission not accomplished! 😛

OK, I had to reroll again due to #3085908: Media library thumbnails are blurry/skewed in IE11 causing a pile of merge conflicts. 🤪

But, this is it. I don't think we'll need any more rerolls -- just iterations (and hopefully few of those, at that). This also fixes the stuff in #75, as @effulgentsia and I agreed on in #84.

Looks like there's a bunch of weird patch noise in #86. I can't explain what's going on; I originally got it due to PHPStorm "intelligently" reformatting my code during paste (eurgh), but I thought I'd removed it all.

Trying again...

I have now written four change records, which I think covers the complete scope of this issue. That has to be some sort of record. Removing the tag. :)

Also... @lauriii and I did a substantial amount of pair programming on this patch. He has guided and approved the approach we took at every step of the way. He did a detailed patch read-through as we approached the final form, and he reviewed and validated @effulgentsia's feedback and suggestions in Slack. I have exactly zero reasons to believe he has any problem with what we've wrought here.

In light of all this, I'm removing the "needs frontend framework manager review" tag.

Crediting @lauriii and @effulgentsia for guidance, pairing, and reviews.

I think it's time I un-assigned this.

I think my only remaining concern with this patch is that a bunch of CSS class assignments are moved to Seven, but remain part of selectors in media_library.modules.css and media_library.theme.css. One example is the media-library-view class, but there are also a few others.

I'm RTBCing this, because I think it's ok to commit this as a great step, but I don't think we should mark Media Library as stable until we bring consistency to that (either by moving those classes to Classy, or else moving the CSS rules to Seven).

I think my only remaining concern with this patch is that a bunch of CSS class assignments are moved to Seven, but remain part of selectors in media_library.modules.css and media_library.theme.css. One example is the media-library-view class, but there are also a few others.

This is actually addressed in the stable-maker; that patch includes sections that move virtually all of the CSS to Seven. @lauriii and I discovered that the media library works fine in Stark, and most of the styling that is currently in Media Library (both in the theme.css and module.css files) is geared towards Seven.

So, tl;dr: we don't need a new issue or anything for this; we will sort out the correct locations for the CSS in the same commit that marks Media Library stable. :)

Linking the "stable maker" I keep talking about, so that future readers of this issue will be able to find it.

Something I realized about the stable maker is that the only remaining CSS selector .media-library-view--widget .media-library-select-all will break on Stark and Stable as a result of this patch. Could this be replaced with a visually-hidden class for example?

I don't think so, because visually-hidden will still keep it accessible to screen readers. We want to suppress it entirely, which is why it uses display: none, IIRC.

My suggestion would be, instead, to change the selector take advantage of the default classes Views adds to a view. So something like: .view-media-library.view-display-id-page .media-library-select-all. That's not perfect, but it might be good enough for now, and in theory we could change it later since the select box is added squarely by module-provided JavaScript, and suppressed by module-provided CSS. This is, for sure, the quickest and easiest option for us to do in this patch.

Another option is for us to change that JavaScript so that it only adds the checkbox to .js-media-library-view.view-display-id-page: a combination of the default classes Views adds, and our own functional class. That would also be an internal module-level change.

Yet another option would be for media_library_views_pre_render() to add a js-no-select-all class to the widget displays in media_library_views_pre_render(), then adjust the JS to recognize that. This, too, would be an internal module-level change.

Do you have a preference?

To address #92 I'd recommend we move this to the stable maker patch.

I'm not sure what "this" is referring to here?

Addressing #95:

1. 👍 Sure, okay. No problem; please just mark the issue fixed when the patch looks like something you would commit, and I'll transfer our work to the stable-maker.
2. ✅ Changed the JavaScript so that the "select all" is only added to .js-media-library-view.view-display-id-page. This allowed me to remove the CSS rule entirely, and remove MediaLibrarySevenTest::openMediaLibraryForField(). Hooray for better tests!
3. 👀 Ooh, nice catch! Fixed.
4. Still need to open this, but 👍

Opened #3089416: Media name should not be hardcoded in templates to address #95.4.

Discussed with @phenaproxima about how to recognize the View ID reliably and we agreed that we would use hook_preprocess_views_view in the Media Library module to add a new data attribute for the View ID.

Almost. 😛 It's the display ID I need, not the view ID. So I'll add this data attribute to the media_library view only, and it will contain the display ID for targeting.

Okay! This addresses #100.

Forgot to run yarn prettier on the previous patch. 🙄

Looks like all the feedback is properly addressed and it's been scrutinized by Drupal's finest, so RTBC.

This patch fixes #108 thusly:

• I removed the media-library-item__click-to-select-trigger class from media_library_preprocess_views_view_fields(). The previous patch already moved it to seven_preprocess_views_view_fields__media_library().
• I moved the after_build callback, and the related stuff in media_library_form_alter(), to Seven.

The failing tests reveal a problem: the stuff I moved out of media_library_form_alter() is actually functional, not presentational, in nature. The #after_build function is responsible for ensuring that the "Apply filters" button is not moved into the modal dialog's button area. So that must live in the Media Library module.

It will be a little verbose for sure, but I'll try to have it both ways -- keep the button where it is, but move the CSS class into Seven. I think it should be doable.

OK. This should fix the tests. I was able to have my cake and eat it too; the stuff that keeps the "Apply filters" button out of the modal dialog's button area is in the Media Library module, as it should be. And the code that adds the media-library-view-form--actions class is in Seven, as it should be.

Well, okay. This oughta do the trick.

Improving and expanding a comment for accuracy, per @lauriii's request.

Oh, and: moved all of the Media Library templates to Seven's templates directory. 😅No interdiff because literally all I did was move some files around.

Rerolled following the revert of #3085908: Media library thumbnails are blurry/skewed in IE11.

#3082690-62: Mark Media Library as a stable core module already includes #118, so that's done. Marking fixed. Thanks!