feat(core): add eslint-plugin-jsx-a11y and fix accessibility violations#930
feat(core): add eslint-plugin-jsx-a11y and fix accessibility violations#930
Conversation
- Add eslint-plugin-jsx-a11y with recommended rules to ESLint config - Fix cat-select: add tabIndex, aria-expanded, aria-controls, role="option", keyboard handlers - Fix cat-tag: add aria-expanded and aria-controls to combobox input - Fix cat-radio: remove incorrect role="radio" from label element - Fix cat-date-inline: remove redundant onClick from label (htmlFor handles focus) - Fix cat-input: add role="presentation" to wrapper div - Fix cat-i18n-registry: remove unused variable in catch block This enables static a11y linting as part of the standard lint workflow.
- Add axe-core dependency for runtime a11y checks
- Create reusable checkA11y() helper that injects axe-core into Stencil E2E pages
- Add formatViolations() helper for readable test output
- Disable document-level rules by default (title, lang, landmarks) for component testing
- Add a11y test examples to cat-button component
Usage:
import { checkA11y, formatViolations } from '../../utils/a11y-test';
const violations = await checkA11y(page);
expect(violations).toEqual([]);
| 'input-disabled': this.disabled, | ||
| 'input-invalid': this.invalid | ||
| }} | ||
| role="presentation" |
There was a problem hiding this comment.
role="presentation" removes semantic from the element. Since div doesn't have any semantic role="presentation" does nothing. It may trick eslint but it may confuse developers, I recommend replace it with eslint disable line
| ref={el => (this.trigger = el)} | ||
| id={this.id} | ||
| role="combobox" | ||
| tabIndex={0} |
There was a problem hiding this comment.
I see that tabIndex is assigned programmatically in onClick and in onKeyDown, I'm not sure if this don't break the current implementation, if you are not sure - easier to add eslint-disable
| aria-required={this.required ? 'true' : false} | ||
| aria-activedescendant={this.activeDescendant} | ||
| onClick={e => this.onClick(e)} | ||
| onKeyDown={e => e.key === 'Enter' && this.onClick(e as unknown as MouseEvent)} |
There was a problem hiding this comment.
I think there is no need in this, key handler is already working, you can also double check here https://haiilo.github.io/catalyst/ (I guess because of tabindex)
| @@ -637,11 +637,13 @@ export class CatSelect { | |||
| ref={el => (this.trigger = el)} | |||
| id={this.id} | |||
| role="combobox" | |||
There was a problem hiding this comment.
not related to you changes, but I think we should not have role="combobox" on the wrapper, but only on the input
| role="listbox" | ||
| aria-multiselectable={this.multiple} | ||
| aria-setsize={this.state.totalElements} | ||
| aria-label={this.label || 'Options'} |
There was a problem hiding this comment.
we should probably use i18n instead of hardcoded strings
| 'select-option-active': this.state.activeOptionIndex === i | ||
| }} | ||
| role="option" | ||
| aria-selected={false} |
There was a problem hiding this comment.
li element above already has role option and the correct implementation of aria-selected
| aria-selected={false} | ||
| onFocus={() => this.input?.focus()} | ||
| onClick={() => (isTagOption ? this.createTag(item.render.label) : this.select(item))} | ||
| onKeyDown={e => |
There was a problem hiding this comment.
not needed, enter keydown is already working
| role="combobox" | ||
| ref={el => (this.input = el as HTMLInputElement)} | ||
| aria-expanded={false} | ||
| aria-controls={`tags-${this.id}-list`} |
There was a problem hiding this comment.
instead we should probably remove combobox role, it's not a combobox
2 participants
This enables static a11y linting as part of the standard lint workflow.