Media Library add form should suppress extraneous components of image fields using a form alter, not CSS

Blocker is in!

I started working on it.

Actually is working, but still requires to restrict of the alter to media library. I would like to know if looks like the correct approach before working further on it.

thanks!

Adding a new version of the patch with the missing part of the previous one, so forget #4.

Is altering the image widget render to remove the data that was previously hidden by CSS, and removing the widget title in the form building. Also cleaned up the CSS.

One nice option here is not to provide a widget, but instead to implement hook_field_widget_WIDGET_TYPE_form_alter() in Media Library to suppress these components.

Promoting to major as per #2834729: [META] Roadmap to stabilize Media Library.

@seanB and I discussed how to implement this in Slack and we agreed on an approach. The implementation, however, depends on an API change that is made by #3082690: Mark Media Library as a stable core module, so this is blocked on that.

The idea: Media Library will implement hook_field_widget_WIDGET_TYPE_form_alter() for the image_image widget. If the base form ID is media_library_add_form, it will add a #process callback to the image widget, which will suppress the components we want to suppress. We can easily test for this in MediaLibraryTest::testWidgetUpload() and ::testWidgetUploadAdvancedUi().

Here's a patch to get us going in the right direction after the blocker lands.

Tagging this to be worked on at DrupalCon Amsterdam, assuming the blocker lands.

Here's a patch.
It needs:
1) test coverage
2) follow-up for file-size bug. The file-size appears even if you remove filename element.

Update: here's the follow-up: #3090505: File size appears even if filename is removed in a process function

Postponing on #3090505: File size appears even if filename is removed in a process function as well.

This includes a fix for #3090505: File size appears even if filename is removed in a process function in the combined patch.

This patch adds test coverage to MediaLibraryTest. Also, the test caught a bug I would have introduced with the previous patch.

This process function needs to only affect the source field. If there's an extra image field on the media, it shouldn't affect it!

1) Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest::testWidgetUpload
Behat\Mink\Exception\ResponseTextException: The text "Extra Image" was not found anywhere in the text of the current page.

Added this:

+  // Only alter the source field.
+  $media_element = NestedArray::getValue($form, array_slice($element['#array_parents'], 0, 2));
+  if (empty($media_element['fields']['#source_field_name']) || $element['#field_name'] !== $media_element['fields']['#source_field_name']) {
+    return $element;
+  }

There could be a more efficient way to get the source field name by using $form_state->getFormObject, but it would require adding a getter for AddFormBase::mediaType. Thoughts?

We could add ::getMediaType() and then use that to find the source field name. This would be less brittle in if the form structure changes, such as someone adding a wrapper around the image_image widget.

Hmm, getMediaType is already used.

As I was looking at how to get the source field info, I stumbled across a @todo that references this issue, and I was able to figure out a more efficient way to test that we are only affecting the source field. To do this, I found the code that was adding the source field info to the form, and in the same place, I add the process function if applicable. When adding the process function in AddFormBase, it makes sense to move the process function there as well.

1. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -331,9 +331,11 @@ protected function buildEntityFormElement(MediaInterface $media, array $form, Fo
        // We hide the preview of the uploaded file in the image widget with CSS, so
        // set a property so themes and form_alter hooks can easily identify the
        // source field.
   

   I think this is no longer true and needs to be removed as well.

2. +++ b/core/themes/seven/css/theme/media-library.css
   @@ -658,15 +658,6 @@
   -/* @todo Remove in https://www.drupal.org/project/drupal/issues/2987921 */
   -.media-library-add-form__source-field .file,
   -.media-library-add-form__source-field .button,
   -.media-library-add-form__source-field .image-preview,
   -.media-library-add-form__source-field .form-type-managed-file > label,
   -.media-library-add-form__source-field .file-size {
   

   This styling also hides some fields for the document media type. We need to apply the same changes to the file_generic widget. See attached screenshots.

   Before:
   

   After:
   

3. +++ b/core/themes/stable/stable.theme
   @@ -73,6 +73,8 @@ function stable_process_text_format(array $element) {
   +    if (!empty($variables['data']['file_' . $file->id()]['filename'])) {
   +      $variables['data']['file_' . $file->id()]['filename']['#suffix'] = ' <span class="file-size">(' . format_size($file->getSize()) . ')</span> ';
   +    }
   

   I guess this fixes a bug in stable, since outputting the file size when the name is somehow empty doesn't make a lot of sense, but I think we need to confirm that.

Re #21.3, see #3090505: File size appears even if filename is removed in a process function, I created a separate bug ticket for it, but I'm not sure if it needs separate test coverage because it's so self-documenting, and there's no other known use case. You'd have to create a test module that alters the output to test the fix. I'm not sure if it's worth it when this issue will provide ample test coverage.

Addressing #21
1. ✅Removed that comment.
2. ✅Expanded the scope to include any element using FileUploadForm. In fact, I moved everything into that class.
3. I created a separate bug ticket for that originally, but I'm not so sure it needs separate test coverage.

This seems to work for documents as well now, so thanks!

1. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -328,11 +328,8 @@ protected function buildEntityFormElement(MediaInterface $media, array $form, Fo
   +    // Add source field name so that it can be identified in form alter and
   +    // widget alter hooks.
        $element['fields']['#source_field_name'] = $this->getSourceFieldName($media->bundle->entity);
   

   It seems that this can also be removed. I don't see #source_field_name being used anymore?

2. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -244,6 +244,51 @@ public function processUploadElement(array $element, FormStateInterface $form_st
   +    // Remove original button added by ManagedFile::processManagedFile().
   ...
   +    // Remove preview added by ImageWidget::process().
   

   Very clear! Love it.

3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -2412,4 +2419,22 @@ protected function assertSelectedMediaCount($text) {
   +  protected function assertImageWidgetElementsRemoved() {
   

   Do we have a test for documents as well since we originally missed that? I think that would be good to make sure we don't break that in the future.

   That being said, maybe we don't have to check this after every upload and just have a separate test method to verify this for images and documents only once.

Addressing #23
1. I think it's useful for contrib. I suggest we leave it.
3. That's fine. I'll add a dedicated test. MediaLibraryTest is bloated as is.

This looks great, and clean! +1 for the approach here.

1. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -244,6 +244,51 @@ public function processUploadElement(array $element, FormStateInterface $form_st
   +    $element['fields'][$this->getSourceFieldName($media->bundle->entity)]['widget'][0]['#process'][] = [$this, 'processSourceFieldElement'];
   

   This needs to be wrapped in an isset(), just in case the entity form display is in an unexpected state. Something like this:

   $source_field = $this->getSourceFieldName($media->bundle->entity);
   if (isset($element['fields'][$source_field])) {
     $element['fields'][$source_field]['widget'][0]['#process'][] = [static::class, 'processSourceFieldElement'];
   }
   

2. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -244,6 +244,51 @@ public function processUploadElement(array $element, FormStateInterface $form_st
   +  public static function processSourceFieldElement($element, FormStateInterface $form_state, $form) {
   

   We should rename this to something more descriptive, like hideExtraSourceFieldComponents() or something.

3. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -244,6 +244,51 @@ public function processUploadElement(array $element, FormStateInterface $form_st
   +    if (!empty($element['remove_button'])) {
   +      unset($element['remove_button']);
   +    }
   +    // Remove preview added by ImageWidget::process().
   +    if (!empty($element['preview'])) {
   +      unset($element['preview']);
   +    }
   

   We should not be unsetting these, as it means extending code has less flexibility. Instead, let's set #access to FALSE. Because we're doing that, we don't need the !empty() checks. Just doing this should suffice:

   $element['remove_button']['#access'] = FALSE;
   $element['preview']['#access'] = FALSE;
   // ...etc.
   

4. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -244,6 +244,51 @@ public function processUploadElement(array $element, FormStateInterface $form_st
   +    $element['#title_display'] = 'none';
   +    $element['#description_display'] = 'none';
   

   Are there other values besides "none" that would be more appropriate here for accessibility (i.e., should we just make these visually hidden, rather than gone entirely)?

5. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -244,6 +244,51 @@ public function processUploadElement(array $element, FormStateInterface $form_st
   +    // Remove the filename display.
   +    foreach ($element['#files'] as $file) {
   +      if (!empty($element['file_' . $file->id()]['filename'])) {
   +        unset($element['file_' . $file->id()]['filename']);
   +      }
   +    }
   

   This also does not need the !empty() checks, and should use #access => FALSE instead of unset.

6. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -2412,4 +2419,22 @@ protected function assertSelectedMediaCount($text) {
   +  /**
   +   * Assert expected image_image widget elements removed.
   +   */
   +  protected function assertImageWidgetElementsRemoved() {
   +    // Assert that some elements were removed in
   +    // media_library_field_widget_image_image_process().
   +    $removed_elements = [
   +      '.media-library-add-form__source-field .file',
   +      '.media-library-add-form__source-field .button',
   +      '.media-library-add-form__source-field .form-type-managed-file > label',
   +      '.media-library-add-form__source-field .image-preview',
   +      '.media-library-add-form__source-field .file-size',
   +    ];
   +    foreach ($removed_elements as $element) {
   +      $this->assertSession()->elementNotExists('css', $element);
   +    }
   +  }
   

   We can just merge this into assertMediaAdded(). There is no case where we would want these to show up when adding media; we can always assert that they're absent any time we add new media.

   Additionally, we probably shouldn't be asserting the absence of CSS classes, since they are representational and added by Seven. Instead, we should assert the absence of particular fields/buttons by label or name, or elements identified by their data-drupal-selector attribute.

i'm working on it at drupalcon Amsterdam2019

Changing to PP-1, due to what I've found in #3090505-10: File size appears even if filename is removed in a process function. If you use ['filename']['#access'] = FALSE; instead of removing the filename, you don't encounter the bug in that issue.

ok, attached you can find a patch (with related interdiff) that:
- fixes points 1,2,3 and 5
- for points 4 and 6 as discussed with @seanB we have that the classy template doesn't respect the access and still renders an empty div, so we added a css rule to make sure that the image preview is completely removed and in functionalTests we check at least that the image preview is empty.

moving to needs review for automated tests.

just changed the "js-media-library-add-form-added-media" class with "media-library-add-form__fields" css class to target the right remove button

just changed the "js-media-library-add-form-added-media" class with "media-library-add-form__fields" css class to target the right remove button

This isn't going to fly, unfortunately :(

In tests, we cannot use CSS classes which are not prefixed with js-, because they are normally added by themes (and this will be the case once Media Library is stable) -- and therefore, we cannot rely on them. We can use data attributes, or js- prefixed classes, though. The data-drupal-selector attribute should be sufficient.

Media Library is stable, so this is no longer needing the PP-1 designation.

• Rerolled
• Updating css to use data attributes where applicable
• Used twig logic to remove the <div class="image-preview"> element being added in classy:
  

     -  {% if data.preview %}
    +  {% if data.preview|render is not empty %}
    

• Restored empty checks. I think they're good to have in case someone has previously removed these elements in another process function.

I want to try a few things here:

• Removed the reliance on Seven-only CSS classes in MediaLibraryTest, and made the assertions more explicit and specific.
• Removed the if data.preview|render is not empty thing in favor of a new preprocess function in Classy. IMHO the current syntax is more straightforward from a theming standpoint; the preview is either there or it's not. Using the render filter complicates it and exposes the underlying weirdness, which is something we would ideally avoid if we can.
• Removed the change to Stable. I would imagine that, if filename's #access is FALSE, the #suffix won't be printed either. (Let's see what the tests say about this.) Ideally we should avoid changing Stable in any way, since its purpose is to guarantee backwards compatibility.

It looks like any approach we take here is going to require some front-end framework changes, so tagging for front-end framework manager review/sign-off, especially since we're so close to the beta period.

Also removing the "needs tests" tag, since we have coverage for the changes we're making here.

Regarding #38.3, "Removed the change to Stable", it looks like patch #38 accidentally removed the .file-size span, so fixing that. I was meaning to undo the condition as you did (see #28), but forgot about it in #37, so 👍for this change.

Updating the IS to explain the current problem and its proposed solution.

Some minor nits, but I think this is really close. Looking good!

1. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -328,11 +328,8 @@ protected function buildEntityFormElement(MediaInterface $media, array $form, Fo
   +    // Add source field name so that it can be identified in form alter and
   +    // widget alter hooks.
        $element['fields']['#source_field_name'] = $this->getSourceFieldName($media->bundle->entity);
   

   Still not sure if we should keep this...

2. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -244,6 +244,52 @@ public function processUploadElement(array $element, FormStateInterface $form_st
   +   * Processes an image or file source field element.
   ...
   +  public static function hideExtraSourceFieldComponents($element, FormStateInterface $form_state, $form) {
   

   Minor nit, but we only process images/files. The method names suggests we might also change other types of source fields. Maybe hideExtraFileSourceFieldComponents or hideExtraUploadSourceFieldComponents or something?

3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -2289,8 +2289,15 @@ protected function assertMediaAdded($index = 0) {
   +    $fields = $this->assertElementExistsAfterWait('css', '[data-drupal-selector="edit-media-' . $index . '-fields"]');
   ...
   +    $assert_session->elementNotExists('css', '[data-drupal-selector$="preview"]', $fields);
   ...
   +    $assert_session->elementNotExists('css', '[data-drupal-selector$="filename"]', $fields);
   

   Apparently we use data-drupal selectors in tests all the time, so I guess this is fine!

4. +++ b/core/themes/classy/classy.theme
   @@ -32,3 +32,13 @@ function classy_form_alter(array &$form, FormStateInterface $form_state, $form_i
   +/**
   + * Implements hook_preprocess_image_widget().
   + */
   +function classy_preprocess_image_widget(array &$variables) {
   +  $data = &$variables['data'];
   +  if (isset($data['preview']['#access']) && $data['preview']['#access'] === FALSE) {
   +    unset($data['preview']);
   

   Nice one, I like it!

Still not sure if we should keep this...

This is currently used by Seven, and removing it would therefore, technically, constitute a BC-breaking API change in a stable module. Granted, I'm not sure render arrays constitute "APIs"...it's a grey area. Either way, I think this will be helpful to themes and form_alters, so IMHO we should keep it. It harms nothing by its presence.

Minor nit, but we only process images/files. The method names suggests we might also change other types of source fields. Maybe hideExtraFileSourceFieldComponents or hideExtraUploadSourceFieldComponents or something?

I think we can leave it as-is. The method exists on a form called FileUploadForm, which implies that it deals with, well, file uploads. Seems reasonably self-explanatory to me.

#45 Fair enough. If seven already uses this then it makes sense to keep it. And you are right that it lives in a FileUploadForm class, so I can live with that as well. Have nothing else to complain about really. Just needs frontend framework manager signoff I guess?

Consider this RTBC from me.

Discussed with @alexpott and @xjm and we agreed to backport this to 8.8.x during the beta phase because it would be great to get this done as soon as possible to avoid any risk of disruption.

RTBC as per @seanB's feedback in #46.

Awesome, yay for doing this UX fix-up for all themes.

Committed and pushed to 9.0.x; 8.9.x; 8.8.x. Thanks!