Account for responsive images being disabled when generating a PICTURE element #1449

mukeshpanchal27 · 2024-08-08T11:35:25Z

Summary

After the PR:

github-actions · 2024-08-08T11:40:55Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mukeshpanchal27 <[email protected]>
Co-authored-by: westonruter <[email protected]>
Co-authored-by: joemcgill <[email protected]>
Co-authored-by: adamsilverstein <[email protected]>
Co-authored-by: felixarntz <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

joemcgill

Nice catch! Just some small nitpicks here related to my comment on the original issue.

plugins/webp-uploads/picture-element.php

plugins/webp-uploads/tests/test-picture-element.php

mukeshpanchal27 · 2024-08-12T06:33:04Z

After 7e09a72, The picture element outputs:

Responsive image:

<picture class="wp-picture-12" style="display: contents;">
  <source type="image/webp" 
    srcset="
       http://localhost:8888/wp-content/uploads/2024/08/car-300x169-jpeg.webp 300w, 
       http://localhost:8888/wp-content/uploads/2024/08/car-1024x576-jpeg.webp 1024w, 
       http://localhost:8888/wp-content/uploads/2024/08/car-768x432-jpeg.webp 768w
    " 
    sizes="(max-width: 1080px) 100vw, 1080px"
  >
  <img 
    fetchpriority="high" 
    decoding="async" 
    width="1080" 
    height="608" 
    src="http://localhost:8888/wp-content/uploads/2024/08/car.jpeg" 
    alt="" 
    class="wp-image-12" 
    srcset="
      http://localhost:8888/wp-content/uploads/2024/08/car.jpeg 1080w, 
      http://localhost:8888/wp-content/uploads/2024/08/car-300x169.jpeg 300w, 
      http://localhost:8888/wp-content/uploads/2024/08/car-1024x576.jpeg 1024w, 
      http://localhost:8888/wp-content/uploads/2024/08/car-768x432.jpeg 768w
    "
    sizes="(max-width: 1080px) 100vw, 1080px"
    >
</picture>

No responsive image (use `wp_calculate_image_sizes` filter):

<picture class="wp-picture-12" style="display: contents;">
  <source type="image/webp" srcset="http://localhost:8888/wp-content/uploads/2024/08/car-jpeg.webp">
  <img 
    fetchpriority="high" 
    decoding="async" 
    width="1080" 
    height="608" 
    src="http://localhost:8888/wp-content/uploads/2024/08/car.jpeg" 
    alt="" 
    class="wp-image-12"
  >
</picture>

… fix/follow-up-1399

plugins/webp-uploads/picture-element.php

plugins/webp-uploads/tests/test-picture-element.php

mukeshpanchal27 · 2024-08-13T09:36:26Z

@westonruter Thanks for the feedback. After the new helper function the result of single and multi-mime type images are:

Multi Mime

'image/jpeg' => array( 'image/jpeg', 'image/webp', 'image/avif' )

Medium Size

<picture class="wp-picture-43" style="display: contents;">
  <source 
    type="image/avif"
    srcset="http://localhost:8888/wp-content/uploads/2024/08/car-300x169-jpeg.avif"
  >
  <source
    type="image/webp"
    srcset="http://localhost:8888/wp-content/uploads/2024/08/car-300x169-jpeg.webp"
  >
  <img
    fetchpriority="high"
    decoding="async"
    width="300"
    height="169"
    src="http://localhost:8888/wp-content/uploads/2024/08/car-300x169.jpeg"
    alt=""
    class="wp-image-43"
  >
</picture>

Large Size

<picture class="wp-picture-43" style="display: contents;">
  <source
    type="image/avif"
    srcset="http://localhost:8888/wp-content/uploads/2024/08/car-1024x576-jpeg.avif"
  >
  <source
    type="image/webp"
    srcset="http://localhost:8888/wp-content/uploads/2024/08/car-1024x576-jpeg.webp"
  >
  <img
    decoding="async"
    width="1024" 
    height="576" 
    src="http://localhost:8888/wp-content/uploads/2024/08/car-1024x576.jpeg" 
    alt="" 
    class="wp-image-43"
  >
</picture>

Full Size

<picture class="wp-picture-43" style="display: contents;">
  <source 
    type="image/avif" 
    srcset="http://localhost:8888/wp-content/uploads/2024/08/car-jpeg.avif"
  >
  <source
    type="image/webp"
    srcset="http://localhost:8888/wp-content/uploads/2024/08/car-jpeg.webp"
  >
  <img
    decoding="async" 
    width="1080" 
    height="608" 
    src="http://localhost:8888/wp-content/uploads/2024/08/car.jpeg" 
    alt="" 
    class="wp-image-43"
  >
</picture>

Single Mime

'image/jpeg' => array( 'image/jpeg', 'image/avif)

Medium Size

<picture class="wp-picture-46" style="display: contents;">
  <source
    type="image/avif"
    srcset="http://localhost:8888/wp-content/uploads/2024/08/leaves-300x200-jpg.avif"
  >
  <img 
    loading="lazy" 
    decoding="async"
    width="300" 
    height="200" 
    src="http://localhost:8888/wp-content/uploads/2024/08/leaves-300x200.jpg" 
    alt="" 
    class="wp-image-46"
  >
</picture>

Large Size

<picture class="wp-picture-46" style="display: contents;">
  <source 
    type="image/avif" 
    srcset="http://localhost:8888/wp-content/uploads/2024/08/leaves-1024x683-jpg.avif"
  >
  <img 
    loading="lazy" 
    decoding="async" 
    width="1024" 
    height="683" 
    src="http://localhost:8888/wp-content/uploads/2024/08/leaves-1024x683.jpg" 
    alt="" 
    class="wp-image-46"
  >
</picture>

Full Size

<picture class="wp-picture-46" style="display: contents;">
  <source
    type="image/avif"
    srcset="http://localhost:8888/wp-content/uploads/2024/08/leaves-jpg.avif"
  >
  <img
    loading="lazy" 
    decoding="async" 
    width="1080" 
    height="720" 
    src="http://localhost:8888/wp-content/uploads/2024/08/leaves.jpg" 
    alt="" 
    class="wp-image-46"
  >
</picture>

westonruter

Curious: in #1449 (comment) you show examples of how this works when different image sizes are requested, but how is the size being selected for the alternative mime types? I don't see how the size selection works.

plugins/webp-uploads/helper.php

westonruter · 2024-08-13T13:01:58Z

plugins/webp-uploads/helper.php

+	if ( isset( $metadata['sizes'] ) && is_array( $metadata['sizes'] ) ) {
+		foreach ( $metadata['sizes'] as $size => $size_data ) {
+
+			if ( empty( $size_data['file'] ) ) {


Can we avoid using empty() here?

plugins/webp-uploads/helper.php

Co-authored-by: Weston Ruter <[email protected]>

mukeshpanchal27 · 2024-08-14T05:37:49Z

Curious: in #1449 (comment) you show examples of how this works when different image sizes are requested, but how is the size being selected for the alternative mime types? I don't see how the size selection works.

@westonruter When we disabled the responsive images then it will return only src for the image with size( Large, Medium , Full ) that we selected. #1449 (comment) i share the result for each image size.

westonruter · 2024-08-14T17:55:53Z

I'm not actively familiar enough with the internals of image handling in WP without taking time to dive in deeper. I'll defer to @joemcgill and @adamsilverstein who have more experience to finish the reviews.

westonruter · 2024-08-16T23:22:31Z

Punting from the 3.4.0 release since we're out of time. If this does get reviewed by Monday morning, make sure the PR's base branch is updated from trunk to release/3.4.0.

adamsilverstein

Nice fix!

felixarntz · 2024-08-19T15:39:03Z

IMO too big of a change to merge now 1 hour before release. I'd say let's get this into the next one.

westonruter · 2024-08-19T15:56:12Z

Agreed. Merging into trunk and not into the release branch.

Requested changes addressed

westonruter · 2024-08-19T16:04:47Z

@mukeshpanchal27 I updated the PR title to be more clear. Please review and amend further as needed.

mukeshpanchal27 added 2 commits August 8, 2024 16:36

Remove source when the responsive image disabled

7546f34

Update copy text

427fa20

mukeshpanchal27 added [Type] Bug An existing feature is broken [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) labels Aug 8, 2024

mukeshpanchal27 added this to the webp-uploads n.e.x.t milestone Aug 8, 2024

mukeshpanchal27 self-assigned this Aug 8, 2024

mukeshpanchal27 marked this pull request as ready for review August 8, 2024 11:40

mukeshpanchal27 requested review from adamsilverstein, felixarntz and westonruter as code owners August 8, 2024 11:40

joemcgill previously requested changes Aug 8, 2024

View reviewed changes

plugins/webp-uploads/picture-element.php Outdated Show resolved Hide resolved

plugins/webp-uploads/tests/test-picture-element.php Show resolved Hide resolved

westonruter requested changes Aug 8, 2024

View reviewed changes

plugins/webp-uploads/tests/test-picture-element.php Outdated Show resolved Hide resolved

westonruter mentioned this pull request Aug 8, 2024

Picture element: Return additional mime type IMG>src when the responsive image disabled #1448

Closed

mukeshpanchal27 added 2 commits August 12, 2024 10:22

Merge branch 'trunk' into fix/follow-up-1399

893d4ef

Return Picture element, if responsive image disable

7e09a72

mukeshpanchal27 changed the title ~~Picture element: Remove picture and source tags when the responsive image disabled~~ Picture element: Return additional mime type IMG >src when the responsive image disabled Aug 12, 2024

mukeshpanchal27 requested review from westonruter and joemcgill August 12, 2024 06:56

mukeshpanchal27 changed the title ~~Picture element: Return additional mime type IMG >src when the responsive image disabled~~ Picture element: Return additional mime type IMG>src when the responsive image disabled Aug 12, 2024

mukeshpanchal27 and others added 2 commits August 12, 2024 12:36

Doc updates

6bb9ef8

Merge branch 'trunk' of https://github.com/WordPress/performance into…

f4a79b5

… fix/follow-up-1399

westonruter requested changes Aug 12, 2024

View reviewed changes

mukeshpanchal27 and others added 6 commits August 13, 2024 09:10

Merge branch 'trunk' into fix/follow-up-1399

999545e

Check if IMG have src

b4b6b23

Add check for is_string

a732593

Minor updates

9d66642

Address feedback for unit test

a94d48b

Introduce webp_uploads_get_mime_type_image helper function

35a626a

mukeshpanchal27 requested a review from westonruter August 13, 2024 09:37

westonruter mentioned this pull request Aug 13, 2024

Test unit tests mukeshpanchal27/performance#32

Closed

westonruter reviewed Aug 13, 2024

View reviewed changes

mukeshpanchal27 and others added 3 commits August 14, 2024 10:29

Merge branch 'trunk' into fix/follow-up-1399

c89b0e6

Apply suggestions from code review

a885d7b

Co-authored-by: Weston Ruter <[email protected]>

Remove use of empty

0f9fb41

mukeshpanchal27 requested a review from westonruter August 14, 2024 05:39

westonruter modified the milestones: webp-uploads 2.1.0, webp-uploads n.e.x.t Aug 16, 2024

adamsilverstein approved these changes Aug 19, 2024

View reviewed changes

westonruter approved these changes Aug 19, 2024

View reviewed changes

westonruter changed the title ~~Picture element: Return additional mime type IMG>src when the responsive image disabled~~ Account for responsive images being disabled when generating a PICTURE element Aug 19, 2024

westonruter merged commit a3835ea into trunk Aug 19, 2024
18 checks passed

westonruter deleted the fix/follow-up-1399 branch August 19, 2024 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account for responsive images being disabled when generating a PICTURE element #1449

Account for responsive images being disabled when generating a PICTURE element #1449

mukeshpanchal27 commented Aug 8, 2024 •

edited

Loading

github-actions bot commented Aug 8, 2024 •

edited

Loading

joemcgill left a comment

mukeshpanchal27 commented Aug 12, 2024

mukeshpanchal27 commented Aug 13, 2024

westonruter left a comment

westonruter Aug 13, 2024

mukeshpanchal27 commented Aug 14, 2024

westonruter commented Aug 14, 2024

westonruter commented Aug 16, 2024

adamsilverstein left a comment

felixarntz commented Aug 19, 2024

westonruter commented Aug 19, 2024

westonruter commented Aug 19, 2024

Account for responsive images being disabled when generating a PICTURE element #1449

Account for responsive images being disabled when generating a PICTURE element #1449

Conversation

mukeshpanchal27 commented Aug 8, 2024 • edited Loading

Summary

github-actions bot commented Aug 8, 2024 • edited Loading

joemcgill left a comment

Choose a reason for hiding this comment

mukeshpanchal27 commented Aug 12, 2024

Responsive image:

No responsive image (use wp_calculate_image_sizes filter):

mukeshpanchal27 commented Aug 13, 2024

Multi Mime

Single Mime

westonruter left a comment

Choose a reason for hiding this comment

westonruter Aug 13, 2024

Choose a reason for hiding this comment

mukeshpanchal27 commented Aug 14, 2024

westonruter commented Aug 14, 2024

westonruter commented Aug 16, 2024

adamsilverstein left a comment

Choose a reason for hiding this comment

felixarntz commented Aug 19, 2024

westonruter commented Aug 19, 2024

westonruter commented Aug 19, 2024

mukeshpanchal27 commented Aug 8, 2024 •

edited

Loading

github-actions bot commented Aug 8, 2024 •

edited

Loading

No responsive image (use `wp_calculate_image_sizes` filter):