Picture element images: Add missing alt text #1319

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

Merged

westonruter merged 12 commits into WordPress:trunk from devansh016:fix-missing-alt-text

Jul 9, 2024

Contributor

devansh016 commented Jun 25, 2024

Summary

Add alt text to original_image_without_srcset variable which was missing.

Fixes #1315

Relevant technical choices

For maintainers only, please make sure:

PR has a [Type] label.
PR has a plugin-specific milestone, or the no milestone label if it does not apply to any specific plugin.
PR has a changelog-friendly title, or the skip changelog label if it should not be mentioned in the plugin's changelog.
-->


          Add alt tag to original_image_without_srcset

2aca958

devansh016 requested review from adamsilverstein, felixarntz and westonruter as code owners

June 25, 2024 10:48

github-actions bot commented Jun 25, 2024 •

edited

Loading

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: devansh016 <devansh2002@git.wordpress.org>
Co-authored-by: thelovekesh <thelovekesh@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: adamsilverstein <adamsilverstein@git.wordpress.org>
Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org>

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

mukeshpanchal27 added [Type] Bug [Plugin] Modern Image Formats no milestone [Module] Picture Element labels

thelovekesh reviewed

View reviewed changes

plugins/webp-uploads/picture-element.php

Member

thelovekesh Jun 25, 2024

It might need some test cases to be written/updated?

westonruter requested changes

View reviewed changes

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

westonruter reviewed

View reviewed changes

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

devansh016 added 2 commits

June 26, 2024 11:54


          Revert "Add alt tag to original_image_without_srcset"

3895dc3

This reverts commit 2aca958.


          Add srcset back to the original image

d73fa78

westonruter reviewed

View reviewed changes

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

devansh016 and others added 2 commits

June 27, 2024 02:18


          Merge branch 'WordPress:trunk' into fix-missing-alt-text

5f8a6ad


          Update testcase

16fcca0

adamsilverstein reviewed

View reviewed changes

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

westonruter reviewed

View reviewed changes

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

mukeshpanchal27 requested changes

View reviewed changes

Member

mukeshpanchal27 left a comment

Thanks @devansh016 for working on it. Left one question and code suggestion.

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

plugins/webp-uploads/picture-element.php

               	return sprintf(
               		'<picture class="%s" style="display: contents;">%s%s</picture>',
               		esc_attr( 'wp-picture-' . $attachment_id ),
               		$picture_sources,
-              		$original_image_without_srcset
+              		$image

Member

mukeshpanchal27 Jul 1, 2024

@adamsilverstein As a fallback image, we should return the original image. In this MDN documentation, they return the original image instead of mime-type-specific images.

I couldn't find proper documentation for this. What do you think?

Member

westonruter Jul 1, 2024

Yes, I think this is right.

Also, I think the source being generated for 'image/jpeg' can also be removed as well, because if the browser doesn't support AVIF/WebP then it can just fall back to the img which is a JPEG.

Contributor Author

devansh016 Jul 4, 2024

performance/plugins/webp-uploads/picture-element.php

Lines 56 to 64 in 1ed2fbd

    
           $enabled_mime_types = (array) apply_filters( 
        
           	'webp_uploads_picture_element_mime_types', 
        
           	array( 
        
           		'image/avif', 
        
           		'image/webp', 
        
           		'image/jpeg', 
        
           	), 
        
           	$attachment_id 
        
           );

Should we remove line 61 for the same.

Member

mukeshpanchal27 Jul 8, 2024

I think we can open a separate PR for this issue to avoid complicating the current one. This will allow us to address it more effectively.

westonruter reviewed

View reviewed changes

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

devansh016 and others added 5 commits

July 2, 2024 12:00


          Remove unuse code

ed39189


          Add test for alt attribute and improve test for source

d529d9a


          Merge branch 'trunk' into fix-missing-alt-text

3d6d3fe


          Improve testcase

7d4e568


          Remove redundant tests

49648e6

westonruter reviewed

View reviewed changes

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


          Test less verbose

7329e54

westonruter approved these changes

View reviewed changes

Member

westonruter left a comment

LGTM

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

westonruter requested a review from adamsilverstein

July 8, 2024 18:18

westonruter reviewed

View reviewed changes

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


          Improve testcase structure

04374f8

Co-authored-by: Weston Ruter <westonruter@google.com>

mukeshpanchal27 approved these changes

View reviewed changes

westonruter merged commit 37e6882 into WordPress:trunk

swissspidy removed the [Module] Picture Element label

westonruter added no milestone and removed no milestone labels

westonruter added this to the webp-uploads 2.0.2 milestone

westonruter removed the no milestone label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

westonruter westonruter approved these changes

thelovekesh thelovekesh left review comments

mukeshpanchal27 mukeshpanchal27 approved these changes

felixarntz Awaiting requested review from felixarntz felixarntz is a code owner

adamsilverstein Awaiting requested review from adamsilverstein adamsilverstein is a code owner

Labels

[Plugin] Modern Image Formats [Type] Bug