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?

CommentFileSizeAuthor
#24 sizes_input_is_too_small-2876523-24.patch3.25 KBandreyjan
#24 sizes_input_is_too_small-2876523-interdiff--17-24.txt1.19 KBandreyjan
#17 sizes_input_is_too_small-2876523-17.patch3.2 KBandreyjan
#17 sizes_input_is_too_small-2876523-17-TEST_ONLY.patch2.3 KBandreyjan
#8 sizes_input_is_too_small-2876523-7.patch926 bytesjhuhta
#6 sizes_input_is_too_small-2876523-6.patch890 bytesjhuhta
#2 sizes_input_is_too_small-2876523-2.patch985 byteskeeganstreet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rolfmeijer created an issue. See original summary.

keeganstreet’s picture

keeganstreet CreditAttribution: keeganstreet at Deloitte Digital commented
FileSize
sizes_input_is_too_small-2876523-2.patch985 bytes

I 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...

rolfmeijer’s picture

rolfmeijer CreditAttribution: rolfmeijer at Dutch Open Projects commented

Yes, a textarea is a great idea and makes a lot more sense.

FiNeX’s picture

FiNeX CreditAttribution: FiNeX as a volunteer commented
Status: Active » Reviewed & tested by the community

Hi, I've tried the patch and it correctly works. Please commit :-)

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Status: Reviewed & tested by the community » Needs work

So the textarea element does not have a #maxlength property, so if we do change to use a textarea, specifying '#maxlength' => NULL is not necessary.

jhuhta’s picture

jhuhta CreditAttribution: jhuhta commented
Version: 8.3.1 » 8.7.x-dev
Status: Needs work » Needs review
FileSize
sizes_input_is_too_small-2876523-6.patch890 bytes

A new version of the patch without #maxlength property.

Status: Needs review » Needs work

The last submitted patch, 6: sizes_input_is_too_small-2876523-6.patch, failed testing. View results

jhuhta’s picture

jhuhta CreditAttribution: jhuhta commented
FileSize
sizes_input_is_too_small-2876523-7.patch926 bytes

Oops, fixing directories from the previous one.

jhuhta’s picture

jhuhta CreditAttribution: jhuhta commented
Status: Needs work » Needs review

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

John Pitcairn’s picture

John Pitcairn CreditAttribution: John Pitcairn commented

Patch works as expected against 8.7.x. Needs re-testing against 8.8.x.

John Pitcairn’s picture

John Pitcairn CreditAttribution: John Pitcairn commented
Status: Needs review » Reviewed & tested by the community

Still a pass, let's call this RTBC shall we?

dhirendra.mishra’s picture

dhirendra.mishra CreditAttribution: dhirendra.mishra at Valuebound for Valuebound commented
Issue tags: +vb_global_contribution_hour
dhirendra.mishra’s picture

dhirendra.mishra CreditAttribution: dhirendra.mishra at Valuebound for Valuebound commented
Issue tags: -vb_global_contribution_hour
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue tags: +Usability

LGTM

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Thanks 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!

andreyjan’s picture

andreyjan CreditAttribution: andreyjan at FFW commented
FileSize
sizes_input_is_too_small-2876523-17-TEST_ONLY.patch2.3 KB
sizes_input_is_too_small-2876523-17.patch3.2 KB

Test 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.

andreyjan’s picture

andreyjan CreditAttribution: andreyjan at FFW commented
Status: Needs work » Needs review
Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented

I think we should add to this test a comment something like: Tests that the sizes field allows more than 128 characters.

The last submitted patch, 17: sizes_input_is_too_small-2876523-17-TEST_ONLY.patch, failed testing. View results

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
Issue tags: -Needs tests

This 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.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
Status: Needs review » Needs work
andreyjan’s picture

andreyjan CreditAttribution: andreyjan at FFW commented
Status: Needs work » Needs review
FileSize
sizes_input_is_too_small-2876523-interdiff--17-24.txt1.19 KB
sizes_input_is_too_small-2876523-24.patch3.25 KB

Added the comment.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
Status: Needs review » Reviewed & tested by the community

I think this is good to go.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Title: Sizes input is too small » Breakpoint sizes input is too small

Crediting @rolfmeijer for opening the issue
Crediting @jhedstrom, @Krzysztof Domański, @tstoeckler and @larowlan for reviewing the code

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Version: 8.9.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 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

  • alexpott committed 20236ef on 9.0.x 
    Issue #2876523 by andreyjan, jhuhta, keeganstreet, rolfmeijer, jhedstrom...

  • alexpott committed 47b2252 on 8.9.x 
    Issue #2876523 by andreyjan, jhuhta, keeganstreet, rolfmeijer, jhedstrom...
andreyjan’s picture

andreyjan CreditAttribution: andreyjan at FFW commented
Status: Patch (to be ported) » Needs review

Hi @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?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs review » Patch (to be ported)

@andreyjan nope - patch-to-be-ported is a status committers use where we're

Going to ask other committers about backporting to 8.8.x
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Patch (to be ported) » Fixed

Discussed with @catch who +1'd the backport to 8.8.x

  • alexpott committed 8e9685f on 8.8.x 
    Issue #2876523 by andreyjan, jhuhta, keeganstreet, rolfmeijer, jhedstrom...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.