Apply a filter to the wp_calculate_image_sizes to enable sizes attribute for srcsets

WPBuddy / largo

A WordPress framework for news websites. Finely-crafted by INN and expertly-honed and maintained by the technology team at WP Buddy.

http://largo.wpbuddy.co

GNU General Public License v2.0

170 stars 83 forks source link

Apply a filter to the wp_calculate_image_sizes to enable sizes attribute for srcsets #1577

Open benlk opened 5 years ago

benlk commented 5 years ago

Continuing from #1572's discussion of why Largo isn't doing srcsets, for https://secure.helpscout.net/conversation/706415460/2834?folderId=1219602

Based upon conversation in the WordPress.org Slack, this is something that WordPress left to themes to implement in 4.4:

The way WordPress currently does it in 5.0 RC1 is state every image is displayed at a max width of 1024px meaning any image displayed wider than that (most alignwide and alignfull images, and in Twenty Nineteen’s case any unaligned image displayed on a wider screen) receives an undersized image causing severe blurring and artifacts.

The theme is where you define what the sizes attribute for each image should be. See https://github.com/WordPress/twentynineteen/pull/629/files

This is how it’s been since WordPress 4.4 btw. Every default theme uses the wp_calculate_image_sizes filter for this exact purpose. Twenty Nineteen uses it for Featured Images already. https://github.com/WordPress/twentynineteen/blob/master/inc/template-functions.php#L115-L135

So:

[ ] determine which image sizes Largo should put there
[ ] put them there
[ ] determine if Largo needs to register any additional image sizes to support HiDPI useragents
[ ] make sure this works in 4.9.8 and 5.0

benlk commented 5 years ago

At Largo 0.6, get_the_post_thumbnail($bigStoryPost->ID, 'full'); returns: (with cleaned-up formatting)

<img
    src="https://energynews.test/wp-content/uploads/2015/07/vicki-quek-detective-eduardo-colour-1170x828.jpg"
    class="attachment-full size-full wp-post-image"
    alt=""
    srcset="
        https://energynews.test/wp-content/uploads/2015/07/vicki-quek-detective-eduardo-colour-1170x828.jpg 1170w,
        https://energynews.test/wp-content/uploads/2015/07/vicki-quek-detective-eduardo-colour-336x238.jpg 336w,
        https://energynews.test/wp-content/uploads/2015/07/vicki-quek-detective-eduardo-colour-768x543.jpg 768w,
        https://energynews.test/wp-content/uploads/2015/07/vicki-quek-detective-eduardo-colour-771x545.jpg 771w"
    sizes="(max-width: 1170px) 100vw, 1170px"
>

joshdarby commented 5 years ago

@benlk It looks like images in the content are already getting srcset applied to them. Here's an example:

What else do we need to do in order to close out this issue?

benlk commented 5 years ago

We've got srcsets there, but the sizes attribute doesn't match them.

benlk commented 5 years ago

I guess we need to calculate image sizes and apply a filter to this list: https://developer.wordpress.org/reference/functions/wp_calculate_image_sizes/

    /**
     * Filters the output of 'wp_calculate_image_sizes()'.
     *
     * @since 4.4.0
     *
     * @param string       $sizes         A source size value for use in a 'sizes' attribute.
     * @param array|string $size          Requested size. Image size or array of width and height values
     *                                    in pixels (in that order).
     * @param string|null  $image_src     The URL to the image file or null.
     * @param array|null   $image_meta    The image meta data as returned by wp_get_attachment_metadata() or null.
     * @param int          $attachment_id Image attachment ID of the original image or 0.
     */
    return apply_filters( 'wp_calculate_image_sizes', $sizes, $size, $image_src, $image_meta, $attachment_id );

benlk commented 5 years ago

After reading https://developer.mozilla.org/en-US/docs/Learn/HTML/Multimedia_and_embedding/Responsive_images while working on a response to https://github.com/INN/umbrella-citylimits/pull/33#issuecomment-499175229, it looks like we're going to have to provide different sizes attributes for each of the scenarios described in https://github.com/WordPress/gutenberg/issues/6177#issuecomment-400095305

scenarios a, b, c scenarios d, e, f scenarios g, h, i

The sizes attribute is a mapping of viewport widths to predicted image widths, where the predicted image width is based on a serverside (not CSS-side) understanding of the width of the image displayed at the given viewport width. We want the image sizes to match the image size, or at least be not too much larger than the actual image size. Which means when filtering 'wp_calculate_image_sizes' we need to include information which isn't included in the parameters passed to the filter: the context in which the image is output upon the page.

While we may want to consider image sizes on archive pages, the homepage, widgets, and so on, it's probably easiest to figure out an approach first for images in the post entry content first, because those image sizes are the most variable.

The Core ticket tracking this problem is https://core.trac.wordpress.org/ticket/45407

benlk commented 5 years ago

WP Core's issue tracking this has now moved from 5.2 to 5.3, and is kinda-sorta punted to WHATWG for resolution. https://github.com/whatwg/html/issues/4421

benlk commented 5 years ago

I think that with the most certainty available to us in the present regime, we could at most provide a sizes array that prevents the image used from being dimensionally bigger than the viewport.

benlk commented 5 years ago

Wait, no, Twenty Nineteen has this: https://github.com/WordPress/twentynineteen/pull/629/

Which was implemented when Gutenberg added a bunch more information to a block: https://github.com/WordPress/gutenberg/pull/11973

and later revised to:

https://github.com/WordPress/WordPress/blob/f6fc8025c48509cba63982daae3c99901056840a/wp-content/themes/twentynineteen/inc/template-functions.php#L112-L132

So with that and the expected content widths from the template (single-column, double-column, full-width) we can determine appropriate maximum image sizes, and implement this.

It's going to require a lot of testing and inspecting, though.

benlk commented 5 years ago

add image sizes attribute for srcsets

benlk commented 4 years ago

from https://github.com/WordPress/gutenberg/pull/11973#issuecomment-486390889, things to watch: