Don't wrap PICTURE element if JPEG fallback is not available #1450

sagarnasit · 2024-08-08T13:09:32Z

Summary

This PR accounts following:

Skip wrapping the image in the picture element if the jpeg fallback is not available.

plugins/webp-uploads/picture-element.php

github-actions · 2024-08-09T14:45:11Z

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.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @sagarnasit.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

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

Unlinked contributors: sagarnasit.

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

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

westonruter

Looking good! Just a couple tweaks, and then it seems the only thing left is to add a test.

plugins/webp-uploads/settings.php

sagarnasit · 2024-08-11T10:15:20Z

@westonruter I updated the check for $original_file_mime_type with $sub_size_mime_types and made other changes as per your suggestions.

Additionally, I've included a separate test file for the fallback image as the "Output JPEG" setting needs to be disabled initially, which is not viable in the existing test file.

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

… fix/1447-skip-picture-if-no-jpeg

westonruter · 2024-08-12T21:38:55Z

plugins/webp-uploads/settings.php

 		</label>
 		<p class="description" id="webp_uploads_use_picture_element_description"><?php esc_html_e( 'The picture element serves a modern image format with a fallback to JPEG. Warning: Make sure you test your theme and plugins for compatibility. In particular, CSS selectors will not match images when using the child combinator (e.g. figure > img).', 'webp-uploads' ); ?></p>
+		<div id="webp_uploads_jpeg_fallback_notice" class="notice notice-info inline" <?php echo $picture_element_enabled ? '' : 'hidden'; ?>>
+			<p><?php esc_html_e( 'Picture elements will only be used when JPEG fallback images are available. So this will not apply to any images you uploaded while the "Also generate JPEG" setting was disabled.', 'webp-uploads' ); ?></p>


Another slight copy improvement:

Suggested change

<?php esc_html_e( 'Picture elements will only be used when JPEG fallback images are available. So this will not apply to any images you uploaded while the "Also generate JPEG" setting was disabled.', 'webp-uploads' ); ?>

<?php esc_html_e( 'Picture elements will only be used when JPEG fallback images are available. So this will not apply to any images you may have uploaded while the "Also generate JPEG" setting was disabled.', 'webp-uploads' ); ?>

… fix/1447-skip-picture-if-no-jpeg

mukeshpanchal27 · 2024-08-13T10:04:37Z

@sagarnasit Have you replicate https://github.com/WordPress/performance/actions/runs/10364966081/job/28691196072?pr=1450#step:9:290 unit test failed in PHP 8.3 in your local? I didn't replicate it 🤔

sagarnasit · 2024-08-13T11:43:49Z

@mukeshpanchal27 The issue is not with PHP 8.3 but with the WordPress trunk. I tried to load the WordPress trunk on a local setup and was able to replicate the issue with the following.

WP_ENV_CORE=WordPress/WordPress WP_ENV_PHP_VERSION=8.3 npx wp-env start

I also attempted to debug further, but I was unsuccessful in finding out why next_tag is returning true even if there is no PICTURE tag in the markup.

westonruter · 2024-08-13T13:22:38Z

Probably a regression in the HTML Tag Processor in WP Core trunk since a lot has been going into that lately. We should do a git bisect on trunk to pinpoint the problematic commit.

westonruter · 2024-08-13T13:32:00Z

Doing so now...

westonruter · 2024-08-13T13:41:00Z

OK, I found that the regression was introduced in r58613 (in WordPress/WordPress this is commit df598e1d9). If I check out the parent commit, then I get no failure here.

westonruter · 2024-08-13T13:46:10Z

For reference, the unit test assertion is failing here:

performance/plugins/webp-uploads/tests/test-picture-element-original-fallback.php

Lines 79 to 80 in da7d55f

    
           $picture_processor = new WP_HTML_Tag_Processor( $picture_markup ); 
        
           $this->assertFalse( $picture_processor->next_tag( array( 'tag_name' => 'picture' ) ), 'There should not be a PICTURE tag.' );

The markup contained in $picture_markup is the following:

<p><img width="1024" height="683" src="http://localhost:8889/wp-content/uploads/2024/08/leaves-232-1024x683.webp" class="wp-image-9" alt="Green Leaves" decoding="async" loading="lazy" srcset="http://localhost:8889/wp-content/uploads/2024/08/leaves-232-1024x683.webp 1024w, http://localhost:8889/wp-content/uploads/2024/08/leaves-232-300x200.webp 300w, http://localhost:8889/wp-content/uploads/2024/08/leaves-232-768x512.webp 768w, http://localhost:8889/wp-content/uploads/2024/08/leaves-232-jpg.webp 1080w" sizes="(max-width: 1024px) 100vw, 1024px" /></p>

With formatting:

<p>
  <img
    width="1024"
    height="683"
    src="http://localhost:8889/wp-content/uploads/2024/08/leaves-232-1024x683.webp"
    class="wp-image-9"
    alt="Green Leaves"
    decoding="async"
    loading="lazy"
    srcset="
      http://localhost:8889/wp-content/uploads/2024/08/leaves-232-1024x683.webp 1024w,
      http://localhost:8889/wp-content/uploads/2024/08/leaves-232-300x200.webp   300w,
      http://localhost:8889/wp-content/uploads/2024/08/leaves-232-768x512.webp   768w,
      http://localhost:8889/wp-content/uploads/2024/08/leaves-232-jpg.webp      1080w
    "
    sizes="(max-width: 1024px) 100vw, 1024px"
  />
</p>

There is definitely no PICTURE tag here.

And yet, for some reason, $picture_processor->next_tag( array( 'tag_name' => 'picture' ) ) is returning true.

mukeshpanchal27 · 2024-08-13T13:49:31Z

It's WP Trunk error not related to PHP version. I have open WordPress/wordpress-develop#7185 to verify that.

westonruter · 2024-08-13T13:54:51Z

Given this code:

$picture_markup = '<p><img width="1024" height="683" src="http://localhost:8889/wp-content/uploads/2024/08/leaves-232-1024x683.webp" class="wp-image-9" alt="Green Leaves" decoding="async" loading="lazy" srcset="http://localhost:8889/wp-content/uploads/2024/08/leaves-232-1024x683.webp 1024w, http://localhost:8889/wp-content/uploads/2024/08/leaves-232-300x200.webp 300w, http://localhost:8889/wp-content/uploads/2024/08/leaves-232-768x512.webp 768w, http://localhost:8889/wp-content/uploads/2024/08/leaves-232-jpg.webp 1080w" sizes="(max-width: 1024px) 100vw, 1024px" /></p>';
$picture_processor = new WP_HTML_Tag_Processor( $picture_markup );
var_dump( $picture_processor->next_tag( array( 'tag_name' => 'picture' ) ) );
var_dump( $picture_processor->get_tag() );

It outputs:

bool(true)
string(1) "P"

If I remove the  from the markup, then I get:

bool(false)
NULL

So it seems that the Tag Processor is only looking at the first character of the tag name now, which is a bug.

Quick workaround to get the tests to pass: remove the  tag.

westonruter · 2024-08-13T13:59:05Z

Reported upstream: https://core.trac.wordpress.org/ticket/61545#comment:6

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

westonruter · 2024-08-13T21:23:03Z

✅ Behavior looks good.

Before:

<picture
  class="not-transparent wp-picture-155"
  style="--dominant-color: #594e4a; display: contents"
  ><source
    type="image/webp"
    srcset="
      http://localhost:8888/wp-content/uploads/2024/08/image-1-jpeg.webp     1300w,
      http://localhost:8888/wp-content/uploads/2024/08/image-1-300x201.webp   300w,
      http://localhost:8888/wp-content/uploads/2024/08/image-1-1024x688.webp 1024w,
      http://localhost:8888/wp-content/uploads/2024/08/image-1-768x516.webp   768w
    "
    sizes="(max-width: 650px) 100vw, 650px" />
  <img
    data-dominant-color="594e4a"
    data-has-transparency="false"
    decoding="async"
    width="1300"
    height="873"
    src="http://localhost:8888/wp-content/uploads/2024/08/image-1-jpeg.webp"
    alt=""
    class="not-transparent wp-image-155"
    srcset="
      http://localhost:8888/wp-content/uploads/2024/08/image-1-jpeg.webp     1300w,
      http://localhost:8888/wp-content/uploads/2024/08/image-1-300x201.webp   300w,
      http://localhost:8888/wp-content/uploads/2024/08/image-1-1024x688.webp 1024w,
      http://localhost:8888/wp-content/uploads/2024/08/image-1-768x516.webp   768w
    "
    sizes="(max-width: 650px) 100vw, 650px"
/></picture>

After:

<img
  data-dominant-color="594e4a"
  data-has-transparency="false"
  style="--dominant-color: #594e4a"
  decoding="async"
  width="1300"
  height="873"
  src="http://localhost:8888/wp-content/uploads/2024/08/image-1-jpeg.webp"
  alt=""
  class="not-transparent wp-image-155"
  srcset="
    http://localhost:8888/wp-content/uploads/2024/08/image-1-jpeg.webp     1300w,
    http://localhost:8888/wp-content/uploads/2024/08/image-1-300x201.webp   300w,
    http://localhost:8888/wp-content/uploads/2024/08/image-1-1024x688.webp 1024w,
    http://localhost:8888/wp-content/uploads/2024/08/image-1-768x516.webp   768w
  "
  sizes="(max-width: 650px) 100vw, 650px"
/>

Settings screen:

mukeshpanchal27 · 2024-08-14T04:19:09Z

#1450 (comment) the core issue is fixed now in https://core.trac.wordpress.org/changeset/58893

sagarnasit added 2 commits August 8, 2024 18:31

Don't wrap picture element if jpeg fallback not available

c8af496

Update test case to remove picture element

50094f0

adamsilverstein reviewed Aug 8, 2024

View reviewed changes

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

sagarnasit added 3 commits August 9, 2024 12:13

Add JPEG fallback notice for picture element setting

ec72890

Update fallback check with original mime type

ba3a6ed

Resolve merge conflict in test cases

c845eb4

sagarnasit marked this pull request as ready for review August 9, 2024 14:44

sagarnasit requested review from felixarntz and westonruter as code owners August 9, 2024 14:45

westonruter reviewed Aug 9, 2024

View reviewed changes

plugins/webp-uploads/settings.php Outdated Show resolved Hide resolved

plugins/webp-uploads/settings.php Outdated Show resolved Hide resolved

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

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

sagarnasit added 3 commits August 9, 2024 21:59

Update JPEG fallback notice

55a25fe

Update mime-type check for fallback image

61bebf2

Add test case for origianl fallback image

7ae26d6

sagarnasit added 2 commits August 11, 2024 15:48

Update original image fallback test case

16a646f

Enable 'Original JPEG' after image upload

934a7f0

mukeshpanchal27 reviewed Aug 12, 2024

View reviewed changes

sagarnasit added 2 commits August 12, 2024 15:23

Remove srcset asserts and add markup check assert

398c213

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

c4ae260

… fix/1447-skip-picture-if-no-jpeg

westonruter reviewed Aug 12, 2024

View reviewed changes

sagarnasit added 2 commits August 13, 2024 12:16

Improve JPEG fallback notice

574d7f5

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

da7d55f

… fix/1447-skip-picture-if-no-jpeg

mukeshpanchal27 reviewed Aug 13, 2024

View reviewed changes

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

Remove PICTURE tag assert

bf1491a

westonruter approved these changes Aug 13, 2024

View reviewed changes

mukeshpanchal27 approved these changes Aug 14, 2024

View reviewed changes

mukeshpanchal27 merged commit 4f4a54f into WordPress:trunk Aug 14, 2024

westonruter changed the title ~~Don't wrap picture element if jpeg fallback not available~~ Don't wrap PICTURE element if JPEG fallback is not available Aug 16, 2024

	<p><?php esc_html_e( 'Picture elements will only be used when JPEG fallback images are available. So this will not apply to any images you uploaded while the "Also generate JPEG" setting was disabled.', 'webp-uploads' ); ?></p>
	<p><?php esc_html_e( 'Picture elements will only be used when JPEG fallback images are available. So this will not apply to any images you may have uploaded while the "Also generate JPEG" setting was disabled.', 'webp-uploads' ); ?></p>

Don't wrap PICTURE element if JPEG fallback is not available #1450

Don't wrap PICTURE element if JPEG fallback is not available #1450

Uh oh!

Conversation

sagarnasit commented Aug 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Uh oh!

github-actions bot commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unlinked Accounts

Uh oh!

westonruter left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sagarnasit commented Aug 11, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

westonruter Aug 12, 2024

Choose a reason for hiding this comment

Uh oh!

mukeshpanchal27 commented Aug 13, 2024

Uh oh!

sagarnasit commented Aug 13, 2024

Uh oh!

westonruter commented Aug 13, 2024

Uh oh!

westonruter commented Aug 13, 2024

Uh oh!

westonruter commented Aug 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

westonruter commented Aug 13, 2024

Uh oh!

mukeshpanchal27 commented Aug 13, 2024

Uh oh!

westonruter commented Aug 13, 2024

Uh oh!

westonruter commented Aug 13, 2024

Uh oh!

Uh oh!

westonruter commented Aug 13, 2024

Uh oh!

mukeshpanchal27 commented Aug 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sagarnasit commented Aug 8, 2024 •

edited

Loading

github-actions bot commented Aug 9, 2024 •

edited

Loading

westonruter left a comment •

edited

Loading

westonruter commented Aug 13, 2024 •

edited

Loading