Add cropping to default image styles

[stpaultim]

Description of the need

PROBLEM: Our current default image styles in core specify height and width, but don't crop to match these dimensions.

This became a issue with issue: #4903. The PR for that issue contains pre-cropped images that display using the large image style. But, it was noted that if someone adds another image to a new node, that does not match the exact proportions of the existing images, it might not match the height of existing images.

Regardless of issue #4903 - it seems reasonable to me that two images with an image style of "Large (800x600)" would display at the same size. It seems like it would be strange and confusing to a new user that they do not.

The same logic applies to medium and thumbnail images. The description of the image style does not match the output.

Proposed solution

Change the default for image styles from "Crop" to "Scale and Crop" and crop them to the exact sizes listed.

I never thought of it before, but it is a bit weird that an image displayed using the image style "Large (800x600)" might or might not be 600px high.

Alternatives that have been considered

We could change the name/label of image styles to better reflect the actual output they produce.
If we don't add the cropping to these images, it might be good to change their names to (or something else that is actually accurate):

• Large (800wide)
• Medium (300wide)
• Thumb (100wide)

Or

• Large (800wide or 600high)
• Medium (300wide or 250high)
• Thumb (100wide or 100high)

Additional information

I used the Sample Animal Content module to quickly add some nodes with images of different sizes. I then created a grid of all images using the "Medium (300x250)" image style:

This is what it looks like. Would it not be reasonable to expect that all these images would be the same size?
Remember - all of these images are using an image filter that is labelled "Medium (300x250)

[indigoxela]

Change the default for image styles from "Crop" to "Scale and Crop"

Uh-oh, that will have impact on existing sites. Changing an images style that's already in use seems like a dangerous idea.

[olafgrabienski]

Changing an images style that's already in use seems like a dangerous idea.

I agree, the impact on existing sites would be huge. Let's not change the configuration of the default styles. In my opinion it's a good idea to add new styles - should we discuss that here or in a separate issue?

it is a bit weird that an image displayed using the image style "Large (800x600)" might or might not be 600px high

In a way, it is weird, but it's not exactly wrong. Such an image will become 800px wide OR 600px high. Examples:

• 960 x 600 becomes 800 x 500 => width changed to 800 (height respectively)
• 600 x 960 becomes 375 x 600 => width unchanged, height changed to 600

If we don't add the cropping to these images, it might be good to change their names to: Large (800wide) [...]

I guess, names related only to width wouldn't work, see the second example above. Other ideas? (If we want to rename the styles, let's also consider if changing the machine names will have side-effects for existing sites.)

[jenlampton]

Uh-oh, that will have impact on existing sites. Changing an images style that's already in use seems like a dangerous idea.

It doesn't have to. We could change the image styles that are initially installed without adding an update hook that would affect existing sites.

[stpaultim]

Uh-oh, that will have impact on existing sites. Changing an images style that's already in use seems like a dangerous idea.

I'll need further clarification of how this effects existing sites. I certainly thought about this problem, but assume that we are safe. Image styles are stored in config with default config files provided by core. It is my understanding that these config files are copied into the config directory during installation and never used again.

If my understanding is correct, then this will not impact existing sites - which clearly it should not and can not.

Is there something about my understanding that is not correct? @indigoxela or @olafgrabienski ?

[stpaultim]

In thinking about it, the only danger that I can imagine is if anyone has edited their image styles and then wants to revert to the original image styles that came with core. Is there a "revert" option built in to core for edited image styles?

[ghost]

It doesn't have to. We could change the image styles that are initially installed without adding an update hook that would affect existing sites.

True, but considering people are used to how this works currently, I'm not in favour of changing this at all. If we changed the default image styles to be scaled and cropped, existing users of Backdrop setting up new sites would be surprised by this (and maybe not in a good way).

I would be in favour of changing the names of the default styles and/or adding description text to avoid confusion, but in a separate issue.

[indigoxela]

Hm. What are we actually trying to fix here?

My understanding: @jenlampton opposes to adding another image style that's very similar to an existing one. So it has been suggested to alter the existing one instead. But there are concerns.

Then it has been suggested to change the names - currently I fail to understand the benefit of that.

But actually I don't think we need another one. If people upload custom images into the new content type, it should be their decision whether the aspect ratio should be as-is, or if images should be cropped. We just could use "large" and save us some discussions about naming and implementation.

considering people are used to how this works currently, I'm not in favour of changing this at all.

TBH, same here.

[stpaultim]

@indigoxela There are two questions that need to be answered before this issue is settled.

1. Can this be done in backward compatible way. In a previous comment you suggested that this was not backward compatible, but I explained why I think it is. I would like to settle this question, because it is more objective and question (2) will be subjective. We don't need to consider question (2), if the answer to question (1) is that this can not be done in backwards compatible way.

Based upon my explanation above, do you still believe this this will effect existing sites?

2. Assuming that this can be done, we can then address the issue of whether or not it should be done. I would argue that this issue makes sense independent of issue Provide "Cards" by default – a new hidden-path content type. #4903. I think I made the argument pretty clear above (including the problem that we are trying to solve), but would rather not debate it further, if the solution is NOT backward compatible.

[ghost]

1. "Can it be done?" Yes, the default config can be updated to 'Scale & crop' which'll only affect new sites.
2. "Should it be done?" No, I don't think so (as explained above).

[ghost]

Marking this as needing more feedback/discussion before we test/review the PR.

[indigoxela]

"Can it be done?" Yes, the default config can be updated to 'Scale & crop' which'll only affect new sites.

Hm, yes and no... Consider the "Overridden / Revert" functionality. If people with existing sites and overridden image styles revert, they'll get a different style than was there initially. This may not be a big problem, but is it worth it, anyway?

[ghost]

Good point. Perhaps a new issue should be opened proposing that default config for stateful (i.e. can be overridden/reset) things be copied somewhere upon install, then that copy is used for resetting and will therefore always be as it was when installed. The default config in the module is then free to be updated and will then truely only affect new sites...

[stpaultim]

Hm. What are we actually trying to fix here?

I thought I did a pretty good job of explaining it the first time in the original post, but I've updated the original post to hopefully add clarity, including an improved screenshot to help demonstrate what I think the problem is. The Medium image style says (300 x 250) but it does not produce images that are 300 x 250. It does not even produce images that are all the same size.

I don't think that this would make sense to someone using image styles for the first time. In retrospect, it does not make sense to me now. It does not confuse me anymore, because I've been living with this inconsistency for a very long time and gotten used to it.

Why do we put dimensions in the name of the filter if the filter is not outputting those dimensions?

To fix this problem (if anyone else agrees it is a problem) we could:

1. Change the default behavior of these image styles on new sites to match the label
2. We could alternatively change the label to be less confusing/misleading.

I have proposed changing the behavior to something that I think would be a better default behavior (if we can do so in backward compatible way), BUT I'm also open to changing the labels.

[olafgrabienski]

... the impact on existing sites would be huge.

Ok, I see, backwards compatibility doesn't seem to be an issue, thanks for the clarification, and sorry for the noise. I must admit, I don't fully understand these technical aspects, therefore a related question: Would changed default styles also work for sites upgraded from Drupal 7 with D7 default image styles (IIRC the same as in Backdrop)?

[indigoxela]

I thought I did a pretty good job of explaining it

Yes, of course you did, my question/concern should have been clearer. 😉

This issue's title says "Add cropping to default image styles", but the discussion quickly it turned into a "rename all image styles", and then another suggestion popped up to change the whole base logic of image styles config (keep a copy of the initial config) 😕

The current issue title suggests, that we should add cropping to all default image styles - not only the "large" style. (Expect some opposition.)

Actually we don't even need cropping for the cards content type (I hope I explained at least that reasonably).

So why all that over-acting? (Sorry that I call it that way, but it really feels like that for me 😉)

[olafgrabienski]

Actually we don't even need cropping for the cards content type (I hope I explained at least that reasonably).

I don't agree with this, but I think it's time to decouple the general image style discussion from (possible) needs of the cards content type. While the cards PR should make it in the next minor release, this issue here may need more time.

[indigoxela]

I don't agree with this, ...

Aha, I failed to properly explain why I think it's not necessary. 😉

... but I think it's time to decouple the general image style discussion from (possible) needs of the cards content type.

Yes, please!

Would changed default styles also work for sites upgraded from Drupal 7 with D7 default image styles

Hm, interesting point. 🤔 The styles are pretty different in D7 ("large" is tiny...). What happens if they "Revert"? Never tried that so far.

[olafgrabienski]

I've updated the original post to hopefully add clarity, including an improved screenshot to help demonstrate what I think the problem is

Thanks for updating the issue description, very helpful. I see the problem, and I agree that it would be good to fix it somehow. I'm not yet sure which way I prefer.

Actually, I have the impression the task is more general, something like "Overhaul the default image styles", considering more aspects of the existing image styles and of our needs. To give an example for an aspect which seems to be missing so far: the current default styles don't change the aspect ratio, which isn't only a problem, but (also) a feature, as they work for portrait and landscape images.

[stpaultim]

It has been pointed out to me that some folks might feel as if this issue was opened in an attempt to rush something through. I'm sorry if I gave that impression or if I "overacted" in anyway (that was never my intention). Again, this issue was opened so that we don't need to rush a decision. This issue was opened specifically to break this decision away from #4903 and to ensure that image styles don't hold up that issue.

[quicksketch]

I'm glad this was separated from #4903, but I'm of similar opinion that changing the default image styles specifically to address the specific use-case of the new cards is not the best solution.

Separately, adding crop to thumbnails I think is problematic. Seeing the entire image in the image browser can be important when selecting an image, as it is when showing the thumbnail on the node form.

I do think we need to overhaul the default styles at some point however, like @indigoxela points out, the "large" image style is quite small by today's standards.

[stpaultim]

I'm glad this was separated from #4903, but I'm of similar opinion that changing the default image styles specifically to address the specific use-case of the new cards is not the best solution.

I agree 100%. This PR was from the start intended to address issues that extend beyond the scope of issue #4903. I think I will need to create a new issue that ONLY addresses the issue raised by #4903 so that we can have a clean discussion here about the merits of this PR without continually linking it to #4903. :-)

I do appreciate the following suggestions - if we do proceed with updating our default image styles:

1. A larger default size for the large image style
2. Possibly treating the thumbnail differently from the other two (not cropping this one, even if we decide the crop the other two).

[stpaultim]

I've opened an issue where folks can focus on a solution specific to #4903. My preference would be to make the larger changes to the default image styles described in this issue (making that issue unnecessary).

However, if you are opposed to overhauling image styles in general and would like to discuss a solution specific to the problem of #4903, here is another issue: #5593

[stpaultim]

We have added a single cropped image style to core AND this idea didn't get much traction. I'm going to close this issue.