Create focal point specific image field widget

I can confirm this. #2 works for me (I can see image focal point widget now under form display).

The idea was to not create a separate widget but rather to add all the logic needed directly to the existing image widget. I'm open to a discussion, but I don't really see a reason to have a completely separate widget.

I don't see the disadvantages (see https://www.drupal.org/node/1882526#why-annotations)

I don't see the disadvantages

There is no technical disadvantage, but I feel like that would be very confusing to a site builder. Right now, when it's time to choose a field widget for an image field they just choose "image" as they always have. With this patch they will see two choices (a) the standard image widget and (b) the focal point image widget both of which do exactly the same thing.

If I was a random site builder and saw this I would file a bug saying that I chose the image widget and not the focal point image widget but I'm still seeing the focal point functionality.

At the end of the day I'll turn this around and say, what are the advantages?

Advantage of dedicated widget:
1) You (builder) can decide per field to use Focal Point, some other image widget or not
2) Content editor doesn't need to be bothered with Focal Point widgets on every image

Things todo:
1) Remove image widget alter
2) Notify builders that default image widget no longer has 'focal point', hook_update_N() ?

Alternative:
* Image Widget setting to enable/disable Focal Point as *third_party_settings*
* (nice 2 have) Default focal point X,Y per image widget as *third_party_settings*

After some thinking, I liked the idea of some extra widget settings the most.
The patch will create widget settings to: disable/enable Focal Point, hide/show preview link, and set the default location of the focal point.

If needed I can create a update hook to enable Focal Point by default for existing image widgets.
As you can see, I didn't enabled Focal Point by default.

I tested #9 and it works. I really like the new settings added in #9, but I agree with kikoalonsob and slasher13 that focal point should use it's own form widget. The current implementation uses the Drupal 7 way to alter a widget while we are developing for Drupal 8.

If we'd make a custom widget, site administrators could easily choose whether they want to use focal point on a per instance basis. An other plus point is that the code for the widget can be located in one class, the widget class, instead of some code in the module and some in the class.

I totally agree with frank.schalkwijk! It was very hard and ugly to create the current patch.

I'm happy to create a new patch if the maintainer is willing to commit to a "new" Image Widget for Focal Point.

I'm happy to create a new patch if the maintainer is willing to commit to a "new" Image Widget for Focal Point.

What would happen to existing sites in this case?

A hook_update_N should update all existing image field instances to use the new widget.

I wouldn't use the focal point widget as default widget for new image fields, but that would also be possible.

Patch in progress

New patch, without an update hook for now. An update hook can be created after approval of the patch.

There are some todos left, but that is more for after applying this patch.

Why are the validateFocalPoint and process methods static?

$element['#process'][] = [get_called_class(), 'process'];

can be written like:

$element['#process'][] = [$this, 'process'];

and the same for

'#element_validate' => [[get_called_class(),'validateFocalPoint']],

to:

'#element_validate' => [[$this, 'validateFocalPoint']],

or am I missing something from just reading the patch file? :)

To be honest, I'm still not 100% sold on this, but I am warming to the idea (mostly because it seems that more and more people are asking for it). I still like the idea that installing focal point will work for existing image fields without any changes at all. I like that the is no setup needed. This adds something for the site builder to do to get focal point working (albeit something pretty trivial).

I'm putting some feelers out to some other folks to see if I can get even more opinions, but like I said I am warming to this.

That said:

1. +++ /dev/null
   @@ -1 +0,0 @@
   -display_link: true
   
   +++ b/config/install/focal_point.settings.yml
   @@ -1,2 +1 @@
   -default_value: '50,50'
   

   this should be kept in mind when writing an update hook. If these values have been changed, they should be used by the field specific configs

2. +++ b/src/Plugin/Field/FieldWidget/FocalPointImageWidget.php
   @@ -23,12 +26,85 @@ class FocalPointImageWidget extends ImageWidget {
   +   * @todo Make Focal Point element accessible
   +   *   by using visually-hidden, focusable, and cleanup focal_point.js.
   

   this ++

3. +++ b/src/Plugin/Field/FieldWidget/FocalPointImageWidget.php
   @@ -147,16 +221,16 @@ class FocalPointImageWidget extends ImageWidget {
   +   * @todo Merge validateFocalPoint() & validateFocalPointWidget().
   

   I'm not sure about this. By keeping them separate, a focal point value can be validated programmatically.

1. +++ b/src/Plugin/Field/FieldWidget/FocalPointImageWidget.php
   @@ -23,12 +26,85 @@ class FocalPointImageWidget extends ImageWidget {
   +    return array(
   +        'preview_link' => TRUE,
   +        'offsets' => '50,50',
   +      ) + parent::defaultSettings();
   

   Indented weird. Also use [] not array().

   Also I believe you need config schema for these settings. If this doesn't fail tests, there's not enough coverage.

2. +++ b/src/Plugin/Field/FieldWidget/FocalPointImageWidget.php
   @@ -23,12 +26,85 @@ class FocalPointImageWidget extends ImageWidget {
   +   * @todo Make Focal Point element accessible
   +   *   by using visually-hidden, focusable, and cleanup focal_point.js.
   
   @@ -147,16 +221,16 @@ class FocalPointImageWidget extends ImageWidget {
   +   * @todo Merge validateFocalPoint() & validateFocalPointWidget().
   
   @@ -166,6 +240,18 @@ class FocalPointImageWidget extends ImageWidget {
   +   * @todo Merge validateFocalPoint() & validateFocalPointWidget().
   

   @todo should have issue links

3. +++ b/src/Plugin/Field/FieldWidget/FocalPointImageWidget.php
   @@ -70,7 +146,7 @@ class FocalPointImageWidget extends ImageWidget {
   -          '#title' => t('Preview'),
   +          '#title' => new TranslatableMarkup('Preview'),
   

   Out of scope

4. +++ b/src/Plugin/Field/FieldWidget/FocalPointImageWidget.php
   @@ -102,10 +178,10 @@ class FocalPointImageWidget extends ImageWidget {
   +      '#element_validate' => [[get_called_class(),'validateFocalPoint']],
   

   can use static::class instead of get_called_class()

Note that I fixed the translatable string issues in #2848530: Make field label and link translatable on image widget

... I spoke to a few more people today and I think that the benefits of this outweigh my desire to keep the setup as simple as humanly possible. Its not so unreasonable to expect a site builder to choose a different widget in order to have different functionality. Soooo... if we can whip this patch into shape and have a solid upgrade path then I'm on board.

Shout out to for giving me feedback
elijahlynn
jfrederick
tim.plunkett

Thanks for the feedback! Will implement the feedback a.s.a.p. but if anyone can fix it sooner, feel free to assign the issue to yourself. :)

Will this only affect image fields using the 'image' widget? What about users with the file/entity browser widget? Will this patch have any impact on the work done on #2701725: Integrate focal point with file entity and entity browser?

Hi tanc,

I think this will affect every issue. But if it's done right, everything will work in the end.

Plan to get this work could be something like this:

1. Create a field widget (this issue)
2. Create update hook (this issue)
3. Adjust the automatic tests (this issue)
4. #2657592: Convert focal point selector tool into a standalone form element - cleaner code / easier to extend
5. #2701725: Integrate focal point with file entity and entity browser
6. Create more tests
7. Fix other issues

Sounds good :-)

I'll keep an eye on this and #2657592: Convert focal point selector tool into a standalone form element and help out where I can.

In progress..

New patch with the processed feedback and a few small fixes.

The interdiff is not 100% based on patch #15, this is because the translation issue has been fixed in dev.

+++ b/config/schema/focal_point.schema.yml
@@ -28,3 +20,20 @@ image.effect.focal_point_crop:
+    progress_indicator:
+      type: string
+      label: 'Progress indicator'

+++ b/src/Plugin/Field/FieldWidget/FocalPointImageWidget.php
@@ -23,12 +26,87 @@ class FocalPointImageWidget extends ImageWidget {
+      'progress_indicator' => 'throbber',

Not sure why you are adding info about a progress indicator. What progress are we indicating? The image upload itself? Isn't that inherited from the standard image field widget?

Other than that, this is getting pretty close except for the update hook...

Anyone able to take a swing on the update hook?

I'm working on hook_update, will submit a patch in a while.

+++ b/focal_point.install
@@ -142,3 +142,36 @@ function focal_point_update_8003() {
+              $componentConfig['type'] = 'image_focal_point';

I think we should have another condition which checks if the widget was image_image (image default widget).

You are right.

The patch from @berdyshev in #33 works great. Thanks!

The updated failed on my site with "Failed: The "image_focal_point" plugin does not exist.". I think we should flush the widget definitions before update.

This is REALLY REALLY close. I'm going to make it my unofficial goal to try and get this committed before the end of Drupalcon. If any of you are here... lets meet and finish this. :)

1. +++ b/src/Plugin/Field/FieldWidget/FocalPointImageWidget.php
   @@ -23,12 +26,87 @@ class FocalPointImageWidget extends ImageWidget {
   +    // We need a preview image for this widget.
   +    $form['preview_image_style']['#required'] = TRUE;
   +    unset($form['preview_image_style']['#empty_option']);
   

   Yes! Good catch

   But we need to figure out how to communicate (or force?) that people should pick an image style that maintains the aspect ratio of the original image AND does not include a focal point effect. Can we (at least) add to the field description right now and add an @todo for enforcing this at a later date.

2. +++ b/src/Plugin/Field/FieldWidget/FocalPointImageWidget.php
   @@ -23,12 +26,87 @@ class FocalPointImageWidget extends ImageWidget {
   +    $form['offsets'] = [
   +      '#type' => 'textfield',
   +      '#title' => $this->t('Focal point offsets'),
   +      '#default_value' => $this->getSetting('offsets'),
   +      '#description' => $this->t('Specify the default focus of this widget in the form "leftoffset,topoffset" where offsets are in percentages. Ex: 25,75.'),
   +      '#size' => 7,
   +      '#maxlength' => 7,
   +      '#element_validate' => [[$this, 'validateFocalPointWidget']],
   +      '#required' => TRUE,
   +      '#weight' => 35,
   +    ];
   

   I'm concerned that this is confusing in the context of a field widget setting... It makes sense as a global default value that only a dev can really set but it seems very confusing to me for a site admin who is setting up focal point for the first time to be presented with this option.

   The term offset is not meaningful to end users.

   I'm going to try and find some people that are unfamiliar with focal point and do some informal, unscientific user testing

   Thoughts?

3. +++ b/src/Plugin/Field/FieldWidget/FocalPointImageWidget.php
   @@ -23,12 +26,87 @@ class FocalPointImageWidget extends ImageWidget {
   +    $summary[] = $this->t('Offsets: @offsets', array('@offsets' => $offsets));
   

   This really should be $summary[] = $this->t('Default focal point: @offsets', array('@offsets' => $offsets));

   The term offset is not meaningful to end users.

Ok ... so I think this might be the one. The only differences are around clarifying some of the language... but otherwise I think this is good to go.

Looks good

I'll take that as RTBC ... COMMITTED!

Awesome, thanks! :)