Media library should not use js- prefixed CSS classes for styling

Just a l'il ol' reroll, so leaving RTBC in place.

I think this looks pretty good and thorough. Nice test coverage! I'd like to see a few small changes, but overall I am +1 for the approach.

1. +++ b/core/modules/media_library/media_library.module
   @@ -115,7 +115,7 @@ function media_library_preprocess_media(&$variables) {
   -    $variables['preview_attributes']->addClass('media-library-item__preview', 'js-media-library-item-preview');
   +    $variables['preview_attributes']->addClass('media-library-item__preview', 'js-media-library-item-preview', 'js-click-to-select-trigger', 'media-library-item__click-to-select-trigger');
   

   Why was this changed? Perhaps this warrants a comment.

2. +++ b/core/modules/media_library/media_library.post_update.php
   @@ -529,3 +529,42 @@ function media_library_post_update_add_buttons_to_page_view() {
   +  $config = \Drupal::configFactory()->getEditable('views.view.media_library');
   

   This is a post-update function, so we can use the full Drupal APIs (e.g., View::load() and friends).

   On second thought, I see that we're doing this to improve the abstraction. So okay, we can keep it this way if we want to.

3. +++ b/core/modules/media_library/media_library.post_update.php
   @@ -529,3 +529,42 @@ function media_library_post_update_add_buttons_to_page_view() {
   +      'display_type' => 'default',
   

   display_type is a misleading name for this key. It's really the display_id, so let's change it to that.

4. +++ b/core/modules/media_library/media_library.post_update.php
   @@ -529,3 +529,42 @@ function media_library_post_update_add_buttons_to_page_view() {
   +      'class_key' => 'element_class',
   

   This is misleading too. 'class_key' doesn't really mean anything to me; this is actually a configuration option for a particular field in a particular display of the view. So maybe something like 'option' instead?

5. +++ b/core/modules/media_library/media_library.post_update.php
   @@ -529,3 +529,42 @@ function media_library_post_update_add_buttons_to_page_view() {
   +      'form' => 'media_bulk_form',
   

   Similarly, I think 'form' is misleading. It's the name of the field in the view, so perhaps something like 'field' would be better.

6. +++ b/core/modules/media_library/media_library.post_update.php
   @@ -529,3 +529,42 @@ function media_library_post_update_add_buttons_to_page_view() {
   +    if (strpos($classes, 'media-library-item__click-to-select-checkbox') === FALSE) {
   

   Although I find it extremely unlikely, this will collide with existing classes like foo--media-library-item__click-to-select-checkbox. I don't think we necessarily need to change anything here, but a safer thing to do (if we want to be extra paranoid) is to split the classes into an array and use in_array(). But that's extra complexity for, probably, little gain.

7. +++ b/core/modules/media_library/tests/src/Functional/Update/MediaLibraryUpdateCheckboxClassesTest.php
   @@ -0,0 +1,83 @@
   +    $display_items = [
   +      [
   +        'display_type' => 'default',
   +        'class_key' => 'element_class',
   +        'form' => 'media_bulk_form',
   +      ],
   +      [
   +        'display_type' => 'page',
   +        'class_key' => 'element_class',
   +        'form' => 'media_bulk_form',
   +      ],
   +      [
   +        'display_type' => 'widget',
   +        'class_key' => 'element_wrapper_class',
   +        'form' => 'media_library_select_form',
   +      ],
   +      [
   +        'display_type' => 'widget_table',
   +        'class_key' => 'element_wrapper_class',
   +        'form' => 'media_library_select_form',
   +      ],
   +    ];
   

   We should change the misleading keys here too.

8. +++ b/core/modules/media_library/tests/src/Functional/Update/MediaLibraryUpdateCheckboxClassesTest.php
   @@ -0,0 +1,83 @@
   +      $has_prefixed_class = strpos($classes, 'js-click-to-select-checkbox') !== FALSE;
   +      $has_non_prefixed_class = strpos($classes, 'media-library-item__click-to-select-checkbox') !== FALSE;
   +      $this->assertTrue($has_prefixed_class);
   +      $this->assertFalse($has_non_prefixed_class);
   

   This is a bit awkward to read; I think we can use assertContains() and assertNotContains() instead.

9. +++ b/core/modules/media_library/tests/src/Functional/Update/MediaLibraryUpdateCheckboxClassesTest.php
   @@ -0,0 +1,83 @@
   +      $has_prefixed_class = strpos($classes, 'js-click-to-select-checkbox') !== FALSE;
   +      $has_non_prefixed_class = strpos($classes, 'media-library-item__click-to-select-checkbox') !== FALSE;
   +      $this->assertTrue($has_prefixed_class);
   +      $this->assertTrue($has_non_prefixed_class);
   

   Same here.

Only minor stuff, but otherwise this looks good code-wise.

1. +++ b/core/modules/media_library/media_library.post_update.php
   @@ -537,33 +537,34 @@ function media_library_post_update_checkbox_classes() {
   +    $classes_array = explode(' ', $classes);
   

   This needs to be preg_split(), I think, to account for the possibility of multiple spaces between class names. Something like: $classes_array = preg_split('/\s+/', $classes);.

2. +++ b/core/modules/media_library/media_library.post_update.php
   @@ -537,33 +537,34 @@ function media_library_post_update_checkbox_classes() {
   +    if (!in_array('media-library-item__click-to-select-checkbox', $classes_array)) {
   

   We should pass TRUE as the third argument to in_array().

3. +++ b/core/modules/media_library/tests/src/Functional/Update/MediaLibraryUpdateCheckboxClassesTest.php
   @@ -32,51 +32,47 @@ public function testAddNonPrefixedClasses() {
   +      $classes = explode(' ', $config->get($config_location));
   

   Might wanna use preg_split() here too.

Yeah, I'm cool with it. 😎 RTBC once green on all backends.

1. +++ b/core/modules/media_library/media_library.post_update.php
   @@ -529,3 +529,49 @@ function media_library_post_update_add_buttons_to_page_view() {
   + * hook_post_update_NAME() implementations within the same file are  executed
   

   Supernit: there are two spaces before "executed".

2. +++ b/core/modules/media_library/media_library.post_update.php
   @@ -529,3 +529,49 @@ function media_library_post_update_add_buttons_to_page_view() {
   +  $view = Views::getView('media_library');
   

   We should bail out if the view has been deleted for some reason:

   if (!$view) {
     return;
   }
   

3. +++ b/core/modules/media_library/media_library.post_update.php
   @@ -529,3 +529,49 @@ function media_library_post_update_add_buttons_to_page_view() {
   +      $view->storage->save(TRUE);
   

   Nit: This can be $view->save().

4. +++ b/core/modules/media_library/tests/src/Functional/Update/MediaLibraryUpdateCheckboxClassesTest.php
   @@ -0,0 +1,84 @@
   +    ];
   +    ¶
   +    foreach ($display_items as $item) {
   

   Whoops, looks like there's some extra white space right above the foreach.

5. +++ b/core/modules/media_library/tests/src/Functional/Update/MediaLibraryUpdateCheckboxClassesTest.php
   @@ -0,0 +1,84 @@
   +      $this->assertContains('media-library-item__click-to-select-checkbox', $classes, "NAH $display_id.$field.$option");
   

   Should "NAH" be in there?

Another thing that might be worth looking into: over in #3081587-58: Multilingual content is shown double in the media library view, @seanB encountered a similar "hey, we can't control the order of updates" thing. His solution was to add the required changes to both updates that are affected by it, effectively removing any need for them to happen in a particular order. Is that a tactic we might be able to take advantage of here, too?

+++ b/core/modules/media_library/media_library.post_update.php
@@ -570,6 +570,13 @@ function media_library_post_update_update_8001_checkbox_classes() {
+    if (!$display ||
+      !isset($display['display_options']['fields'][$field]) ||
+      !isset($display['display_options']['fields'][$field][$option])) {
+      continue;
+    }

Small nit here -- we don't need to do the two isset checks, since there's implicit checking, AFAIK. So we can do something like: if (!$display || !isset($display['display_options']['fields'][$field][$option])).

+++ b/core/modules/media_library/media_library.post_update.php
@@ -572,9 +572,7 @@ function media_library_post_update_update_8001_checkbox_classes() {
+    if (!$display || !isset($display['display_options']['fields'][$field][$option])) {

Sorry to move the goal post again -- I know how frustrating that is.

I noticed a thing here, though -- we probably shouldn't check if the option is set, since for all we know, a NULL/unset option could be totally legitimate. Views plugins work in mysterious ways. So I would remove [$option] from the isset() check.

Otherwise, this is fine. Please restore RTBC when a new patch is up.

Nope! On second thought, it's good as-is. The reason being, as @bnjmnm explained in Slack, that the alternative is this:

if (!$display || !extremely_long_isset_call()) {
  continue;
}
$classes = extremely_long_isset_call() ? $display[$very][$long][$key][$ending][$with][$option] : '';

Ugh.

So no: it's fine the way it is. Let's go for it.