mlisook / plastic-image

A Polymer 3.0 element which adds extra plasticity to <iron-image> with support for srcset and lazy loading
MIT License
30 stars 6 forks source link

Example srcset shows console error message #34

Closed zbury closed 6 years ago

zbury commented 6 years ago

The example from the docs srcset="foo-s.jpg 150w, foo-sh.jpg 150w 2.0x, foo-m.jpg 405w, foo-mh 2.0x 405w, foo-l 1024w, foo-t 500w 750h" results in two error messages in the console:

Invalid srcset descriptor found in 'foo-s.jpg 150w, foo-sh.jpg 150w 2.0x, foo-m.jpg 405w, foo-mh 2.0x 405w, foo-l 1024w, foo-t 500w 750h' at '2.0x'.

Invalid srcset descriptor found in 'foo-s.jpg 150w, foo-sh.jpg 150w 2.0x, foo-m.jpg 405w, foo-mh 2.0x 405w, foo-l 1024w, foo-t 500w 750h' at '405w'.

 

I believe this happens if x and w descriptors are provided. If width comes first, as with foo-sh.jpg 150w 2.0x, then when parsing x this evaluates to true:

// If width, density and future-compat-h are not all absent, then let error
// be yes.
if (w || d || h) {
    pError = true;
}

And if density comes first, as with foo-mh 2.0x 405w, then when parsing w this evaluates to true:

// If width and density are not both absent, then let error be yes.
if (w || d) {
    pError = true;
}

If I comment out those lines it seems to work fine, though I'm not sure if that breaks it in other scenarios.

mlisook commented 6 years ago

Thank you for this report.

Recently (PR #25 - Better spec-compliant implementation for srcsetParse()) the srcset parser was swapped out for one that is better, but is also meticulously spec compliant. This was necessary because the prior parser could not handle URLs with embedded commas as used by image hosting CDNs like Cloudinary or imgix.

Standards are good, but plastic-image has extended the image selection in a number of ways (optionally using element dimensions instead of viewport, automatically adjusting for display density, automatically selecting or excluding webp format depending on browser support and allowing selector combinations) so it is likely we want to allow certain srcset constructs that are not strictly spec compliant.

That said, the test case suite has weak (OK, minimal) coverage and any change to this critical function needs to be carefully evaluated so as not to break existing uses. If it is really just the order of descriptors, I'm likely to be in favor of documenting the required order rather than modifying the function. OTOH, if density is only being allowed if width and height are not present as per your observations (which appear to be correct), then I'd want to modify the function to allow that case. Either way more test cases will be required, so I'll start there.

mlisook commented 6 years ago

Based on additional test cases, it's confirmed that the srcset parser is rejecting any entry with a density that also has a height or width. I plan to push an update within 24 hours.

mlisook commented 6 years ago

This issue is corrected in PR #35 - version 1.0.12

zbury commented 6 years ago

Great, thanks for the quick response and fix!