Integrate focal point with media_library

The patch works for me in drupal 8.8.0
Also I used https://www.drupal.org/project/focal_point/issues/3098235#comment-13386688 for fix issue with preview button.

1. +++ b/src/Plugin/Field/FieldWidget/FocalPointImageWidget.php
   @@ -132,9 +132,12 @@ public static function process($element, FormStateInterface $form_state, $form)
   +    if (
   +      $form['#form_id'] == 'media_library_upload_form'
   +      || $form['#form_id'] == 'media_library_add_form'
   +      || $form['#form_id'] == 'media_library_add_form_upload'
   +    ) {
   

   There may be a better way to do this. In Drupal 8.8, the add form has a base form ID (media_library_add_form) -- we should target that if we can, like so:

   $form_object = $form_state->getFormObject();
   if ($form_object instanceof BaseFormIdInterface && $form_object->getBaseFormId() === 'media_lbrary_add_form') {...}

2. +++ b/src/Plugin/Field/FieldWidget/FocalPointImageWidget.php
   @@ -157,6 +160,11 @@ public static function process($element, FormStateInterface $form_state, $form)
   +      // Override preview for each media element.
   +      if (isset($element['#parents'][1]) && isset($form['media'][$element['#parents'][1]]['preview']['thumbnail'])) {
   +        $form['media'][$element['#parents'][1]]['preview']['thumbnail'] = $preview;
   +      }
   

   I don't know if this is the approach I would take here, either. The fields are displayed with a form mode (called media_library); IMHO, the site builder should configure the source field to be displayed in that form mode using the Focal Point widget, and then hide the preview. I don't think Focal Point should take on that responsibility; the preview was never meant to be treated as a live field, and doing it will likely cause trouble in the future.

The patch from #2 is not working for me with the module using version 1.x-dev.
No focal point settings are being displayed when uploading an image through the media library.

(cross-posting on #3098235 because I'm not sure which parts are relevant to which issue)

Tried the patch at #2 here *and* the patch on 3098235 (comment #6) -- and, of course, with my "form display" settings using "focal point" for my image field. Here are my results:

• I do get the get the focal point widget when I upload a new image via the media library field widget -- and, when I move the "focal point" plus-sign, the focal point values are saved when I save the image media entity and the node to which its attached (I went to the media entity and checked the focal point setting on /media/MID/edit).
  • When I modify the focal point value, there's a theme "warning" in watchdog/dblog -- see "log message details" below this list for output.
• If I don't modify the focal point value for the image (after I upload a new image via the media library field widget, before saving that new media entity), I can click "Preview" and the modal appears, but then I'm stuck -- there's no way back, if I click the "x" on the preview modal, the media library modal is gone, too, and my image/media reference field is empty. (Also, the file exists in the system as a temporary file, but no media entity is created/saved.)
• If I modify the focal point value for the image, the Preview link no longer works. There's an ajax error in my browser console, but it isn't very specific (and it's super long so I'm not pasting it here -- I can add it if y'all want, just, I don't think it says much) -- here's the start of it:
  An AJAX HTTP error occurred. HTTP Result Code: 200

Log message details (see above for context):

 ID         :  1417                                                                                                                                                                                            
 Type       :  theme                                                                                                                                                                                           
 Message    :  Theme hook focal_point_media_library_image_widget not found.                                                                                                                                    
 Severity   :  warning                                                                                                                                                                                         
 Location   :  http://biotech.alison/media-library?_wrapper_format=drupal_ajax&ajax_form=1&hash=PY2jVzh-azEWPr3v6AXbPX2f6t1C-MEQcL7d1uOnd-k&media_library_allowed_types%5Bimage%5D=image&media_library_opener_ 
               id=media_library.opener.field_widget&media_library_opener_parameters%5Bbundle%5D=page&media_library_opener_parameters%5Bentity_id%5D=24&media_library_opener_parameters%5Bentity_type_id%5D=nod 
               e&media_library_opener_parameters%5Bfield_name%5D=field_banner_image&media_library_opener_parameters%5Bfield_widget_id%5D=field_banner_image&media_library_remaining=1&media_library_selected_t 
               ype=image                                                                                                                                                                                       
 Hostname   :  127.0.0.1                                                                                                                                                                                       
 Date       :  18/Dec 17:44                                                                                                                                                                                    
 Username   :                                                                                                                                                                                                  

...................
Drupal 8.8.0
Focal point 8.x-1.2
(Crop API 8.x-2.0-rc1)

I confirm patch #2 working fine with core 8.8.1 + focal_point 8.x-1.2)

I also confirm dblog warning mentioned by @alisonjo315 is brought by this patch (and not patch from this other issue)

Type	theme
Date	Friday, January 3, 2020 - 15:55
User	admin
Location	http://192.168.224.4/media-library?_wrapper_format=drupal_ajax&_wrapper_format=drupal_ajax&ajax_form=1&hash=AsHWQqfaUsd7xRm4l1-J4RHuR1gvjXQOZUHrsOUuSkE&media_library_allowed_types%5B0%5D=remote_video&media_library_allowed_types%5B1%5D=local_video&media_library_allowed_types%5B2%5D=document&media_library_allowed_types%5B3%5D=image&media_library_allowed_types%5B4%5D=audio&media_library_content=1&media_library_opener_id=media_library.opener.field_widget&media_library_opener_parameters%5Bbundle%5D=media&media_library_opener_parameters%5Bentity_type_id%5D=paragraph&media_library_opener_parameters%5Bfield_name%5D=media&media_library_opener_parameters%5Bfield_widget_id%5D=media%3A-content-0-subform&media_library_remaining=25&media_library_selected_type=image
Referrer	http://192.168.224.4/node/add/basic_page
Message	Theme hook focal_point_media_library_image_widget not found.
Severity	Warning
Hostname	192.168.224.1
Operations	

So remaining todo here : Fix that warning

My last patch good working with this patch https://www.drupal.org/files/issues/2019-12-11/focal_point-3098235-6.patch

Using this latest patch I don't get an error when trying to submit an image through the media library, as part of a node's media reference field. That said, it didn't seem to enable the control to actually place the focal point, so I assume it was able to use the default (50, 50). I checked and didn't see any console or watchdog errors.

This may be something of a tangent, but when I tried the same operation within a workspace, I got an error that the form couldn't be submitted within the workspace, only live.

isset($element['#parents'][1])

i suspect no digit should be hardcoded like this.

Tested with Drupal 8.7.7 and patch #10 appears to be working for us, we can specify the focal point and it saves.

Tested with 8.7.11 and v1.2 using both patches as in #10 and so far, so good.

Confirm patch #10 is working with Drupal 8.8.1 against focal_point 1.2.

Tested with Drupal 8.8.2 and v1.2 using both the patches in #10, Focal Point appears. However, I received the same issues as #7. I then tried patch #2 instead (still with this patch) and it still failed. I get the same issues as #7 along with #8.
I've also tried path #10 by itself and I get the same issues as #7.

I've been testing out the cause for the AJAX error. It seems that Focal Point is missing a few attributes when rendering in the media library preview (when using the patch #10).

The first image shows what the image widget looks like with the correct attributes when it's not using the media library.
The second image shows what the preview in the media library without the correct attributes when using the patch.

I also noticed that the media library (src/Form/FileUploadForm.php) is hiding the image from displaying in the fields section:

 // Remove preview added by ImageWidget::process().
    if (!empty($element['preview'])) {
      $element['preview']['#access'] = FALSE;
    }

It might be beneficial to show the image in the fields section instead of the media library preview.

I'm still learning about Drupal and it's taking me longer than expected to find a solution. I'm hoping my findings can push forward a patch that shows Focal Point and allows the users to move the indicator and click on the preview link without issues.

Thanks.

Patch #10 works for me.

Patch file in #10 works for me. (https://www.drupal.org/files/issues/2020-01-09/3094478-10.patch)

I've been getting the same AJAX error as #7 mentions:

If I modify the focal point value for the image (moving the cross hair), the Preview link no longer works. There's an ajax error in my browser console, but it isn't very specific (and it's super long so I'm not pasting it here -- I can add it if y'all want, just, I don't think it says much) -- here's the start of it:
An AJAX HTTP error occurred. HTTP Result Code: 200

I have found out that

we need to run some server-side code to flush the old image derivatives

from a comment on ticket "Preview Modal Conflicts with Media Library Modal #4" after moving the cross hair.

I have combined patch #10 from here and some of patch #4 (effulgentsia) from "Preview Modal Conflicts with Media Library Modal". The combination seems to be working great.

However, I am unsure if this ticket is the correct location to include the server-side code or if it should be posted at "Preview Modal Conflicts with Media Library Modal".

Ignore, wrong patch

Ignore

How do I delete my bad patches? I've finally included the correct one. Sorry, I'm having an off day.

Here's the correct patch tied to #22

(re: hiding/deleting the other patches -- you got it, they're hidden from the top of the issue, etc.!)

Attached is an interdiff of 10 to "23_1" "goes with #22, posted on #25" :)

@thetaPC Are you saying your patch includes parts of the changes they're doing on our partner issue #3098235? I don't think you'd want to combine the two efforts, right? Sorry if I'm misunderstanding.

(At this point, though, I can't apply #25 and 3098235 comment #13 together, and I can apply #10 and that patch from the other thread together... I'm not sure what that means, but, that's what I've run into at this point. But also, I haven't gotten to test the functionality itself, ack, sorry! Anyway, sharing what I can share. Thank you!)

@alisonjo315 thank you for the interdiff! I'm very new to creating patches.

I didn't combine them. I just used the partner issue #3098235 caching code that effulgentsia points out that is missing. I believe his caching code should be pushed to this ticket instead of the partner issue.

I do have the same conflict issue when apply #25 and 3098235 comment #13. I'm not sure if I should fix #25 when this issue is stand alone. Plus 3098235 ticket appears to be a duplicate from a core ticket.

Tested the latest patch #25, and it works! I'm using Drupal 8.8.1 with focal point 1.4 and a media entity reference field with a media library widget.

Not sure what/where everyone else is testing. I'm trying to edit an image media entity, directly at /media/[eid]/edit.

With #25 applied I briefly see the focal point form elements (until the JS loads, presumably) but then they disappear and the focal point drag-and-drop widget does not appear.

I also tried the patch from #10 (no better for me), and the patch from #10 with the latest patch from #3098235: Preview Modal Conflicts with Media Library Modal (still no better). #25 appears to conflict with the patch from this other issue.

I couldn't observe any JS or db errors. Anything else I should try?

Here's another attempt. It needs tests, but in manual testing it adds Focal Point to the preview when adding an image in the media library, and it avoids the problem reported in #29. It's very targeted and only affects the media library.

Slight refactoring, and bumped the minimum required version of core to 8.8 in order to support my use of the entity_display.repository service. We could loosen that, of course, but I'd prefer to do that in a later patch if we decide we want to support 8.7.

More cleanup and comments to make it clearer what the code is doing.

@mandclu I'm testing it through the media library modal (media library form display). I believe that "/media/[eid]/edit" uses the default form display which shouldn't render the media library modal.
(Please correct me if I'm wrong especially since I don't know your setup.)

"Preview Modal Conflicts with Media Library Modal" appears to be a duplicate ticket of "Nested modals don't work: opening a modal from a modal closes the original." We might want to consider closing ticket #3098235 as a duplicate.

@phenaproxima I can't help with testing since I couldn't apply the patch through composer. I'm not sure what caused it to fail. Sorry that I can't help.

This patch adds a test to the patch in #32.

borrowed something from Media Library. :)

1. +++ b/tests/src/FunctionalJavascript/MediaLibraryIntegrationTest.php
   @@ -0,0 +1,106 @@
   +    $this->drupalGet('/admin/structure/types/manage/article/fields/add-field');
   +    $page->selectFieldOption('new_storage_type', 'field_ui:entity_reference:media');
   +    $this->assertNotNull($assert_session->waitForField('label'));
   

   Nice work on these tests! However I think that all of this setup can be done programatically instead of stepping through the UI. Testing the UI can be done separately as we're only concerned here with the integration between media library and focal point.

2. +++ b/tests/src/FunctionalJavascript/MediaLibraryIntegrationTest.php
   @@ -0,0 +1,106 @@
   +    $this->assertElementExistsAfterWait('css', '[name="settings[handler_settings][target_bundles][image]"][checked="checked"]');
   ...
   +  protected function assertElementExistsAfterWait($selector, $locator, $timeout = 10000) {
   

   This function is unnecessary, you can simply wrap the calls inline so waitForElement is inside assertNotEmpty.

That test looks great! I streamlined it to do the setup programmatically and was able to remove the helper method. I also made a couple of assertions a bit more specific.

Added a little comment to explain a previous choice.

The latest patch applies clean for me, but I still am not seeing the focal point widget, either in the standalone media edit form or the modal. That said, even though I have selected the focal point widget for both the default and Media Library form view modes, I don't see the preview link, even though I have selected that too. Is there maybe an additional patch I need?

@mandclu, there shouldn't be any additional patch needed. Can you post screenshots of what you're seeing in the modal, and of how the media library form display is configured for the media type you're trying to add? If you can, a full config export would also be really useful to see if we can reproduce your problem. It's totally possible that something is broken and we're missing test coverage. :)

Thanks @phenaproxima! It does work for me: I changed the form display both in the default form mode (/admin/structure/media/manage/image/form-display) and in the Media Library one (/admin/structure/media/manage/image/form-display/media_library) and it worked.

I did a bunch of playing around with the patch in #42 and it seems to work well... When looking at the code one thing worries me:

+++ b/focal_point.module
@@ -109,3 +110,54 @@ function focal_point_entity_update(EntityInterface $entity) {
+    array_splice($target, -4, count($target), ['preview', 'thumbnail']);

How confident are we in these array_parent values? That `-4` sticks out like a hard-coded sore thumb... is there another way to achieve this?

How confident are we in these array_parent values? That `-4` sticks out like a hard-coded sore thumb... is there another way to achieve this?

I feel pretty confident about it.

That structure comes from the Media Library's AddFormBase, which is (as the name implies) a base class that's meant to be extended (I helped write it, and continue to maintain it ;-). Therefore, that form array is basically an API.

Besides that, I've seen this same pattern used elsewhere, relying on a specific "distance" between levels of a form array. Obviously, because of the inherent flexibility of PHP arrays and the existence of so many alter hooks and callbacks, no form structure is 100% reliable, but that's par for the course in Drupal, and I'm not sure how else we could achieve this.

Ok ... sold!

Thanks for all the work on this one!

@phenaproxima thanks for the offer to investigate why this isn't working for me. One obvious difference I notice vs. your screen captures is that your appear to be using Claro as your admin them, where as I am using Adminimal. I did also switch to Claro but there was no difference, functionally.

Default form setting:

Media Library form setting:

Editing in the Media Library:

Note that the no-js form is in the page markup, and is briefly visible on page load, then disappears (gets assigned a class of visually-hidden). But the interactive widget never loads as a replacement, so there is no way to set the focal point.

Same form, within the Media Library modal. Same as above, the standard form loads then is hidden, but the widget never loads. No JS errors logged in either case.

I did some more investigation, after I realized that I wasn't getting the Preview link either. As an experiment, I went into the FocalPointImageWidget and changed line 158 from $element['preview'] = $preview; to $element['preview_link'] = $preview;. The result was very interesting:

I now had two previews, one of which has the Preview button and the focal point widget! I was able to find another module that also overwrites $element['preview'], namely cohesion_elements, in a preprocess function. If I comment out the relevant code, the focal point displays as expected.

Should I file this as a bug report with Cohesion?

I finally managed to test the patch that was committed and the AJAX error is back.

The interdiff from comment #26 fixed the AJAX error but it's missing in the committed code. This code flushes the cache when the cursor is moved.

Steps to reproduce:

1. Upload an image to the media library widget.
2. Move the Focal Point cursor anywhere.
3. Click on the preview link and nothing happens. AJAX error appears in the console.

@mandclu and @thetaPC, will you file separate bug reports for these things? They look like legitimate issues to me, but this issue is basically done, so IMHO we should fix further integration bugs in new issues.

@phenaproxima Got it, thank you!! I'll go ahead and make a ticket for it.

@phenaproxima roger that, added #3144968: Incompatible with Acquia Cohesion. Will also open an issue with Acquia.

Is there a release planned for this patch?

I created a follow-up for #49: #3173647: Preview button broken when using media_library for uplading

Was there a release for this? It's marked as fixed but not sure what version is supposed to include the fix.

@rembrandx it's included in the latest stable release 8.x-1.5.

Would it be wise to acknowledge that this module is not yet compatible with Drupal media/Library (Drupal 9+)? To be sure, It works perfectly fine with regular image field, but doesn't show when using media library. Perhaps, this limitation needs to be flagged up, rather than encouraging tinkering with patches which works for some and not for others. Just a thought.

@Basseye - this does in fact work with media library, but you have to configure the form view for media library to use Focal Point. Documentation absolutely needs to be updated to let the non-developer know about how to reconfigure Media Library to use this module.

To @SuperfluousApostrophe''s point, I struggled to find this and I've been using Drupal for a long time. It is not readily apparent.

For anyone who might stumble upon this, I found this at /admin/structure/media/manage/image/form-display/media_library. Choose the "Image (Focal Point)" widget. Then when you edit any image at /admin/content/media you will see the Focal Point crosshairs.

Lastly, after installing and enabling the Focal Point module be sure to run drush updb or visit /update.php or Focal Point may not work.

I've created #3268541: Document how to integrate focal point with media library for the documentation.

I've created a module that allows you to set the focal point on the embed token with a media embed. I actually found this issue after building it.

https://www.drupal.org/project/media_embed_focal_point.

I stumbled across https://www.drupal.org/project/media_library_media_modify and https://www.drupal.org/project/multi_crop and saw a gap where we could pass focal point data from the embed tokens.

Can someone explain how this patch compares to my module? In the end, I really want this feature so want to plug in wherever is best.

It seems like the patch #25 was working with the media library ckeditor but the code that has been commited does not (works fine when adding media through field widget, but not in modal through ckeditor).

Just putting a reroll of #25 that fixes the issue in ckeditor in case anybody needs it.

Reroll of #64 to fix ckeditor issue to work with 2.1.2