Always adds thumbnail only (unspecified image preset width or height)

sampeckham - August 11, 2008 - 16:47
Project:Image Assist
Version:6.x-2.x-dev
Component:Code
Category:bug report
Priority:normal
Assigned:TwoD
Status:needs work
Description

I've been using this dev version and the beta version and in both cases it keeps reverting to the thumbnail version of the image, no matter what size I select in the drop down list. I had the beta version working once, then it stopped for some reason. I've tried with and without TinyMCE and have been mostly using the 'HTML code' insert option.

The only way i can show any other version is by manually editing the HTML img tag produced.

Anyone else have this problem, or know of a possible cause (other than being in the dev stage!)?

#1

sun - August 11, 2008 - 18:37
Status:active» postponed (maintainer needs more info)

As for the HTML tag feature: #284447: Remove HTML output feature - it's about to be dropped, because it causes plenty of bugs.

Could you please verify whether this bug happens if you're using inline tags?

#2

sampeckham - August 12, 2008 - 08:53

OK, I've played around a bit more and this still happens when I use the Image Filter tags. I'm going back to the beta version (which seemed a bit more stable, but has the same problem) and will report any further details about this. I'm using D6.3 btw.

Also, an aside the dev version doesn't seem to work with tinyMCE, I've set it up just as the beta version and I see the camera icon, but clicking it does nothing - maybe this is expected in dev?

#3

sampeckham - August 12, 2008 - 11:32

I wondering if this is related to the Image module and dimensions. Although all size images are generated correctly, when I use just one dimension value for scaling, Image Assist reverts to the thumbnail version. If I select an image size with both values it embeds the correct size, but adds the wrong dimension values to the code.

EG.
I have Thumbnail 100x100 and Preview 300x300, and also 'full' 530x*** and 'half' 260x***

If I select 'full' or 'half' the thumbnail is used.

If I change 'full' values in Image module to 530x250 (scale and crop and regenerate image derivatives), Image Assist embeds the 'full' version (with correct file name) BUT creates dimensions of ***x250, resizing the image proportional to it's height. Not what you want to happen, as Image module has already scaled and cropped.

What I want is to upload and define two widths for images and use either full or half width image in the node.

So Image module is working and doing exactly what t should, but Image_assist is not quite reading the file dimensions somehow. Is it designed to "fall back" on the thumbnail view if it can't work out dimensions? Maybe that's what's happening.

Hope this helps understand the problem, been testing it all morning, including uninstalling, deleting all image nodes and files and downloading a fresh copy of the Beta version.

#4

sun - August 17, 2008 - 12:59
Title:Always adds Thumbnail only to body» Always adds thumbnail only
Status:postponed (maintainer needs more info)» active

We had a similar issue in the past, but that was related to Image 5.x-1.x. It is possible that something changed since 6.x-1.x (which seems functionally to be the same as 5.x-2.x).

Needs further analysis.

#5

zoo33 - August 19, 2008 - 23:34

I think the issue is still there: you have to specify both width and height in Image's settings för IA to work correctly. A workaround is possibly to use a size like 100x999 px.

#6

sun - September 13, 2008 - 13:02
Priority:normal» critical

Bumping to critical. This issue needs to be fixed for 2.0 to be released.

#7

alienbrain - October 26, 2008 - 11:40

Here is a patch, my first img_assist patch. Watch out! :)

AttachmentSize
img_assist_support_missing_dimension.patch 1.23 KB

#8

sun - October 26, 2008 - 12:24
Status:active» needs review

From a first, quick peek, this looks pretty good and reasonable. 4 confirmations from other users are required though:

2x This patch does not break images on existing sites using 6.x-1.x or 6.x-2.x (until now, nothing changed in this logic since 1.x).

2x This patch works, i.e. allows to leave out either width OR height in Image's settings for a preset and the resulting images have the correct/expected size.

#9

zoo33 - October 26, 2008 - 13:07

I don't think this code handles all possible cases for whether $width and $height are set. We need to end up with both $width and $height set, in order for $diag_size_new to get a proper value on the next couple of lines.

There are four possible cases:

both $height and $width set:
1) $height larger than $width
2) $width larger than $height

only one is set:
3) only $height set
4) only $width set

I think only 3) and 4) is being properly checked. 1) is covered, but there is no check that both values are set before comparing them.

#10

alienbrain - October 26, 2008 - 14:26

Yay quick reviews. I see your zoo33 point regarding 2). Here is a new patch.

But for 1), it doesn't create a problem anyway, because if $width is 0 then it would be less than $height anyway.

Cheers,

AttachmentSize
img_assist_support_missing_dimension2.patch 1.46 KB

#11

zoo33 - October 26, 2008 - 16:41

My concern is this:

$diag_size_new = sqrt(pow($width, 2) + pow($height, 2));

We need both $width and $height to be correct in order to determine the diagonal size.

#12

alienbrain - October 29, 2008 - 14:27

Both $width and $height are fine. Because the whole block of code will only execute if either of the dimensions is defined (if ($width || $height)). Then another condition (if (!$height || ...)) checks and sets the remaining dimension.

I left the code running on my personal blog and so far it's working. I would happily work on a better patch if I'm able to reproduce.

Cheers! :)

#13

zoo33 - October 29, 2008 - 18:26

Hm, guess you're right.

#14

zoo33 - January 15, 2009 - 01:34

We need someone to test and confirm this.

#15

sun - February 2, 2009 - 01:58

#163043: Unspecified image preset width/height: Wrong image size calculated, missing images, etc. has been marked as duplicate of this issue due to some further debugging by TwoD.

Additionally: In img_assist_properties_form() we are populating the select field for the property window, but if an image preset size does not specify a width or height, the corresponding values are empty, which completely breaks the aspect ratio calculations performed elsewhere.

With "elsewhere" I mean the calculations that happen mainly in JS. I would like to remove that JS calculation code altogether in favor of properly prepared values on the server-side.

#16

sun - February 2, 2009 - 02:30
Title:Always adds thumbnail only» Always adds thumbnail only (unspecified image preset width or height)

This should be all that's required for the properties form. May or may not need to be merged with alienbrain's patch.

AttachmentSize
img_assist-DRUPAL-6--2.properties-empty-sizes.patch 1.47 KB

#17

TwoD - February 3, 2009 - 01:13

Patches in #10 and #16 confirmed. Both individually and together.

I think the JavaScript (img_assist_popup.js, onChangeSizeLabel()) should also be altered too at the same time. It should no longer [re-]caluclate the width/height values and instead just pass them on since they are now ensured to be correct from the server. But I don't see this as very important since the values should come out correct in the end anyway.

#18

sun - February 3, 2009 - 14:31
Status:needs review» fixed

Committed both patches to ALL branches - literally.

This was the first commit since months to 1.x... ;)

#19

System Message - February 17, 2009 - 14:40
Status:fixed» closed

Automatically closed -- issue fixed for 2 weeks with no activity.

#20

jofje - April 25, 2009 - 19:52
Status:closed» active

I've run into the same problem described by the original poster (and better described by poster #5 here: #163043: Unspecified image preset width/height: Wrong image size calculated, missing images, etc.):

Inserting image using IA, and ending up with just thumbnail in resulting page, even if I enter custom width and heigth. Going back into editing mode, I just see a small red dot instead of image. Disabling rich text and manually entering w x h works.

I have all the latest dev snapshots installed:
Image 6.x-1.x-dev (2009-apr-18)
Image Assist 6.x-3.x-dev (2009-mar-29)
Inline 6.x-2.x-dev (2009-feb-13)
Wysiwyg API 6.x-2.x-dev (2009-apr-21)

Note: For me this happens only in IE (version 6, 7 or 8) - when using Firefox everything works without any problem! I've not seen any mentioning of this beeing a browser dependant error, neither in this thread nor in any of the different duplicates.

#21

skierboy - May 20, 2009 - 21:41

this is still an issue using 2.0 alpha 3.

#22

TwoD - May 26, 2009 - 22:19
Assigned to:Anonymous» TwoD

I've also seen this problem appear on my sites again. Same symptoms as in #20 but in Firefox too.
I think I could avoid it by selecting that small red dot (which is just the image resized btw), opening the Image Assist window, selecting image size and hitting update again.

I'll take a look at it tomorrow again, too late here now, assigning this to me in the meanwhile.

Edit: I've found the problem. So far, this seems to be specific to IE and possibly combined with TinyMCE. Will try with FCKeditor and others later.
When Wysiwyg API initiates TinyMCE, it sets up a listener on the onGetContent event to know when to detach plugins. In Image Assist's case this means converting the placeholder img tag back into a filtertag so the outside world will not see the placeholder. That's usually exactly what we want, but TinyMCE itself makes a call to getContent to store the initial contents of the editor, which of course triggers that event.

Due to a timing issue, IE has not yet completely parsed the placeholder img tag into the DOM (or that's what I'm guessing so far). So the contents returned to TinyMCE will get a filtertag with width and height set to 0, because that's what the Image Assist plugin got back when looking at this.width and this.height. this being the img tag which was just recently created.

I've tried various ways to access the width/height but no luck so far, except with using /width=(\d+)/i.test(this.outerHTML);alert(RegExp.$1) (where this is the img tag) which does indeed work but feels a bit awkward. Sadly, I see no easy way around implementing IE-specific behaviour into the Image Assist plugin by relying on the .outerHTML property and manual parsing like that...

#23

TwoD - May 27, 2009 - 15:08
Status:active» needs review

I createded a workaround patch for this. It's not as pretty as the original solution but works.
If this.outerHTML exists, use a simple regular expression to parse out width and then height and build the second part of the filtertag using the matched values.

I tried setting this.width and this.height but it wouldn't accept it. They're probably read-only at that time...

AttachmentSize
img_assist.HEAD_.#293909.empty-sizes.patch 1.93 KB

#24

jofje - May 27, 2009 - 21:15

Sorry, but have problem downloading patch - clicking on attachment link above only redirects me to Drupal home page.

#25

TwoD - May 27, 2009 - 21:56

I'll try uploading it again. I got a notice before saying that my file was renamed due to security reasons. Maybe there's a bug which makes it forget to change the url as well?
I got it this time as well, had removed the # so I guess that wasn't it. Removed .HEAD. as well (noticed it added an extra _ after it) and now it seems to work.

AttachmentSize
img_assist.293909.empty_sizes.patch 1.93 KB

#26

jofje - May 28, 2009 - 00:13
Status:needs review» needs work

Good news is that after patching, IE8 does not mess up the image size when opening a page for editing, and I'm even able to insert a new image using IA, ending up with correct size.

Bad news is that if I try to edit the image size of an existing image using the IA pop-up, when hitting "Update" the image disappears! Well, in fact it doesn't disappear completely - it jumps to the top left corner of the page - but if I try to save the page, the image is totally gone. Going back into editing mode, the image is gone, both in wysiwyg view and in rich text disabled.

As usual there is no problems using Firefox.

#27

TwoD - May 28, 2009 - 17:16

The bug jofje mentions in #26 has been driving me mad for a couple of hours now...
I've at least been able to determine that it also happens without my patch from #25, and I've found a way to almost always reproduce it in IE (6).

  1. Make sure the HTML/Source view button is enabled in TinyMCE. Go to the edit tab on a node containing an Image Assist inserted image.
  2. Click the image once to select it.
  3. Click the HTML view button. Doing something which doesn't deselect the image but potentially moves the caret position might also work.
  4. Close the popup window, I used the titlebar X, but that's probably irrelevant.
  5. Without clicking anything else, so the image is still selected, click the Image Assist button.
  6. Change the size and click update.
  7. The image should now have been inserted right below the <body> tag of the main page instead of where it's supposed to. (Which is also why it disappears if you save the node, it's no longer in the wysiwyg area.)

This makes me think it might be a TinyMCE problem. Maybe the caret position is completely off in IE if the last thing clicked before executing the mceInsertContent command (which is what Image Assist does) was not actually the image we intend to replace with an updated version.

A comment in the TinyMCE setContent method (called by the content insertion command) states:

// Delete the marker, and hopefully the caret gets placed in the right location

// Removed this since it seems to remove &nbsp; in FF and simply deleting it

// doesn't seem to affect the caret position in any browser

I tried to verify this by getting what was selected from TinyMCE but that looked ok (the img tag was returned), so I guess selection and caret position are out of sync.

In any case, I believe this is a separate issue and not related to the patch I posted. The patched detach() method also does not run when updating the image, at which point the above problem occurs.

Btw, I also noticed that IE never seems to be able to get the width and height from the placeholder img tag, regardless of when the detachment happens.

#28

mooreds - June 26, 2009 - 12:55

subscribe

#29

TwoD - June 26, 2009 - 15:54

I almost forgot about this issue... Could somone confirm that the issue in #26/#27 is unrelated? I don't really like this hack (parsing out the value from .outerHTML) but it's the only thing I found which actually worked. I can't find much to add to it exceept for maybe changing the comment so it reads "#293909: IE has not specified width and height properties.".

#30

portulaca - July 4, 2009 - 15:36

I also had the same problem, I was using Filter option and width and height weren't inserted correctly. I upgraded to the dev version 6.x-3.x-dev and the problem is no longer there :)

#31

sun - July 20, 2009 - 17:39
Status:needs work» needs review

The properties form probably still needs to be fixed.

However, we missed to update the rest of the img_assist_render_image() function in those previous patches. The diagonal image size dimension calculation fails when a default image size has no width or height or misses both (like for IMAGE_ORIGINAL). Also, the previous patches did not account the possibility that the width may be not be specified in the inline macro (we only fixed height). So, when for any reason, width or height or both are 0, '', or undefined in the inline macro, then also the diagonal image size calculation failed, resulting in IMAGE_ORIGINAL being used by default.

Attached patch should fix this. Took me some hours to wrap my head around that code.

AttachmentSize
img_assist.missing-width-height.patch 5.55 KB

#32

sun - July 20, 2009 - 17:41

Oopsie, with debug code commented out... (I want to leave this in there to quickly be able to debug)

AttachmentSize
img_assist.missing-width-height.32.patch 5.56 KB

#33

sun - July 20, 2009 - 18:26

Fixed a comment.

I've deployed this to a large-scale production site and it seems to work flawlessly. Any objections?

AttachmentSize
img_assist.missing-width-height.33.patch 5.56 KB

#34

zoo33 - July 23, 2009 - 08:57
Status:needs review» reviewed & tested by the community

The last patch looks good to me, and your explanation sounds reasonable. It sounds like you have tested it well enough, so RTBC i guess?

#35

sun - July 23, 2009 - 10:02
Priority:critical» normal
Status:reviewed & tested by the community» needs work

Thanks for reviewing! Committed to 2.x and 3.x branches.

Now back to #26/27... fixing property window data processing in IE. (Yuck.)

#36

that0n3guy - July 28, 2009 - 15:43

subscribing...

#37

hokuspokus - August 13, 2009 - 21:54

subscribin'

#38

hoyer - September 4, 2009 - 08:38

Sorry, going back to #10:

I think this only works for images in portrait format (where height is larger than width). This is because, to get to know which side is larger, you just need to compare "$width <= $height", not "round($width / $aspect_ratio) <= $height". The latter leads to "true" in many cases even if width is larger than height.

Following changes work for me (sorry, don't know how to create a patch and don't got time for it, just want to let you know):

   1134           // Get new width and height for this inline image.
   1135           // If height is either left out or larger than width then
   1136           // Width is controlling factor.
   1137           if (!$height || ($width <= $height)) {
   1138             $height = round($width / $aspect_ratio);
   1139           }
   1140           // Else, width is either left out or larger than height so
   1141           // Height is controlling factor.
   1142           else {
   1143             $width = round($height * $aspect_ratio);
   1144           }


   1171             // Calculate default width and height based on aspect ratio.
   1172             // Width is controlling factor.
   1173             if (!$height_std || ($width_std <= $height_std)) {
   1174               $height_std = round($width_std / $aspect_ratio);
   1175             }
   1176             // Height is controlling factor.
   1177             else {
   1178               $width_std = round($height_std * $aspect_ratio);
   1179             }

#39

iko - October 5, 2009 - 09:57

subscribing

#40

MadtownLems - October 5, 2009 - 14:07

Subscribing - #27 is frustrating me like no other....

#41

Troodon - October 11, 2009 - 13:31

Thank you for the quick fix hoyer, it worked for me too.

EDIT: Perhaps not, with just this quick fix applied Portrait images default to thumbnail size rather than the specified one.

#42

MadtownLems - October 7, 2009 - 20:59

I applied the patch from 33, and it has not fixed my problem. Probably because it wasn't intended do, but alas.

The explanation in #27is exactly what's happening to me. If I edit an inline image with img_assist from my tinymce wysiwyg, and push update, the image will appear on my page, but WAY outside of my textarea. It will appear in the body of my actual template, way out on the sides.

Has anyone found a solution to this? It's very troubling. thanks in advance.

As always, FF is fine...

#43

saml - October 11, 2009 - 00:56

Subscribe.

#44

rondev - October 13, 2009 - 17:56

#38 solved my problem

#45

Ether - October 26, 2009 - 15:39

subscribe

#46

wuwei23 - October 29, 2009 - 03:07

Subscribing, I think I'm seeing the same behaviour.

Sometimes when I upload an image, it doesn't respect the size chosen and inserts a link to the original. The behaviour isn't consistent, and two images from the same source can produce entirely different behaviour, with one resizing correctly and the other not. The only solution I have atm is to resize the images before uploading.

 
 

Drupal is a registered trademark of Dries Buytaert.