search

ResponsiveImagesCG / wp-tevko-responsive-images

Fully responsive image plugin for wordpress
449 stars 53 forks source link

Don't override `sizes` attributes set in the post editor #227

Closed joemcgill closed 8 years ago

joemcgill commented 8 years ago

If a user sets a custom sizes attribute in the post editor but no srcset attribute, the image will get filtered because we only check for srcset attributes in wp_make_content_images_responsive before passing them off to wp_image_add_srcset_and_sizes. We don't do any sanity checking in wp_image_add_srcset_and_sizes, so a duplicate sizes attribute will get added.

Example:

<img src="[url]" alt="[alt]" height="2000" width="3000" sizes="33vw"/>

Compiles to:

<img src="[url]" alt="[alt]" height="2000" width="3000" sizes="33vw"
   srcset="[image sizes list]" sizes="(max-width: 3000px) 100vw, 3000px"/>

See: https://wordpress.org/support/topic/set-custom-sizes-attribute-on-img-in-the-post-editor

jaspermdegroot commented 8 years ago

The WordPress Trac ticket for reference: https://core.trac.wordpress.org/ticket/34678

joemcgill commented 8 years ago

The only time this will be an issue is when someone adds a manual sizes attribute to an image but no srcset, which is really rare.

I'm a bit concerned that wp_image_add_srcset_and_sizes() doesn't contain any checks for the existence of a srcset attribute either. However, we do check for the existence of a srcset attribute in wp_make_content_images_responsive() before passing the image element off to wp_image_add_srcset_and_sizes().

Seems like we should be consistent in the way these are handled but there's some tradeoffs. If we add a srcset check to wp_image_add_srcset_and_sizes(), we end up with extra calculations, which would be a bit of a perf hit. On the other hand, if someone uses that function directly, they could end up with double srcset attributes as well.

jaspermdegroot commented 8 years ago

Yeah, I have been thinking about this too. At first I also thought we should check both in wp_image_add_srcset_and_sizes(), but I don't like to drop the srcset check in wp_make_content_images_responsive(). Checking twice for srcset doesn't feel efficient either.

I agree that adding only a sizes attribute in the post editor is probably rare (although it is one of the few issues reported after 3.0.0), but I think calling wp_image_add_srcset_and_sizes() on an image that already has a srcset is probably rare too.

joemcgill commented 8 years ago

but I think calling wp_image_add_srcset_and_sizes() on an image that already has a srcset is probably rare too

This is true. And since this is a content filter, we should favor efficiency over covering hypothetical edge cases. Commit looks good.