WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#36318 closed defect (bug) (fixed)

Twenty Fifteen: Custom logo can be distorted unexpectedly

Reported by: iamtakashi Owned by: obenland
Milestone: 4.5 Priority: normal
Severity: major Version: 4.6
Component: Customize Keywords: has-patch needs-testing commit
Focuses: Cc:

Description

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.

https://cldup.com/R4ZpvCGXea.png

https://cldup.com/_NhTppK6cI.png

We also need flex-width to be allowed.

Attachments (4)

36318.diff (449 bytes) - added by iamtakashi 4 years ago.
Twenty Fifteen: Allow flex-width for Custom Logo
36318.2.diff (2.0 KB) - added by obenland 4 years ago.
36318.3.diff (2.6 KB) - added by obenland 4 years ago.
Screen Shot 2016-03-29 at 5.29.20 PM.png (69.7 KB) - added by obenland 4 years ago.

Download all attachments as: .zip

Change History (33)

@iamtakashi
4 years ago

Twenty Fifteen: Allow flex-width for Custom Logo

#1 @iamtakashi
4 years ago

With 36318.diff:

https://cldup.com/Jyh-bUy-Tl.png

https://cldup.com/lH-lG0sInY.png

#2 @iamtakashi
4 years ago

  • Keywords has-patch added

#3 @celloexpressions
4 years ago

  • Component changed from Bundled Theme to Customize
  • Keywords needs-patch added; has-patch removed

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.

#4 @celloexpressions
4 years ago

  • Severity changed from normal to major

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.

#5 @obenland
4 years ago

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

#6 @celloexpressions
4 years ago

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
4 years ago

#7 @obenland
4 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

@celloexpressions How does 36318.2.diff look?

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


4 years ago

#9 follow-up: @mikeschroder
4 years ago

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.

#10 @karmatosed
4 years ago

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

#11 in reply to: ↑ 9 @iamtakashi
4 years ago

36318.2.diff works for me too.

Replying to mikeschroder:

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

#12 @ocean90
4 years ago

  • Keywords commit added

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

#13 @celloexpressions
4 years ago

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.

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


4 years ago

@obenland
4 years ago

#15 @obenland
4 years ago

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

#16 @mikeschroder
4 years ago

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

#17 @karmatosed
4 years ago

This looks good for me and signing off for themes.

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


4 years ago

#20 @westonruter
4 years ago

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

#21 @celloexpressions
4 years ago

Looks good to me also.

#22 @iamtakashi
4 years ago

LGTM too. Thanks for the fix!

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


4 years ago

#24 follow-up: @DrewAPicture
4 years ago

  • Keywords commit removed

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.

#25 in reply to: ↑ 24 @obenland
4 years ago

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:

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


4 years ago

#27 follow-up: @DrewAPicture
4 years ago

  • Keywords commit added

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

#28 in reply to: ↑ 27 @obenland
4 years ago

Replying to DrewAPicture:

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

Awesome, thanks again for taking a look!

#29 @obenland
4 years ago

  • Owner set to obenland
  • Resolution set to fixed
  • Status changed from new to closed

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.

Note: See TracTickets for help on using tickets.