Updated: Comment #0
Problem/Motivation
When setting up a picture mapping with the 3 default image styles (Small, Medium, Large), a problem arises because those three image styles allow upscaling.
The problem: when the source image is smaller than one or more of the image styles (e.g. a 60x60 image), then due to the allowed upscaling, responsive image magic is applied at all three breakpoints: the small image is upscaled to Small (100x100), Medium (220x220) and Large (480x480). This is inappropriate.
Not only does it look crappy, it also results in pointless image derivatives being generated. I realize this is somewhat of an edge case, but the experience is very unpleasant.
Proposed resolution
Make the Picture formatter smart enough to only apply those image styles (with associated breakpoints) that don't cause upscaling.
Remaining tasks
TBD.
User interface changes
None.
API changes
None.
Related Issues
#2061377: [drupalImage] Optionally apply image style to images uploaded in CKEditor 5
Comment | File | Size | Author |
---|---|---|---|
#27 | i2123225-27.patch | 3.25 KB | attiks |
#22 | node-2123225-22.patch | 1.41 KB | Risse |
#20 | node-2123225-20.patch | 1.41 KB | bradklaver |
#7 | drupal8-picture.module-imagestyle_default_no_upscale-2123225-7.patch | 1.31 KB | Jelle_S |
Comments
Comment #1
attiks CreditAttribution: attiks commentedIf I understand correctly, you mean that picture needs to figure out the max dimensions as defined by the image style and make sure that the breakpoint does not cause any upscaling?
So given a breakpoint "all and (min-width: 560px) and (max-width: 850px)" this might be doable, but given a breakpoint "all and (min-width: 22em) and (max-width: 38em)" this will get pretty nasty?
Comment #2
Wim Leers#1: The nice thing is that we don't need to look at breakpoints, which could indeed be em-based, but at the image styles, which are always px-based :)
Comment #3
attiks CreditAttribution: attiks commented#2 Two options
The other option is that people are smart enough to not map small images to 'big' breakpoints. I understand the unpleasantness but I'm afraid this will complicate picture module.
Comment #4
Wim LeersWell, either there must be a huge disclaimer for the Picture module, to explain that you should not ever check the "Allow Upscaling" checkbox for the "scale" effect, and we should disable upscaling for Drupal 8's default image styles.
That, or Picture module should indeed handle all this additional complexity, simply because it causes terrible results.
(I just demoed a prototype for #2061377: [drupalImage] Optionally apply image style to images uploaded in CKEditor 5 to Angie and this problem was the very first thing she asked about, even though I demoed with a large image and did not even hint at this issue!)
Comment #5
attiks CreditAttribution: attiks commented#4 IMOH we just should remove the upscale option all together, or at least make it non-default behavior. I never used the option to upscale any image and I guess most site builders/themers disable is as well.
PS: I would not expect any less from Angie.
Comment #6
Wim Leers+1 for making it non-default.
Comment #7
Jelle_SThe first patch (imagestyle_default_no_upscale) basicly makes the image style not upscale by default (I wasn't sure if I had to change _image_update_get_default_styles() in image.install as well as it is used in image_update_8000(), so I didn't change them there).
The second patch (picture_no_upscale) tries to be smart and see whether or not the images will be upscaled. If so, the original images will be used.
Couple of notes:
Let's see what the bot has to say about these
Comment #8
Jelle_Strailing spaces... As said in my previous comment this patch will not pass tests anyway, so I'm not going to fix that right now :-)
Comment #9
Wim LeersThe
imagestyle_default_no_upscale
patch looks fine :) You don't want to modify people's image styles in the upgrade path — that would amount to data loss. So this is good to go AFAICT.RE: the second patch, I'm not sure I understand the second bullet. In general, it's not very clear what it does. I assume this is about handling the case of upscaling being enabled and preventing that from being applied to a small image?
I wonder what attiks thinks about this approach.
Generally though, it feels like we should only do one of the two things. And I'm inclined to agree with what attiks was saying in #5: just changing the default image styles to not use upscaling should be fine. If people choose to use upscaling *and* Picture module, it's their responsibility.
Comment #12
Jelle_SOk, to clarify on the second bullet:
For example:
Say you have an image style 'Large' and added the 'Desaturate' effect and the scale effect (480x480, upscaling allowed). Then you mapped this image style to the 'wide' breakpoint of Bartik. With this patch, when this breakpoint is active, following scenarios occur:
The image derivative that the user will get to see will be scaled to 480x480 and desaturated.
The user will not see a derivative, but the original image (size 400x300 and not desaturated)
I hope that clarifies things.
Comment #13
RainbowArrayIt seems to me that the default no upscale approach is sufficient.
Picking the right image dimensions for a particular breakpoint isn't simple. It's not just a matter of checking if the image's width (e.g. 400x400) matches up with a breakpoint (max-width: 400px), because in most scenarios the image's width won't be taking up the entire width of the viewport, which is what the media query for the breakpoint is testing. So to test if upscaling should be disallowed, you'd really need to know the percentage of the viewport that an image was taking up at a breakpoint, and that's way too complicated, in my opinion.
So I'd suggest the way to go here is to get the default no upscaling patch working correctly and stick with that as a solution.
Comment #14
attiks CreditAttribution: attiks commentedFYI: picture is renamed to responsive_image so this needs a reroll once #2124377-74: Rename "Picture" module to "Responsive Image" module is committed
Comment #15
Eli-TComment #16
attiks CreditAttribution: attiks commented7: drupal8-picture.module-imagestyle_default_no_upscale-2123225-7.patch queued for re-testing.
Comment #18
attiks CreditAttribution: attiks commentedRe-titling this issue, since the easiest solution is to disable the upscale by default
Comment #19
attiks CreditAttribution: attiks commentedGo bot
Comment #20
bradklaver CreditAttribution: bradklaver commentedRerolling Jelle_S's #7 patch that simply sets true to false in the Image style's yml files.
Comment #22
Risse CreditAttribution: Risse commentedRerolled previous patch.
Comment #24
RainbowArrayFailures are in ImageFieldDisplayTest and ResponsiveImageFieldDisplayTest if somebody wants to look into this.
Comment #27
attiks CreditAttribution: attiks commentedtests adapted
Comment #28
attiks CreditAttribution: attiks commentedNo brainer patch
Comment #29
alexpottI think we need a change notice to tell site builders that the default image style provided by core no longer upscale by default. Also what happens when someone sets them to upscale?
Comment #30
attiks CreditAttribution: attiks commentedI think a change notice is a bit OTT, but I'll create one anyway ([#2335637]). The only thing that will happen if somebody enables the upscale is that they end up with ugly images. I think most people disable this setting when using Drupal 7.
Back to RTBC?
Comment #31
alexpottDiscussed with @xjm and who pointed out that this change improves actual site builder experience. @xjm also pointed out the existence of #872206: Add possibility to not upscale for "Scale and crop" effect
Committed 535c590 and pushed to 8.0.x. Thanks!