Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update the_content with the appropiate image format #152

Merged
merged 31 commits into from
Feb 28, 2022

Conversation

mitogh
Copy link
Member

@mitogh mitogh commented Feb 4, 2022

Summary

Uses image/webp if supported by the client and updates the image/jpeg, image/jpg formats with the modern format if available.

References in the content are updated with the WebP format if available.

Fixes:

Related with:

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.

Uses `image/webp` if supported by the client and updates the
`image/jpeg`, `image/jpg` formats with the modern format if avaiable.

References in the content are updated with the WebP format if available.

Fixes #149
@mitogh mitogh self-assigned this Feb 4, 2022
@mitogh mitogh marked this pull request as draft February 4, 2022 16:26
@mitogh mitogh added this to the 1.0.0-beta.1 milestone Feb 4, 2022
@mitogh mitogh marked this pull request as ready for review February 25, 2022 03:01
@mitogh mitogh added [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) [Type] Feature A new feature within an existing module labels Feb 25, 2022
@mitogh mitogh changed the base branch from feature/142-multiple-mimes-for-image-sizes to feature/142-multiple-mimes-for-image-sizes-with-retry February 25, 2022 17:22
The assertion would make sure both are not the same, meaning
at least one has a different markup.
@adamsilverstein
Copy link
Member

This worked well in my testing across a variety of themes. The only issue I noticed was when inserting an image at the "large" size with a large image upload that results in a -scaled image, the jpeg is used for the image src, since the webp version isn't available yet (see #174).

image

@mitogh
Copy link
Member Author

mitogh commented Feb 25, 2022

Thanks for taking the time to test it, currently this is expected and described here:

if ( isset( $metadata['file'] ) && strpos( $url, $basename ) !== false ) {
// TODO: we don't have a replacement for full image yet, issue. See: https://github.com/WordPress/performance/issues/174.
continue;
}

Based on the decision from:

The code would be updated accordingly if required.

mitogh and others added 5 commits February 25, 2022 14:57
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
…/performance into feature/149-update-the-content
@felixarntz
Copy link
Member

@adamsilverstein

This worked well in my testing across a variety of themes. The only issue I noticed was when inserting an image at the "large" size with a large image upload that results in a -scaled image, the jpeg is used for the image src, since the webp version isn't available yet (see #174).

image

I noticed that as well, it depends on the viewport size and pixel density. Since we currently don't have WebP versions for the "full" size (whether -scaled or original), it's best to currently test this PR on a mobile viewport since there it's most likely to get served a WebP image.

The problem is that most modern screens have a high pixel density like @2x, which means if I look at an image of 600px (or more accurately in a viewport of 600px), the browser will look for a size >=1200px, which would already be more than WordPress core's large by default.

If you want to test this more specifically without the above caveats, you can put a temporary filter in place to ensure WordPress never chooses a size greater than the image size you actually specified (which may cause blurry images on larger or high-density devices though):

add_filter(
	'wp_calculate_image_srcset',
	function( $sources, $size_array ) {
		foreach ( $sources as $index => $source ) {
			if ( 'w' === $source['descriptor'] && $source['value'] > $size_array[0] ) {
				unset( $sources[ $index ] );
			}
		}
		return $sources;
	},
	10,
	2
);

There's certainly room for optimization in core's default behavior there, but that's a separate rabbit hole :)

Right, this answers that question for me - we need the full size webp available for the correct markup when users select the full size image.

I agree this gives us an answer for #174, that we should also generate a WebP version of the full size.

Move logic to replace image references into a separate function,
update tests accordingly as well.
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mitogh Great! I left a few follow-up comments, mostly minor though. This is almost ready.

modules/images/webp-uploads/load.php Outdated Show resolved Hide resolved
modules/images/webp-uploads/load.php Outdated Show resolved Hide resolved
modules/images/webp-uploads/load.php Outdated Show resolved Hide resolved
modules/images/webp-uploads/load.php Outdated Show resolved Hide resolved
modules/images/webp-uploads/load.php Outdated Show resolved Hide resolved
modules/images/webp-uploads/load.php Show resolved Hide resolved
modules/images/webp-uploads/load.php Show resolved Hide resolved
modules/images/webp-uploads/load.php Outdated Show resolved Hide resolved
mitogh and others added 5 commits February 25, 2022 19:40
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Adding `$context` parameter to match what core uses inside of `wp_filter_content_tags`
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one open discussion is the only thing we need to finalize before we can merge this.

modules/images/webp-uploads/load.php Outdated Show resolved Hide resolved
Update mechanism to use WP core mechanism to parse image
tags inside of the content of the post.
@mitogh
Copy link
Member Author

mitogh commented Feb 28, 2022

Thanks, @felixarntz just updated to code to follow what WP Core uses in order to reduce the friction in the scenario this would be merged into WP Core.

Let me know in case you have any additional feedback.

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work @mitogh!

modules/images/webp-uploads/load.php Outdated Show resolved Hide resolved
Copy link
Member

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks great to me, thanks for the structural changes that will make prepping this for core more straightforward. Also retested and everything worked well in my testing.

@felixarntz felixarntz merged commit 81b2171 into trunk Feb 28, 2022
@mitogh mitogh deleted the feature/149-update-the-content branch February 28, 2022 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) [Type] Feature A new feature within an existing module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants