Provide fallback JPEG images in frontend when WebP is not supported by the browser

[eugene-manuilov]

Summary

Fixes #341

Relevant technical choices

Checklist

• PR has either [Focus] or Infrastructure label.
• PR has a [Type] label.
• PR has a milestone or the no milestone label.

[mukeshpanchal27]

@eugene-manuilov Is it possible that we can check the dynamic array key value based on webp_uploads_get_upload_image_mime_transforms() function data because that function use one filter webp_uploads_upload_image_mime_transforms to alter that mime type transform and and edge case user change it then it create fatal error in checking.

For filter edge cases please check #361 in which I added some cases for the filter.

[eugene-manuilov]

@mukeshpanchal27 i have added a check to make sure that the mime types transforms array is an array.

[mitogh]

Small suggestion would be to replace the array with an object structure in that sense you don't need to check if the item exists as you can set it regardless like:

ids[ attachment[1] ] = true;

Then you can just return the keys from it, if you only need the values:

return Object.keys( ids );

[felixarntz]

@eugene-manuilov Great work so far!

I still think we should think through the differences in approach 2 vs 3 a bit further. You went with the latter which is more reliable, but it also comes with more negative performance implications, e.g. the REST requests, and additional document.querySelectorAll calls - with approach 2 we could just once iterate through img tags and update all their src and srcset files.

With that said, indeed approach 3 is more reliable, and maybe that is the better option here. Curious what you think.

[mukeshpanchal27]

@mukeshpanchal27 i have added a check to make sure that the mime types transforms array is an array.

Thanks for the update @eugene-manuilov

$transforms = webp_uploads_get_upload_image_mime_transforms(); always return array value as per filter code snippets.

/**
 * Filter to allow the definition of a custom mime types, in which a defined mime type
 * can be transformed and provide a wide range of mime types.
 *
 * The order of supported mime types matters. If the original mime type of the uploaded image
 * is not needed, then the first mime type in the list supported by the image editor will be
 * selected for the default subsizes.
 *
 * @since 1.0.0
 *
 * @param array $default_transforms A map with the valid mime transforms.
 */
$transforms = (array) apply_filters( 'webp_uploads_upload_image_mime_transforms', $default_transforms );

It's worth checking the mime types transforms array is an array.

For the below edge case of webp_uploads_upload_image_mime_transforms filter it creates an error in current PR.

1. If the filter returns an empty value.

    add_filter('webp_uploads_upload_image_mime_transforms', function () {
    	return;
    });
   

2. If the filter returns PNG mime transforms in the array.

    add_filter('webp_uploads_upload_image_mime_transforms', function () {
    	return [
    		'image/png' =>  [ 'image/png', 'image/webp' ]
    	];
    });
   

To tackle this we should check isset value in the array respectively. Will add a review on the PR soon.

[mitogh]

Small question besides that looking good.

[adamsilverstein]

I did some testing with Browserstack across several older Safari versions on Mac OS Sierra, El Capitan and old iOS devices like iPhone 6. The approach worked well in my testing in all cases and the image loaded correctly after a brief delay. Still reviewing code.

Screen.Recording.2022-06-17.at.04.33.23.PM.mp4

[felixarntz]

@eugene-manuilov Thank you for the updates, this is looking great. I also agree with you now that we should use the REST based approach - it will work better for any WebP images that don't have JPEG versions available.

I left a few more comments with follow-up feedback. Let's also add at least some basic PHPUnit tests later, to ensure that the script is only loaded when needed.

[felixarntz]

@eugene-manuilov Excellent work, LGTM!

[adamsilverstein]

Great work!

[felixarntz]

@peterwilsoncc Would you mind reviewing this one as well?

[peterwilsoncc]

I've added some notes inline.

The only things I'd consider blockers are the escaping of URLs.

[peterwilsoncc]

Will the file name always match?

The thought behind the question: Someone uploads the image hamilton-the-musical.jpg, this will generate fallback image hamilton-the-musical.webp. Later the image hamilton-the-musical.jpeg is uploaded (note the e), this will generate images of a different name for the fallback -- probably hamilton-the-musical-2.webp.

🔢 The same question applies for srcset.

[eugene-manuilov]

This is actually a good point but I think it is out of scope for this issue. The problem is bigger that just properly replacing webp files here. I tried to replicate your edge case and it looks like in this case webp images for the second image will override webp images for the first image 🤔.

I uploaded 20180607_144302.jpg image first, webp images have been created correctly. Then I uploaded the same image but with another extension 20180607_144302.jpeg and in this time webp images overriden webp images generated for the first image. Here is what I got on the server:

-rw-r--r-- 1 www-data www-data 161891 Jul  8 18:15 20180607_144302-1024x576.jpeg
-rw-r--r-- 1 www-data www-data 161891 Jul  8 18:13 20180607_144302-1024x576.jpg
-rw-r--r-- 1 www-data www-data 170276 Jul  8 18:14 20180607_144302-1024x576.webp
-rw-r--r-- 1 www-data www-data   8220 Jul  8 18:15 20180607_144302-150x150.jpeg
-rw-r--r-- 1 www-data www-data   8220 Jul  8 18:13 20180607_144302-150x150.jpg
-rw-r--r-- 1 www-data www-data   8724 Jul  8 18:14 20180607_144302-150x150.webp
-rw-r--r-- 1 www-data www-data 330858 Jul  8 18:15 20180607_144302-1536x864.jpeg
-rw-r--r-- 1 www-data www-data 330858 Jul  8 18:14 20180607_144302-1536x864.jpg
-rw-r--r-- 1 www-data www-data 331106 Jul  8 18:14 20180607_144302-1536x864.webp
-rw-r--r-- 1 www-data www-data 529186 Jul  8 18:15 20180607_144302-2048x1152.jpeg
-rw-r--r-- 1 www-data www-data 529186 Jul  8 18:14 20180607_144302-2048x1152.jpg
-rw-r--r-- 1 www-data www-data 506556 Jul  8 18:14 20180607_144302-2048x1152.webp
-rw-r--r-- 1 www-data www-data  17533 Jul  8 18:15 20180607_144302-300x169.jpeg
-rw-r--r-- 1 www-data www-data  17533 Jul  8 18:13 20180607_144302-300x169.jpg
-rw-r--r-- 1 www-data www-data  18692 Jul  8 18:14 20180607_144302-300x169.webp
-rw-r--r-- 1 www-data www-data  96605 Jul  8 18:15 20180607_144302-768x432.jpeg
-rw-r--r-- 1 www-data www-data  96605 Jul  8 18:14 20180607_144302-768x432.jpg
-rw-r--r-- 1 www-data www-data 102924 Jul  8 18:14 20180607_144302-768x432.webp
-rw-r--r-- 1 www-data www-data 934789 Jul  8 18:15 20180607_144302.jpeg
-rw-r--r-- 1 www-data www-data 934789 Jul  8 18:13 20180607_144302.jpg
-rw-r--r-- 1 www-data www-data 826980 Jul  8 18:14 20180607_144302.webp

This doesn't look right although this is a very unlikely edge case. I think it is worth creating a separate issue for it 🤔.

cc @felixarntz @adamsilverstein

[adamsilverstein]

This was previously discussed and should have been fixed in the plugin - see #358 - not sure why you still see this happening. I'm not sure this fix is great though, it skips the duplicate generation; instead behavior should be like or use wp_unique_filename.

For core I am working on including this in the core patch as well, I tried one approach in 93542af (#2393) - that isn't quite finished - note there can actually be three variations - cat.jpe, cat.jpeg, and cat.jpg - yes ".jpe" is also valid!

Another option would be to incorporate the fix into WP_Image_Editor::generate_filename or save - again the tricky part is making sure overwriting works when you want it to (ie. regenerate images).

[peterwilsoncc]

This is actually a good point but I think it is out of scope for this issue. The problem is bigger that just properly replacing webp files here.

Thanks for testing this. I'm happy if y'all decide it is out of scope for this ticket. It seems like a much bigger problem than I thought when asking the question :)

[felixarntz]

The problem of avoiding the override was indeed fixed in #358 - however that shouldn't have affected the PR here anyway, since realistically the problem is only on the JPEG (jpeg/jpg/jpe) side. Now that #358 is in place, no conflicting WebP image would be created so we should be good for now.

With that said, I agree this is a valid point, and I think a much more stable implementation for the replacement would be to replace the full file name for the image/webp version per size with its corresponding full file name for the image/jpeg version. Let's open a follow-up issue to add that.

[peterwilsoncc]

Couple more notes, nothing major.

[peterwilsoncc]

Suggested change

	ob_start();
	wp_footer();
	$footer = ob_get_clean();
	$footer = get_echo( 'wp_footer' );

The WordPress test suite includes a helper for doing this, see source code.

🔢 Applies to other tests in this file too.

[felixarntz]

@eugene-manuilov This looks solid to me. I'll address the few remaining quirks.

@peterwilsoncc Thank you for the additional feedback! Regarding the bigger conversation on more accurate replacement of image file URLs, I think right now this is not a "real" problem because of the way that WebP files are conditionally generated, but it is certainly something we should address to avoid problems in the future. So my take would be to open a follow-up issue.

[felixarntz]

The problem of avoiding the override was indeed fixed in #358 - however that shouldn't have affected the PR here anyway, since realistically the problem is only on the JPEG (jpeg/jpg/jpe) side. Now that #358 is in place, no conflicting WebP image would be created so we should be good for now.

With that said, I agree this is a valid point, and I think a much more stable implementation for the replacement would be to replace the full file name for the image/webp version per size with its corresponding full file name for the image/jpeg version. Let's open a follow-up issue to add that.

[felixarntz]

LGTM!

@peterwilsoncc Your feedback should be addressed, will pass back to you for a final review.

[felixarntz]

Opened #426 as a follow-up issue to enhance the WebP file replacement in the frontend.

[peterwilsoncc]

LGTM, thanks folks.