Twenty Fifteen: Custom logo can be distorted unexpectedly

[iamtakashi]

In [37077] on #36255, flex-height was allowed but not flex-width. This causes logo to be unexpectedly distorted.

Steps to reproduce

• Upload a logo image, say 500px by 500px.
• Leave the cropping tool as it is and click "Crop Image" button.
• Save the customizer.

What happens
The logo is distorted. This is because of flex-height that respects the size of cropped image, in this case, 500px because we didn't touch. But the width is restricted by the suggested size, 248px.

We also need flex-width to be allowed.

[iamtakashi]

Twenty Fifteen: Allow flex-width for Custom Logo

[iamtakashi]

With 36318.diff:

[celloexpressions]

I think this is a core bug. If flex-height is true and flex width isn't, any aspect ratio should be allowed including square. The fixed width results in an image that's the appropriate width for the container (controlling size but not aspect ratio in this case). The cropper should never stretch an image as is happening here. Instead, if the image is too large but otherwise the crop isn't changed it should only be resized, proportionally in both directions.

The current usage in Twenty Fifteen is correct for its use-case, but the Customizer is not handling these arguments correctly.

[celloexpressions]

There is also what looks like a 500px max width being set when working with the cropper with Twenty Fifteen, for no apparent reason. Something is fairly severely messed up with the cropper right now.

[obenland]

@celloexpressions Any chance this could be cause by [37042]? the ajax request only sends dst_width and not dst_height. Reverting [37042] doesn't seem to change that for me for some reason though. Not sure what I'm doing wrong.

[celloexpressions]

Yeah looking more closely, I think the logic in [37042] is wrong. dst_height and dst_width need to be set regardless of the flexibility. We should add an else statement for each that proportionally sets those values based on the cropped aspect ratio, because otherwise the original value is used.

The maximum allowed width issue is likely cause by something else in there.

[obenland]

@celloexpressions How does 36318.2.diff​ look?

[kirasong]

36318.2.diff​ fixes the issue for me on Twenty Fifteen and related issues on trunk Twenty Sixteen.

Just a note that we'll want to make sure we're sync'd with @iamtakashi to make sure there wasn't any change made with Twenty Sixteen in ​the crop support commit that should be stepped back due to this fix.

[karmatosed]

I can also confirm that this fixes Twenty Fifteen for me now.

[iamtakashi]

36318.2.diff works for me too.

Replying to mikeschroder:

Thanks for the note. I'll take care of Twenty Sixteen in Github.

[ocean90]

36318.2.diff​ looks good to me, but let's not align the var declarations.

[celloexpressions]

Looks good to me as well. Does it work with flex_width and flex_height are both true? In that case it should be using exactly the size that the user crops to based on the original image size.

[obenland]

36318.3.diff​: Doesn't constrain image size if both width and height are flexible.

[kirasong]

@iamtakashi @celloexpressions @karmatosed @westonruter How does 36318.3.diff​ look to you?

[karmatosed]

This looks good for me and signing off for themes.

[ocean90]

36318.3.diff​ Looks good.

[westonruter]

@mikeschroder Looks/works good to/for me as well.

[celloexpressions]

Looks good to me also.

[iamtakashi]

LGTM too. Thanks for the fix!

[DrewAPicture]

I'm still seeing the distortion behavior on a 512x512 image with 36318.3.diff​.

A 248x512 image is getting generated even though I'm cropping it square. If it helps, I'm not actually touching the cropper UI at all, simply clicking the Crop button because it's my only choice.

[obenland]

Replying to DrewAPicture:

A 248x512 image is getting generated even though I'm cropping it square.

Hm, I can't reproduce that. What does your Twenty Fifteen theme support look like?

This is what the cropper sends to the callback on my end:

[DrewAPicture]

@obenland: Yeah, idk what was going on last night, maybe I had multiple patches applied or something. Either way, just tested again and it works as expected. Do it.

[obenland]

Replying to DrewAPicture:

Either way, just tested again and it works as expected. Do it.

Awesome, thanks again for taking a look!

[obenland]

In 37113:

Customize: Respect aspect ratio on cropped images.

Takes into account whether the control supports flex_width and/or
flex_height and adjusts destination measurements accordingly.

Fixes #36318.