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.

Sign up for GitHub

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

Jump to bottom

Add picture element support #73

Merged
merged 86 commits into from
May 30, 2024
Merged

Add picture element support #73

merged 86 commits into from
May 30, 2024

Conversation

adamsilverstein
Copy link
Member

@adamsilverstein adamsilverstein commented Dec 31, 2021 •
edited by westonruter
Loading

Fixes #21
Closes #1236

  • Filters on wp_content_img_tag to potentially wrap (media) images in a picture element.
  • Works best with themes that support responsive images, for example twentysixteen.
  • Check the network panel to see which images the browser actually loads.

Picture element and AVIF support by version

AVIF support is being worked on and picture element support can be especially useful for adding AVIFs to your site if you have users on browsers that don't support AVIF.

Browser picture Support (Version, Date) AVIF Support (Version, Date)
Chrome 4 (January 2010) 85 (August 2020)
Edge 12 (July 2015) 121 (January 2024)
Safari 9 (September 2015) 16.1 (October 2023)
Firefox 38 (May 2015) 93 (October 2021)

@adamsilverstein adamsilverstein linked an issue Dec 31, 2021 that may be closed by this pull request
Enable <picture> element support #21
Closed
@adamsilverstein adamsilverstein modified the milestone: 1.0.0-beta.1 Dec 31, 2021
@adamsilverstein adamsilverstein added the [Type] Enhancement A suggestion for improvement of an existing feature label Dec 31, 2021
@eugene-manuilov
Copy link
Contributor

Filters on the_content to potentially wrap (media) images in a picture element.

@adamsilverstein did you add the hook for the the_content filter just for testing purposes or we are going to keep it?

@mitogh mitogh marked this pull request as ready for review January 6, 2022 11:09
@mitogh mitogh marked this pull request as draft January 6, 2022 16:35
@adamsilverstein
Copy link
Member Author

@adamsilverstein did you add the hook for the the_content filter just for testing purposes or we are going to keep it?

The entire PR is for testing.

@adamsilverstein
Copy link
Member Author

reopening to bring this PR up to date...

@adamsilverstein adamsilverstein added this to the webp-uploads n.e.x.t milestone May 21, 2024
@adamsilverstein
Copy link
Member Author

Confirmed this works with AVIF selected as the output format:

image

joemcgill
joemcgill reviewed May 29, 2024
View reviewed changes
Copy link
Member

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

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

This is looking good and surprisingly doesn't seem to impact default styling of images even though the markup changes. Left a couple of questions.

plugins/webp-uploads/picture-element.php Outdated Show resolved Hide resolved
plugins/webp-uploads/picture-element.php Outdated
Comment on lines 78 to 81
// If the original mime types is the only one available, no picture element is needed.
if ( 1 === count( $mime_types ) && $mime_types[0] === $original_file_mime_type ) {
return $image;
}
Copy link
Member

@joemcgill joemcgill May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For sites that don't support AVIF or WebP, if someone uploads an original in one of those formats, won't all the intermediate sizes be JPEG? If so, do we want this to result in using picture or would a simple img element be preferred?

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't tested with WebP support missing, but my expectation is that you shouldn't be able to upload a WebP image if your server doesn't support it. This may not be working correctly in the block editor currently, so for now I am referring to uploading in the media library. I'm not sure I have a test version of PHP that doesn't support WebP.

~98% of WordPress sites are on a host that supports WebP so this is an edge case.

Copy link
Member Author

Choose a reason for hiding this comment

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

@joemcgill - I created this follow up Issue to work on improving the settings screen in particular when WebP isn't supported.

@westonruter
Copy link
Member

I found that the Settings link was pointing to the "Also output JPEG" checkbox:

image

I've fixed this in 2e68884 so that the settings link points to the entire section.

adamsilverstein and others added 5 commits May 29, 2024 15:47
@adamsilverstein @westonruter
Update plugins/webp-uploads/settings.php
e88caca 
Co-authored-by: Weston Ruter <westonruter@google.com>
@adamsilverstein @westonruter
Update plugins/webp-uploads/picture-element.php
2d57e43 
Co-authored-by: Weston Ruter <westonruter@google.com>
@adamsilverstein @westonruter
Update plugins/webp-uploads/settings.php
6d024de 
Co-authored-by: Weston Ruter <westonruter@google.com>
@adamsilverstein @westonruter
Update plugins/webp-uploads/settings.php
7d5f911 
Co-authored-by: Weston Ruter <westonruter@google.com>
@westonruter
fixup! Update settings link to point to the entire settings section
86a03f2
@adamsilverstein
Copy link
Member Author

I found that the Settings link was pointing to the "Also output JPEG" checkbox:

image

I've fixed this in 2e68884 so that the settings link points to the entire section.

Nice catch!

westonruter
westonruter approved these changes May 29, 2024
View reviewed changes
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

With https://github.com/WordPress/performance/pull/73/files#r1619520322 this looks good to go! 🥳

@adamsilverstein @westonruter
Update plugins/webp-uploads/settings.php
70400c8 
Co-authored-by: Weston Ruter <westonruter@google.com>
@adamsilverstein
Copy link
Member Author

With #73 (files) this looks good to go! 🥳

Thanks the reviews and feedback🎉 !

@westonruter westonruter requested a review from joemcgill May 29, 2024 22:30
@westonruter westonruter mentioned this pull request May 29, 2024
Merged
@adamsilverstein adamsilverstein mentioned this pull request May 30, 2024
Closed
@adamsilverstein adamsilverstein merged commit d998aa9 into trunk May 30, 2024
14 checks passed
@adamsilverstein adamsilverstein deleted the add/picture-support branch May 30, 2024 13:25
@adamsilverstein adamsilverstein mentioned this pull request May 30, 2024
Closed
@westonruter westonruter added [Type] Feature A new feature within an existing module and removed [Type] Enhancement A suggestion for improvement of an existing feature labels Jun 6, 2024
@swissspidy swissspidy added [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) and removed [Module] Picture Element labels Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spacedmonkey spacedmonkey spacedmonkey left review comments

@westonruter westonruter westonruter approved these changes

@felixarntz felixarntz Awaiting requested review from felixarntz felixarntz is a code owner

@pbearne pbearne Awaiting requested review from pbearne

@mukeshpanchal27 mukeshpanchal27 Awaiting requested review from mukeshpanchal27

@thelovekesh thelovekesh Awaiting requested review from thelovekesh

@joemcgill joemcgill Awaiting requested review from joemcgill

Assignees
No one assigned
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
Milestone
webp-uploads 2.0.0
Development

Successfully merging this pull request may close these issues.

Enable <picture> element support
7 participants
@adamsilverstein @eugene-manuilov @westonruter @spacedmonkey @joemcgill @swissspidy @thelovekesh