Avoid redundant crops for custom headers in the media library

[koopersmith]

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.

[SergeyBiryukov]

Related: #27202

[celloexpressions]

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.

[celloexpressions]

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.

[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. 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.

[joemcgill]

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.

[westonruter]

@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).

[joemcgill]

@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.

[westonruter]

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.

[joemcgill]

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.

[swissspidy]

Moving this to the more realistic 4.8 milestone as mentioned during today's bug scrub.

[jbpaul17]

Punting to 4.8.1 per bug scrub earlier this week.

[westonruter]

Moving this to the Media component since it's primarily involving media code.

[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.

[melchoyce]

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. 😅

[westonruter]

Bumping priority to high for visibility and alignment with 4.9 goals, and given proximity to beta 1 deadline.

[joemcgill]

As discussed in the Media component meeting, the approach we want to pursue for 4.9 is as follows:

1. 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
2. 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.

[joemcgill]

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.

[westonruter]

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.

[joemcgill]

In 41732:

Customizer: Minimize duplicate header crops in the media library.

This adds Custom_Image_Header::get_previous_crop(), which finds any
previously cropped headers created from the same base image and replaces
that attachment rather than creating a new attachment.

After updating a crop, the replaced images is also removed from the list
of previous header images in the Customizer.

See #21819.

[joemcgill]

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.

[joemcgill]

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.

[joemcgill]

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?

[joemcgill]

In 41937:

Customizer: Improve handling of crops in the media library.

This is a follow up on r41732, implementing the following improvements:

• Attachment parent info is now stored in attachment meta rather than a

separate post meta key.

• Attachments created from contextual crops (e.g. header, logos, etc.) are

filtered out of the media library using a new _filterContext method in
wp.media.controller.Library.

Props joemcgill, westonruter.
See #21819.

[westonruter]

@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?

[joemcgill]

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.