Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

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

Merged
merged 13 commits into from
Nov 16, 2022
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
82db041
Fix typo in HTML markup of ProductImage block placeholder
Nov 9, 2022
1993312
Remove inline styles (width and height) from ImagePlaceholder in Prod…
kmanijak Nov 9, 2022
c3a374c
Remove inline styles (width and height) from placeholder image in Pro…
kmanijak Nov 9, 2022
d44008a
Add test to check placeholder image renders without width and height …
kmanijak Nov 10, 2022
9f3677a
bot: update checkstyle.xml
github-actions[bot] Nov 10, 2022
4763d7f
Fix TypeScript errors in the block test file of Product Image
kmanijak Nov 10, 2022
4004875
bot: update checkstyle.xml
github-actions[bot] Nov 10, 2022
826f1d1
Add generic alt text to placeholder image
kmanijak Nov 11, 2022
a8d13c8
Revert adding placeholder image alt text and add comments to make the…
kmanijak Nov 15, 2022
20eef31
bot: update checkstyle.xml
github-actions[bot] Nov 15, 2022
51434e5
Merge branch 'trunk' into fix/placeholder-product-image-is-skewed
kmanijak Nov 15, 2022
53b608f
bot: update checkstyle.xml
github-actions[bot] Nov 15, 2022
794b9ad
Merge branch 'trunk' into fix/placeholder-product-image-is-skewed
kmanijak Nov 16, 2022
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
4 changes: 1 addition & 3 deletions assets/js/atomic/blocks/product-elements/image/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,7 @@ export const Block = ( props ) => {
};

const ImagePlaceholder = () => {
return (
<img src={ PLACEHOLDER_IMG_SRC } alt="" width={ 500 } height={ 500 } />
);
return <img src={ PLACEHOLDER_IMG_SRC } alt="" />;
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw that alt prop was already empty before, but I'm wondering if it would be better to add some text to explicitly say it is a placeholder image instead. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Super-duper agreed!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for this suggestion! I was thinking about that, but then in a test I spotted the placeholder is searched by the empty alt:

const placeholderImage = component.getByAltText( '' );

and assumed it may be empty for a reason. But two aye!s are enough to convince me! 💪

I'll double-check there's no other logic based on that and provide it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alt text added in the commit.

I considered what should be the alt text:

  • what's really on the image, like "Simple landscape drawing"
  • what's the purpose, like "Generic placeholder", since in this case it doesn't really matter what's there
  • something else

I went for a second option since the image itself is set externally through the settings and may change over time. Also, the content is kind of redundant in this context, the purpose matters. There are even opinions that placeholder image doesn't need alt text at all (https://www.seroundtable.com/google-alt-text-small-images-23672.html - thanks @imanish003 for sharing that! 🙌 ), but I'm not convinced.

Happy to hear your opinion!

Copy link
Contributor

Choose a reason for hiding this comment

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

Great work @kmanijak. I think it is great that @imanish003 mentioned the fact that placeholder images may not need an alt text and yes I just remembered that are some situations in which the alt text must be empty, but I cannot remember from the top of my mind all those situations so I went ahead looking for resources related to that.

I found this alt Decision Tree from the W3C Web Accessibility Initiative (WAI) that I think we can use to make this decision: https://www.w3.org/WAI/tutorials/images/decision-tree/. It seems that if we consider the placeholder image as being just decorative or not intended for the user the best solution would be to leave it empty. Here is also a resource talking about Decorative Images: https://www.w3.org/WAI/tutorials/images/decorative/, it states that "Text values for these types of images would add audible clutter to screen reader output or could distract users if the topic is different from that in adjacent text", which is something I can agree since we can have dozens of placeholder images that is going to be read by the screen readers.

Copy link
Contributor

Choose a reason for hiding this comment

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

This was a very interesting discussion. After reading it, I agree that alt text would add audible clutter to screen reader users. People do not need to hear over and over about the placeholder image which is there just to keep the layout, actually. Does not add any information.

I agree with removing it. Sorry @kmanijak for the extra work. But it's a good opportunity also because I saw there were some mistakes in the implementation which we take for granted and it's a good time to inform you about them. We never use naked strings in either JS or PHP code, we always use WordPress localization functions.

Here are two resources:

Most generally, you will see that you'll often find yourself using the __ function.

With that said, I think we should make this decision of leaving the alt text empty explicit by adding a comment in the relevant places, perhaps linking to this discussion thread. Someone else could see the empty alt text and it would be the same story all over again!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @thealexandrelara and @sunyatasattva for your valuable input! I'm happy with the conclusion.

The changes are available in this commit:

  • remove alt text (especially as an explicit string, thanks Lucio for pointing this out, I'll keep that in mind!)
  • add explicit comments to all the relevant places I found.

};

const Image = ( { image, loaded, showFullSize, fallbackAlt } ) => {
Expand Down
46 changes: 39 additions & 7 deletions assets/js/atomic/blocks/product-elements/image/test/block.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clarify why isLoading is hardcoded to false in the context of this test, and why was it needed to be added as a fix for this issue? 🤔

Copy link
Contributor Author

@kmanijak kmanijak Nov 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Can you clarify why isLoading is hardcoded to false in the context of this test

Sure! I added isLoading prop to each test, as I realised it's a required prop for ProductDataContextProvider and TypeScript checker complained about that.
And considering the tests are checking the behaviour of component with images loaded already, I checked the code and isLoading passed as false seems to set such condition. Also, there's no default value for isLoading, so before adding that to tests, it was implicitly falsy.
But I may be missing something, so I'm all ears if you see that may be troublesome! 👂

and why was it needed to be added as a fix for this issue? 🤔

TS fixes/improvements were added as an extra to this PR and are not required as a fix for issue. I gathered them in a separate commit, but maybe I should've added it as a separate PR to avoid confusion. Let me know the way forward 🙌
EDIT: Or add it to the PR description in "Changes" section. Will keep that in mind for future.

>
<Block showProductLink={ true } />
</ProductDataContextProvider>
);
Expand All @@ -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>
);
Expand All @@ -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 }`
);
} );
Expand All @@ -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>
);
Expand All @@ -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>
);
Expand All @@ -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 );
} );
} );
} );
17 changes: 4 additions & 13 deletions checkstyle.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1121,10 +1121,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="135" column="19" severity="error" message="Binding element &apos;image&apos; implicitly has an &apos;any&apos; type." source="TS7031" />
<error line="135" column="26" severity="error" message="Binding element &apos;loaded&apos; implicitly has an &apos;any&apos; type." source="TS7031" />
<error line="135" column="34" severity="error" message="Binding element &apos;showFullSize&apos; implicitly has an &apos;any&apos; type." source="TS7031" />
<error line="135" 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;.
Expand Down Expand Up @@ -1457,15 +1457,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" />
Expand Down
2 changes: 1 addition & 1 deletion src/BlockTypes/ProductImage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ 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' ) );
return sprintf( '<img src="%s" alt="" />', wc_placeholder_img_src( 'woocommerce_thumbnail' ) );
}

return sprintf(
Expand Down