Custom Logo: Crop UI

[mor10]

When an image is uploaded, the expectation is for a cropping UI similar to what's provided from Site Icon and Header Image to show up if that image does not have the same proportions as what the logo area is defined as. Currently (4.5-beta3-36995), that is not the case, and from what I can tell, the only control of crop factor is at the theme level through the add_image_size() function. From a user experience perspective this is sub-optimal.

I would recommend adding cropping functionality similar to what we have for the Site Icon and Header Image functions to the Custom Logo function.

Not technically a bug, but will be perceived as such by end-users imo.

[westonruter]

This makes sense to me, and incidentally it also seems to resolve #36256.

@celloexpressions @obenland thoughts on 36255.0.diff​?

[celloexpressions]

I don't think logos should support crop by default, because the image being uploaded is theoretically already cropped appropriately as a logo. Additionally, themes can't define the exact dimensions of the requested logo directly (only via the image size and the crop option there). The general principle is currently that themes should be able to handle a variety of logo aspect ratios so that the content (the logo image) can be presented in its intended format rather than needing to be adjusted (cropped) to fit properly in the theme.

If there are problems with the way themes are implementing this, I would recommend revisiting the way themes can specify logo sizes, and finding a way to prevent themes from automatically cropping images. Logos should be presented at whatever size the theme requires, but themes shouldn't be modifying the logo aspect ratio/image content. At most the theme could provide recommended dimensions to design the image to before uploading.

This would also add a step to the user flow whenever a logo is added, asking if the user wants to crop their image rather than setting it as soon as it's selected from the media library. Letting users crop logos doesn't solve the problem here - the logo would then still end up being partially cut off if it were the wrong aspect ratio and would likely need to be redesigned outside of WordPress anyway.

[obenland]

Sure, why not. Initially I also thought that logos are likely uploaded in a format that is ready to use, but why not let folks crop their images if it can be done cheaply. Which it seems is the case.

[celloexpressions]

I'll note that it is not a complicated technical change, but it has significant usability implications, particularly with the flow disruption of an added step. It can also be implemented in a plugin fairly easily in theory. I think it could cause many users to crop images unnecessarily. Would definitely want to see some usability research or targeted user testing.

[mor10]

From a user perspective, this is necessary. While designers and developers may have images that are cropped to fit, most users do not, or do not know how to do it. WordPress has trained its users to assume a crop UI will appear whenever a crop is relevant (Site Icon, Header Image, etc), and the lack of such a UI in this features seems arbitrary.

More importantly, if no crop option is provided, theme developers will resort to automatic hard crop on the back end to ensure the image is output in the correct proportions. For users who do not crop their images in advance, this will appear random and cause frustration.

The larger concern of a crop UI adding an extra step in the user flow is equally true for Site Icon and Header Image, so if that's the conversation we're having, it must be expanded to these features. From a user perspective there is no qualitative difference between these three features, and arguing that the UI is too obtrusive in this instance but not the two other ones is not consistent.

[mor10]

Replying to celloexpressions:

There are a ton of examples of sites and solutions that require a crop for a logo to be displayed, most prominently Twitter and Facebook, both of which crop a logo to a square. IMO the requirement that the feature must allow the user to upload a logo image of any proportion is an unreasonable constraint for theme designers and developers. A logo can be incorporated in a theme in many different ways, and a large number of those ways would require some sort of proportion restriction to avoid layout and/or functionality issues. Furthermore, without restrictions, a user could upload logos with proportions that would create severe layout issues that are then attributed to the theme. I can mock up some examples of this if you like.

I think the solution here is to approach this feature more like the Header Image feature by providing the theme author the option of defining a size and whether or not the width and height of a logo can be flexible. I understand this is not how the feature currently works, but it would provide theme authors with the control they need and uses the ability to configure their logos to fit within those defined parameters. It is also an established methodology in WordPress and a flow the user is familiar with.

To me as a designer, the display of logo is not flexible in the context of the overall design. Its exact position, size, and proportions play a significant role in how the overall theme is designed and laid out, and while flexibility can be added, it is not automatically assumed. Forcing this kind of restriction on me as a designer will ensure I find other ways of restricting the logo display (ie hard-crop on the back end through the custom image size). Removing this option would result in me not using the feature at all.

[TimothyBlynJacobs]

Presenting a user with a crop control for a logo doesn't make any sense. Header images can be cropped because they are images, not graphic design elements. What would a user be expected to do when presented with a crop image control for their logo? Cut their logo in half?

[mor10]

Replying to TimothyBlynJacobs:

Presenting a user with a crop control for a logo doesn't make any sense. Header images can be cropped because they are images, not graphic design elements. What would a user be expected to do when presented with a crop image control for their logo? Cut their logo in half?

You are using a narrow definition of the word "logo". Yes, in a case where a user has a custom properly rendered logo, cropping it would make no sense. In the wild, many users will not have a file of that type. Instead they will have an image or graphic or part of either that they want to use as a logo. In those cases, a cropping feature is important.

Observing how people use existing logo plugins and the header image function, I think this is vital. Making the assumption that people have custom logos is not realistic imo. We need to provide workable options for the average user.

[ashiquzzaman]

I think it'd be better if there's an optional extra crop option available for users, that way they can either simply upload and use as it is without cropping or they can choose to crop the photo if they wish to.

[westonruter]

@mor10 Can you give 36255.0.diff​ a try, both with a custom-logo specifying an image size and not specifying an image size? If an image size is specified for the custom logo, you should see the crop dialog appear if the aspect ratio isn't correct. If there is no image size specified, then the full size should be used in the theme instead (without a crop step).

[mor10]

Replying to westonruter:

@mor10 Can you give 36255.0.diff​ a try, both with a custom-logo specifying an image size and not specifying an image size? If an image size is specified for the custom logo, you should see the crop dialog appear if the aspect ratio isn't correct. If there is no image size specified, then the full size should be used in the theme instead (without a crop step).

Tested, and works as you describe it. This alleviates the immediate problem in a clean way.

[westonruter]

I'm convinced it is better to have this than to not have it, so I'm proposing commit.

[obenland]

Replying to westonruter:

I'm convinced it is better to have this than to not have it, so I'm proposing commit.

Second'd.

[kirasong]

+1

[westonruter]

In some additional testing I found a key problem with [36255.0.diff] specifically in regards to Twenty Sixteen. It defines its logo image size as:

	add_image_size( 'twentysixteen-logo', 1200, 175 );

Notice the very wide width but the absence of the $crop argument. In other words, this image size is indicating the maximum width and maximum height that that the image can be. Nevertheless, in the patch it would present the fixed crop at that ratio, resulting in a very wide image crop, which likely would not be desired. I've improved this in 36255.1.diff​ by taking into account the $crop arg, opting to not use a cropped image control if this is not defined.

[westonruter]

Another option would be to go ahead and use the cropped image control, but then to define the $flex_width and $flex_height args to correspond to the image size's $crop value. See 36255.2-alt.diff​. There's a big problem here though: it seems that if you select a crop that doesn't match the aspect ratio defined in the image size, the cropped image will appear get stretched according to the $width and $height initially defined, not the actual width and height that the user selected during the crop.

[westonruter]

In the crop-image WP Admin Ajax request, it sends along parameters like:

wp_customize:on
id:5291
context:custom_logo
cropDetails[x1]:300
cropDetails[y1]:549
cropDetails[x2]:1336
cropDetails[y2]:1356
cropDetails[width]:1036
cropDetails[height]:807
cropDetails[dst_width]:1200
cropDetails[dst_height]:175
action:crop-image

It seems the problem is that the destination height/width (the defined image size) are overriding the width/height as defined by the user-selected crop region.

[westonruter]

OK, I think I figured out the issue with cropping. When Site Icon was added (#16434) it defined a CustomizeImageCropper which included (r33154):

cropDetails.dst_width  = control.params.width;
cropDetails.dst_height = control.params.height;

This worked fine for an image with a hard crop, such as a square site icon, which is what this new cropper was added for. However, defining the destination width and height is not desired when flex_width and flex_height have been defined. So I believe all that is needing to be done is change the above to:

if ( ! control.params.flex_width ) {
        cropDetails.dst_width = control.params.width;
}
if ( ! control.params.flex_height ) {
        cropDetails.dst_height = control.params.height;
}

See 36255.3.diff​.

Aside: I don't see any WP_Customize_Cropped_Image_Control instances in Core even making use of flex_width or flex_height.

@obenland @celloexpressions please review, as you both were the Site Icon developers.

[babbardel]

What would happen when a different file format is uploaded (eg. SVG or GIF)? Should this functionality be implemented by a specific list of formats (e.g. PNG, JPG)?

[obenland]

Replying to westonruter:
Makes sense.

Would it make sense to always offer the cropper, not just when there is an image size? If there is no image size the cropper could be fluid, without a pre-set ratio.

[mor10]

Replying to obenland:

Replying to westonruter:
Makes sense.

Would it make sense to always offer the cropper, not just when there is an image size? If there is no image size the cropper could be fluid, without a pre-set ratio.

From my perspective, yes. Cropper has value, especially when the proportions are undefined. Provides room for experimentation in context.

[westonruter]

@obenland @mor10 thanks! OK, please give 36255.5.diff​ a try. There are 3 different scenarios that can happen now:

1. Theme without defined image size: suggested size is the medium image width/height, with flex for custom ratios.
2. Theme with defined image size without hard crop (allows width/height flex).
3. Theme with defined image size with a hard crop (disallows width/height flex; keeps aspect ratio).

Please let me know if if I'm missing anything else, and if the medium size is a good fallback image size.

[celloexpressions]

I think the missing flex-width/height checks were an oversight, since we didn't have any places using it in core yet (but we wanted to provide that functionality in the control). This is definitely a place to use both flex width and flex height.

That being said, I'm starting to think that the way logo size is defined with add_theme_support() is problematic - by using a separately-defined image size. If we are going to support crop, wouldn't it be better to let the theme directly specify the desired width and height of the logo, and also the flex-height and flex-width arguments, in the add theme support arguments? For example,

add_theme_support( 'custom_logo', array( 
     'width' => 1200, // Suggested width, because flex_width.
     'height' => 175, // Required height, because not flex_height.
     'flex_width' => true,
     'flex_height' => false,
) );

Are there any benefits to defining logos as an image size that could end up with an arbitrary forced crop, instead of defining the arguments that get passed to the customizer control directly? With the above, the theme can basically say that it can accommodate images of any aspect ratio as long as either the height or the width is fixed at a size (one flex_ is true), images of any size (both flex_s are true), or only a specific aspect ratio and size (both flex_s false). This would also avoid creating an extra image size for every image uploaded to a site that isn't the logo.

As an aside, I understood the reasoning for having a separate logo option in addition to header images and site icons was that it would be used specifically for traditional specifically-designed logos, not general images. Header images and site icons are more appropriate for things that could be cropped from another image, but if logos are intended to be able to operate the same way I really don't see why we would want to have a separate logos feature also (keeping in mind that it's not difficult for themes to add the appropriate image controls that they may need in the Customizer).

[celloexpressions]

We definitely need to make a decision regarding the potential change (above) to how the add_theme_support arguments work before the first RC.

[westonruter]

@celloexpressions so then really what you've identified is that the theme support feature for custom-logo should have better alignment with the existing feature arguments for custom-header? In Twenty Sixteen, the custom header support is added via:

<?php
add_theme_support( 'custom-header', apply_filters( 'twentysixteen_custom_header_args', array(
        'default-text-color'     => $default_text_color,
        'width'                  => 1200,
        'height'                 => 280,
        'flex-height'            => true,
        'wp-head-callback'       => 'twentysixteen_header_style',
) ) );

So the custom-header supports flex-height and flex-width arguments, as well as width and height arguments. You're saying that these should be used for Custom Logo instead of adding a custom image size and referencing its name. I suppose the feature could allow the size to be defined via a size argument as it is now, or the dimensions could be defined via attributes like custom-header accepts.

As for how custom logo relates to custom header, I'll defer to @obenland.

[obenland]

I don't see a reason why we shouldn't allow themes to specify it as suggested in 25 in addition to optionally define a custom image size.

[mor10]

@westonruter @celloexpressions

Yes! This is precisely what I was hinting at in my earlier comment. Theme authors should be able to define the size, width, height, and flexibility of the logo to the design specs. This is in line with other features, and provides the theme author with detail control to choose how a logo is incorporated most appropriately in the design.

[coreymckrill4ttf]

Incidentally, with @westonruter 's patch for CustomizeImageCropper, I suspect #34851 can now be marked as duplicate.

[celloexpressions]

Yes, the arguments I suggested are more inline of what would be needed for something that supports cropping/uses the cropped image control, which is based on header images. Sounds like we should move in this direction now.

I don't see much of a reason to also support image sizes other than providing back-compat for themes that have already adjusted in anticipation of 4.5, but I may be missing something. Seems like not a great practice to be generating that additional size for every image uploaded, unless the theme is also using that exact size for something else like featured images. We should update Twenty Fifteen and Twenty Sixteen accordingly.

[mor10]

Replying to celloexpressions:

I don't see much of a reason to also support image sizes other than providing back-compat for themes that have already adjusted in anticipation of 4.5, but I may be missing something.

IMO this is a non-issue: As the feature is not finalized, any inclusion in an existing theme at this point is experimental. Providing a fallback for in-development experimental features in a finalized product makes no sense.

[celloexpressions]

Rough (untested) pass at revising the way the theme support works with image sizes, and adding crop support. Also updates Twenty Fifteen.

[celloexpressions]

I uploaded a rough, untested first pass at adding crop support and adjusting the theme support arguments accordingly, as well as updating Twenty Fifteen. Twenty Sixteen will need to be uploaded as well.

[mor10]

Replying to celloexpressions:

I uploaded a rough, untested first pass at adding crop support and adjusting the theme support arguments accordingly, as well as updating Twenty Fifteen. Twenty Sixteen will need to be uploaded as well.

I'll test this in Popper tomorrow and report back with a link so you can test as well.

[westonruter]

In 37042:

Customize: Add support for flex sizes in CustomizeImageCropper.

Ensures that $flex_width and $flex_height as specified on WP_Customize_Cropped_Image_Control will be honored when a crop is saved.

See #36255.
Fixes #34851.

[mor10]

@celloexpressions

I've tested the patch and it works as expected in Popper (​https://github.com/mor10/popper/tree/custom-logo)

One note: For whatever reason, the Customizer preview is not updating after this change. It is possible this is caused by some other issue in my install, but worth checking nonetheless.

Also note, the particular implementation in this theme is a bit odd because I have to provide a fallback for older functionality that used the site icon as the theme logo.

[celloexpressions]

I don't have time to investigate, but I can confirm that the preview doesn't work with my latest patch using Twenty Fifteen. May be unrelated, though.

[celloexpressions]

Maybe the new JS in the media controls that sends info to the preview needs to be added to the cropped image control in JS, since that probably overrides the function that sends it?

[westonruter]

I'm seeing a PHP warning with the patch applied:

Warning: Illegal string offset 'id' in /srv/www/wordpress-develop/src/wp-content/plugins/jetpack/modules/theme-tools/site-logo.php on line 30

[westonruter]

Nevermind. It was a Jetpack problem that went away when I updated to the latest.

[westonruter]

In 37066:

Customize: Replace site logo with custom logo terminology, fixing failure to preview logo changes.

Fixes regression introduced in [37040] which was from a patch that did not include the terminology change.

See #36255.
Fixes #35855.

[westonruter]

@mor10 @celloexpressions unrelated indeed. I just fixed this regression in [37066].

Replying to celloexpressions:

I don't have time to investigate, but I can confirm that the preview doesn't work with my latest patch using Twenty Fifteen. May be unrelated, though.

[celloexpressions]

Cross-posting from slack for reference:

• Re: supporting size - should core be providing back-compat for Jetpack? It won't break anything, the size just wouldn't be exactly what the theme selected but the feature still works. Also, this encourages a potentially wasteful practice of registering an image size that would only be used for one image but created for every image uploaded to the site.
• 36255.diff​ looks like it would require explicitly specifying both flex-width and flex-height, I think it would be better to let it work if only a width and height are specified and fall back to defaults (true) for those two if they aren't included.
• 36255.diff​ - I don't think we should allow control args to be overwritten be the theme support array (via wp_parse_args), only the four size options. Otherwise, the add theme support starts to become more like a customizer add_control, and things can be changes that shouldn't be from there. Also seems like a potential security issue to be able to pass anything else in there.
• We should update Twenty Fifteen to not use size due to the unnecessary image generation and to allow flex-height, per 36255.6.diff​.

[obenland]

I don't feel very strongly about supporting image sizes. I guess Jetpack could do that too. The latest patch address all other concerns raised.

[obenland]

In 37077:

Customize: Bring custom-logo args closer to custom-header.

Allows themes to specify the desired width and height of logos, and whether
that is flexible or not. Has the benefit of not having to generate a logo-sized
file for every image uploaded.

Props westonruter, celloexpressions.
Fixes #36255.