Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#25 | 2749477-wrong_positioned_cross-25.patch | 1.52 KB | JonMcL |
#4 | wrong_positioned_cross-2749477-4.patch | 2.35 KB | hctom |
| |||
screenshot.png | 114.03 KB | hctom |
Comments
Comment #2
bleen CreditAttribution: bleen at NBCUniversal commentedHrmmm ... 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)
Comment #3
hctomI 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.
Comment #4
hctomAfter some research, I found several posts saying that
$.height()
and$.width()
only work for visible elements. See http://api.jquery.com/height for details: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.
Comment #5
bleen CreditAttribution: bleen at NBCUniversal commentedI 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?
Comment #6
hctomYou 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 ;)
Comment #7
hctomOkay... this really seems to be a hard one ;)
width
orheight
JavaScript properties do not take inline styles for width/height into account,clientWidth
/clientHeight
ignore them, toowidth
orheight
HTML attributes do not take inline styles for width/height into account and might not even be present in the markupnaturalWidth
ornaturalHeight
return the size of the original loaded image while ignoring width/height HTML attributes, CSS styles and inline stylesMutationObserver
to watch visibility changes is not supported by all browsersUnfortunately I am running out of ideas... anybody else has one?
Comment #8
shadcn CreditAttribution: shadcn as a volunteer and at Chapter Three commented@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.
Comment #9
hctom@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.
Comment #10
shadcn CreditAttribution: shadcn as a volunteer and at Chapter Three commentedThanks @hctom. I'll try this.
Comment #11
shadcn CreditAttribution: shadcn as a volunteer and at Chapter Three commented@hctom yes I can see the bug now. Looking into it.
Comment #12
shadcn CreditAttribution: shadcn as a volunteer and at Chapter Three commented@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?Comment #13
hctom@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.Comment #14
bleen CreditAttribution: bleen at NBCUniversal commented@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:
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...
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?
Comment #15
DarkstarTom CreditAttribution: DarkstarTom commentedI'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?
Comment #16
DarkstarTom CreditAttribution: DarkstarTom commentedEven 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.
Comment #17
redbrickone CreditAttribution: redbrickone at Code Koalas commentedAlso having this issue, leaving this here for updates.
Comment #18
bleen CreditAttribution: bleen at NBCUniversal commentedcould we consider this: https://www.w3schools.com/jsref/prop_img_complete.asp ?
... or
http://dreamerslab.com/blog/en/get-hidden-elements-width-and-height-with...
Comment #19
esod CreditAttribution: esod at Memorial Sloan Kettering Cancer Center commentedWe'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.
Comment #20
Chi CreditAttribution: Chi commentedHere is how CodeMirror solves very similar problem with textarea rendering in a hidden region.
https://codemirror.net/addon/display/autorefresh.js
Comment #21
ayalon CreditAttribution: ayalon commentedI 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.
Comment #22
EgbertB CreditAttribution: EgbertB commentedUsing seven maintenance theme, I solved it using this (added this to the nav tab.js)
Comment #23
thejimbirch CreditAttribution: thejimbirch at Kanopi Studios commentedThanks @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
I then created the library in
module_name.libraries.yml
I then attached that library on the specific node form in
module_name.module
Comment #24
bleen CreditAttribution: bleen at NBCUniversal commentedinstead 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?
Comment #25
JonMcL CreditAttribution: JonMcL commentedMy 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
andkeyup
, 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.
Comment #26
Rar9 CreditAttribution: Rar9 commentedI'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.
Comment #27
JonMcL CreditAttribution: JonMcL commented@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.
Comment #28
Rar9 CreditAttribution: Rar9 commentedThanks, was a long day yesterday - Cleared Browser Cache --- and all is as it should.
Comment #29
JonMcL CreditAttribution: JonMcL commentedNo 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)
Comment #30
bleen CreditAttribution: bleen at NBCUniversal commentedI'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.
Comment #31
bvoynickI 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.
Comment #32
pcambraI've reviewed this from the UI point of view and #25 seems to be working correctly
Comment #33
bleen CreditAttribution: bleen at NBCUniversal commentedRelated but no time to look right now: https://git.drupalcode.org/project/color_thief/-/blob/afc33a6141685f32a9...
Comment #34
mogio_hh CreditAttribution: mogio_hh commented#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