WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 23 months ago

Last modified 8 months ago

#29211 closed task (blessed) (fixed)

Customizer: extract a cropped-image control from header images

Reported by: celloexpressions Owned by: celloexpressions
Milestone: 4.3 Priority: normal
Severity: normal Version: 3.9
Component: Customize Keywords: has-patch
Focuses: javascript Cc:

Description

Headers in the Customizer do some awesome stuff like allowing an image selected from the media library to be cropped to specific dimensions. We should extract this functionality to a new control, which headers would extend, that allows developers to specify exactly the image size needed for a given image.

Possible API for developers to add such a control:

$wp_customize->add_control( new WP_Customize_Cropped_Image_Control( 
	$wp_customize,
	'setting_id',
	array( // Control args, including inherited ones like label, description, section, etc.
		'width'  => 200, // px, defaults to 0 (flex-width)
		'height' => 200, // px, defaults to 0 (flex-height)
	),
) );

This control would obviously extend WP_Customize_Image control ideally, once we hook it up to the media modal.

Attachments (7)

29211.diff (21.7 KB) - added by markoheijnen 2 years ago.
Example PHP side
29211.2.diff (13.9 KB) - added by markoheijnen 2 years ago.
Revert part of JS so it's easier to find all HeaderTool cases
29211.3.diff (24.9 KB) - added by markoheijnen 2 years ago.
29211.4.diff (27.4 KB) - added by celloexpressions 2 years ago.
background-image-cropper.php (2.0 KB) - added by celloexpressions 23 months ago.
Test plugin/example implementation - background image cropping (with flex width and height).
29211.5.diff (6.6 KB) - added by celloexpressions 23 months ago.
Make the filter for fallback ajax image cropping attachment handling usable, handle attachments for cropped images for the Customizer, fix script dependencies, and clarify Customizer control class inheritance for the cropped-image control.
29211.6.diff (4.2 KB) - added by obenland 23 months ago.

Download all attachments as: .zip

Change History (39)

This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.


2 years ago

#2 @valendesigns
2 years ago

Let's get on this. I like the idea of having some fine tuning in our image controls.

This ticket was mentioned in Slack in #core by johnbillion. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by johnbillion. View the logs.


2 years ago

@markoheijnen
2 years ago

Example PHP side

#5 @markoheijnen
2 years ago

Played with how this could work. For now I only managed to get the PHP side the way I wanted. I do still want to look at how current settings could/should work.

I did deleted method get_current_image_src() due to dead code since the property {{{get_url}}] can't get set since it's not defined as a property. Unsure yet how it does know what the current image is but I guess I will find out when diving more in the JS side.

This ticket was mentioned in Slack in #core by markoheijnen. View the logs.


2 years ago

@markoheijnen
2 years ago

Revert part of JS so it's easier to find all HeaderTool cases

#7 @corradomatt
2 years ago

I really like this idea but why couple it to the customizer so closely? I realize that we want all theme design and layout decisions to go through the customizer but it would also be great to allow usage of image cropping on upload outside of the customizer. Like in the featured image thumbnail of a post or (dare I say) in a front-end interface for non-logged in users to interface with a WP website in some way (think custom post type that can be generated by guest users as an example).

#8 @markoheijnen
2 years ago

For now I still need to play more on the JS part but I could see that cropping would be part of wp.media. I guess that would be something for a new ticket but I do like the idea.

#9 @celloexpressions
2 years ago

The most important thing here with regards to the Customizer is that this new control needs to extend WP_Customize_Media_Control (see the image/background image controls for more basic implementations of extending the base media control). Then the cropping bits within the media modal should be able to be implemented from that child control. Once it's working, we can go back and update the header images control to extend this new control. If that's working, we should have no trouble at all adding site icons into the mix with either a simple add_control or another child control of the cropping control if that's needed.

In terms of the Customizer API, the current header images control doesn't do a great job because it was created before the other media controls used the media modal (think 3.4 media). So the goals would be to take the media modal parts of that and build it onto WP_Customize_Media_Control as a child control class.

#10 @markoheijnen
2 years ago

I did saw it but didn't looked at it. That does make sense. Will see tomorrow if no one beats me before that.

#11 @markoheijnen
2 years ago

Currently that does happen but longer way to get there

WP_Customize_Cropped_Image_Control extends
WP_Customize_Image_Control extends
WP_Customize_Upload_Control extends
WP_Customize_Media_Control extends
WP_Customize_Control.

#12 @markoheijnen
2 years ago

Bit by bit I'm getting a bit closer. I'm curious how the end of cropping should work. Currently with header it's an AJAX call inside Custom_Image_Header. We could abstract some of it's functionality. It's getting more tricky then as in how nonces should work. 1 for all controls?

@markoheijnen
2 years ago

#13 @markoheijnen
2 years ago

29211.3.diff is the current state I'm at. Will look tomorrow at how to deal with Custom_Image_Header. Ideas are welcome if someone has any idea how saving would/should work.

This ticket was mentioned in Slack in #core by markoheijnen. View the logs.


2 years ago

#15 @celloexpressions
2 years ago

I'd say the base control should ideally save either the attachment id of the cropped image (if it becomes a new, separate attachment, which is probably best) or the direct URL to the cropped image if only one size is present. Then specific controls can customize that handling in their subclasses of the cropping control.

Headers and site icons would both use custom subclasses - the key is to make the cropped image control as modular as possible so that only the necessary functions need to be overwritten in child controls. For example, the function that handles the primary action once the process in the modal is complete would be the most common one (typically that's select(), but we'll need a different one here since that takes it to the cropping state). Then child controls can change what's done with the resulting attachment model - whether the setting saves the URL or id or something else, updating any UI in the Customizer controls panel, etc. Being able to get all of the cropping/modal stuff for free and then only needing to change how it's handled when it comes back from the modal will make this incredibly useful for developers, including in core.

The header control will need to be largely rewritten to leverage this, of course, but it'll just reuse the media modal part from the cropped control and handle all of the other stuff (uploaded images, etc.) in the child control.

#16 @celloexpressions
2 years ago

#32765 was marked as a duplicate.

#17 @celloexpressions
2 years ago

Made a good amount of progress here today. It's looking like our best option will be to leave the header image control as-is for now, as it's doing a lot more than we need for a basic cropping control. .4.diff has various code cleanup and refactoring to better leverage the Customizer API, and extracts the cropping ajax to a custom one (although it isn't yet functional). Patch also adds the baseline UI for site icons, using the cropped image control directly. I'm currently waiting for the settings version to be committed so that we can use the same functions for the output with only the administration layer being done in two places.

Once the basic cropping control is completed and working well for site icons, we can do a custom site icon control that extends it and adds browser/device previews, including live-updating the favicon in-browser.

The cropping could theoretically be abstracted further to also be used for the settings version, but that's not 4.3 material, as the current Customizer iteration is heavily rooted in the Customizer API. cc @obenland

This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.


2 years ago

#19 @celloexpressions
2 years ago

  • Milestone changed from Future Release to 4.3

#20 @celloexpressions
2 years ago

  • Owner set to celloexpressions
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.


2 years ago

#22 @celloexpressions
2 years ago

  • Type changed from enhancement to task (blessed)

As @obenland mentioned in Slack, we're okay to slide into the first beta here. I'm going to try to get a fully-functional patch worked out ASAP though.

#23 @celloexpressions
2 years ago

  • Keywords has-patch added; needs-patch removed

New patch that fixes this is coming soon on #16434.

This ticket was mentioned in Slack in #core by celloexpressions. View the logs.


23 months ago

#25 @celloexpressions
23 months ago

  • Keywords needs-patch added; has-patch removed

[33154] missed this ticket and introduced WP_Customize_Cropped_Image_Control to support site icons.

Currently, adding a cropped-image control causes a JS error that prevents the Customizer from loading. There are additional implementation issues here beyond that that we need to fix here in 4.3. I'm investigating further on the best solution and will report back with a patch, implementation examples, and a more detailed explanation tonight. Bottom line: the developer experience is severely broken right now.

@celloexpressions
23 months ago

Test plugin/example implementation - background image cropping (with flex width and height).

@celloexpressions
23 months ago

Make the filter for fallback ajax image cropping attachment handling usable, handle attachments for cropped images for the Customizer, fix script dependencies, and clarify Customizer control class inheritance for the cropped-image control.

#26 @celloexpressions
23 months ago

  • Keywords has-patch added; needs-patch removed

There are several major issues here still. 29211.5.diff addresses all of them. This may need some tweaking in terms of the way the code is placed/structured, but we need to address all of the problems here in a fix before this ticket can be closed, and these fixes need to be in 4.3.


Adding a WP_Customize_Cropped_Image_Control currently causes the Customizer to throw a JS error, rendering the entire Customizer completely unusable. Fortunately this fix is simple: we need to ensure that the customize-views script is loaded after the media scripts. Fixed in 29211.5.diff.


The hook introduced in [33154] in wp_ajax_crop_image is not usable because insufficient information is passed to the hook to do anything. You get only the context (which is presumably known and something you'd test against to determine handling) and 0 currently. 29211.5.diff passes the cropped image URL and the original attachment id to this filter, and adds clarification of exactly what the filter does. Specifically, you start with the original attachment id but need to return an attachment id for an attachment object that has the cropped image in it. That means either changing the original attachment or creating a new one. Alternatively, a callback on this filter could presumably bail out of the ajax call and simply return the cropped image URL to the JS client. This is based on the information that is needed for the core site icon implementation of attachment handling after the cropped image is created.

But when cropping is done from a Customizer control, the ajax action needs to give developers a reasonable result by default regardless. Currently there is a vague error that shows up in the media modal after submitting the copping step that is hard to debug, requiring a deep traversal through the intricacies of the media JS and the Customizer's implementation of it, and severely diminishing the developer experience. They're forced to debug what is really a core bug. The Customizer does not require developers to hook into core ajax functions in order for a basic control to work. You add the control and an associated setting and it "just works". Therefore, this filter is only used when an image is being cropped in ajax by a non-core feature or from a Customizer control.


Continuing on from the issue with the filter, we need to implement the attachment-handling part for WP_Customize_Cropped_Image_Control. One of the best features of the Customizer is its API. The developer experience with the Customizer is just as important if not more important than the user experience. As the API grows, we need to ensure that we stay consistent with its established patterns, maximize forwards and backwards compatibility, optimize both ease of implementation and extensibility, and regularly do QA/QC on our API just like we're trying to do with our UI and UX. With UI and UX, it's all about screenshots and user testing. With developer experience, it's all about code examples, test plugins, and developer testing. When we find issues, we need to fix them, especially if we're still in beta and can fix them. A good example of this is the decision in WordPress 4.0 to make Panels a distinct object instead of being a custom section type (implemented as a subclass), as they had initially been developed.

To ensure that the cropped-image control works as intended, we initially included the site icon feature with it in core, as an extension on the basic functionality offered via the API. This allowed us to catch many of the issues in the initial implementation, but we missed some things. To further test our implementation of this new control, this addition to our API and marketable improvement to the developer experience -- the power of the media modal and a core-provided image cropper that allows images to be fine-tuned to countless uses in themes and plugins with minimal developer effort -- I implemented WP_Customize_Cropped_Image_Control in another place where it's been proposed to be used in core: background images.

The reasons for first implementing background image cropping (#32403) in a plugin are explained in the plugin and on the plugin proposal, but I've attached the plugin to this ticket also so that it can be evaluated as an example and used directly for testing. Note that due to some of the oddities of the way this core feature is implemented, the previewing part won't work yet, but the focus here is on the control and the preview will work with other custom cropped-image controls. The code is remarkably minimal for implementing such a historically complex (see: header images) feature, but considering the Customizer, and the developer experience that it offers, it's a reasonable and expected implementation:

add_action( 'customize_register', 'background_image_cropper_register', 11 ); // after core
function background_image_cropper_register( $wp_customize ) {
	$wp_customize->remove_control( 'background_image' );
	$wp_customize->add_control( new WP_Customize_Cropped_Image_Control( $wp_customize, 'background_image', array(
		'section'     => 'background_image',
		'priority'    => 0,
		'flex_width'  => true, // Allow custom aspect ratios and dimensions.
		'flex_height' => true,
		'width'       => 1920, // "Suggested" size.
		'height'      => 1080,
	) ) );
}

After testing this plugin, the critical script dependency issue mentioned above was discovered. Fixing that seemed to allow this control to work exactly as expected - the currently selected image was displayed just like it is with a regular image control, the change button opened the media modal, and after selecting an image, the cropper came up in the modal just like it does with header images. I successfully adjusted the crop handles to select the part of the image that I wanted to use and hit the crop button, expecting the modal to seamlessly close and the cropped image to show up in the Customizer controls and the preview. But instead, after a brief pause, I got a vague error message that the image could not be cropped, leaving closing the media modal as my only option.

This flow is a failure in both user experience and developer experience. For the user, everything almost worked, then hit a major, insurmountable roadblock. Similarly, for the developer, the Customizer API offered its usual power and flexibility... until it wasn't quite able to do what it usually does, crashing and burning in an error message that is difficult to parse and assign as the fault of core or the developer's simple implementation of the typically-easy Customizer API. Even if the developer were familiar with the basics of custom controls in the Customizer, the necessity of their understanding the media js would present a significant barrier to finding the cause of the error and fixing it.

We can, and really must, do better. When you add an image control to the Customizer, users can access the media modal select an image, and see it previewed without any additional work on behalf of the developer. When you add a cropped-image control to the Customizer, it should work just the same. Sure there are a few more optional parameters on the control to deal with the specifics of the cropping to be done, but these only offer more flexibility within the core control before needed to build a custom child control. With 29211.5.diff, the highly backend process of cropping an image is coupled with the very necessary step of creating a new attachment to represent the cropped image and returning that object to the Customizer UI. The switch for context is replaced by direct checks for core features with custom handling, followed by the default handling in the Customizer (complete with filters for use in more customized implementations), and finally followed by the above-mentioned filter for custom handling when a plugin uses this function outside of the Customizer.

This fix allows the API to remain optimized for simplicity and ease of use, while still allowing for more advanced solutions if needed.


One final fix here is to make the cropped-image control extend WP_Customize_Media_Control directly in PHP like it does in JS. The only difference between the media control and the image control is that the media control saves the attachment id while the image control saves the url (and is restricted to images, which the media control does with a parameter). The cropped-image control saves the attachment id to the setting, so it should extend the media control, for clarity. The distinct image control likely wouldn't have existed had the Customizer been introduced after the media manager, and is largely technical debt at this point. Also fixed in 29211.5.diff.

#27 follow-up: @obenland
23 months ago

Cropping doesn't work with touch currently.

#28 in reply to: ↑ 27 @celloexpressions
23 months ago

Replying to obenland:

Cropping doesn't work with touch currently.

This is an issue everywhere we have cropping - needs to be fixed upstream in the imgAreaSelect jQuery plugin. I assumed that wasn't in scope for 4.3, but we should investigate in another ticket regardless.

Still awaiting feedback on 26 and 29211.5.diff.

@obenland
23 months ago

#29 @obenland
23 months ago

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

In 33280:

Customize: Provide a default way to save cropped images.

Allows plugins and themes to use WP_Customize_Cropped_Image_Control without
having to define their own way of saving the cropped image.

Props celloexpressions for initial patch.
Fixes #29211.

#30 follow-up: @markoheijnen
23 months ago

Not so whine but I'm curious why I wasn't mentioned in [33154]. I can only guess that part of my code was used.

#31 in reply to: ↑ 30 @obenland
23 months ago

Replying to markoheijnen:

Not so whine but I'm curious why I wasn't mentioned in [33154]. I can only guess that part of my code was used.

I didn't realize that the patch submitted to #16434 contained parts of a patch from this ticket.

This ticket was mentioned in Slack in #core-themes by celloexpressions. View the logs.


8 months ago

Note: See TracTickets for help on using tickets.