The input-field for Sizes (keyed-styles-responsive-imageviewport-sizing-1x-sizes) can only be 128 characters long. There are situations where this is not enough. I would suggest making this limit relatively high (1024?), so we don’t run into limitations too soon here.
For example, if you have a few breakpoints and/or screen resolutions to support you might end up with a rather complicated media query with more than 128 characters. I mainly use it for resolutions switching, f.i. (min-resolution: 192dpi) and (min-width: 170px) 386px, (min-width: 170px) 193px, (min-width: 768px) 18vw, (min-width: 480px) 30vw, 48vw
, which is 135 characters long. And I can easily make up even more complicated ones. :-)
The field is defined in src/ResponsiveImageStyleForm.php on line 156 ($form['keyed_styles'][$breakpoint_id][$multiplier]['sizes']). There is no #sizes or #maxlength given here, so is 128 a sort of default then?
Comments
Comment #2
keeganstreet CreditAttribution: keeganstreet at Deloitte Digital commentedI think you're absolutely right @rolfmeijer. Its easy to go over 128 characters when setting up source size list. I hit this issue with a 138 character value:
(min-width: 77.5em) 766px, (min-width: 64em) calc(70vw - (3 * 34px)), (min-width: 30em) calc(100vw - (2 * 34px)), calc(100vw - (2 * 17px))
Here's a patch to remove the limit and also change the input field to a textarea for easier authoring. I don't think its appropriate to enter an arbitrary limit of 1024. The HTML spec does not specify any maximum length of the sizes attribute - http://w3c.github.io/html/semantics-embedded-content.html#element-attrde...
Comment #3
rolfmeijer CreditAttribution: rolfmeijer at Dutch Open Projects commentedYes, a textarea is a great idea and makes a lot more sense.
Comment #4
FiNeX CreditAttribution: FiNeX as a volunteer commentedHi, I've tried the patch and it correctly works. Please commit :-)
Comment #5
tstoecklerSo the textarea element does not have a
#maxlength
property, so if we do change to use a textarea, specifying'#maxlength' => NULL
is not necessary.Comment #6
jhuhta CreditAttribution: jhuhta commentedA new version of the patch without
#maxlength
property.Comment #8
jhuhta CreditAttribution: jhuhta commentedOops, fixing directories from the previous one.
Comment #9
jhuhta CreditAttribution: jhuhta commentedComment #11
John Pitcairn CreditAttribution: John Pitcairn commentedPatch works as expected against 8.7.x. Needs re-testing against 8.8.x.
Comment #12
John Pitcairn CreditAttribution: John Pitcairn commentedStill a pass, let's call this RTBC shall we?
Comment #13
dhirendra.mishra CreditAttribution: dhirendra.mishra at Valuebound for Valuebound commentedComment #14
dhirendra.mishra CreditAttribution: dhirendra.mishra at Valuebound for Valuebound commentedComment #15
Wim LeersLGTM
Comment #16
larowlanThanks for this, as it is a bug fix, we should add test coverage for this functionality, to ensure it stays fixed.
You should be able to modify the existing test
\Drupal\Tests\responsive_image\Functional\ResponsiveImageAdminUITest::testResponsiveImageAdmin
to use a particularly long breakpoint size and cause the test to fail without this patch.Please upload two versions of the patch, one with just the test change (should fail) and then one that also includes the fix (should pass).
Thanks for your work here folks!
Comment #17
andreyjan CreditAttribution: andreyjan at FFW commentedTest only and the combined test/fix patches.
The media query which I used in the test is taken from one of the above comments and is not really a one for the narrow breakpoint, but for this test it should be OK.
Comment #18
andreyjan CreditAttribution: andreyjan at FFW commentedComment #19
Krzysztof DomańskiI think we should add to this test a comment something like: Tests that the sizes field allows more than 128 characters.
Comment #22
jhedstromThis has a test now. Setting to NW to add a code comment as per #20.
I'd suggest a code comment of
// Ensure the Sizes field allows long values.
Comment #23
jhedstromComment #24
andreyjan CreditAttribution: andreyjan at FFW commentedAdded the comment.
Comment #25
jhedstromI think this is good to go.
Comment #26
alexpottCrediting @rolfmeijer for opening the issue
Crediting @jhedstrom, @Krzysztof Domański, @tstoeckler and @larowlan for reviewing the code
Comment #27
alexpottCommitted and pushed 2f585eff49 to 9.0.x and 47b225251c to 8.9.x. Thanks!
Going to ask other committers about backporting to 8.8.x
Comment #30
andreyjan CreditAttribution: andreyjan at FFW commentedHi @alexpott,
I have tried the last patch with 8.8.x-dev and it's applying on this branch as well. Is anything else needed to do here?
Comment #31
alexpott@andreyjan nope - patch-to-be-ported is a status committers use where we're
Comment #32
alexpottDiscussed with @catch who +1'd the backport to 8.8.x