Make WordPress Core

Opened 13 years ago

Closed 12 years ago

#21100 closed defect (bug) (fixed)

Uploading Header Images that are too small

Reported by: madtownlems's profile MadtownLems Owned by: ryan's profile ryan
Milestone: 3.5 Priority: normal
Severity: minor Version:
Component: Themes Keywords: has-patch commit
Focuses: Cc:

Description

Prior to 3.4, if you uploaded a Header Image that was too small, you'd be prompted to choose a region with the correct aspect ratio and then WordPress would scale the region to fit - resulting in a slightly pixelated image that at least fit the entire space.

With 3.4, a button is introduced to "Skip Cropping, Publish Image as Is." In my experiences, this keeps leading to much worse looking images than cropping and getting a little pixelation.

Here is an example of a duck outside my building:
http://imgur.com/a/hTwiN

When you select a crop region, yes, the image gets slightly distorted, but at least it fills the entire space.
When you publish 'as is', you don't get to choose what gets cropped where, and it doesn't even fill the whole space.
For reference, this theme has a header are of 960x190 and I was working with an image sized at 800x534.

In #wordpress-dev, Sergey suggested displaying a warning when uploading an image size that is too small - which I think is a fantastic idea. Something like:

"Your theme wants a header image of 960 wide x 190 high. The image you've selected is only 800 wide. You can proceed with the image, but it may result in a distorted header image on your site. Consider finding a larger image to use as your header."

I think displaying such a warning would be good customer service for users who don't know much about image sizes or working with images. Ideally, though, as a Network Admin, I'd actually love the ability to disallow selecting too small of images as Header Images completely.

Attachments (3)

Screen_Shot.jpg (105.0 KB) - added by kobenland 13 years ago.
21100.diff (3.4 KB) - added by kobenland 13 years ago.
This is what I came up with: Only display skip crop button, wenn flex-width or height are supported
custom-header-21100-madtownlems.diff (1.1 KB) - added by MadtownLems 13 years ago.
patch for ticket 21100 - hide "skip crop" button when flex headers not in use

Download all attachments as: .zip

Change History (17)

#1 @jane
13 years ago

  • Keywords reporter-feedback added

I may be misunderstanding the screen grab you linked to, but it looks like your theme isn't respecting flexible height header image... if it was, it would have printed the entire image, not chopped it with a menu at 190px. Main intent of flex headers was to stop forcing users to use the theme's desired header dimensions. I wouldn't want us to remove that flexibility (in a network admin situation where you want to have complete control over what your site admins do, I think a plugin providing that option might be worth it?). Can you confirm if your theme was supporting the flex headers feature?

#2 @MadtownLems
13 years ago

Ah sorry - I meant to clarify this in my initial post and then totally blanked on it...

We are _not_ using flexible image heights here.

(And yes, that's intentional. We want to maintain a relatively consistent look/feel across dozens of sites and give Site Admins some control over their header, but not let them alter the size.)

#3 @jane
13 years ago

So, that changes things, a bit. If you are overriding the feature, then the option to skip cropping and publish as is doesn't make any sense. I would suggest writing a plugin to remove the 'skip cropping' button if the active theme doesn't support flex headers, and then turn that into a patch (that way you don't have to wait until the end of the year for it).

#4 @MadtownLems
13 years ago

If you are overriding the feature

Just to clarify, is choosing not to use a flexible height header considered 'overriding' a feature? I'd suggest that add_theme_support takes it as an optional parameter for a reason: to have it be optional. However, displaying the "publish as is" button all the time seems to suggest that all themes are *expected* to use flexible height headers.

Yes, for a quick solution for us I'll probably hide the "Publish without Cropping" button from Site Admins using css, (since I don't see a way to hook into that part of the upload process) but, at least to me, it seems like core is inconsistent about if it expects all themes to support flexible height headers.

#5 @kobenland
13 years ago

Tested in trunk with Twenty Eleven and flex-height argument removed, the picture still gets blown up...

What Theme are you using?

#6 follow-up: @MadtownLems
13 years ago

@Kobenland:

To confirm, you removed the flex-height argument from functions.php, just setting 'flex-height' to false in the argument array?

I'm confused now. Your screenshot suggests (and my testing verifies) that, after specifically removing the ability for the header image to be a flexible height... you were able to publish a ginormous image as the header image. Shouldn't you be forced to choose a section of the image with an appropriate aspect ratio and have it resized to the pre-determined header image size of 1000x288?

#7 in reply to: ↑ 6 @kobenland
13 years ago

Replying to MadtownLems:

To confirm, you removed the flex-height argument from functions.php, just setting 'flex-height' to false in the argument array?

No, I commented the line out


Shouldn't you be forced to choose a section of the image with an appropriate aspect ratio and have it resized to the pre-determined header image size of 1000x288?

I guess that would be something for UX to decide.

@kobenland
13 years ago

This is what I came up with: Only display skip crop button, wenn flex-width or height are supported

#8 @MadtownLems
13 years ago

Kobenland's patch is the same logic I came up with and is working well for me

#9 @SergeyBiryukov
13 years ago

  • Keywords has-patch added; reporter-feedback removed
  • Milestone changed from Awaiting Review to 3.5

#10 @kobenland
13 years ago

Nacin just added on IRC, that the patch should also block the skip-cropping block of code in step_3().
I think MadtownLems is on it.

#11 @MadtownLems
13 years ago

I wasn't sure exactly the best way to deal with the crop code in step_3, but seeing as if we hid the button from the UI in step 2, the only way I could fathom that $_POSTskip-cropping? could be !empty() is if they were, in fact, cheatin', so I went that route.

@MadtownLems
13 years ago

patch for ticket 21100 - hide "skip crop" button when flex headers not in use

#12 @nacin
12 years ago

  • Type changed from enhancement to defect (bug)

custom-header-21100-madtownlems.diff appears proper. In that regard, this is a bug fix.

#13 @nacin
12 years ago

  • Keywords commit added

Looks good. Just needs final testing.

#14 @ryan
12 years ago

  • Owner set to ryan
  • Resolution set to fixed
  • Status changed from new to closed

In 22468:

Don't show "Skip Cropping, Publish Image as Is" button for themes that do not support headers with flexible width or height. Forces too small images to be scaled to fit in the absence of flex support.

Props MadtownLems, kobenland
fixes #21100

Note: See TracTickets for help on using tickets.