WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 9 days ago

#19393 closed enhancement (fixed)

Image crop position

Reported by: bradt Owned by: 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 2 years ago.
19393.diff (2.8 KB) - added by wonderboymusic 3 months ago.
19393.2.diff (4.6 KB) - added by DH-Shredder 6 weeks ago.
Refreshed and updated docs for image_resize_dimensions() to reflect optional array argument.
19393.3.diff (5.6 KB) - added by DrewAPicture 6 weeks ago.
doc fixes
19393.4.diff (5.4 KB) - added by nacin 6 weeks ago.
19393.5.diff (738 bytes) - added by SergeyBiryukov 10 days ago.

Download all attachments as: .zip

Change History (97)

comment:1 follow-up: nacin2 years ago

Swoon.

comment:2 jkudish2 years ago

  • Cc joachim.kudish@… added

comment:3 in reply to: ↑ 1 DrewAPicture2 years ago

Replying to nacin:

Swoon.

Indeed.

comment:4 Jayjdk2 years ago

  • Cc kontakt@… added

comment:5 wpdavis2 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.

comment:6 dwenaus2 years ago

  • Cc deryk@… added

double swoon

comment:7 miklb2 years ago

  • Cc miklb@… added

comment:8 hd-J2 years ago

  • Cc jeremy@… added

comment:9 anointed2 years ago

  • Cc anointed added

comment:10 tammyhart2 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.

comment:11 follow-up: dwenaus2 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?

comment:12 jane2 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.

comment:13 in reply to: ↑ 11 ; follow-up: azaozz2 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.

comment:14 goto102 years ago

  • Cc dromsey@… added

comment:15 in reply to: ↑ 13 bradt2 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.

comment:16 ryan2 years ago

  • Milestone changed from 3.4 to Future Release

comment:17 brasofilo2 years ago

  • Cc brasofilo@… added

comment:18 pauldewouters23 months ago

  • Cc pauldewouters added

comment:19 georgestephanis22 months ago

Related: #15989

Last edited 21 months ago by SergeyBiryukov (previous) (diff)

comment:20 sillybean21 months ago

  • Cc steph@… added

comment:21 sabreuse21 months ago

  • Cc sabreuse@… added

comment:22 philiparthurmoore20 months ago

  • Cc philip@… added

comment:23 Viper007Bond20 months ago

  • Cc Viper007Bond added

comment:24 scribu20 months ago

Awesome idea. Related: #21810

comment:25 georgestephanis20 months 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.

comment:26 navjotjsingh20 months ago

  • Cc navjotjsingh@… added

comment:27 mordauk19 months ago

  • Cc pippin@… added

comment:28 sc0ttkclark19 months ago

  • Cc lol@… added

comment:29 LisaSabinWilson19 months ago

  • Cc LisaSabinWilson added

comment:30 ethitter19 months ago

  • Cc erick@… added

comment:32 richardmtl18 months ago

  • Cc richardmtl added

comment:33 alexvorn217 months ago

  • Cc alexvornoffice@… added

comment:34 lkraav17 months ago

  • Cc leho@… added

comment:35 SergeyBiryukov15 months ago

#23161 was marked as a duplicate.

comment:36 glueckpress15 months ago

  • Cc caspar@… added

comment:37 follow-up: markoheijnen13 months 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 :)

comment:38 stevenkword10 months ago

  • Cc stevenword@… added

comment:39 wpsmith9 months ago

  • Cc t@… added

comment:40 dwenaus9 months ago

  • Cc deryk@… removed

comment:41 mau9 months ago

  • Cc ngomau@… added

comment:42 mordauk8 months ago

Any chance of milestoning this for 3.7?

comment:43 in reply to: ↑ 37 helen8 months 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.

comment:44 helen8 months ago

  • Keywords has-patch added; needs-patch removed

comment:45 follow-up: markoheijnen8 months 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

comment:46 thelukemcdonald8 months ago

  • Cc thelukemcdonald@… added

comment:47 in reply to: ↑ 45 nacin7 months 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.

comment:48 alex-ye7 months ago

  • Cc nashwan.doaqan@… added

comment:49 jaredatch6 months ago

  • Cc jared@… added

comment:50 scottsweb6 months ago

  • Cc scottsweb added

comment:51 buffler6 months ago

  • Cc jeremy.buller@… added

comment:52 sccr4105 months ago

  • Cc derek@… added

This needs to get into core. Badly.

comment:53 follow-up: markoheijnen5 months 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.

comment:54 in reply to: ↑ 53 goto105 months 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 4 months ago by SergeyBiryukov (previous) (diff)

comment:55 sccr4104 months 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?

comment:56 markoheijnen4 months 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.

comment:57 bradt4 months 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 4 months ago by bradt (previous) (diff)

comment:58 goto104 months 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.)

comment:59 dwenaus4 months 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.

comment:60 SergeyBiryukov4 months 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.

comment:61 markoheijnen4 months 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.

comment:62 thelukemcdonald4 months 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 4 months ago by thelukemcdonald (previous) (diff)

comment:63 markoheijnen4 months 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.

comment:64 follow-up: bradt4 months 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 4 months ago by bradt (previous) (diff)

comment:65 helen4 months 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.

comment:66 in reply to: ↑ 64 SergeyBiryukov4 months 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.

comment:67 bradt4 months 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?

comment:68 markoheijnen4 months 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

wonderboymusic3 months ago

comment:69 wonderboymusic3 months ago

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

19393.diff refreshes the patch.

comment:70 chriswallace2 months 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.

comment:71 rickalee2 months ago

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

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

comment:72 helgatheviking2 months 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.

comment:73 markoheijnen2 months 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

comment:74 helen2 months 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?

comment:75 sccr4102 months 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.

comment:76 ircbot7 weeks ago

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

comment:77 follow-up: DH-Shredder7 weeks 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.

comment:78 in reply to: ↑ 77 ; follow-up: tomauger7 weeks 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.

comment:79 in reply to: ↑ 78 ; follow-up: DH-Shredder7 weeks 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.

comment:80 in reply to: ↑ 79 tomauger7 weeks 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.

comment:81 ircbot6 weeks ago

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

DH-Shredder6 weeks ago

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

comment:82 DH-Shredder6 weeks ago

Refreshed patch attached.

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

DrewAPicture6 weeks ago

doc fixes

comment:83 DrewAPicture6 weeks ago

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

nacin6 weeks ago

comment:84 nacin6 weeks 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.

comment:85 photographer12 days 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?

comment:86 Otto4210 days 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.

SergeyBiryukov10 days ago

comment:87 SergeyBiryukov10 days 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.

comment:88 DH-Shredder9 days ago

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

comment:89 ircbot9 days ago

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

comment:90 nacin9 days 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.

comment:91 nacin9 days 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.