#21819 closed task (blessed) (fixed)
Avoid redundant crops for custom headers in the media library
Reported by: | koopersmith | Owned by: | joemcgill |
---|---|---|---|
Milestone: | 4.9 | Priority: | high |
Severity: | normal | Version: | 3.5 |
Component: | Media | Keywords: | has-patch needs-testing 2nd-opinion |
Focuses: | Cc: |
Description
The improvements in #21810 will allow us to use an image size for custom headers, and we definitely should โย it results in fewer attachments, allows the user to recrop the header, and allows us to make intelligent decisions regarding how to treat custom headers when the theme is changed.
Attachments (4)
Change History (59)
#2
@
12 years ago
- Milestone changed from 3.5 to Future Release
- Owner nacin deleted
- Type changed from task (blessed) to enhancement
This ticket was mentioned in โSlack in #core-customize by valendesigns. โView the logs.
10 years ago
#6
@
10 years ago
- Keywords needs-patch added
The current way that attachments are duplicated can result in some strange experiences for users, especially if the cropped images are significantly cropped, but then appear to be the originls in the media library and accidentally get inserted into posts or elsewhere, or reused with other themes (especially an issue with Twenty Fifteen's portrait featured images).
However, we still need a patch here, and at least some movement on dependency tickets, to work out a solution.
#7
@
9 years ago
- Keywords close added
- Milestone changed from Future Release to Awaiting Review
We now use the approach of creating duplicated attachments for all cropped image controls in the customizer, including site icon and custom logo in core. It seems to be working okay, and there are some ways to improve the experience, such as displaying the context of the cropped images in the media library. We should probably maybelater this, and revisit if we're able to actually tackle this from a technical perspective in the future.
This ticket was mentioned in โSlack in #core-customize by westonruter. โView the logs.
8 years ago
#9
follow-up:
โย 11
@
8 years ago
- Keywords close removed
Technically it seems like we could solve it by adding the new crop among the stored intermediate image sizes associated to the original image, as opposed to creating a new attachment. And then custom headers, custom logos, and site icons can all pick out the size that they need. The challenge there could be then picking from between two identical sizes. Perhaps there could be a flag among the sizes to indicate if the crop was made for custom logo, custom header, or site icon to eliminate the guess work.
#11
in reply to:
โย 9
@
8 years ago
Replying to westonruter:
Technically it seems like we could solve it by adding the new crop among the stored intermediate image sizes associated to the original image, as opposed to creating a new attachment.
There are other use cases for WordPress to define a data structure for handling variations on a single attachment, which should be kept in mind here. We would want to consider create multiple image sizes associated with a crop so we could do responsive images and not cross-pollute between different crop types.
This ticket was mentioned in โSlack in #core by jeffpaul. โView the logs.
8 years ago
This ticket was mentioned in โSlack in #core-media by desrosj. โView the logs.
8 years ago
#15
@
8 years ago
@joemcgill could you elaborate?
Let's talk about the use case for a custom logo. Let's say the logo requires a 400x400 image, but we upload a 640x480 image. The user performs the crop and thenโฆ you're saying that there could very well be multiple 400x400 crops of an image that capture different parts of the image (e.g. center,center vs top,left) and so each should be able to co-exist?
What if the cropped image created a new ad hoc image size on the fly and gave it a name like, custom_logo-400x400
and then stored it among the _wp_attachment_metadata
for the main attachment. The then problem is that the different responsive image sizes for this 400x400 image would not be created, unless we were to loop through all of the theme's image sizes that are smaller than 400x400 and create a similar ad hoc image size for each, e.g. custom_logo-{$image_size}
.
This also then crosses into similar territory with #36441, and what happens if the user uploads a square 512x512 image when a 400x400 image is required. They shouldn't have to crop it since it's already square. But then could we create an ad hoc custom_logo-400x400
image size?
All of this is closely related to responsive image support for images in the customizer media control (#36191).
#16
follow-up:
โย 17
@
8 years ago
@westonruter happy to elaborate, apologies for the quick reply and run earlier.
There are two closely related issues related to responsive image support that concern me about how this is implemented.
First, when someone displays an image, we add responsive image support in wp_calculate_image_srcset()
by looping through all intermediate sizes in the attachment metadata array and including any image that matches the aspect ratio of our src
image, as long as it was created during the same edit as the src
file (by searching for the existence of an edit hash). So imagine you upload a file to a blog post and WordPress creates a 150x150 thumbnail cropped from the middle. Later, you decide you want to make this image your custom logo, but want to crop it from top at 400x400. Unless WP knows that these are from different edits when calculating srcset
attributes, both could end up being selected, which could mean different crops for the same image depending on your screen size and/or display density. Gross. In short, we need a way to make sure custom crops are treated as a separate edit from preexisting intermediate sizes.
Second, to make sure we include responsive image support for custom logos, it would be nice to generate additional intermediate sizes that relate to the crop being made for the logo. So for instance, if the original image is 2000x1200 and the custom crop is square, it would be nice to generate a series of intermediate sizes that were 1200x1200, 900x900, 450x450, and 200x200 (for example). I'm not sure how we decide what these sizes should be at this point, or if we should just use the registered intermediate sizes.
The couple ideas I have for this off the top of my head would be to extend the sizes
property in attachment meta so that each named size could include a sub-array of sizes. Like:
'sizes' => [ 'thumbnail' => [ 'file' => 'path-to-thumb.jpg', 'width' => 150, 'height' => 150, ], 'medium' => [ 'file' => 'path-to-medium.jpg', 'width' => 300, 'height' => 250, ], 'custom_logo' => [ 'small' => [ 'file' => 'path-to-small-logo.jpg', 'width' => 200, 'height' => 200, ], 'medium' => [ 'file' => 'path-to-medium-logo.jpg', 'width' => 400, 'height' => 400, ], ], ]
Alternately, we could add a new property like alternate_sizes
which contain sets of edited versions of an attachment.
'sizes' => [ 'thumbnail' => [ 'file' => 'path-to-thumb.jpg', 'width' => 150, 'height' => 150, ], 'medium' => [ 'file' => 'path-to-medium.jpg', 'width' => 300, 'height' => 250, ], ], 'alternate_sizes' => [ 'custom_logo' => [ 'small' => [ 'file' => 'path-to-small-logo.jpg', 'width' => 200, 'height' => 200, ], 'medium' => [ 'file' => 'path-to-medium-crop.jpg', 'width' => 400, 'height' => 400, ], ], 'custom_header' => [ 'small' => [ 'file' => 'path-to-small-header.jpg', 'width' => 600, 'height' => 200, ], 'medium' => [ 'file' => 'path-to-medium-header.jpg', 'width' => 1200, 'height' => 400, ], ], ]
Last idea (and probably the most straightforward) is to create a new attachment for custom logo/header crops, like we're currently doing, and set the post parent to the original attachment to keep the association. To keep from having duplicate images in the media library, we might be able to set some metadata on the parent attachment, which indicates that the file should be hidden from the media library if the file was uploaded from one of these upload and crop controls.
Hope this is helpful.
#17
in reply to:
โย 16
;
follow-up:
โย 18
@
8 years ago
Replying to joemcgill:
Alternately, we could add a new property like alternate_sizes which contain sets of edited versions of an attachment.
This makes a lot of sense to me. It would eliminate having to keep track of two different attachment posts. Maybe instead of alternate_sizes
it could be variations
and then this could include a name (e.g. custom_logo
) and then the value could contain the width
, height
, whether cropping was performed done, and so on. Essentially these would be ad hoc image sizes that would be specific to this one attachment
.
When opening the media manager, we'd need to pass in a specific variation name as being desired so that when an image is cropped it gets routed to that variation instead of creating a new attachment. It would be nice if, when re-selecting a previous image that was cropped for a given variation, if the cropper tool opened with the original full size image shown but with the cropper configured to have the same bounding box as was previously selected. If they Skip Crop, then the variation would be unchanged. If they Crop, then it would override the previous variation stored in meta and create a new file on disk.
Last idea (and probably the most straightforward) is to create a new attachment for custom logo/header crops, like we're currently doing, and set the post parent to the original attachment to keep the association. To keep from having duplicate images in the media library, we might be able to set some metadata on the parent attachment, which indicates that the file should be hidden from the media library if the file was uploaded from one of these upload and crop controls.
So essentially this would be introducing the concept of derived images. It does seem a bit tricky in the media select frame what to do with the original vs the derived. Also, we'd need to figure out what to do when the original image is deleted. I was thinking this would be more feasible at first, but I like the idea of alternate_sizes
/variations
more.
#18
in reply to:
โย 17
@
8 years ago
Replying to westonruter:
So essentially this would be introducing the concept of derived images.
In short, adding support for derived images is exactly how I would think about this feature. The main decision I see is determining how we would store/retrieve data about a derived image and how those derived images relate to the original attachment they were derived from.
Something like alternate_sizes
or variations
on the attachment metadata could be simpler, but I do think there are times where we would want a variation of an image to be treated like a separate image entirely. For example, if we used this same structure to store a variation on an image that was treated by a filter, we might want both the filtered and unfiltered variation visible in the media library.
Also, regardless of the storage mechanism for each named variation, we should create sets of sizes like custom_logo
containing its own small
, medium
, large
, etc. sizes. That way we can add responsive image support. For that, I'm not sure if we should just use the default registered intermediate sizes for the site, or if alternate sizes for a custom logo variation should be registered when someone adds support for custom logosโas an additional setting.
This ticket was mentioned in โSlack in #core by swissspidy. โView the logs.
8 years ago
#20
@
8 years ago
- Milestone changed from 4.7.4 to 4.8
Moving this to the more realistic 4.8 milestone as mentioned during today's bug scrub.
This ticket was mentioned in โSlack in #core-media by melchoyce. โView the logs.
8 years ago
This ticket was mentioned in โSlack in #core by jeffpaul. โView the logs.
8 years ago
#23
@
8 years ago
- Milestone changed from 4.8 to 4.8.1
Punting to 4.8.1 per bug scrub earlier this week.
#25
@
7 years ago
- Component changed from Customize to Media
Moving this to the Media component since it's primarily involving media code.
This ticket was mentioned in โSlack in #core-media by desrosj. โView the logs.
7 years ago
#28
follow-up:
โย 29
@
7 years ago
@melchoyce I'm curious if you could restate the needs of this ticket from a UX point of view, as I'd like to avoid going down a huge technical rabbit hole if it's unclear on what the user goals are for this ticket. Any thoughts you (or others) have in this regard would be appreciated.
#29
in reply to:
โย 28
@
7 years ago
Replying to joemcgill:
@melchoyce I'm curious if you could restate the needs of this ticket from a UX point of view, as I'd like to avoid going down a huge technical rabbit hole if it's unclear on what the user goals are for this ticket. Any thoughts you (or others) have in this regard would be appreciated.
People like me upload a million different header images trying to figure out what image and crop looks best, and then have two million copies hanging out in our media libraries, which is super annoying. ๐
This ticket was mentioned in โSlack in #core-media by joemcgill. โView the logs.
7 years ago
This ticket was mentioned in โSlack in #core-media by joemcgill. โView the logs.
7 years ago
#32
@
7 years ago
- Priority changed from normal to high
Bumping priority to high for visibility and alignment with 4.9 goals, and given proximity to beta 1 deadline.
This ticket was mentioned in โSlack in #core-media by joemcgill. โView the logs.
7 years ago
This ticket was mentioned in โSlack in #core by joemcgill. โView the logs.
7 years ago
This ticket was mentioned in โSlack in #core-media by joemcgill. โView the logs.
7 years ago
#36
@
7 years ago
As discussed in the Media component meeting, the approach we want to pursue for 4.9 is as follows:
- Try replacing existing crops of an image which has been cropped in the same context (e.g., custom header, site icon, etc). We had two potential ideas for handling this (either is acceptable, unless we discover a strong preference during implementation/discussion):
- Create a new attachment and delete the existing crop attachment
- Update the file paths on the existing crop attachment
- When showing the media library in a post context, we should attempt to hide crops that were created in a Customizer context. Note that we can make use of the
_wp_attachment_context
meta value for this, but we should't query that value specifically, because meta values are unindexed and will cause performance issues.
We had some discussion about cleaning up orphaned image files after crop replacement, and while we should consider improvements here, it's safer for now to leave these files in place, which is consistent with current behavior in other parts of core.
#37
@
7 years ago
- Keywords has-patch needs-testing 2nd-opinion added; needs-patch removed
21819.diffโ is a first pass at solving the problem as articulated by @melchoyce:
People like me upload a million different header images trying to figure out what image and crop looks best, and then have two million copies hanging out in our media libraries, which is super annoying. ๐
So far, this tackles 2/3 of the problem by ensuring only one crop exists per theme for any given base image and removes old crops from the "previous header images" list in the customizer. The final piece of the puzzle is to hide all attachments created in the header image context from the regular media library, where they just add noise.
It's worth noting that the current implementation makes use of a new post meta key to track the original image from which each crop is created. We might want to consider saving this as data to the regular attachment metadata array or by making use of the native post_parent field of the attachment.
Work in progress is happening in โthis GitHub PR.
This ticket was mentioned in โSlack in #core-media by joemcgill. โView the logs.
7 years ago
#39
@
7 years ago
Test environment has been created (from PR commit 7405e4c
) at โhttp://trac-21819-xwp-testbed.pantheonsite.io/
For a user account, @joemcgill is an admin so he can get you access.
#42
@
7 years ago
- Type changed from task (blessed) to enhancement
I'm not ready to close this out yet for 4.9 because I'd still like to iterate on alternative storage methods and potentially investigate extending this solution to other crop controls in the customizer (e.g., custom logo, site icon, etc.). However, [41732] is already a nice UX improvement over what we currently have.
This ticket was mentioned in โSlack in #core-media by joemcgill. โView the logs.
7 years ago
#45
@
7 years ago
21819.2.diffโ is a riff off [41732] where the attachment parent info is stored in _wp_attachment_metadata
instead of as a separate post meta value.
This ticket was mentioned in โSlack in #core-media by melchoyce. โView the logs.
7 years ago
This ticket was mentioned in โSlack in #core by joemcgill. โView the logs.
7 years ago
#48
@
7 years ago
21819.3.diffโ incorporates the changes from 21819.2.diffโ and adds a method to wp.media.controller.Library
which filters any contextually created attachments (e.g. headers, logos, etc.) out of the media library. @adamsilverstein do you know of a better way to handle that part?
This ticket was mentioned in โSlack in #core by joemcgill. โView the logs.
7 years ago
This ticket was mentioned in โSlack in #core by jeffpaul. โView the logs.
7 years ago
#52
@
7 years ago
@joemcgill would you re-word this ticket to be specifically about what the changes you made, and then create a ticket for any remaining issues that you didn't get to in this one?
This ticket was mentioned in โSlack in #core-media by melchoyce. โView the logs.
7 years ago
#54
@
7 years ago
- Resolution set to fixed
- Status changed from assigned to closed
- Summary changed from Use an image size for custom headers instead of duplicating an attachment to Avoid redundant crops for custom headers in the media library
As of [41732] and [41937], we no longer show duplicate crops in the media library for custom headers. While the implementation was different than initially proposed, the current solution leaves the flexibility to explore support for a first-class derived image system (as discussed in comment #16) and fixes the main UX problem that this ticket was trying to address.
Future work to extend this solution to other Customizer controls, and to work on derived images (in general), can be addressed in separate tickets.
Related: #27202