Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
joemcgill opened this issue Nov 8, 2015 · 4 comments
Closed

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

joemcgill opened this issue Nov 8, 2015 · 4 comments
Assignees
Labels
Milestone

Comments

@joemcgill
Copy link
Member

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

@joemcgill joemcgill added the bug label Nov 8, 2015
@joemcgill joemcgill added this to the Next release milestone Nov 8, 2015
@jaspermdegroot jaspermdegroot self-assigned this Nov 13, 2015
@jaspermdegroot
Copy link
Member

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

@joemcgill
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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.

joemcgill added a commit that referenced this issue Nov 13, 2015
Check if there is no sizes attribute before adding it (Resolves #227)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants