Problem/Motivation

When the focal point widget is used within a collapsible Details element, which is collapsed initially, the cross indicating the focal point is positioned wrong:

(I made the corresponding focal point settings field visible to show the real values that should be respected when the cross is positioned. You can also see the CSS applied to the cross)

Proposed resolution

Use correct position for cross regardless of whether the image/container is visible initially.

Steps to reproduce

  • Create an image field or base field definition of type image for the node entity type - e.g.:
    /**
     * Implements hook_entity_base_field_info().
     */
    function YOUR_MODULE_entity_base_field_info(EntityTypeInterface $entity_type) {
        $fields['FIELD_NAME'] = BaseFieldDefinition::create('image')
          ->setLabel('Image')
          ->setDescription('The image.')
          ->setRevisionable(TRUE)
          ->setTranslatable(TRUE)
          ->setSetting('alt_field', 0)
          ->setSetting('file_directory', 'images/[date:custom:Y]-[date:custom:m]')
          ->setDisplayOptions('form', array(
            'type' => 'image_image',
            'weight' => 10,
            'settings' => array(
              'preview_image_style' => 'thumbnail',
            ),
          ))
          ->setDisplayOptions('view', array(
            'type' => 'hidden',
          ));
    
      return $fields;
    }
    
  • Create custom Details element and move image field into it:
    /**
     * Implements hook_form_FORM_ID_alter().
     */
    function YOUR_MODULE_form_node_form_alter(&$form, FormStateInterface $form_state, $form_id) {
      // Create details element.
      $form['GROUP_NAME'] = [
        '#type' => 'details',
        '#group' => 'advanced',
        '#title' => t('Group name'),
        '#open' => FALSE, // Initially collapse details element.
        '#attributes' => array(
          'class' => array('node-form-group-name'),
        ),
        '#weight' => -20,
        '#optional' => FALSE,
      ];
    
      // Add field to details element.
      $form['FIELD_NAME']['#group'] = 'GROUP_NAME';
    }
    
  • Create node with image with custom focal point
  • Edit that node again and expand details element to see nested image field - cross is not positioned correctly
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hctom created an issue. See original summary.

bleen’s picture

Hrmmm ... I think to solve this we need to add a JS event for when the crosshair becomes visible.

To be honest, I'm surprised that it's not positioned at the top left (0,0)

hctom’s picture

I haven't had a look at the code yet, but i guess it is because the width is calculable, but the height of the Container is 0 when it is initially collapsed.

I will have a look at the Scripts as soon as I can find the time.

hctom’s picture

Status: Active » Needs review
FileSize
2.35 KB

After some research, I found several posts saying that $.height() and $.width() only work for visible elements. See http://api.jquery.com/height for details:

The value reported by .height() is not guaranteed to be accurate when the element or its parent is hidden. To get an accurate value, ensure the element is visible before using .height(). jQuery will attempt to temporarily show and then re-hide an element in order to measure its dimensions, but this is unreliable and (even when accurate) can significantly impact page performance. This show-and-rehide measurement feature may be removed in a future version of jQuery.

But this does not seem to work, if the image's parent container is not visible.

Attached you can find a patch, which introduces a new JavaScript method, which determines the image's size. This method checks if the image is visible - if not, it clones the image and attaches it temporarily to the body, to be able to get its size.

I'd appreciate your feedback.

bleen’s picture

I haven't been able to look at this issue too closely, but I'm not sure that the patch in #4 is a great solution. I dont like the idea of having to manipulate the DOM like that every time we need the height/width of an image. For one thing, this will happen *every* time whether or not the image was hidden. Even if this does turn out to be the best strategy I would want to see the height/width values stored in some property somewhere so that we dont have do the DOM manipulation multiple times.

As an alternate strategy, couldn't we simply check the image height/width using the width and height attributes on the img tag or in JS using the naturalHeight & naturalWidth instead?

Thoughts?

hctom’s picture

Status: Needs review » Needs work

You are right... Manipulating the DOM should be the last try to get the real values. So I will provide a new patch having some other methods trying to determine the image's size before finally falling back to the DOM manipulation.

Storing the size in a property on the other hand is not good idea, because during resizes of the window, the image size may change as well (e.g. for images with a max-width set).

... and to clear things out: The fallback implementation of determining the size of the image with other methods than $.width() or $:height() is only performed for hidden / not visible images. So this should not happen too often on the page.

Okay... but I will now work on my patch to get some better fallbacks before manipulating the DOM ;)

hctom’s picture

Okay... this really seems to be a hard one ;)

  • Checking the image size with width or height JavaScript properties do not take inline styles for width/height into account, clientWidth / clientHeight ignore them, too
  • Checking the image size with width or height HTML attributes do not take inline styles for width/height into account and might not even be present in the markup
  • Using naturalWidth or naturalHeight return the size of the original loaded image while ignoring width/height HTML attributes, CSS styles and inline styles
  • Temporary cloning the image and appending it to the DOM somewhere outside the viewport might also not use the size of the real displayed image (e.g. if the wrapper of the image forces it to a specific size)
  • As far as I see there is no real event in Drupal core's JavaScripts for collapsible elements (like a Details container) which might be observed to correct the cross position on visibility change
  • Using something like MutationObserver to watch visibility changes is not supported by all browsers

Unfortunately I am running out of ideas... anybody else has one?

shadcn’s picture

@hctom what are the steps to reproduce this? I'm using the latest development version of focal_point and field_group 8.x-1.0-rc4.

I have an image field inside a Detail element and it seems to be working fine.

hctom’s picture

Issue summary: View changes

@arshadcn: I just updated issue with steps to reproduce. I did not use the field_group module, because I did not find a way to get a custom details element into the default node form sidebar with it.

shadcn’s picture

Thanks @hctom. I'll try this.

shadcn’s picture

@hctom yes I can see the bug now. Looking into it.

shadcn’s picture

@hctom I can see the crosshair wrongly positioned while the image loads (simulating a slow load by throttling the network) but as soon as the image loads, fp.setIndicator(); kicks in and fixes the position. Are you seeing the same thing?

hctom’s picture

@arshadcn I just gave it a try... I throttled the network to GPRS, left the details element closed until the whole page is loaded and the crosshair is again positioned at the top of the image. When opening the details element during load, the crosshair is of course positioned at the correct position, because at that point the image is visible and so $.height() returns correct value.

bleen’s picture

@arshadcn ... yeah, I think throttling the network only helps to illustrate the issue, but the real problem occurs when the fp.setIndicator() function is called while the image thumbnail is hidden (because its in a collapsed element).

While I still haven't had time to dig into the code myself, I'm still leaning towards using naturalWidth and naturalHeight as the solution to this. Let me comment on this:

Using naturalWidth or naturalHeight return the size of the original loaded image while ignoring width/height HTML attributes, CSS styles and inline styles

Thats 100% true, but if a site uses CSS to manipulate the size of the thumbnail image, aren't all bets off? Since focal point relies on being able to accurately set the proportional left/top offsets in relation to the thumbnail then a site that manipulates that thumbnail needs to be responsible for fixing the behavior of focal point. Not to mention that the identical problem exists if you temporarily clone the image and stick it on the DOM somewhere... you'd have to accurately bring in all the CSS styles into that equation to get the width/height.

Alternatively...

Using something like MutationObserver to watch visibility changes is not supported by all browsers

The browser support seems pretty reasonable, actually: http://caniuse.com/#search=MutationObserver

..if not, is there a way to force the image to be displayed (e.g. expand the detail pane) in a generic way just long enough to get its width/height?

DarkstarTom’s picture

I'm having this problem too. I have an image field displayed within a field group and the focal point crosshair is positioned at the top of the image. Is there a fix or any update on this?

DarkstarTom’s picture

Even if the details element is set to open by default there is still a problem when the Bootstrap "img-responsive" class is applied to the Focal Point previews. Any responsive resizing of the previews makes the white cross move to a wrong location.

redbrickone’s picture

Also having this issue, leaving this here for updates.

bleen’s picture

esod’s picture

We're also having this problem. The source of our problem is we group node fields together and hide/display them in jquery tabs. jQuery tabs is a nice solution for us since our node forms contain dozens of fields. But we run into the same problem with the module.

I'll share our solution when we write it.

Chi’s picture

Here is how CodeMirror solves very similar problem with textarea rendering in a hidden region.
https://codemirror.net/addon/display/autorefresh.js

ayalon’s picture

I fixed it in a custom behaviour in a custom module for vertical tabs (Default Drupal Backend).

Basically I just re-attach the behaviour when you click on the tab.

Easy fix without patching.

/**
 * Drupal Behaviors
 */
(function (Drupal) {

  Drupal.behaviors.backend_fixes = {
    attach: function (context, settings) {
      jQuery('.vertical-tabs__menu-item a').on('click', function () {
        var id = jQuery(this).attr('href');

        //See https://github.com/RobLoach/jquery-once/blob/master/jquery.once.js#L133
        jQuery(id).find('.focal-point-indicator').removeOnce('focal-point-indicator');
        Drupal.behaviors.focalPointIndicator.attach(jQuery(id));

      });
    }
  };

})(window.Drupal);
EgbertB’s picture

Using seven maintenance theme, I solved it using this (added this to the nav tab.js)

  Drupal.behaviors.backend_fixes = {
		    attach: function (context, settings) {
		      $('.seven-details summary').on('click', function () {
		        var $id = $(this).parent().find('.focal-point-wrapper');
		        $id.find('.focal-point-indicator').removeOnce('focal-point-indicator');
		        Drupal.behaviors.focalPointIndicator.attach($id);

		      });
		    }
		  }
thejimbirch’s picture

Thanks @ayalon @EgbertB

Adding the JavaScript to remove and re-attach worked for me having 2 focal point images in one closed horizontal tab.

I did not have a custom admin theme, so I added the JavaScript in a custom module.

First I created the js/focal-point-fix.js

/**
 * Drupal Behaviors
 */
(function (Drupal) {

  Drupal.behaviors.backend_fixes = {
    attach: function (context, settings) {
      jQuery('.horizontal-tab-button a').on('click', function () {
        var id = jQuery(this).attr('href');

        //See https://github.com/RobLoach/jquery-once/blob/master/jquery.once.js#L133
        jQuery(id).find('.focal-point-indicator').removeOnce('focal-point-indicator');
        Drupal.behaviors.focalPointIndicator.attach(jQuery(id));

      });
    }
  };

})(window.Drupal);

I then created the library in module_name.libraries.yml

focal-point-fix:
  js:
    js/focal-point-fix.js: {}
  dependencies:
    - core/jquery

I then attached that library on the specific node form in module_name.module

/**
 * Implements hook_form_alter().
 *
 * Add Focal Point Fix JS to node edit form.
 */
function module_name_form_node_form_alter(&$form, &$form_state, $form_id) {
  $node = $form_state->getFormObject()->getEntity();

  if ($node->getType() == 'content_type_name') {
    $form['#attached']['library'][] = 'module_name/focal-point-fix';
  }
}
bleen’s picture

instead of adding this behavior to the horizontal-tab-button, can this be generalized somehow? Can it be attached to a change to the nearest visible parent of the img? or a change to the img itself somehow?

JonMcL’s picture

My attempt at a solution. Possibility controversial because it uses timeouts to retry every focal point image if the first attempt fails to find the image visible.

This approach does solve the issue in a generalized way -- no matter what is causing the image to be hidden (tabs, etc), the call to setIndicator will be retried every 250ms until the image becomes visible.

This approach is possibility problematic in a situation with a large number of Focal Point images -- the setIndicator function will be called over and over again for each image.

I'm thinking that the conditionals are quick enough on most browsers that the repeated calls to setIndicator might be quick enough to not be noticed at all?

I experimented with event handlers tied to mouseup and keyup, as suggested by Chi with their CodeMirror reference, but I found that these event handlers were being called before horizontal-tabs.js had a chance to make the image visible.

250ms might be too short of a duration, but any longer and the crosshair indicator is left at the 0,0 coordinates for noticeable period of time.

Rar9’s picture

I'm facing this problem in D9.03 Images that are in Details Collapsed + using File Field Path.

How to resolve as patch #25 is not resolving the issue and the cross is showing on the wrong position , creating a lot of confusion.

From what I see it saves, but as soon as you re-open the node, all image cross are again showing on the wrong position.
Saving the node without any modification luckily won't change the once defined Focal position.

JonMcL’s picture

@Rar9: I have to ask, did the patch apply cleanly and did you make sure to clear your browser cache before testing?

The patch should work for your situation.

Rar9’s picture

Thanks, was a long day yesterday - Cleared Browser Cache --- and all is as it should.

JonMcL’s picture

Status: Needs work » Needs review

No problem, it happens to all of us :)

Anyone else get a chance to test this yet? Any feedback on the method I took? (using setTimeout)

bleen’s picture

I've been googling this problem for years and the only solution that Ive ever seen has been some variation of setTimeout (or setInterval) to wait until the image dimensions were accessible. I always assumed that JS would magically be updated one day, but alas...

I'd love to have someone who is more of a JS expert then I am chime in on this (specifically the question you asked about a form with dozens (hundreds?) of focal point image fields present) but it seems like this is the "correct" solution.

bvoynick’s picture

I believe IntersectionObserver allows for detecting visibility changes without timeouts. However, it's also a newer thing not supported by IE. I don't know if this module is following Core's lead or if it might have a slightly tighter group of browser targets.

pcambra’s picture

I've reviewed this from the UI point of view and #25 seems to be working correctly

bleen’s picture

mogio_hh’s picture

#25 should make it into the main version.

Client wanted us to remove the whole focal point module as he had to reset the focal point on "each" node change.

Thanks god we found this issue...
It also works within initially closed paragraphs + field groups.

Is there any Drupal website these days that is not using Paragraphs and field groups?
Again :)
#25 should make it into the main version!

btw thanks @JonMcL