Make WordPress Core

Opened 13 years ago

Closed 11 years ago

#19393 closed enhancement (fixed)

Image crop position

Reported by: bradt's profile bradt Owned by: markoheijnen's profile markoheijnen
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.3
Component: Media Keywords: has-patch commit dev-reviewed
Focuses: Cc:

Description

In WP, images are cropped to center horizontally and vertically. Setting a different crop position is very painful and a bit of hack using filters/hooks (see https://gist.github.com/1405838). The small attached patch enhances the $crop parameter of add_image_size, allowing an array to be passed in containing the crop position. For example,

add_image_size( 'product-screenshot', 300, 300, array( 'left', 'top' ) );

The $crop parameter still accepts true/false values and the patch should be fully backward compatible. Syntax is borrowed from CSS' background-position property, so it should be familiar to designers/developers.

Attachments (6)

wp-image-crop-position.patch (2.7 KB) - added by bradt 13 years ago.
19393.diff (2.8 KB) - added by wonderboymusic 11 years ago.
19393.2.diff (4.6 KB) - added by kirasong 11 years ago.
Refreshed and updated docs for image_resize_dimensions() to reflect optional array argument.
19393.3.diff (5.6 KB) - added by DrewAPicture 11 years ago.
doc fixes
19393.4.diff (5.4 KB) - added by nacin 11 years ago.
19393.5.diff (738 bytes) - added by SergeyBiryukov 11 years ago.

Download all attachments as: .zip

Change History (97)

#1 follow-up: @nacin
13 years ago

Swoon.

#2 @jkudish
13 years ago

  • Cc joachim.kudish@… added

#3 in reply to: ↑ 1 @DrewAPicture
13 years ago

Replying to nacin:

Swoon.

Indeed.

#4 @Jayjdk
13 years ago

  • Cc kontakt@… added

#5 @wpdavis
13 years ago

This isn't in the scope of this patch, but it might be a good thing for down the road: It'd be great if you could identify a scope for image sizes, as well. For example, we have a 250x250 thumbnail size, but also a 75x75 thumbnail size. If you crop only the thumbnail, the 75x75 size doesn't crop with it. Then when cropping, instead of selecting "thumbnail" or "all other image sizes," you could select a scope.

#6 @dwenaus
13 years ago

  • Cc deryk@… added

double swoon

#7 @miklb
13 years ago

  • Cc miklb@… added

#8 @hd-J
13 years ago

  • Cc jeremy@… added

#9 @anointed
13 years ago

  • Cc anointed added

#10 @tammyhart
13 years ago

  • Cc tammyhart added

Besides on the fly cropping, this is the one thing that I hated about dropping Tim Thumb. Changing the position really helps with things like website screenshots where you want the logo/header to be in the crop, not a chunk out of the middle. The filter/hook hacks work great, but I would love to see this in core.

#11 follow-up: @dwenaus
13 years ago

there are many times when you have pictures of people, and cropping to the middle chops off their head. uncool. :) is this going to make it into 3.4?

#12 @jane
13 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.4

I would say this falls under editor/media improvements and should therefore be looked at by @azaozz.

@bradt and everyone: Don't forget to add the has-patch keyword when you post a patch, or the appropriate leads won't know to look it over.

#13 in reply to: ↑ 11 ; follow-up: @azaozz
13 years ago

Replying to dwenaus:

there are many times when you have pictures of people, and cropping to the middle chops off their head. uncool. :) is this going to make it into 3.4?

Thinking this would need more general solution. Currently users can crop the images (or just the thumbs) any way they like by using the image editing tab in the image settings.

Similarly we have two sets of image manipulation functions: one "general" and another specific for the image editor. Imho it's worth merging them and making a nicer API for image manipulation in core.

#14 @goto10
13 years ago

  • Cc dromsey@… added

#15 in reply to: ↑ 13 @bradt
13 years ago

Replying to azaozz:

Replying to dwenaus:

there are many times when you have pictures of people, and cropping to the middle chops off their head. uncool. :) is this going to make it into 3.4?

Thinking this would need more general solution. Currently users can crop the images (or just the thumbs) any way they like by using the image editing tab in the image settings.

Are you saying that every time a user uploads a headshot, screenshot, or any other image that requires a different crop position, we should expect the user to go into the cropper tool and crop the image and/or its thumbnails? I don't see any reason why we shouldn't be able to set a different default crop position for each image size and save the user this hassle. I can tell you first hand that when the user is a client, this is a tall order.

Similarly we have two sets of image manipulation functions: one "general" and another specific for the image editor. Imho it's worth merging them and making a nicer API for image manipulation in core.

Agreed, but I believe it's outside the scope of this patch. This patch is about extending the $crop variable of the the add_image_size API function to solve a common problem where the top solutions found on Google has long been to *gulp* ...hack the core.

#16 @ryan
13 years ago

  • Milestone changed from 3.4 to Future Release

#17 @brasofilo
13 years ago

  • Cc brasofilo@… added

#18 @pauldewouters
13 years ago

  • Cc pauldewouters added

#19 @georgestephanis
12 years ago

Related: #15989

Last edited 12 years ago by SergeyBiryukov (previous) (diff)

#20 @sillybean
12 years ago

  • Cc steph@… added

#21 @sabreuse
12 years ago

  • Cc sabreuse@… added

#22 @philiparthurmoore
12 years ago

  • Cc philip@… added

#23 @Viper007Bond
12 years ago

  • Cc Viper007Bond added

#24 @scribu
12 years ago

Awesome idea. Related: #21810

#25 @georgestephanis
12 years ago

If the intval() casting in wp-admin/includes/images.php weren't there (as per existing patch) the rest of it could be done via a plugin or theme's functions.php implementing the image_resize_dimensions filter ... no?

I wouldn't mind stripping the casting and calling the rest plugin/theme territory.

#26 @navjotjsingh
12 years ago

  • Cc navjotjsingh@… added

#27 @mordauk
12 years ago

  • Cc pippin@… added

#28 @sc0ttkclark
12 years ago

  • Cc lol@… added

#29 @LisaSabinWilson
12 years ago

  • Cc LisaSabinWilson added

#30 @ethitter
12 years ago

  • Cc erick@… added

#31 @scribu
12 years ago

Related: #22100

#32 @richardmtl
12 years ago

  • Cc richardmtl added

#33 @alexvorn2
12 years ago

  • Cc alexvornoffice@… added

#34 @lkraav
12 years ago

  • Cc leho@… added

#35 @SergeyBiryukov
12 years ago

#23161 was marked as a duplicate.

#36 @anonymized_7658014
12 years ago

  • Cc caspar@… added

#37 follow-up: @markoheijnen
12 years ago

  • Keywords needs-patch added; has-patch removed

Remove has patch since the changes of 3.5 but would be cool to add this in 3.7 :)

#38 @stevenkword
11 years ago

  • Cc stevenword@… added

#39 @wpsmith
11 years ago

  • Cc t@… added

#40 @dwenaus
11 years ago

  • Cc deryk@… removed

#41 @mau
11 years ago

  • Cc ngomau@… added

#42 @mordauk
11 years ago

Any chance of milestoning this for 3.7?

#43 in reply to: ↑ 37 @helen
11 years ago

Replying to markoheijnen:

Remove has patch since the changes of 3.5 but would be cool to add this in 3.7 :)

Patch still applies fine for me - I'm sure you know better what impact the 3.5 changes have and whether or not this would be appropriate for 3.7. I didn't test.

#44 @helen
11 years ago

  • Keywords has-patch added; needs-patch removed

#45 follow-up: @markoheijnen
11 years ago

  • Milestone changed from Future Release to 3.7
  • Owner set to markoheijnen
  • Status changed from new to assigned

Moving to 3.7. I will spend some time this weekend to check the patch and rewrite if needed

#46 @thelukemcdonald
11 years ago

  • Cc thelukemcdonald@… added

#47 in reply to: ↑ 45 @nacin
11 years ago

  • Milestone changed from 3.7 to Future Release

Replying to markoheijnen — 4 weeks ago:

Moving to 3.7. I will spend some time this weekend to check the patch and rewrite if needed

Moving to 3.8.

#48 @alex-ye
11 years ago

  • Cc nashwan.doaqan@… added

#49 @jaredatch
11 years ago

  • Cc jared@… added

#50 @scottsweb
11 years ago

  • Cc scottsweb added

#51 @buffler
11 years ago

  • Cc jeremy.buller@… added

#52 @sccr410
11 years ago

  • Cc derek@… added

This needs to get into core. Badly.

#53 follow-up: @markoheijnen
11 years ago

More and more I disagree the need of this. When do you really want to set a crop like this? I rather would invest time in having better tools to create a good crop like a better UI and auto detect the best crop.

#54 in reply to: ↑ 53 @goto10
11 years ago

Replying to markoheijnen:

... When do you really want to set a crop like this? I rather would invest time in having better tools to create a good crop like a better UI and auto detect the best crop.

The typical scenario for me is when I need to have a portrait-sized image (custom image size) for the single page and a square image (WP's thumbnail size) on an archive page. Typically, the square image on the archive page will be cropped incorrectly. To solve this, the user has to edit the image and apply a crop manually to only the thumbnail. Experience has shown this to be tricky for users to do.

Alternatively, I could add another image upload box so that there's a dedicated image for both the square and portrait images, but I feel like that's kind of lame.

BTW, the reason I have to use the WP thumbnail size is that the crop tool won't allow us to apply a crop to only a custom image size. I'm all for improving that situation too, but this ticket seemed like a decent stopgap. Possibly relevant: #19889

Last edited 11 years ago by SergeyBiryukov (previous) (diff)

#55 @sccr410
11 years ago

This is needed for screenshots or person profile photos where you want the cropped thumbnail to show the top of the image. Client's don't want to have to mess with Photoshop or WordPress cropping tools - they just want to upload an image and be done. I get "Why is my head cut off?" comments all the time from clients.

This feature is also part of TimThumb and other image resizing libraries. Why not include it in WP core image resizing?

This was pushed to 3.8 but looks like it didn't make it. Can we see this in 3.9?

#56 @markoheijnen
11 years ago

I'm going to spend a lot of my time towards improving image/media stuff for 3.9. I maybe will close this ticket depending on the outcome. Defining the position doesn't help. You never know what the exact position is you want to crop unless you are the only user. I think the biggest issue here is that the current image editor isn't really reusable/integrated in the workflow.

#57 @bradt
11 years ago

@markoheijnen I disagree. If you have a post type for team members, you could have a custom image size to display headshots, which by default should all be cropped top, center instead of center, center. I don't understand the reluctance to offer the ability to define a more suitable default crop when you know the center, center default will often fail.

Last edited 11 years ago by bradt (previous) (diff)

#58 @goto10
11 years ago

I agree with @markoheijnen that the image editor could be more integrated. I also agree with @bradt that it would be quite helpful to be able to set a default crop position.

I ran into a use-case just last week where having a default crop position would have really helped. I regenerated all of the image sizes on a site using the Regenerate Thumbnails plugin, only to realize that this overwrote all of the custom crops I had set for staff profile images. (The staff images use manual top-center square crops for the archive page, and portrait sized images on the single pages.)

#59 @dwenaus
11 years ago

On sites where content is controlled and predictable having a default crop position can be necessary. Years ago I worked on a fashion blog and I had to hack core to ensure that heads were not being chopped of all the photos.

#60 @SergeyBiryukov
11 years ago

  • Milestone changed from Future Release to 3.9

wp-image-crop-position.patch needs a refresh in terms of coding standards, but otherwise looks good and makes sense to me.

#61 @markoheijnen
11 years ago

  • Keywords needs-patch added; has-patch removed

@Bradt You make an assumption that can be incorrect. You always expect that the upload an image with only with them self in it.

I will make a blog post about my ideas on how things like this can work out and how we should prevent issues that goto10 had. WordPress should always store the center point of an image. That would mean that we don't change image sizes anymore and just the image. What to me makes te most sense and is needed for future on-the-fly generation.

This patch needs indeed a refresh but also a recoding of it. This code needs to be inside WP_Image_Editor.

#62 @thelukemcdonald
11 years ago

In the case where someone is using WordPress to create their own site or a site for a client in which they control, one can safely assume the correct crop position of an image. WordPress itself is making a default assumption that can be incorrect, too.

That said, a possible solution may be to allow theme developers to define a default crop that best fits the intended purpose and will more than likely be the correct assumption a majority of the time. Then, with an improved WP_Image_Editor, users can re-crop the image as desired. It might even be advantageous to create a "Quick Crop" option to select one of the various image crop positions (top left, top center, top right, etc.).

Anything we can do to help the users experience will be a bonus, and being able to define a crop position where it makes sense is one of them. More often than not, a crop position will not need to be set. But again, when working on projects you control, this would be very helpful. Not allowing a default crop position to be set just seems silly.

Last edited 11 years ago by thelukemcdonald (previous) (diff)

#63 @markoheijnen
11 years ago

Center crop is the safest assumption for the default value and obviously is most of the time wrong. You can't fix that with a default option. The only fix for this is adding intelligent way for detecting the best the center point for a crop.

A theme developer doesn't know the content most of the time so that will not work so that doesn't work.

#64 follow-up: @bradt
11 years ago

Sorry @markoheijnen, but I'm not making the assumption you're accusing me of. Maybe you've misunderstood? I thought I'd given a very clear, very specific example where the ability to adjust the default crop position would have solved a painful real-world problem that has afflicted myself and I assume the majority of people cc'ed on this ticket.

It seems that whenever this patch gets to someone with the power to push it forward, they insist that the scope must be expanded to include a complete revamp of the image editor. This patch is very focused on default crop position so I don't understand why the scope must be expanded. I feel like I'm trying to get a bill through congress here. There must be an unknown unknown that I'm missing. Perhaps someone can enlighten me?

I'd update this patch today if I was told the scope doesn't need to be expanded for it to be rolled into core.

Last edited 11 years ago by bradt (previous) (diff)

#65 @helen
11 years ago

I don't understand why improving the manual crop would render this invalid. Bespoke client sites in particular could benefit from a programmatic way of changing the default crop. Yes, you can predict that when a fashion site uploads a portrait-oriented image that it's a picture of a person whose head shouldn't be cut off - I have experienced this exact scenario specifically. This is a separate issue from manual crop or any sort of UI; let's not mix them up or rabbithole this poor ticket - nobody is talking about changing the WP default, and if a general release theme developer does this, well, frankly it's no worse than any number of other things they can do, anyway.

#66 in reply to: ↑ 64 @SergeyBiryukov
11 years ago

Replying to bradt:

I'd update this patch today if I was told the scope doesn't need to be expanded for it to be rolled into core.

An updated patch would definitely be helpful here.

#67 @bradt
11 years ago

@helen Couldn't agree with you more.

@Sergey It will not help to update the patch if it's already decided that the ability to change the default crop position is not an API change that will be accepted into WP core. That's the sticky point here. Not that the patch needs an update. I'd like a decision on this first, but I'm confused as to how decisions are made here (this is my first patch submission). There seems to be plenty of people on this ticket supporting it and only a couple who are suggesting to expand the scope to include the manual image editor. Does the majority rule?

#68 @markoheijnen
11 years ago

I just posted a blog post about the whole image workflow: http://markoheijnen.com/changing-the-image-workflow. Also I wasn't talking here about any UI specific things.

It was not my intention to make it harder then it is. But the code behind this patch was completely revamped. So it does needs to be checked if this is still the right way.

So this patch is good for me but a few things need to change:

  • coding standards need to be followed
  • The filter needs documentation
  • The filter also need to pass the given $crop value
  • Needs unit tests

#69 @wonderboymusic
11 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

19393.diff refreshes the patch.

#70 @chriswallace
11 years ago

So what if instead of adding a way to define the crop with a filter we instead give the user the ability to crop the various image sizes themselves?

This is just a rough wireframe of the idea, but imagine uploading an image and being presented with this screen:

http://f.cl.ly/items/0S3Q030o462E3X0R2r2v/crop-tool-single-image-upload.jpg

The user would be able to preview the crop for all image sizes and decide if anything needs to be done, then if they decide to edit the crop, they would easily be able to do so for any image sizes added by themes, plugins or Core, all in one screen.

I know there are workflow issues here for multiple images, but it could be an accordion drop-down type list for multiple images and perhaps allow users to bypass this entire screen by having a checkbox for "Crop my images automatically" that is enabled by default.

The other issue is if users have Regenerate Thumbnails installed and are changing the dimensions of their images which could potentially screw up their manual cropping. There would need to be a way to edit the crops or set new ones if a user changes their theme and wants to continue using the images they already have loaded in.

Anyway, just a thought. Take it or leave it.

#71 @rickalee
11 years ago

Chris' illustration looks quite similar to workflow for Post Thumbnail Editor plugin.

http://wordpress.org/plugins/post-thumbnail-editor/screenshots/

#72 @helgatheviking
11 years ago

+1 for Chris' mockup. I have need to be able to crop the different image sizes differently. I think I have used the Post Thumbnail Editor, but it didn't feel "native". The thing I might add would be a focal point... maybe the automatic cropping could be based on that.

#73 @markoheijnen
11 years ago

I believe it's best for core to drop editing the image sizes. For me that is better for plugins to take care of. Also how it should be done in UI is something that should be addressed in #21811

#74 @helen
11 years ago

And I think we are hijacking both tickets. There have been a few explorations around individual image size cropping here and there, and I love seeing them and would like to find the right solution, but I feel so bad that what should have been a straightforward change to allow programmatic crop position definitions has thrown us down such a giant rabbithole.

Being willing to iterate and get smaller, more achievable pieces done sooner is not antithetical to eventual larger changes. Can we please stop acting like it is?

#75 @sccr410
11 years ago

The point of this is for a specific custom image size, set a crop location. So I see absolutely no need for an editor to be involved with what is being asked here. I want to be able to create an image size that shrinks the image and crop is to the top - so people can start getting their foreheads back. This request is not asking to be able to upload an image and then manipulate all the various image sizes like the comp that @chriswallace posted. It is to have an option in the add_image_size function to set the crop position of the image which automatically is generated. Seems like people are over thinking this request.

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.


11 years ago

#77 follow-up: @kirasong
11 years ago

I like this idea, although I'd prefer to see the cropping logic live in WP_Image_Editor. Perhaps within ->resize(), with the same array syntax as the above, so that it's simple to perform a crop with certain orientations without adding an image size.

If the above patch is used, we can still decide to refactor it into WP_Image_Editor later, if we want to go with an incremental approach.

#78 in reply to: ↑ 77 ; follow-up: @tomauger
11 years ago

In case this helps, I'd like to cross-post from #21811 in which (possibly mistakenly) I had posted some patches that address the same issue in a different way (though I really like the UI proposed by chriswallace).

The solution was broken into 3 patches:
Patch 1 - Extend the ImageEditor to allow modular editor groups
Patch 2 - The actual patch that allows multiple image sizes to be targeted from the Editor
Plugin ZIP - Demo plugin that demonstrates arbitrary cropping of any intermediate sizes

If there's merit to this approach, I'll refresh the patches and upload them to this ticket. I'll keep my eye on this ticket to see if there's any traction.

#79 in reply to: ↑ 78 ; follow-up: @kirasong
11 years ago

Replying to tomauger:

In case this helps, I'd like to cross-post from #21811 in which (possibly mistakenly) I had posted some patches that address the same issue in a different way (though I really like the UI proposed by chriswallace).
If there's merit to this approach, I'll refresh the patches and upload them to this ticket. I'll keep my eye on this ticket to see if there's any traction.

While related, this ticket only has to do with support of setting crop locations at the API level, and not UI/UX.
I'd love to see the modal better support such things, but I think that's for a separate ticket.

#80 in reply to: ↑ 79 @tomauger
11 years ago

Replying to DH-Shredder:

While related, this ticket only has to do with support of setting crop locations at the API level, and not UI/UX.
I'd love to see the modal better support such things, but I think that's for a separate ticket.

Thanks for this clarification. I seem to have trouble assessing the intent / scope of tickets! I'll get it right eventually.

This ticket was mentioned in IRC in #wordpress-dev by DH-Shredder. View the logs.


11 years ago

@kirasong
11 years ago

Refreshed and updated docs for image_resize_dimensions() to reflect optional array argument.

#82 @kirasong
11 years ago

Refreshed patch attached.

Also in the patch, added appropriate docs to image_resize_dimensions() to reflect the new optional array argument.

@DrewAPicture
11 years ago

doc fixes

#83 @DrewAPicture
11 years ago

19393.3.diff fixes up the docs a bit for clarity.

@nacin
11 years ago

#84 @nacin
11 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 27472:

Allow $crop in add_image_size() to receive crop anchors (top, left, right, bottom, center).

This also applies to set_post_thumbnail_size() and image_resize_dimensions().

props bradt, wonderboymusic, DH-Shredder.
fixes #19393.

#85 @photographer
11 years ago

how can i use this ?

i'm looking for a solution for issue described here:
http://wordpress.org/support/topic/woocommerce-images-hard-crop-in-how-to-create-even-square-tiles?replies=1

can this work ? how do i use this code?

#86 @Otto42
11 years ago

Think this might need an additional patch to be complete.

https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/image-edit.php#L781

This bit still assumes that the $crop parameter is a true/false/int type of value, so the new cropping settings won't be applied properly to the additional image sizes after the image is run through the image editor.

#87 @SergeyBiryukov
11 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Looks like [27522] didn't account for the changes in [27472].

19393.5.diff is untested, but, from my reading, should fix this.

#88 @kirasong
11 years ago

Reproduced. 19393.5.diff looks to fix the problem on "Save," for me.

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.


11 years ago

#90 @nacin
11 years ago

  • Keywords commit dev-reviewed added; needs-testing removed

Reviewed by both me and MarkJaquith. Good to go.

The good thing is, any other instances of int or bool casts would result in this being considered "yes" as an answer to cropping, not "no", and would minimize breakage.

#91 @nacin
11 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 28072:

Account for new cropping settings in the image editor.

props Otto42, SergeyBiryukov.
fixes #19393.

Note: See TracTickets for help on using tickets.