We're really close to deploying image support on d.o. so people don't abuse this and upload 12000px wide images we should add a max-width property to the CSS.

CommentFileSizeAuthor
#15 bluecheese-imagemaxwidth-1289434-15.patch304 bytesLewisNyman
#5 not-so-large-file.png1.67 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Active » Closed (won't fix)

I'd rather restrict the size of images themselves. Even if we do let CSS handle the scaling, we are still abusing the bandwidth and browser's time for scaling it. If image uploads are through imagefield, or similar, that should be configured with a max size.

Or, set up imagecache to do the scaling.

Or, just treat overly-large images like any other content abuse in the webmasters queue. I'd rather have huge images be obvious rather than quietly scaling them client-side.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Closed (won't fix) » Active

Moving back to active just to discuss briefly.

The image uploads in Project* are done through upload.module (well, comment_upload module, which piggybacks off it). So no options for setting a max size there. And I would really rather not tie an issue as important as #528682: Allow inline images to be posted to Drupal.org project pages, docs pages, and comments without any special permissions to a mass "comment_upload to filefield.module migration + some kind of fancy image solution for the D6 project module" initiative, especially when we know d.o on D6's days are numbered, and hopefully Project*'s days are as well.

Setting a max-width on images (via height and width attributes on the img tag) was originally handled by the Local image input filter module, but that functionality was removed by sun at #1281194-6: Roll a 1.0 release of this module in favour of this issue, since filters are not per-theme specific. If this issue gets won't fixed, we're at a stand-still on that, and have to deal with people being able to post images that look like this:

REALLY BIG image of a Drupal ninja

If that is not a blocker to deploying this functionality, then that's fine with me. There are enough of us with edit comment permissions that we could fix this with editing the image and adding height/width attributes, though it's super convenient to not even have to worry about it and just be able to put in an img tag (as well as try and remember the magic 940px number). If fixing this layout breakage is a blocker to deploying this functionality, then I'm going to get very cranky if we come to a stand-still because of a drumm vs. sun fight. ;)

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented

I wouldn't consider this a blocker, if large images happen, and this is indeed a good fix, then we deploy it. Issue pages are particularly okay with big things due to code listings, I doubt we will want to put much to the right.

To be clear- I'd like to see image any resizing happening server-side, not via CSS or HTML. This will save a little bandwidth. If it helps, drupalorg already has a feature with imagecache presets for all the grid widths- grid-8 for the standard page width with a sidebar or grid-12 for the full width.

(Wrote this up really quickly, I haven't read through the other issues lately.)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Active » Closed (won't fix)

Ok cool. I'll move back to won't fix, and post back in the other issue that you don't see this as a blocker, and we can come back to it if needed. Thanks, Neil!

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
not-so-large-file.png1.67 KB

Somehow, this issue was quickly turned into a discussion about file sizes.

But file sizes are completely irrelevant to image dimensions.

Even if you'd restrict images to be smaller than 10 KB, they may break your site's layout:

not-so-large-file.png

A theme needs to decide whether there's sufficient room for an image to display. Unless you write tons of code for determining context to predict the generally available width and height in a certain output area/region that depends on the output of other regions on the page (also accounting for the visitor's user agent, viewport, etc), this cannot be sufficiently nor reliably solved on the server-side.

The only reliable solution is to make the theme leverage the CSS grid that has been mentioned already:

.grid-12 .content img {
  max-width: 800px;
}
.grid-8 .content img {
  max-width: 500px;
}
.grid-4 .content img {
  max-width: 250px;
}

(pseudo-values)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

Well, that's not quite accurate. Another way to handle it would be for local_image_filter_whatever module to output the filtered image in a theme function which bluecheese could override and run through theme('imagecache') of the proper dimensions.

tvn’s picture

tvn CreditAttribution: tvn commented
Status: Closed (won't fix) » Active

Re-opening because I really think problem with too wide images should be addressed - does not matter will it be via css or in any other way. Browsing d.o I see horizontal scrollbar way too often, on issue pages (like http://drupal.org/node/1289072#comment-5423512) and even remember seeing image on doc page breaking right sidebar (unfortunately can't find the link now to illustrate).

tvn’s picture

tvn CreditAttribution: tvn commented

Closed #1502920: Potential image overflow as duplicate of this one.

kingandy’s picture

kingandy CreditAttribution: kingandy commented

Yep, image overflow is happening - see http://drupal.org/node/1054474/revisions/1415202/view (#1500010: Image overflow on sidebar).

Is it relevant that this page was created in Feb 2011? I don't remember when Bluecheese went live, I'm wondering if there could be an issue with a lot of content that was created under the old theme.

FWIW, I support the CSS max-width option.

Senpai’s picture

Senpai CreditAttribution: Senpai commented
Title: restrict images to max width to not break layout » Restrict user-uploaded images to a maximum width
Issue tags: +drupal.org D7, +bluecheese

Tagging for the Bluecheese team's eyes.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented

This is a problem that has come up pretty often on sites with fluid grids.

Adding this property to the CSS ensures an image never scales larger than its parent.

img {
  max-width: 100%;
}

This fix will be included as part of the d.o Drupal 7 port but I can write a patch if we need it in asap.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented

This doesn't have anything to do with restricting file sizes. We do want to keep files from being unnecessarily large. Usually, an image's file size will go down when it is scaled down. We shouldn't be sending huge images to clients when they will only render 960px, or even less on smaller devices.

However, now that we have higher-resolution displays, like the new iPad, the actual dimensions of the file are becoming less important. I think this CSS is okay to add. Hopefully it doesn't hide too many images with larger-than-necessary dimensions (and file size).

tvn’s picture

tvn CreditAttribution: tvn commented

yay! thanks drumm! Lewis if you could write a patch that'd be great.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Assigned: Unassigned » LewisNyman
LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Status: Active » Needs review
FileSize
bluecheese-imagemaxwidth-1289434-15.patch304 bytes

Patch

rupl’s picture

rupl
he/him
Primary language English
CreditAttribution: rupl commented
Status: Needs review » Reviewed & tested by the community

This patch works and I approve!

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Reviewed & tested by the community » Fixed

Committed to BZR. I'll deploy this on Drupal.org along with #371999: Configure multiple images for project nodes. The subsites will follow as they get blucheese deployments.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Fixed » Needs work

This CSS is distorting images, like the screenshot at http://drupal.org/node/1572164. We should always scale proportionally.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

That's because the IMG markup contains a hardcoded "height" attribute.

Removing that attribute resolves the distortion. Not sure whether this needs to be (and can be) fixed.

kingandy’s picture

kingandy CreditAttribution: kingandy commented

That image is being displayed by the 'Primary Screenshot' field on the Case Study content type, which currently allows up to 1800x1800px images.

I think this is more a problem with the Case Study content type than with the BlueCheese theme. The theme is correctly stopping the image from exceeding the available space. (The main page area is currently set to 960px, and the main content area is only 620.)

I'd say that field should be resizing the image to match the available space, either at point of upload (which I believe Imagefield can handle?) or using Imagecache.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented

I did mention the case study problem at #1561466: Update Case Study view to separate 'featured' from 'normal' case studies. However, we still don't want CSS improperly scaling images.

Snugug’s picture

Snugug CreditAttribution: Snugug commented

The scaling issue is very easy to fix:

img {
  max-width: 100%;
  height: auto;
}
kingandy’s picture

kingandy CreditAttribution: kingandy commented

Editing my local CSS using Chrome developer tools confirms the "height:auto" fix should work (tested on http://drupal.org/node/1572164).

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Needs work » Fixed

Committed the additional CSS and deploying today.

kingandy’s picture

kingandy CreditAttribution: kingandy commented

Ha, I went to http://drupal.org/node/1572164 again to check but it looks like the Case Studies gang got around to implementing imagecache resizing since yesterday.

Testing with Sun's image from above (with extra added height attribute):

kingandy’s picture

kingandy CreditAttribution: kingandy commented

Checked in Firefox, Chrome, IE7-9; image is correctly resized to less than its natural (explicit) 500px height. Looks like the CSS is holding.

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