Add support for the sizes attribute.

[joemcgill]

Here's an initial candidate for how we can handle default support for the sizes attribute in this plugin.

Background

As discussed in #34, we need a way to set a smart default value for the sizes attribute when we are including a source set list using w descriptors. Otherwise, we run the risk of fooling the browser into downloading larger resources than necessary by assuming the image layout width is equal to 100% of the viewport width (among other issues). For the purpose of this pull request, I'm using the following pattern as the default for sizes:

sizes="( max-width: {{image-width}} ) 100vw, {{image-width}}"

In other words, when the viewport is less than or equal to the width of the main image, assume the image takes up the full screen width. If the viewport is greater than the width of the image, just use the width of the image itself as the layout width.

The case for data-sizes

For this implementation, I've opted to include the sizes list as a data-sizes attribute instead of a sizes attribute directly. Here are my reasons:

First, It's important to remember that this plugin works by adding markup directly to the image HTML element before it's inserted into the editor, so we need to be careful about what we're saving to user's databases. Since srcset lists are unlikely to change over time and the attribute is now part of the HTML spec, we're probably safe there. On the other hand, the sizes attribute is completely dependent on the site's layout, which is theme territory and something we should be very careful about saving into users' databases.

Also, as of this writing, the version of TinyMCE that is bundled in WordPress core does not include support for the sizes attribute on image elements, so even if we did add sizes to images, TinyMCE would just strip them right off. We can extend TinyMCE, and I've included a filter which does just that, but it currently breaks the wp.media view so I have it commented out for now. 👎

I've added new filters to the_content and post_thumbnail_html that simply looks for images with data-sizes attributes and change them to sizes attributes instead.

While I'll be the first to admit that this approach isn't perfect, I see several benefits this strategy. Primarily, it allows us to deliver a default sizes attribute out of the box, which is a nice win. Developers will have the flexibility to alter the filters to their own liking in order to improve the accuracy of the sizes attributes without writing a bunch of code, which is pretty nice. Maybe most importantly, saving data attributes to the post data instead of the sizes attribute buys us some flexibility while we work out better ways of handling this in the future.

[tevko]

this should probably be vw no? vm units don't seem to have great support http://caniuse.com/#search=vm

[joemcgill]

I've got to stop coding upside down.

[joemcgill]

One thing to consider is whether tevkori_get_sizes_string() should just return the attribute value instead of the whole attribute so we don't have to filter post_thumbnail_html twice. I opted to make the grammar match tevkori_get_srcset_string(), which returns the full attribute string and not just the value.

We could abstract the attribute strings into their own functions—tevkori_get_sizes() and tevkori_get_srcset(), repectively—which return the attribute values so we don't break backwards compatibility, and make the _string() variations wrapper functions that look like this:

tevkori_get_sizes_string($id, $size, $args) {
  // get the attribute value
  $sizes = tevkori_get_sizes($id, $size, $args);
  // if the value returns true, build the string
  $sizes = ($sizes) ? 'data-sizess="' . $sizes . '"' : false;
  return $sizes;
}

Thoughts?

[joemcgill]

FWIW, over in Slack, @pento just recommended using data-attributes as well. https://wordpress.slack.com/archives/feature-respimg/p1423993863000283

[joemcgill]

I've made a change to this PR that separates the creation of the sizes value from building the actual attribute so that we can use the same function to create both a data-sizes attribute and a sizes attribute without the need for an additional filter. We may want to do the same for building the srcset attribute, but that's outside the scope of this fix.

[side777]

the sizes attribute is completely dependent on the site's layout

that's why i would completely leave it in the theme developer's hands. they know their switches and sizes and can easily add the sizes attribute by filtering the_content. they can put whatever value they use to set the image witdh - rem, em, vw... - in their own filter. no need to write to the user database and everything is fine.

If the viewport is greater than the width of the image, just use the width of the image itself as the layout width.

you assume that the editor image width is the highest needed width... this way it would be impossible to add bigger images for higher resolutions in the layout.

[joemcgill]

Hi @side777. Generally, I am leaving the sizes attribute in theme developers hands, I'm just setting a default sizes attribute that is more sensible than 100% of the viewport for users/themes that aren't handling sizes themselves. The default is completely overridable by developers.

[side777]

that's good to know!

simply looks for images with data-sizes attributes and change them to sizes attributes instead.

so, why not leave the database part out and only add sizes by searching the content for srcset?

[joemcgill]

why not leave the database part out and only add sizes by searching the content for srcset?

Not a bad idea. By setting a data-sizes attribute, we leave room for content creators to modify the sizes attribute on an image specific basis (by editing the HTML in the editor). Should we retain that ability?

[side777]

content creators to modify the sizes attribute on an image specific basis

a check for existing sizes attribute (and srcset btw!) would solve this...

now that i think about that - should the srcset be added via the_content too? i think about a scenario where the image dimensions get changed by user or theme...

[joemcgill]

should the srcset be added via the_content too?

This would require running extra database queries to build the srcset arrays at runtime, which is probably a bad idea. In reality, the only way these should change is if someone regenerates all of their thumbnails. Otherwise, changing image sizes only affect images uploaded after the change.

[joemcgill]

Now that #56 has landed, I'm going to write tests for this before we merge it.

[joemcgill]

I've refreshed this patch and have added tests. In the process, I found a potential issue with the way the tests for srcset were written which could cause them to fail under certain conditions and have supplied a patch for that issue in #58. There's no need to merge that first, but it would be nice to get them both in around the same time.

For simplicity sake, I've squashed this change down to a single commit, but have left the change history in a separate branch in my fork of the repo in case you're interested (https://github.com/joemcgill/wp-tevko-responsive-images/compare/feature/sizes-wip). I'll leave that branch open until this has been merged so I can go back to it if needed.