Fix: skewed placeholder of product image - issue#7553 #7651
Conversation
Typo caused rendering of incorrect HTML attributes: width and height of Product Image placeholder that were anyway ignored by the browser
…uctElements > 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
…ductImage.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
…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
rubikuserbot
requested review from
a team and
thealexandrelara
and removed request for
a team
|
The release ZIP for this PR is accessible via: |
TypeScript Errors ReportFiles with errors: 431 🎉 🎉 This PR does not introduce new TS errors. |
kmanijak
changed the title
kmanijak
changed the title
|
Size Change: -58 B (0%) Total Size: 991 kB
ℹ️ View Unchanged
|
| <ProductDataContextProvider product={ productWithImages }> | ||
| <ProductDataContextProvider | ||
| product={ productWithImages } | ||
| isLoading={ false } |
There was a problem hiding this comment.
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? 🤔
There was a problem hiding this comment.
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.
| return ( | ||
| <img src={ PLACEHOLDER_IMG_SRC } alt="" width={ 500 } height={ 500 } /> | ||
| ); | ||
| return <img src={ PLACEHOLDER_IMG_SRC } alt="" />; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- https://codex.wordpress.org/I18n_for_WordPress_Developers
- https://www.npmjs.com/package/@wordpress/i18n
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!
There was a problem hiding this comment.
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.
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
|
@sunyatasattva, I responded to your comments. Happy to hear your opinion regarding |
… 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
|
No worries @sunyatasattva, I'm happy we came up with a proper conclusion 🙌
I'd appreciate another round of review. |
!
Summary
Fix for the issue #7553.
Placeholder image was skewed (during loading and once loaded) when there was not enough space for it to expand, for example:
What wasn't mention in the issue is that the issue could be reproduced on too wide screens as well (when single product had more than 500px width).
Cause
The cause of the glitch were inline styles on the placeholder image in
Product Imageblock:widthwas overridden by more specific styles, whileheightwas not, which made the placeholder skewed with the height fixed to 500px.Changes
Removal of the inline styles let the placeholder to keep the image ratio depends on the width. That works in both cases: when there's not enough or too much space for a product.
Additionally, the corresponding styles where removed from a PHP code. First commit shows that the styles were anyway not even applied, which proves it's safe to remove them.
Fix a couple of TS errors in tests: commit
Add explicit comments about alt text being empty in decorative images: commit
Fixes:
Screenshots
Testing
Automated Tests
User Facing Testing
Prerequisites:
0. Make sure you have a product without a highlighted image (you can create a new product or remove the image from an existing one).
Case 1: image is wider than 500px
Case 2: image is narrower than 500px
WooCommerce Visibility
Changelog