Support from Acquia helps fund testing for Drupal Acquia logo

Comments

LinL’s picture

I'm experiencing the same problem. When the page is first displayed the images are about 20px high and less than 200px wide, whereas the actual dimensions are 300px wide and 225px high. If I refresh the page the images and captions are displayed correctly.

Using the following:

Drupal 7.4
WYSIWYG 7.x-2.1 with CKEditor 3.6.1.7072
Insert 7.x-1.1

LinL’s picture

Version: 7.x-1.0-beta3 » 7.x-1.x-dev

As a quick fix, temporary solution I've commented out the references to imgheight and imgwidth in image_caption.js and image_caption.min.js.

Lines 36-43 of image_caption.js:

       // Add dimensions, if available
      if(imgwidth){
 //     $(this).width(imgwidth);
 //     $(this).parent().width(imgwidth);
      }
      if(imgheight){
 //     $(this).height(imgheight);
      } 

and the end of line 2 and line 3 of image_caption.min.js:

//if(imgwidth){$(this).width(imgwidth);$(this).parent().width(imgwidth);}
//if(imgheight){$(this).height(imgheight);} 

This works in my particular set up where images are wider than captions, but if captions are wider than the images, it will cause another problem as the caption will no longer wrap properly to the space under the image.

I'll try and come up with a better solution when I've more time!

Anonymous’s picture

subscribe

k4v’s picture

I have exactly the same problem

subscribe

fultonchain’s picture

Same here.

chrisfromredfin’s picture

the jQuery .width() and .height() will return the content's width, regardless of what is specified in attributes or css.

the image_caption.js responds to $(document).ready (because it is attached to Drupal.behaviors), which means it fires when the DOM is ready, NOT when the images are fully loaded.

My theory is that the image is partially downloaded (partial width and/or height) when .width() and .height() are called, resulting in incorrect values and a squished or cut-off looking image. When you refresh, your images have already been downloaded and cached by your browser, so when width/height are called, they know the full dimensions.

To remedy, I think we need to either (a) react on .load (instead of .ready) or (b) use attr('width') and attr('height') to get them from there, or potentially get them from css values.

If the maintainer has any input, I will start in that direction... otherwise, I'll just see what I can do in any regard.

Note that this is difficult to replicate if you're developing on a localhost, because the images load so fast (I think this is really a timing issue.)

==EDIT== Just found this issue -- http://drupal.org/node/675002 -- which seems to imply this is remedied, but then goes on to imply it is not. The latest 7x code does not use load, but still ready.

The following seems to work for me (image_caption.js, line 5 - make sure you also change image_caption.min.js)...

    $("img.caption:not(.caption-processed)").each(function(i) {

BECOMES

    $("img.caption:not(.caption-processed)").load(function() {
LinL’s picture

The solution in #6 works well for me. And it is better than my quick fix in #2 as the caption wraps properly too.

I've attached a patch.

LinL’s picture

Status: Active » Needs review
chrisfromredfin’s picture

LinL - something I discovered soon after is that if someone using IMCE chooses the mouseover and mouseout events, the .load() is fired each time they mouse over and out, resulting in multiple captioning/wrapping.

Later I had to add:

      // Add class to prevent duplicate caption adding
      $(this).addClass('caption-processed');
+      // Also since this is the load event, unbind this handling for future loads (i.e. onmouseover, onmouseout).
+      $(this).unbind(e);
    });

You may want to adjust your patch?

Best,
.cw.

jkwilson’s picture

Unfortunately the solution in #6 seems to prevent captioning from appearing at all in Internet Explorer. Can anyone confirm they made that fix and captions still worked in IE 8?

chrisfromredfin’s picture

This is the page that caused my woes, including the additional patch in #9.

http://macatholic.org/children

It seems to work for me in IE (althought it admittedly is probably IE9).

jkwilson’s picture

I agree this is definitely a timing issue related to the speed at which images are loading. I can regularly reproduce it on pages with large images.

I found this stackoverflow thread pointing to the imagesloaded jQuery plugin. This helps with the known issues with .load(). That seems like the way to go. It's working well for me in preliminary testing across browsers.

jweowu’s picture

jkwilson: That certainly sounds promising. Are you able to provide a patch?

(is it essentially the same patch as #7, but replacing the call to each with imagesLoaded instead of load ?)

jweowu’s picture

Okay, here's a patch to integrate the imagesloaded library via the Libraries API, and then use imagesLoaded() in place of each()

You need to install the library yourself. See INSTALL.txt after patching.

TimG1’s picture

FileSize
327 bytes

Hello!

The patch in #14 worked for me. Although it looks like the patch failed on entering the dependencies. Attached error log.

Many thanks,
-Tim

jweowu’s picture

You're welcome. Note that the patch applies cleanly to the 7.x-1.x git HEAD. I'm guessing that you patched an official release, as they acquire additional meta data in the .info file, which can then cause conflicts when a patch modifies that file.

TimG1’s picture

Hi jweowu,

You're correct, I applied the patch to the -dev version.

Also, I found a bug with the patch. While it fixes the problem, it also seems to make all the images on the page that are using the image caption module the same size in width. So, if I have a smaller image with a caption on the same page as a larger image with a caption, the smaller image will get stretched to the width of the larger image.

Thanks again,
-Tim

jweowu’s picture

I've not tested extensively, but I'd be a bit surprised if this patch is responsible for that. Most of the patch is just integration code for the new library. The only functional change in the patch is:

diff --git a/image_caption.js b/image_caption.js
index 82199ea..e9f2ab3 100644
--- a/image_caption.js
+++ b/image_caption.js
@@ -2,7 +2,7 @@
   
 Drupal.behaviors.image_caption = {
   attach: function (context, settings) {
-    $("img.caption:not(.caption-processed)").each(function(i) {
+    $("img.caption:not(.caption-processed)").imagesLoaded(function(i) {
       var imgwidth = $(this).width() ? $(this).width() : false;
       var imgheight = $(this).height() ? $(this).height() : false;

(plus the equivalent in the minified script)

Can anyone else confirm the problem?

dariogcode’s picture

Issue summary: View changes
FileSize
1.9 KB
1.14 KB

Thanks for the patch, I had to change the code, but it help a lot. Following are my comments:

1 - Imagesloaded require jQuery > 1.5, so it should be in dependencies too

2 - Code

+    $("img.caption:not(.caption-processed)").imagesLoaded(function(i) {

Doesn't work for me, I got an error "Error: Cannot read property 'defaultView' of undefined". It is because $(this) point to imagesload instance instead of image object. width() and height() cause this error. Then I changed this to iterate over imageloaded elements:

$("img.caption:not(.caption-processed)").imagesLoaded(function(instance) {
  $.each(instance.elements, function(i, element) {
  var imgwidth = $(this).width() ? $(this).width() : false;
  .......

Attached is the JS that work for me

jweowu’s picture

Note that imagesloaded v2.1.0 was the current release when that patch was rolled. Chances are it'll still work fine with that version of the library.

https://github.com/desandro/imagesloaded/releases