Make WordPress Core

Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#33641 closed task (blessed) (fixed)

Add support for responsive images using srcset and sizes

Reported by: joemcgill's profile joemcgill Owned by: kirasong's profile kirasong
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch
Focuses: Cc:

Description (last modified by kirasong)

When image markup is generated in WordPress, we should automatically include srcset and sizes attributes when possible in order to provide basic responsive image support.

To provide this, members of the Responsive Images Community Group created a feature plugin, with development on GitHub.

The plugin uses the default image sizes and available custom image sizes of the same aspect ratio when generating markup. It does not create additional sizes.

This is a placeholder ticket for initial patches, to become a tracking ticket if/when the plugin is approved for merge.

Attachments (8)

33641.patch (40.2 KB) - added by joemcgill 8 years ago.
First patch for #33641
test-image-large.png (4.3 KB) - added by joemcgill 8 years ago.
test-image-large.png
33641.2.patch (40.7 KB) - added by DrewAPicture 8 years ago.
Docs / standards fixes
33641.3.patch (643 bytes) - added by joemcgill 8 years ago.
Remove outdated inline comment.
33641.4.patch (360 bytes) - added by kraftbj 8 years ago.
checks for img_width before checking for it.
33641.5.patch (491 bytes) - added by kraftbj 8 years ago.
A better version of .4.
33641.diff (563 bytes) - added by wonderboymusic 8 years ago.
33641.6.diff (2.8 KB) - added by jaspermdegroot 8 years ago.
Changes in media tests

Download all attachments as: .zip

Change History (37)

This ticket was mentioned in Slack in #feature-respimg by joemcgill. View the logs.


9 years ago

#2 @kirasong
9 years ago

  • Description modified (diff)
  • Owner set to DH-Shredder
  • Status changed from new to assigned
  • Summary changed from Add support for srcset and sizes. to Add support for responsive images using srcset and sizes

This ticket was mentioned in Slack in #core by joemcgill. View the logs.


9 years ago

This ticket was mentioned in Slack in #feature-respimg by joemcgill. View the logs.


9 years ago

#5 @wonderboymusic
9 years ago

  • Milestone changed from Awaiting Review to 4.4
  • Type changed from enhancement to task (blessed)

This ticket was mentioned in Slack in #feature-respimg by joemcgill. View the logs.


9 years ago

This ticket was mentioned in Slack in #feature-respimg by joemcgill. View the logs.


9 years ago

This ticket was mentioned in Slack in #core by wonderboymusic. View the logs.


9 years ago

@joemcgill
8 years ago

First patch for #33641

#9 @joemcgill
8 years ago

  • Keywords has-patch added

This is a first pass at a patch for adding responsive image support to core, via srcset and sizes, to core.

## Code changes

This patch adds the following new functions to media.php:

  • wp_get_attachment_image_srcset_array - Returns an array of image candidate string data used to build a srcset value for an attachment given an attachement_id and size.
  • wp_get_attachment_image_srcset - Returns the srcset value for an attachment given an attachement_id and size.
  • wp_get_attachment_image_sized - Returns the sizes value for an attachment given an attachement_id and size and optional arguments used to alter its output.
  • wp_make_content_images_responsive - A display filter for adding srcset and sizes to images embedded in content.
  • wp_img_add_srcset_and_sizes - A utility function used by wp_make_content_images_responsive to add srcset and sizes to a single img element.

This patch also modifies the following existing core functions:

  • Modify wp_get_attachment_image so the HTML returned for an image includes srcset and sizes.
  • Modify get_media_embedded_in_content by adding img to the list of accepted tags that can be matched in content. This is used in wp_make_content_images_responsive to find all of the images embedded in content before passing them off to wp_img_add_srcset_and_sizes.

## Unit tests

This adds 12 new unit tests to tests/media.php and a new factory method to WP_UnitTest_Factory_For_Attachment named create_upload_object which creates an attachment and metadata from a file, as discussed in #34075. Several existing unit tests were updated to make use of this new factory method.

## Performance info

Based on our internal tests, the display filter is adding ~1ms per image being filtered. https://github.com/ResponsiveImagesCG/wp-tevko-responsive-images/pull/207#issuecomment-145506195

Running the wp_make_content_images_responsive display filter adds 2 queries to page loads total. By passing an array of all the attachment ids to _prime_post_caches, we avoid needing to query for post data associated with each individual image being transformed.

This ticket was mentioned in Slack in #core by joemcgill. View the logs.


8 years ago

This ticket was mentioned in Slack in #feature-respimg by joemcgill. View the logs.


8 years ago

@joemcgill
8 years ago

test-image-large.png

@DrewAPicture
8 years ago

Docs / standards fixes

#12 @joemcgill
8 years ago

I noticed that the test image didn't get included in the patch for some reason. test-image-large.png needs to be in the /tests/phpunit/data/images/ folder before running the tests added by this patch.

#13 @DrewAPicture
8 years ago

Added 33641.2.patch which includes some docs and coding standards adjustments.

Notable stuff:

  • Adding backticks in inline comments is moot because they aren't parsed
  • Added a changelog entry for the media_embedded_in_content_allowed_types hook
  • Updated the version on the wp_image_sizes_args hook (safe to assume 2.4.0 is the version of the plugin)
  • In a couple of functions, I renamed the $id parameter to $attachment_id for self-documentation purposes
  • Some spacing and syntax in various places.

Overall, this is extremely well-documented. Nice work.

#14 @Shapeshifter 3
8 years ago

Isn't 2.5.2 the correct version number for the patch, or not?

Version 0, edited 8 years ago by Shapeshifter 3 (next)

#15 follow-up: @joemcgill
8 years ago

Hi Shapeshifter,

The patch number reflects the Trac ticket number associated with the patch, which has no relation to the version number of the plugin this code is coming from.

Thanks!

#16 in reply to: ↑ 15 @Shapeshifter 3
8 years ago

Replying to joemcgill:

Hi Shapeshifter,

The patch number reflects the Trac ticket number associated with the patch, which has no relation to the version number of the plugin this code is coming from.

Thanks!

I learned something...Thank You...Can't wait for this to happen!

#17 @wonderboymusic
8 years ago

In 34855:

Merge the Responsive Images feature plugin into core, initial commit. See: https://github.com/ResponsiveImagesCG/wp-tevko-responsive-images/

New functions in media.php:

  • wp_get_attachment_image_srcset_array() - Returns an array of image candidate string data used to build a srcset value for an attachment given an $attachement_id and $size.
  • wp_get_attachment_image_srcset() - Returns the srcset value for an attachment given an $attachement_id and $size.
  • wp_get_attachment_image_sizes() - Returns the sizes value for an attachment given an $attachement_id and $size and optional arguments used to alter its output.
  • wp_make_content_images_responsive() - A display filter for adding srcset and sizes to images embedded in content.
  • wp_img_add_srcset_and_sizes() - A utility function used by wp_make_content_images_responsive() to add srcset and sizes to a single <img> element.

Modifies existing core functions:

  • Modify wp_get_attachment_image() so the HTML returned for an image includes srcset and sizes.
  • Modify get_media_embedded_in_content() (sup, 3.6 leftover) by adding <img> to the list of accepted tags that can be matched in content. This is used in wp_make_content_images_responsive() to find all of the images embedded in content before passing them off to wp_img_add_srcset_and_sizes().

Tests:

  • Add a new factory method to WP_UnitTest_Factory_For_Attachment named create_upload_object()
  • Adds unit tests
  • Updates unit tests

Props joemcgill, tevko, jaspermdegroot, mdmcginn, barryceelen, peterwilsoncc, fsylum, wonderboymusic, chriscoyier, benjaminpick, jrfnl, #12kingkool68, janhenckens, ryanmarkel, side777, ryelle, wturrell, micahmills, mattbagwell, coliff, DrewAPicture.
See #33641.

@joemcgill
8 years ago

Remove outdated inline comment.

#18 @joemcgill
8 years ago

Caught an outdated inline comment that slipped through. 33641.3.patch removes that comment.

#19 @DrewAPicture
8 years ago

In 34876:

Docs: Update an inline comment in wp_get_attachment_image_sizes(), which came in as part of the Responsive Images merge in [34855].

Props joemcgill.
See #33641.

@kraftbj
8 years ago

checks for img_width before checking for it.

#20 follow-up: @kraftbj
8 years ago

If a filter filters out img_width, a PHP notice is thrown since we're checking for a non-variable.

#21 in reply to: ↑ 20 ; follow-up: @joemcgill
8 years ago

Replying to kraftbj:

Good catch. Maybe we could simplify and just do a if ( ! empty( $image_width ) ) check there.

Last edited 8 years ago by joemcgill (previous) (diff)

#22 in reply to: ↑ 21 ; follow-up: @kraftbj
8 years ago

Replying to joemcgill:

Good catch. Maybe we could simplify and just do a if ( ! empty( $image_width ) ) check there.

Need more coffee. That's the way to go. Adding patch that if ( empty ( $img_width ) ) and adjusts the inline doc above to match var names.

@kraftbj
8 years ago

A better version of .4.

#23 in reply to: ↑ 22 @joemcgill
8 years ago

Replying to kraftbj:

Perfect. And thanks for catching the mistake in my sloppy comment code.

@wonderboymusic
8 years ago

#24 @wonderboymusic
8 years ago

In 35250:

Media: in wp_get_attachment_image_sizes(), ensure that $img_width exists when the image does not.

Props kraftbj.
See #33641.

#25 @wonderboymusic
8 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

New tickets.

#26 @johnbillion
8 years ago

In 35346:

Correct some more tests which were using example.org instead of WP_TESTS_DOMAIN.

See #33641, #34000

@jaspermdegroot
8 years ago

Changes in media tests

#27 @jaspermdegroot
8 years ago

In two tests for wp_calculate_image_srcset() we were using a random size name "yoav". This gives the impression that we are testing what happens if an invalid size name is used, but we are not. The test helper function _get_image_size_array_from_name() returns the default size array in this case so we are testing the code with a normal size array.
In 33641.6.diff I removed this size name from the tests and I added a new test that actually tests an invalid size name.

I thought this was the best place to add the patch even though the ticket closed. Let me know if I should open a new ticket for it.

This ticket was mentioned in Slack in #feature-respimg by joemcgill. View the logs.


8 years ago

#29 @azaozz
8 years ago

In 35560:

Responsive images: add test for invalid size name. Remove invalid size from other tests.

Props jaspermdegroot.
See #33641.

Note: See TracTickets for help on using tickets.