Fix: skewed placeholder of product image - issue#7553 (#7651)

* Fix typo in HTML markup of ProductImage block placeholder

Typo caused rendering of incorrect HTML attributes: width and height of Product Image placeholder that were anyway ignored by the browser

* Remove inline styles (width and height) from ImagePlaceholder in ProductElements > Image block

Inline height took precedence over the inherited styles which made the placeholder image skewed (in loading and loaded state). Removal of those styles allows the ImagePlaceholder to adapt the height to the available space keeping the ratio or inherit the styles from the parent

* Remove inline styles (width and height) from placeholder image in ProductImage.php block

Inline styles applied to the placeholder image of ProductImage block were overriden by other styles with higher specificity, which made them redundant. Additionally, corresponding styles were removed from the placeholder image from Editor code as they caused UI glitches. Additional proof that it's safe to remove them is in the first commit in this branch, the purpose of which was to fix those styles as there was a typo which corrupted them and eventually inline width and height were ignored by the browser and not applied to the element

* Add test to check placeholder image renders without width and height inline attributes

This is to prevent adding inline width and height attributes so the sizing of the placeholder image is controlled by the inherited styles or element styles, in the same way as a regular product image

* Fix TypeScript errors in the block test file of Product Image

* Add generic alt text to placeholder image

Alt text added in this commit is a generic text, not description of the actual image. That's because the image itself is set externally through the settings and may change over time

* Revert adding placeholder image alt text and add comments to make the empty alt explicit

After a Github discussion: https://github.com/woocommerce/woocommerce-blocks/pull/7651\#discussion_r1019560494 it was decided the placeholder should NOT have alt text as it serves the purpose of decorative image. According to the guidelines decorative images should not have alt text: https://www.w3.org/WAI/tutorials/images/decorative/. Comments were added also to other places where it happens

* bot: update checkstyle.xml

Co-authored-by: Karol Manijak <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Author: 3 people

assets/js/atomic/blocks/product-elements/image/block.js

@@ -129,9 +129,10 @@ export const Block = ( props ) => {
 };
 
 const ImagePlaceholder = () => {
-	return (
-		<img src={ PLACEHOLDER_IMG_SRC } alt="" width={ 500 } height={ 500 } />
-	);
+	// The alt text is left empty on purpose, as it's considered a decorative image.
+	// More can be found here: https://www.w3.org/WAI/tutorials/images/decorative/.
+	// Github discussion for a context: https://github.com/woocommerce/woocommerce-blocks/pull/7651#discussion_r1019560494.
+	return <img src={ PLACEHOLDER_IMG_SRC } alt="" />;
 };
 
 const Image = ( { image, loaded, showFullSize, fallbackAlt } ) => {

assets/js/atomic/blocks/product-elements/image/test/block.test.js

@@ -62,7 +62,10 @@ describe( 'Product Image Block', () => {
 	describe( 'with product link', () => {
 		test( 'should render an anchor with the product image', () => {
 			const component = render(
-				<ProductDataContextProvider product={ productWithImages }>
+				<ProductDataContextProvider
+					product={ productWithImages }
+					isLoading={ false }
+				>
 					<Block showProductLink={ true } />
 				</ProductDataContextProvider>
 			);
@@ -79,14 +82,17 @@ describe( 'Product Image Block', () => {
 			);
 
 			const anchor = productImage.closest( 'a' );
-			expect( anchor.getAttribute( 'href' ) ).toBe(
+			expect( anchor?.getAttribute( 'href' ) ).toBe(
 				productWithImages.permalink
 			);
 		} );
 
 		test( 'should render an anchor with the placeholder image', () => {
 			const component = render(
-				<ProductDataContextProvider product={ productWithoutImages }>
+				<ProductDataContextProvider
+					product={ productWithoutImages }
+					isLoading={ false }
+				>
 					<Block showProductLink={ true } />
 				</ProductDataContextProvider>
 			);
@@ -97,10 +103,10 @@ describe( 'Product Image Block', () => {
 			);
 
 			const anchor = placeholderImage.closest( 'a' );
-			expect( anchor.getAttribute( 'href' ) ).toBe(
+			expect( anchor?.getAttribute( 'href' ) ).toBe(
 				productWithoutImages.permalink
 			);
-			expect( anchor.getAttribute( 'aria-label' ) ).toBe(
+			expect( anchor?.getAttribute( 'aria-label' ) ).toBe(
 				`Link to ${ productWithoutImages.name }`
 			);
 		} );
@@ -109,7 +115,10 @@ describe( 'Product Image Block', () => {
 	describe( 'without product link', () => {
 		test( 'should render the product image without an anchor wrapper', () => {
 			const component = render(
-				<ProductDataContextProvider product={ productWithImages }>
+				<ProductDataContextProvider
+					product={ productWithImages }
+					isLoading={ false }
+				>
 					<Block showProductLink={ false } />
 				</ProductDataContextProvider>
 			);
@@ -129,7 +138,10 @@ describe( 'Product Image Block', () => {
 
 		test( 'should render the placeholder image without an anchor wrapper', () => {
 			const component = render(
-				<ProductDataContextProvider product={ productWithoutImages }>
+				<ProductDataContextProvider
+					product={ productWithoutImages }
+					isLoading={ false }
+				>
 					<Block showProductLink={ false } />
 				</ProductDataContextProvider>
 			);
@@ -143,4 +155,24 @@ describe( 'Product Image Block', () => {
 			expect( anchor ).toBe( null );
 		} );
 	} );
+
+	describe( 'without image', () => {
+		test( 'should render the placeholder with no inline width or height attributes', () => {
+			const component = render(
+				<ProductDataContextProvider
+					product={ productWithoutImages }
+					isLoading={ false }
+				>
+					<Block showProductLink={ true } />
+				</ProductDataContextProvider>
+			);
+
+			const placeholderImage = component.getByAltText( '' );
+			expect( placeholderImage.getAttribute( 'src' ) ).toBe(
+				'placeholder.jpg'
+			);
+			expect( placeholderImage.getAttribute( 'width' ) ).toBe( null );
+			expect( placeholderImage.getAttribute( 'height' ) ).toBe( null );
+		} );
+	} );
 } );

assets/js/base/components/block-error-boundary/block-error.tsx

@@ -23,6 +23,9 @@ const BlockError = ( {
 	return showErrorBlock ? (
 		<div className="wc-block-error wc-block-components-error">
 			{ imageUrl && (
+				// The alt text is left empty on purpose, as it's considered a decorative image.
+				// More can be found here: https://www.w3.org/WAI/tutorials/images/decorative/.
+				// Github discussion for a context: https://github.com/woocommerce/woocommerce-blocks/pull/7651#discussion_r1019560494.
 				<img
 					className="wc-block-error__image wc-block-components-error__image"
 					src={ imageUrl }

assets/js/base/components/reviews/review-list-item/index.js

@@ -27,6 +27,9 @@ function getReviewImage( review, imageType, isLoading ) {
 					src={ review.product_image?.thumbnail || '' }
 				/>
 			) : (
+				// The alt text is left empty on purpose, as it's considered a decorative image.
+				// More can be found here: https://www.w3.org/WAI/tutorials/images/decorative/.
+				// Github discussion for a context: https://github.com/woocommerce/woocommerce-blocks/pull/7651#discussion_r1019560494.
 				<img
 					aria-hidden="true"
 					alt=""

checkstyle.xml

@@ -1118,10 +1118,10 @@
 <error line="75" column="36" severity="error" message="Property &apos;max_amount&apos; does not exist on type &apos;never&apos;." source="TS2339" />
 </file>
 <file name="assets/js/atomic/blocks/product-elements/image/block.js">
-<error line="137" column="19" severity="error" message="Binding element &apos;image&apos; implicitly has an &apos;any&apos; type." source="TS7031" />
-<error line="137" column="26" severity="error" message="Binding element &apos;loaded&apos; implicitly has an &apos;any&apos; type." source="TS7031" />
-<error line="137" column="34" severity="error" message="Binding element &apos;showFullSize&apos; implicitly has an &apos;any&apos; type." source="TS7031" />
-<error line="137" column="48" severity="error" message="Binding element &apos;fallbackAlt&apos; implicitly has an &apos;any&apos; type." source="TS7031" />
+<error line="138" column="19" severity="error" message="Binding element &apos;image&apos; implicitly has an &apos;any&apos; type." source="TS7031" />
+<error line="138" column="26" severity="error" message="Binding element &apos;loaded&apos; implicitly has an &apos;any&apos; type." source="TS7031" />
+<error line="138" column="34" severity="error" message="Binding element &apos;showFullSize&apos; implicitly has an &apos;any&apos; type." source="TS7031" />
+<error line="138" column="48" severity="error" message="Binding element &apos;fallbackAlt&apos; implicitly has an &apos;any&apos; type." source="TS7031" />
 </file>
 <file name="assets/js/atomic/blocks/product-elements/rating/block.tsx">
 <error line="55" column="35" severity="error" message="Argument of type &apos;{ id: number; name: string; parent: number; type: string; variation: string; permalink: string; sku: string; short_description: string; description: string; on_sale: boolean; prices: { currency_code: string; ... 9 more ...; price_range: null; }; ... 14 more ...; add_to_cart: { ...; }; }&apos; is not assignable to parameter of type &apos;Omit&lt;ProductResponseItem, &quot;average_rating&quot;&gt; &amp; { average_rating: string; }&apos;.
@@ -1430,15 +1430,6 @@
 <error line="252" column="56" severity="error" message="Argument of type &apos;null&apos; is not assignable to parameter of type &apos;Object&apos;." source="TS2345" />
 <error line="475" column="34" severity="error" message="Argument of type &apos;null&apos; is not assignable to parameter of type &apos;Object | undefined&apos;." source="TS2345" />
 </file>
-<file name="assets/js/atomic/blocks/product-elements/image/test/block.test.js">
-<error line="65" column="6" severity="error" message="Property &apos;isLoading&apos; is missing in type &apos;{ children: Element; product: { name: string; id: number; fallbackAlt: string; permalink: string; images: { id: number; src: string; thumbnail: string; srcset: string; sizes: string; name: string; alt: string; }[]; }; }&apos; but required in type &apos;{ product: any; children: Object; isLoading: boolean; }&apos;." source="TS2741" />
-<error line="82" column="12" severity="error" message="Object is possibly &apos;null&apos;." source="TS2531" />
-<error line="89" column="6" severity="error" message="Property &apos;isLoading&apos; is missing in type &apos;{ children: Element; product: { name: string; id: number; fallbackAlt: string; permalink: string; images: never[]; }; }&apos; but required in type &apos;{ product: any; children: Object; isLoading: boolean; }&apos;." source="TS2741" />
-<error line="100" column="12" severity="error" message="Object is possibly &apos;null&apos;." source="TS2531" />
-<error line="103" column="12" severity="error" message="Object is possibly &apos;null&apos;." source="TS2531" />
-<error line="112" column="6" severity="error" message="Property &apos;isLoading&apos; is missing in type &apos;{ children: Element; product: { name: string; id: number; fallbackAlt: string; permalink: string; images: { id: number; src: string; thumbnail: string; srcset: string; sizes: string; name: string; alt: string; }[]; }; }&apos; but required in type &apos;{ product: any; children: Object; isLoading: boolean; }&apos;." source="TS2741" />
-<error line="132" column="6" severity="error" message="Property &apos;isLoading&apos; is missing in type &apos;{ children: Element; product: { name: string; id: number; fallbackAlt: string; permalink: string; images: never[]; }; }&apos; but required in type &apos;{ product: any; children: Object; isLoading: boolean; }&apos;." source="TS2741" />
-</file>
 <file name="assets/js/atomic/blocks/product-elements/title/test/block.test.js">
 <error line="22" column="6" severity="error" message="Property &apos;isLoading&apos; is missing in type &apos;{ children: Element; product: { id: number; name: string; permalink: string; }; }&apos; but required in type &apos;{ product: any; children: Object; isLoading: boolean; }&apos;." source="TS2741" />
 <error line="23" column="7" severity="error" message="Type &apos;{ showProductLink: false; }&apos; is missing the following properties from type &apos;Attributes&apos;: headingLevel, align" source="TS2739" />
@@ -1726,14 +1717,14 @@
 <error line="14" column="26" severity="error" message="Parameter &apos;review&apos; implicitly has an &apos;any&apos; type." source="TS7006" />
 <error line="14" column="34" severity="error" message="Parameter &apos;imageType&apos; implicitly has an &apos;any&apos; type." source="TS7006" />
 <error line="14" column="45" severity="error" message="Parameter &apos;isLoading&apos; implicitly has an &apos;any&apos; type." source="TS7006" />
-<error line="51" column="28" severity="error" message="Parameter &apos;review&apos; implicitly has an &apos;any&apos; type." source="TS7006" />
-<error line="77" column="32" severity="error" message="Parameter &apos;review&apos; implicitly has an &apos;any&apos; type." source="TS7006" />
-<error line="93" column="27" severity="error" message="Parameter &apos;review&apos; implicitly has an &apos;any&apos; type." source="TS7006" />
-<error line="102" column="25" severity="error" message="Parameter &apos;review&apos; implicitly has an &apos;any&apos; type." source="TS7006" />
-<error line="117" column="27" severity="error" message="Parameter &apos;review&apos; implicitly has an &apos;any&apos; type." source="TS7006" />
-<error line="150" column="28" severity="error" message="Binding element &apos;attributes&apos; implicitly has an &apos;any&apos; type." source="TS7031" />
-<error line="160" column="10" severity="error" message="Property &apos;rating&apos; does not exist on type &apos;{}&apos;." source="TS2339" />
-<error line="161" column="20" severity="error" message="Operator &apos;&gt;&apos; cannot be applied to types &apos;boolean&apos; and &apos;number&apos;." source="TS2365" />
+<error line="54" column="28" severity="error" message="Parameter &apos;review&apos; implicitly has an &apos;any&apos; type." source="TS7006" />
+<error line="80" column="32" severity="error" message="Parameter &apos;review&apos; implicitly has an &apos;any&apos; type." source="TS7006" />
+<error line="96" column="27" severity="error" message="Parameter &apos;review&apos; implicitly has an &apos;any&apos; type." source="TS7006" />
+<error line="105" column="25" severity="error" message="Parameter &apos;review&apos; implicitly has an &apos;any&apos; type." source="TS7006" />
+<error line="120" column="27" severity="error" message="Parameter &apos;review&apos; implicitly has an &apos;any&apos; type." source="TS7006" />
+<error line="153" column="28" severity="error" message="Binding element &apos;attributes&apos; implicitly has an &apos;any&apos; type." source="TS7031" />
+<error line="163" column="10" severity="error" message="Property &apos;rating&apos; does not exist on type &apos;{}&apos;." source="TS2339" />
+<error line="164" column="20" severity="error" message="Operator &apos;&gt;&apos; cannot be applied to types &apos;boolean&apos; and &apos;number&apos;." source="TS2365" />
 </file>
 <file name="assets/js/base/components/reviews/review-list/index.js">
 <error line="13" column="24" severity="error" message="Binding element &apos;attributes&apos; implicitly has an &apos;any&apos; type." source="TS7031" />

src/BlockTypes/ProductImage.php

@@ -147,7 +147,10 @@ private function render_image( $product ) {
 		$image_info = wp_get_attachment_image_src( get_post_thumbnail_id( $product->get_id() ), 'woocommerce_thumbnail' );
 
 		if ( ! isset( $image_info[0] ) ) {
-			return sprintf( '<img src="%s" alt="" width="500 height="500" />', wc_placeholder_img_src( 'woocommerce_thumbnail' ) );
+			// The alt text is left empty on purpose, as it's considered a decorative image.
+			// More can be found here: https://www.w3.org/WAI/tutorials/images/decorative/.
+			// Github discussion for a context: https://github.com/woocommerce/woocommerce-blocks/pull/7651#discussion_r1019560494.
+			return sprintf( '<img src="%s" alt="" />', wc_placeholder_img_src( 'woocommerce_thumbnail' ) );
 		}
 
 		return sprintf(