#36412 closed defect (bug) (fixed)
Custom Logo: Not able to "Skip Cropping" or crop non-square from small images.
Reported by: |
|
Owned by: |
|
---|---|---|---|
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 cutWhen 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
ThemustBeCropped()
method inwp-admin/js/customize-controls.js
does the determination
Attachments (5)
Change History (43)
This ticket was mentioned in Slack in #core-customize by mike. View the logs.
9 years ago
#2
@
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.
This ticket was mentioned in Slack in #core by mike. View the logs.
9 years ago
#6
follow-up:
↓ 7
@
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
@
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
@
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
@
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
@
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...
#14
@
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
@
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:
↓ 18
@
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
@
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
@
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.
#20
@
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
@
9 years ago
@obenland 36412.2.diff looks like a great approach!
#22
@
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:
- Both
width
andheight
are set: we need to lock the aspect ratio and let the user select any part of the image. width
andflex_height
are set: user can select anything or skip cropping, we scale the image to the theme specified width after that.flex_width
andheight
are set: user can select anything or skip cropping, we scale the image to the theme specified height after that.flex_width
andflex_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:
↓ 26
↓ 27
@
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
@
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
@
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
@
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.
#29
@
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
@
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
@
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?
#33
@
9 years ago
@adamsilverstein I assume it's a square here because the suggested size is square, which is expected behavior for the API.
#35
follow-up:
↓ 38
@
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.
This ticket was mentioned in Slack in #core by mike. View the logs.
9 years ago
#38
in reply to:
↑ 35
@
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 :)
cc: @westonruter @celloexpressions