New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

insert_image_dimensions can break an image's aspect ratio #214

Closed
GoogleCodeExporter opened this Issue Apr 6, 2015 · 11 comments

Comments

Projects
None yet
1 participant
@GoogleCodeExporter

GoogleCodeExporter commented Apr 6, 2015

Currently images are sized only based on inline width/height attributes in the 
<img> tag.  It would be beneficial to some sites if the images could also be 
resized based on evaluating css.

One site that would benefit is http://www.nationstates.net.

Original issue reported on code.google.com by jmara...@google.com on 16 Feb 2011 at 4:49

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 6, 2015

Thanks for logging this. Regarding our specific usage case at nationstates.net, 
we actually use 'max-height' and 'max-width' in CSS to scale images (rather 
than 'height' and 'width'), in order to preserve aspect ratio. Just to make 
your life harder.

Original comment by maxba...@gmail.com on 16 Feb 2011 at 5:27

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 6, 2015

I noticed a resizing bug on this page http://www.barox.nl/barok-stoel-rosa.htm

I need a workaround CSS rule
.product-view .product-img-box .product-image img#image { width:300px; 
height:300px; }
Otherwise the aspect ratio of the product image is wrong (see attachement)

It works fine without CSS workaround and ModPageSpeed disabled:
http://www.barox.nl/barok-stoel-rosa.htm?ModPagespeed=off



Original comment by Piet...@gmail.com on 4 Sep 2011 at 2:56

Attachments:

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 6, 2015

Piet -

That's a surprising change, since mod_pagespeed didn't resize the image at all, 
and comparing the two image urls:

http://www.barox.nl/media/catalog/product/cache/5/image/85666d5f7199f550664e78f9
e34b3754/b/a/barox-frame_bruin-stof_donkerblauw-_knopen-nagels_goud.png

and the optimized one:

http://www.barox.nl/media/catalog/product/cache/5/image/85666d5f7199f550664e78f9
e34b3754/b/a/xbarox-frame_bruin-stof_donkerblauw-_knopen-nagels_goud.png.pagespe
ed.ic.UJbFU6BeVV.png

It's pretty clear that this is an interaction between insert_image_dimensions 
and your existing CSS rules.  I was able to work around the problem by 
disabling insert_image_dimensions, as you can see here:

http://www.barox.nl/barok-stoel-rosa.htm?ModPagespeedFilters=inline_images,recom
press_images,resize_images,combine_css,rewrite_css,rewrite_javascript

Your workaround using an additional CSS rule is probably better than just 
putting

ModPagespeedDisableFilters insert_image_dimensions

into your configuration.  But we'll definitely need to think hard about how to 
handle this, and this bug is definitely the appropriate place to do so.  We'd 
like to avoid applying the CSS rules to the HTML on the server side, as this is 
very time-consuming and likely to be extremely buggy (and to expose differences 
between browsers).

Original comment by jmaes...@google.com on 4 Sep 2011 at 8:41

  • Changed state: Accepted
@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 6, 2015

Note that there is a partial fix to this bug in trunk and in the next release 
of mod_pagespeed: we will use image dimension information from an inline style 
tag to guide resizing, and that will override dimension insertion.  We're doing 
a bit more testing on the merits of insert_image_dimensions, and it's possible 
we'll turn it off by default in future if it isn't pulling its weight.

Original comment by jmaes...@google.com on 7 Nov 2011 at 9:51

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 6, 2015

The insert_image_dimensions filter has, in fact, been disabled by default due 
to this bug (and a couple of similar ones where inserted dimensions in the html 
conflicted with a single dimension specified in the css causing distortion).

I've left this issue open as it's still a problem when insert_image_dimensions 
is enabled.

Original comment by jmaes...@google.com on 24 Jan 2012 at 10:54

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 6, 2015

Original comment by matterb...@google.com on 25 Jan 2012 at 6:57

  • Added labels: release-note
@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 6, 2015

Also note that as of 0.10.21.2 the limitations of this filter are well 
documented in the mod_pagespeed documentation.

Original comment by matterb...@google.com on 7 Feb 2012 at 1:37

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 6, 2015

The original issue has been recreated as Issue 376 since this one has mutated 
into a related but different problem and solution.

Marking the original problem as fixed since the new default behavior will not 
exhibit the problem, and the solution for any sites with the problem is to 
disable the insert_image_dimensions filter.

Original comment by matterb...@google.com on 7 Feb 2012 at 3:56

  • Changed state: Fixed
@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 6, 2015

Change summary to better reflect the problem this has mutated into.

Original comment by matterb...@google.com on 7 Feb 2012 at 3:58

  • Changed title: insert_image_dimensions can break an image's aspect ratio
@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 6, 2015

Original comment by jmara...@google.com on 23 May 2012 at 2:36

  • Added labels: Milestone-21
@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 6, 2015

Original comment by jmara...@google.com on 23 May 2012 at 2:47

  • Added labels: Milestone-v21
  • Removed labels: Milestone-21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment