Skip to content

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

Merged
westonruter merged 12 commits into from
Jul 9, 2024
Merged

Picture element images: Add missing alt text #1319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
2aca958
Add alt tag to original_image_without_srcset
devansh016 Jun 25, 2024
3895dc3
Revert "Add alt tag to original_image_without_srcset"
devansh016 Jun 26, 2024
d73fa78
Add srcset back to the original image
devansh016 Jun 26, 2024
5f8a6ad
Merge branch 'WordPress:trunk' into fix-missing-alt-text
devansh016 Jun 26, 2024
16fcca0
Update testcase
devansh016 Jun 26, 2024
ed39189
Remove unuse code
devansh016 Jul 2, 2024
d529d9a
Add test for alt attribute and improve test for source
devansh016 Jul 4, 2024
3d6d3fe
Merge branch 'trunk' into fix-missing-alt-text
devansh016 Jul 4, 2024
7d4e568
Improve testcase
devansh016 Jul 4, 2024
49648e6
Remove redundant tests
devansh016 Jul 4, 2024
7329e54
Test less verbose
devansh016 Jul 5, 2024
04374f8
Improve testcase structure
devansh016 Jul 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 1 addition & 15 deletions plugins/webp-uploads/picture-element.php
View file
Open in desktop
Copy link
Copy Markdown
Member

@thelovekesh thelovekesh Jun 25, 2024

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
Expand Up @@ -131,24 +131,10 @@ function webp_uploads_wrap_image_in_picture( string $image, string $context, int
);
}

// Fall back to the original image without a srcset.
$original_sizes = array( $image_src[1], $image_src[2] );
$original_image = wp_get_original_image_url( $attachment_id );
// Fail gracefully if the original image is not found.
if ( false === $original_image ) {
return $image;
}
$filter = static function (): bool {
return false;
};
add_filter( 'wp_calculate_image_srcset_meta', $filter );
$original_image_without_srcset = wp_get_attachment_image( $attachment_id, $original_sizes, false, array( 'src' => $original_image ) );
remove_filter( 'wp_calculate_image_srcset_meta', $filter );

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

@mukeshpanchal27 mukeshpanchal27 Jul 1, 2024

Choose a reason for hiding this comment

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

@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?

Copy link
Copy Markdown
Member

@westonruter westonruter Jul 1, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@devansh016 devansh016 Jul 4, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@mukeshpanchal27 mukeshpanchal27 Jul 8, 2024

Choose a reason for hiding this comment

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

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.

);
}
74 changes: 58 additions & 16 deletions plugins/webp-uploads/tests/test-picture-element.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,12 @@ class Test_WebP_Uploads_Picture_Element extends TestCase {
*
* @dataProvider data_provider_it_should_maybe_wrap_images_in_picture_element
*
* @param bool $jpeg_and_webp Whether to enable JPEG and WebP output.
* @param bool $picture_element Whether to enable picture element output.
* @param bool $expect_picture_element Whether to expect the image to be wrapped in a picture element.
* @param bool $jpeg_and_webp Whether to enable JPEG and WebP output.
* @param bool $picture_element Whether to enable picture element output.
* @param bool $expect_picture_element Whether to expect the image to be wrapped in a picture element.
* @param string $expected_html The expected HTML output.
*/
public function test_maybe_wrap_images_in_picture_element( bool $jpeg_and_webp, bool $picture_element, bool $expect_picture_element ): void {
public function test_maybe_wrap_images_in_picture_element( bool $jpeg_and_webp, bool $picture_element, bool $expect_picture_element, string $expected_html ): void {
$mime_type = 'image/webp';
if ( ! wp_image_editor_supports( array( 'mime_type' => $mime_type ) ) ) {
$this->markTestSkipped( "Mime type $mime_type is not supported." );
Expand All @@ -37,49 +38,90 @@ public function test_maybe_wrap_images_in_picture_element( bool $jpeg_and_webp,
$attachment_id = self::factory()->attachment->create_upload_object( TESTS_PLUGIN_DIR . '/tests/data/images/leaves.jpg' );

// Create some content with the image.
$the_image = wp_get_attachment_image( $attachment_id, 'medium', false, array( 'class' => "wp-image-{$attachment_id}" ) );
$the_image = wp_get_attachment_image(
$attachment_id,
'medium',
false,
array(
'class' => "wp-image-{$attachment_id}",
'alt' => 'Green Leaves',
)
);

// Apply the wp_content_img_tag filter.
$the_image = apply_filters( 'wp_content_img_tag', $the_image, 'the_content', $attachment_id );
$processor = new WP_HTML_Tag_Processor( $the_image );
$this->assertTrue( $processor->next_tag( array( 'tag_name' => 'IMG' ) ) );
$width = (int) $processor->get_attribute( 'width' );
$height = (int) $processor->get_attribute( 'height' );
$alt = (string) $processor->get_attribute( 'alt' );

// Check that the image is wrapped in a picture element with the correct class.
$picture_element = sprintf( '<picture class="wp-picture-%s" style="display: contents;">', $attachment_id );
if ( $expect_picture_element ) {
$this->assertStringContainsString( $picture_element, $the_image );
} else {
$this->assertStringNotContainsString( $picture_element, $the_image );
$size_to_use = ( $width > 0 && $height > 0 ) ? array( $width, $height ) : 'full';
$image_src = wp_get_attachment_image_src( $attachment_id, $size_to_use );
list( $src, $width, $height ) = $image_src;
$size_array = array( absint( $width ), absint( $height ) );
$image_meta = wp_get_attachment_metadata( $attachment_id );
$sizes = wp_calculate_image_sizes( $size_array, $src, $image_meta, $attachment_id );
$image_srcset = wp_get_attachment_image_srcset( $attachment_id, $size_to_use );

$img_src = '';
if ( $image_src ) {
$img_src = $image_src[0];
}
// Remove the last size in the srcset, as it is not needed.
$jpeg_srcset = substr( $image_srcset, 0, strrpos( $image_srcset, ',' ) );
$webp_srcset = str_replace( '.jpg', '-jpg.webp', $jpeg_srcset );

// Prepare the expected HTML by replacing placeholders with expected values.
$replacements = array(
'{{img-width}}' => $width,
'{{img-height}}' => $height,
'{{img-src}}' => $img_src,
'{{img-attachment-id}}' => $attachment_id,
'{{img-alt}}' => $alt,
'{{img-srcset}}' => $image_srcset,
'{{img-sizes}}' => $sizes,
'{{jpeg-srcset}}' => $jpeg_srcset,
'{{webp-srcset}}' => $webp_srcset,
);

$expected_html = str_replace( array_keys( $replacements ), array_values( $replacements ), $expected_html );

// Apply the wp_content_img_tag filter.
$the_image = apply_filters( 'wp_content_img_tag', $the_image, 'the_content', $attachment_id );

// When both features are enabled, the picture element will contain two srcset elements.
$this->assertEquals( ( $jpeg_and_webp && $expect_picture_element ) ? 2 : 1, substr_count( $the_image, 'srcset=' ) );
// Check that the image has the expected HTML.
$this->assertEquals( $expected_html, $the_image );
}

/**
* Data provider for it_should_maybe_wrap_images_in_picture_element.
*
* @return array<string, array<string, bool>>
* @return array<string, array<string, bool|string>>
*/
public function data_provider_it_should_maybe_wrap_images_in_picture_element(): array {
return array(
'jpeg and picture enabled' => array(
'jpeg_and_webp' => true,
'picture_element' => true,
'expect_picture_element' => true,
'expected_html' => '<picture class="wp-picture-{{img-attachment-id}}" style="display: contents;"><source type="image/webp" srcset="{{webp-srcset}}" sizes="{{img-sizes}}"><source type="image/jpeg" srcset="{{jpeg-srcset}}" sizes="{{img-sizes}}"><img width="{{img-width}}" height="{{img-height}}" src="{{img-src}}" class="wp-image-{{img-attachment-id}}" alt="{{img-alt}}" decoding="async" loading="lazy" srcset="{{img-srcset}}" sizes="{{img-sizes}}" /></picture>',
),
'only picture enabled' => array(
'jpeg_and_webp' => false,
'picture_element' => true,
'expect_picture_element' => true,
'expected_html' => '<picture class="wp-picture-{{img-attachment-id}}" style="display: contents;"><source type="image/webp" srcset="{{webp-srcset}}" sizes="{{img-sizes}}"><img width="{{img-width}}" height="{{img-height}}" src="{{img-src}}" class="wp-image-{{img-attachment-id}}" alt="{{img-alt}}" decoding="async" loading="lazy" srcset="{{img-srcset}}" sizes="{{img-sizes}}" /></picture>',
),
'only jpeg enabled' => array(
'jpeg_and_webp' => true,
'picture_element' => false,
'expect_picture_element' => false,
'expected_html' => '<img width="{{img-width}}" height="{{img-height}}" src="{{img-src}}" class="wp-image-{{img-attachment-id}}" alt="{{img-alt}}" decoding="async" loading="lazy" srcset="{{img-srcset}}" sizes="{{img-sizes}}" />',
),
'neither enabled' => array(
'jpeg_and_webp' => false,
'picture_element' => false,
'expect_picture_element' => false,
'expected_html' => '<img width="{{img-width}}" height="{{img-height}}" src="{{img-src}}" class="wp-image-{{img-attachment-id}}" alt="{{img-alt}}" decoding="async" loading="lazy" srcset="{{img-srcset}}" sizes="{{img-sizes}}" />',
),
);
}
Expand Down