Skip to content

Improve product thumbnails list markup accessibility#1046

Merged
Codencode merged 3 commits into
PrestaShop:2.xfrom
Codencode:fix-product-thumbnails-list-markup
Jun 18, 2026
Merged

Improve product thumbnails list markup accessibility#1046
Codencode merged 3 commits into
PrestaShop:2.xfrom
Codencode:fix-product-thumbnails-list-markup

Conversation

@Codencode

Copy link
Copy Markdown
Contributor
Questions Answers
Description? This PR improves the product thumbnails markup on the product page by wrapping each thumbnail button inside a <li> element.

This makes the HTML structure more semantic and accessible, since the direct children of a <ul> should be list items.
It also adds type="button" to the thumbnail buttons to prevent unwanted submit behavior in form contexts.
Type? bug fix
BC breaks? no
Deprecations? no
Fixed ticket?
Sponsor company Codencode snc
How to test? 1. Go to a product page with multiple images.
2. Check that the product thumbnails are displayed correctly.
3. Click each thumbnail and verify that the main product image changes as expected.
4. Inspect the generated HTML and verify that:
- .product__thumbnails-list is a <ul>;
- each direct child of the <ul> is a <li class="product__thumbnails-item">;
- each thumbnail button has type="button";
- the active thumbnail still has the active class and aria-current="true".
5. Verify that there are no visual regressions on desktop and mobile.

@Codencode Codencode added this to the v2.1.0 milestone Jun 16, 2026
@Codencode Codencode requested a review from tblivet June 16, 2026 14:04
@Codencode Codencode added the Accessibility Screen readers, keyboard navigation, ARIA, contrast... label Jun 16, 2026
@github-project-automation github-project-automation Bot moved this to Ready for review in PR Dashboard Jun 16, 2026

@tblivet tblivet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @Codencode for this PR 👍

The <li> wrapper and type="button" are good improvements. Two things need fixing before merge, both caused by the buttons no longer being direct children of the <ul>:

→ Active-thumbnail highlight (blue outline) breaks on navigation. activeThumbail in selectors-map.ts uses .js-thumb-container:nth-child(n), which assumed the button was a direct child of the <ul>. Now each button is :nth-child(1) of its <li>, so the outline disappears when you click/slide past the first image.

→ Thumbnail no longer fills its grid cell. The <li> is now the grid item, so the button reverts to inline-block. I think we could force &__thumbnail in _product.scss to take 100% of the available width (in case a merchant sets smaller thumbnail dimensions and regenerates them).

@ps-jarvis

Copy link
Copy Markdown

This pull request seems to contain new translation strings. I have summarized them below to ease up review:

  • Shop.Theme.Catalog
    • Slide to product image %number%

(Note: this is an automated message, but answering it will reach a real human)

@Codencode

Copy link
Copy Markdown
Contributor Author

@tblivet I made the 2 changes, I hope I understood what you meant. Can you check?
Thanks.

@tblivet

tblivet commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Sorry @Codencode, few more things 🙏
Could you introduce a new data-ps-ref on the js-thumb-container button instead and also product__thumbnails-item?

The goal is to migrate step by step to this convention, so I don't think we should add selectors like .product__thumbnails-item:nth-child(${id + 1}) .js-thumb-container, which rely only on CSS classes that are meant for styling.

You can keep the classes for styling and just add the refs, e.g. data-ps-ref="product-thumbnail" on the <button> and data-ps-ref="product-thumbnail-item" on the <li>, then update the selector to:

activeThumbail: (id: number): string => `[data-ps-ref="product-thumbnail-item"]:nth-child(${id + 1}) [data-ps-ref="product-thumbnail"]`,

Also, about the width: 100% it should go on .product__thumbnail-image (my bad, sorry) rather than on the button. img-fluid only sets max-width (it caps the image but never stretches it), so putting width: 100% on the image is what makes the thumbnail actually fill its cell.

Thanks 🙌

@Codencode

Copy link
Copy Markdown
Contributor Author

@tblivet no worries, no problem at all.
I applied the 3 changes, hope I understood correctly.

@tblivet tblivet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you @Codencode that's all good 👍

@tblivet tblivet added the Waiting for QA Status: Action required, Waiting for test feedback label Jun 18, 2026
@ps-jarvis ps-jarvis moved this from Ready for review to To be tested in PR Dashboard Jun 18, 2026

@ingridusta ingridusta left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi @Codencode,

Works as expected, and no regressions with keyboard nav :

Image
Enregistrement.de.l.ecran.2026-06-18.a.16.05.04.mov

It's QA Approved ✅

@ingridusta ingridusta self-assigned this Jun 18, 2026
@ingridusta ingridusta added QA ✔️ Status: Check done, Code approved and removed Waiting for QA Status: Action required, Waiting for test feedback labels Jun 18, 2026
@Codencode Codencode merged commit 4cce952 into PrestaShop:2.x Jun 18, 2026
6 checks passed
@github-project-automation github-project-automation Bot moved this from To be tested to Merged in PR Dashboard Jun 18, 2026
@ps-jarvis ps-jarvis moved this from Merged to To be tested in PR Dashboard Jun 18, 2026
@Codencode Codencode deleted the fix-product-thumbnails-list-markup branch June 18, 2026 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Accessibility Screen readers, keyboard navigation, ARIA, contrast... QA ✔️ Status: Check done, Code approved

Projects

Status: To be tested

Development

Successfully merging this pull request may close these issues.

4 participants