Ecodev
Enhance gallery accessibility with alt text, ARIA labels, and keyboard support
Summary
This pull request introduces a comprehensive set of improvements focused on accessibility, code quality, configuration modernization, and testing reliability. These changes enhance the maintainability of the codebase and improve the user experience—particularly for users relying on assistive technologies.
Highlights
♿ Accessibility Improvements
-
Refactored the
Itemclass to add semantic HTML elements and ARIA attributes:- Added
alt,aria-label,aria-pressed,role, andtabindexattributes. - Used
<figure>and<figcaption>to provide image context. - Enabled keyboard interaction for buttons and interactive elements using
keydownhandlers.
(SeeItem.ts: L102–109, L157–186)
- Added
-
Added a
captionfield to theModelAttributesinterface to support descriptive image text.
(SeeAbstractGallery.ts: L97–101)
🧹 Code Refactoring
- Rewrote
Itemrendering logic to improve semantic structure and reduce complexity. - Removed an unused ESLint directive in
getIcon()for cleaner utility code.
(SeeUtility.ts: L20)
🛠 ESLint & Configuration
- Introduced a new flat ESLint config (
eslint.config.cjs) to replace legacy setups. - Updated
.eslintrc.cjsto ignore the root directory (./), aligning with modern project structure.
📦 Dependency Updates
- Upgraded several major packages, including:
eslintjestpuppeteer@typescript-eslint
(Seepackage.json: L52–84)
✅ Testing Enhancements
-
Added unit tests to validate accessibility attributes (
alt,aria-label,role) in rendered gallery items.
(Seenatural.spec.ts: L257–284) -
Replaced
page.waitForTimeoutwith nativesetTimeoutfor better timing control in end-to-end tests.
(Seemasonry.spec.js,natural.spec.js, andsquare.spec.js)
Why This Matters
Prior to these changes, the gallery lacked semantic markup and accessible affordances, making it difficult for screen reader users and keyboard-only users to navigate and interpret content. This update moves the project toward WCAG 2.1 AA compliance by improving HTML semantics, adding accessible labels and descriptions, and ensuring interactive elements are operable via keyboard.
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
![socket-security[bot] avatar](https://avatars.githubusercontent.com/in/156372?v=4 "Published by a pub.dev verified publisher"){kind=small}
@hypnotoad08, I ran the Copilot review out of curiosity. I'll let you be the judge on what might or might not be interesting. One thing for sure though, the CI must be green to merge this PR.
@sambaptista I'll let you review and handle this.
I can't run yarn dev , it crashes when launch dev : tsup --watch.
The issue seems to come from picomatch. Your versions update include many versions of that lib that are probably conflicting
yarn list --pattern picomatch
Actual :
yarn list v1.22.19
└─ [email protected]
Your PR :
yarn list v1.22.19
├─ @jest/[email protected]
│ └─ [email protected]
├─ [email protected]
│ └─ [email protected]
├─ [email protected]
└─ [email protected]
└─ [email protected]
I see you're using npm. Please test with yarn that is used in this project.
At this point I'm just boostrapping the project. I haven't really looked into your code already. The first test I did lead to an unsupported usecase (that is actually supported : item.title + item.link). As the gallery is used in production, we have to preserve actual usecases. I have to dig deeper in your changes to understand your intentions.
I can finetune some breaking changes later, but first let's make the project run correctly in dev http://127.0.0.1:4000. It would be appreciablee to check why CI breaks too.
keep in mind tests are not exhaustive, there may be breakings even if everything is success. I'll try to take time to add tests to cover old usecases.
@sambaptista I have updated most of the feedback. yarn works for running everything locally for me. picomatch still blows up even with the original dependencies. The only thing I can't seem too get to work is the e2e tests now.
yarn works for running everything locally for me. picomatch still blows up even with the original dependencies
What yarn command exactly runs correctly ? Here yarn dev fails with your branch and not with mine on mac os 15.5 (and ~all previous mac os versions). Can you rm -rf node_modules && yarn && yarn dev and tell me if it crash ? what system do you use ?
@PowerKiKi can you please try yarn dev with freshly installed node_modules on this repo's master branch to see if you have any issue ? (including a visit on http://127.0.0.1:4000/)
tsup 8.3 introduces dependance on tinyglobby that seems to deal poorly with picomatch 4.
Downgrading to 8.2.4 seems to be fixing the issue. Can you please try on your side.
"tsup": "~8.2.0",
Appart from the yarn dev there is definitely something else broken and the test fail as expected.
On my side Natural and Masonry e2e fail. On the masonry case, here are a few elements. The reason the tests report 101 items instead of 9, is because images added to DOM don't have their final size. And the Gallery add images to viewport until viewport is full. But if images are only 18px, it add almost all of them to dom.
Before :
After :
Dunno if with your logic, the "loaded" class is still relevant (maybe yes, maybe not, I've not dig deep into it yet), but the height definitely shouldn't be NaN at this point.
Here is the expected result :
And the current state :
One fundamental aspect of the gallery, is to not re-layout (redraw) after adding to DOM to preserve performances.
Can you have a look at it before I continue the review please ?
I probably can solve it myself, but I don't want to (maybe) break your vision of this accessibility implementation.
Tests
On a sidenote, it seems test configuration is duplicated from a central config to each test. Are you sure there is no better way to do it ?
@sambaptista, after my minor commits on master, master does pass CI (and yarn dev) on my machine.
The CI on this branch also does not pass locally, showing the same error as seen in GitHub Action
FYI, the CI can be reproduced locally with those commands:
yarn --frozen-lockfile && \
yarn build-lib && \
HEADLESS=1 yarn test && \
yarn lint && \
./node_modules/.bin/prettier --check .
all e2e tests now pass and node server start stop works. Thank you for your patience while I got it working correctly with Yarn and the headless tests!
Unfortunately, after an additional small review, I easily found additional broken features.
See the last 4 images here : https://ecodev.github.io/natural-gallery-js/docs-api.html. The rendering of the select is different, the button disappeared and api does not emit expected events.
(actual demo seems to show a bug on my side too).
What's next ?
Don't touch the code anymore, I take the hand on it. I notice the old code was hard to understand (even by me) with all that plurality of options. I'll change the strategy to build an Item.
But can you please report here the raw HTML structure for the different use cases you identified ? I'll then consolidate old features and your structure and refactor tests to be more complete.
Unfortunately, after an additional small review, I easily found additional broken features.
See the last 4 images here : https://ecodev.github.io/natural-gallery-js/docs-api.html. The rendering of the select is different, the button disappeared and api does not emit expected events.
(actual demo seems to show a bug on my side too).
What's next ?
Don't touch the code anymore, I take the hand on it. I notice the old code was hard to understand (even by me) with all that plurality of options. I'll change the strategy to build an Item.
But can you please report here the raw HTML structure for the different use cases you identified ? I'll then consolidate old features and your structure and refactor tests to be more complete.
Ok sounds like a plan! I am out of town for the next week so I won't have a chance to get something together until I'm home.
I'm genuinely acknowledging you took time to contribute to that gallery and fix the first iterations. But after a deeper dive in the result, I can't decently accept the PR as it is, and even with new fixes, I feel there is too much disruption with existing features.
Each option I change, I fall on a critical bug. This setting (with lightbox activable) leads to double action on same element (link and photoswipe):
title: i.description ? i.description : i.user.name,
link: 'https://google.com',
Let's continue as suggested on my previous post, let's talk about the html "end result". Here my understanding :
Image without caption visible :
<figure>
<img alt="option.title" src="...">
</figure>
Image with visible caption :
<figure>
<img alt="" src="...">
<figcaption>option.title</figcaption>
</figure>
Image with title and link :
All element is clickable
<a href>
<figure>
<img alt="" src="...">
<figcaption>option.title</figcaption>
</figure>
</a>
Image without title, with link :
<a href>
<figure>
<img alt="" src="...">
</figure>
</a>
Image with title and link and zoom :
Click on image triggers photoswipe, but click on link navigates to destination.
<figure>
<img alt="" src="...">
<figcaption><a href>option.title</a></figcaption>
</figure>
Activation instead of link
Triggers a javascript event instead of navigating with link
Same as previously, but replace <a> by <button role aria-label>
Feel free to alter those suggestions to add attributes or rearrange as you think they should be.
I've gone ahead. I have a lot of uncommited code, please don't push more until I'm clean on my side. I've implemented my insight on accessibility with all use cases tested.
We'll talk about finetunings then.
Merged in master with many improvements (aria and more) and well tested.. Not released yet. If you want to bring additionnal contributions, please submit another pull request.
Coverage :