Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#36412 closed defect (bug) (fixed)

Custom Logo: Not able to "Skip Cropping" or crop non-square from small images.

Reported by: kirasong's profile kirasong Owned by: obenland's profile obenland
Milestone: 4.5 Priority: normal
Severity: major Version: 4.6
Component: Customize Keywords: has-patch dev-feedback needs-testing commit
Focuses: javascript Cc:

Description

While working on the About page, @melchoyce ran into this:
https://cloudup.com/cJjq9VEjgVP

Uploading the (file to be shortly attached) small logo, the user is unable to make it wider, or select entire image, even though this aspect ratio is okay for the theme.

This looks to happen in both Twenty Fifteen and Twenty Sixteen.

Some notes from @obenland:

"It seems like there are still issues with the image cropper

  • Images that are smaller than the desired width or height get magnified
  • Images that are larger than the desired width or height but don’t pass mustBeCropped() can’t be > resized but actually have to be cut

When one side is flexible, the entire image should be selectable so that it can be sized down.
...
With that paradigm you can never scale down an image that is wider than the desired size of the fixed side.
...
[Skip cropping button is] missed because it determines it has to be cropped
The mustBeCropped() method in wp-admin/js/customize-controls.js does the determination

Attachments (5)

wordpress-logo-hoz-rgb.png (8.9 KB) - added by kirasong 9 years ago.
36412.diff (2.0 KB) - added by adamsilverstein 9 years ago.
36412.mustBeCropped.diff (329 bytes) - added by westonruter 9 years ago.
36412.2.diff (1.8 KB) - added by obenland 9 years ago.
36412.3.diff (2.0 KB) - added by adamsilverstein 9 years ago.
select entire image in cropper when accommodating flex sizes

Download all attachments as: .zip

Change History (43)

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


9 years ago

#2 @kirasong
9 years ago

  • Summary changed from Custom Logo: Not able "Skip Cropping" or crop non-square from small images. to Custom Logo: Not able to "Skip Cropping" or crop non-square from small images.

#3 @kirasong
9 years ago

cc: @westonruter @celloexpressions

#4 @kirasong
9 years ago

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

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


9 years ago

#6 follow-up: @iseulde
9 years ago

Uploading the (file to be shortly attached) small logo, the user is unable to make it wider, or select entire image, even though this aspect ratio is okay for the theme.

This seems to be fixable by adding 'flex-width' => true to the theme.

https://github.com/WordPress/twentysixteen/commit/9846ca2ed111b4590b2f532709317863d13cf5ea
#36318

Looking further.

#7 in reply to: ↑ 6 @azaozz
9 years ago

Replying to iseulde:

This seems to be fixable by adding 'flex-width' => true to the theme.

Right. This is controlled by the theme. As far as I see it works as intended at the moment. Not sure why the logo is limited to certain height and when that value is reached the width is "locked" too. If both flex-width and flex-height are locked, the crop aspect ratio is also locked to the ratio of the supplied height and width.

One way to fix this is to allow arbitrary size logos (may not be a good idea). Another way is to lock the aspect ratio (for Twenty Fifteen that would be a square).

This ticket was mentioned in Slack in #core-editor by azaozz. View the logs.


9 years ago

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


9 years ago

#10 @kirasong
9 years ago

@azaozz @iseulde @obenland: My understanding is that this is more of a bug regarding images being too small to be treated properly by the customizer than a theme bug. Do I misunderstand, or are you just suggesting a workaround because we're close to release?

@iamtakashi @karmatosed @westonruter @celloexpressions

#11 @iamtakashi
9 years ago

In #36318, allowing flex-width was thought unnecessary. But what is the drawback of allowing both to be flexible when a theme accommodates a logo with any aspect ratio? To me, as a person who haven't worked on the cropper before, it makes more sense to have both flexible.

#12 @adamsilverstein
9 years ago

In 36412.diff:

  • Address the case where flex-height is true and a short image is uploaded: extends the cropping tool to full image size.
  • Also addresses the case where flex-width is true and a narrow image is uploaded, again extending tool to full image size.

Needs more testing; still need to test with other flex options: both off, both on...

#13 @adamsilverstein
9 years ago

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

Screencasts:

Before:

http://cl.ly/3Q0Y143c0M1i/Screen%20Recording%202016-04-05%20at%2008.03%20PM.gif

---

After:

http://cl.ly/3Q091e2i3S2H/Screen%20Recording%202016-04-05%20at%2009.35%20PM.gif

#14 @adamsilverstein
9 years ago

Note: my patch addresses this issue: "When one side is flexible (and when that side of the image is too small), the entire image should be selectable so that it can be sized down.", it does not change the 'Skip Cropping' button or the logic that shows it.

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


9 years ago

#16 @westonruter
9 years ago

36412.mustBeCropped.diff adds a missing condition to let the crop be skipped if the image is too short. This seemed to be missing because it did handle the condition when the image was too narrow.

#17 follow-up: @azaozz
9 years ago

what is the drawback of allowing both to be flexible when a theme accommodates a logo with any aspect ratio.

No drawback, just the theme should be able to accept "weird" sizes :) We can work on this more in 4.6 and perhaps add some limitations or overrides like 36412.diff (thinking now is too late for that).

Having a skip button is nice too. If arbitrary size is allowed, the whole image will be scaled.

After all this is part of the customizer. If the users chooses "unusual" image (too narrow or too tall) they will be able to see it doesn't look right straight away :)

So the fix here is to add a skip button and add 'flex-width' => true to the theme.

#18 in reply to: ↑ 17 @obenland
9 years ago

Replying to azaozz:

So the fix here is to add a skip button and add 'flex-width' => true to the theme.

I'm afraid it's not that easy. Themes should be able to allow only one side to be flexible. While I haven't tested @adamsilverstein's patch yet, from the screenshot it looks like that is the way to go. The skip button is also a nice addition, but not a fix to the bug.

As I said in Slack, when either side is flexible we should allow arbitrary size/aspect ​selections. That selection would then be downsized to the specified dst hight/width, whichever is not flexible.

#19 @kirasong
9 years ago

For my testing so far, @adamsilverstein's 36412.diff appears to function as expected, although given my unfamiliarity with the code, unsure if it's possible to do in a simpler manner or not (further up in the chain). As many eyes on it as possible would be appreciated.

@westonruter's 36412.mustBeCropped.diff fixes the other half of the bug reported in this ticket, and is pretty straight-forward.

@obenland
9 years ago

#20 @obenland
9 years ago

36412.2.diff attempts to accomplish what 36412.diff does in the controller callback.

It sets a min height/width to avoid images that are too small (unless that side is flexible) and allows to scale images down to specified sizes.

#21 @westonruter
9 years ago

@obenland 36412.2.diff looks like a great approach!

#22 @azaozz
9 years ago

Yes, 36412.2.diff is looking a lot better. The complexity here comes from the fact that we do both cropping and scaling at the same time.

There are still some assumption that are unclear. What are these two lines for? Why do they accept numeric values but turn them into boolean? How are themes supposed to use them?

flexWidth  = !! parseInt( control.params.flex_width, 10 ),
flexHeight = !! parseInt( control.params.flex_height, 10 ),

These are inherited from the header image cropping, but seem to affect logos more.

So what options a theme needs in order to define how an image is cropped and scaled for use as a logo? Logically there should be width and height or min-width, max-width and min-height, max-height.

It sounds like flexWidth should replace min-width, max-width with something like "any width the user likes", but to do that it needs to be exclusive with (fixed) width. Same applies for setting height. So logically themes should be able to set width or flex_width but not both, and height or flex_height but not both. Then we end up with four user cases:

  1. Both width and height are set: we need to lock the aspect ratio and let the user select any part of the image.
  2. width and flex_height are set: user can select anything or skip cropping, we scale the image to the theme specified width after that.
  3. flex_width and height are set: user can select anything or skip cropping, we scale the image to the theme specified height after that.
  4. flex_width and flex_height are set: this is a tough one. The theme has decided to allow *any size* image for logo. We need to have some "sensible" default for that case, perhaps 100x100px or something.

For all the above cases it also makes sense to have min-width and min-height values. These should probably be defaults but can be exposed/changeable by the themes.

Another thing: as width is exclusive with flex_width and height is exclusive with flex_height we don't need all to be set. Logically if width is missing we should assume flex_width, etc.

Also, the x1, y1, x2, y2 settings for imgAreaSelect are for defining the initial coordinates of the crop box. They have no bearing on the actual crop the user would select. Should be used to place the crop rectangle in the middle and make it more visible? Having the crop area stretched to the image edges is probably not best?

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


9 years ago

#24 follow-ups: @azaozz
9 years ago

OK, I agree it is too late at this stage to fix the bigger things like forcing the user to crop an image when cropping is not necessary (when flex_width or flex_height is set, we can scale the whole image, no need of cropping), or the themes always having to set width and height even when they are then overridden with flex_width and/or flex height (code cannot handle not having width or height).

I'm a bit concerned that 36412.2.diff changes the default method, i.e. the changes affect all three places the cropper is used. One thing that's affected is that when setting a site icon and using medium image size (300x400px), the crop rectangle now gets "locked" and cannot be scaned down. When using a large image (3000x4000px) the crop rectangle can be scaled down to 95px (screen resolution).

#25 @celloexpressions
9 years ago

I think 36412.2.diff is definitely a step in the right direction, and I think azaozz's notes are a good starting point for evaluating additional improvements and fixes in a future release.

#26 in reply to: ↑ 24 @obenland
9 years ago

Replying to azaozz:

OK, I agree it is too late at this stage to fix the bigger things like forcing the user to crop an image when cropping is not necessary (when flex_width or flex_height is set, we can scale the whole image, no need of cropping), or the themes always having to set width and height even when they are then overridden with flex_width and/or flex height (code cannot handle not having width or height).

I strongly disagree that these things are "bigger things" or a bug that needs fixing at all. This behavior has been tried and tested for years in Custom Header, and Logos just continue with it and keep both APIs alike. As I said in Slack, Theme authors would be a lot more confused if we changed how width/height and flex-* play together.

#27 in reply to: ↑ 24 @obenland
9 years ago

Replying to azaozz:

One thing that's affected is that when setting a site icon and using medium image size (300x400px), the crop rectangle now gets "locked" and cannot be scaned down. When using a large image (3000x4000px) the crop rectangle can be scaled down to 95px (screen resolution).

Right, for site icon it tries to get the 512x512px we decided to use. Again, going any lower than that with the selector would result in (a) the image being upscaled or (b) the resulting image being too small to create device sizes.

#28 @azaozz
9 years ago

  • Keywords commit added

Opened #36441 and #36442 as follow up.

Right, for site icon it tries to get the 512x512px we decided to use.

In this case 36412.2.diff also improves/fixes setting of a site icon. Looks good here.

#29 @kirasong
9 years ago

36412.2.diff Looks to be working as intended in Twenty Fifteen and Twenty Sixteen. Clearly doesn't fix the skip cropping issue, but I don't mind if that happens in Future Release in #36441.

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


9 years ago

#31 @adamsilverstein
9 years ago

+1 to the new approach @obenland this does a better job of addressing the underlying issue instead of stopping its side effect. Testing now.

#32 @adamsilverstein
9 years ago

This looks good!

One thing I noticed when testing with the original logo test image is when entering the cropper, the select handles were initially the smaller box and i had to manipulate them a bit to select the entire logo. The previous patch resized the initial selector to select the entire image (you can then go down), do you think we should do that here @obenland?

http://cl.ly/0O110B132M0C/Customize_Dev_Test__Just_another_WordPress_site_2016-04-07_15-56-28.jpg

@adamsilverstein
9 years ago

select entire image in cropper when accommodating flex sizes

#33 @kirasong
9 years ago

@adamsilverstein I assume it's a square here because the suggested size is square, which is expected behavior for the API.

#34 @adamsilverstein
9 years ago

Ok, that makes sense. Thanks.

#35 follow-up: @jeremyfelt
9 years ago

Spent some time with 36412.2.diff, and the changes look good.

In Twenty Sixteen, I tested:

  • tall, wide, and square images
  • flex-height, flex-width, and both enabled
  • square, tall, wide ratios in defined width/height

All works as expected. I was able to get the crop tool to disappear (https://cloudup.com/cWka3_cR-Y5), though I confirmed that is not a regression from 4.4.2. Maybe a new ticket for adding a sane min-height/min-width (as @azaozz mentioned earlier)? Even a small default of 5 instead of deleting the property prevents the issue.

In general, +1 for commit unless we want to solve the min height/width issue at the same time.

#36 @azaozz
9 years ago

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

In 37167:

Customizer: fix cropping of small images when setting header image, site icon or logo.

Props obenland.
Fixes #36412.

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


9 years ago

#38 in reply to: ↑ 35 @azaozz
9 years ago

Replying to jeremyfelt:

In general, +1 for commit unless we want to solve the min height/width issue at the same time.

Thinking we should stick with 36412.2.diff as most people tested it. Adding default minWidth/minHeight seems very straightforward, but probably better to avoid even the smallest possibility of a regression at this time :)

Note: See TracTickets for help on using tickets.