Skip to content

feat: add Accessibility infra setup skill - #38

Open
lukasmatta wants to merge 22 commits into
masterfrom
35-add-skill---accessibility-checks-infrastructure-setup
Open

lukasmatta wants to merge 22 commits into
masterfrom
35-add-skill---accessibility-checks-infrastructure-setup

Conversation

@lukasmatta

Copy link
Copy Markdown
Collaborator

Adds a skill that sets up accessibility (a11y) testing infra in an existing Angular app using Playwright + axe-core, checking for WCAG 2.2 AA compliance. It detects the current test setup, scaffolds a Playwright accessibility project and a shared axe fixture, drops in one dummy example scan, wires up npm scripts, and runs the scan to confirm everything works.

The approach follows the a11y testing infra pattern from AbsaOSS/cps-shared-ui, adapted for a plain app rather than a component library.

Scope: just the infrastructure plus one example scan. Writing real per-page tests and fixing violations are left as follow-up work.

Notable behaviors:

  • Extends an existing Playwright config instead of overwriting it
  • Leaves Cypress completely untouched if it's present
  • Done when npm run test:a11y passes green

Includes: the SKILL.md, asset templates (config, fixture, example spec, docs), eval + trigger-eval cases, and a README catalog entry.

Validation: evaluated by running the skill against an internal project, where it set up the a11y infra end to end and the sample scan passed green.

Closes #35

Copilot AI 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.

🟡 Changes recommended

Missing dependency handling, package-manager validation, and inadequate evaluation fixtures must be addressed.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds a skill for configuring Playwright and axe-core accessibility testing in Angular applications.

Changes:

  • Adds setup workflows, reusable templates, documentation, and a sample scan.
  • Adds behavioral and trigger evaluations.
  • Adds the skill to the README catalog.
File summaries
File Review
skills/accessibility-infra-setup/SKILL.md Must install missing axe dependencies when Playwright exists, use the detected package manager, and avoid claiming automated WCAG compliance.
skills/accessibility-infra-setup/evals/trigger-eval.json Adds trigger-boundary evaluations.
skills/accessibility-infra-setup/evals/evals.json Evaluation cases need representative Angular fixtures to verify behavior.
skills/accessibility-infra-setup/assets/playwright.config.ts Provides the Playwright configuration template.
skills/accessibility-infra-setup/assets/example.accessibility.spec.ts Provides an example accessibility scan.
skills/accessibility-infra-setup/assets/axe-helpers.ts Provides shared axe utilities.
skills/accessibility-infra-setup/assets/accessibility-README.md Should clarify trace availability and that axe scans do not establish WCAG compliance.
README.md Adds the skill to the catalog.
Review details

Suppressed comments (4)

skills/accessibility-infra-setup/assets/accessibility-README.md:37

  • What: Traces are documented as failure artifacts, but the template uses trace: 'on-first-retry' and disables retries locally. Why: A developer debugging a normal local failure will not find the promised trace in test-results/. How to fix: Document that traces are recorded on the first retry (CI by default), or change the trace mode to retain them on every failure.
- Screenshots / videos / traces on failure → `test-results/`

skills/accessibility-infra-setup/evals/evals.json:28

  • What: This existing-Playwright eval supplies no playwright.config.* fixture. Why: The core merge behavior—including preserving current projects and testDir—is therefore not exercised, so a destructive overwrite could still satisfy this prose-only eval. How to fix: Add and reference a representative existing Playwright config plus package fixture.
      "files": [],

skills/accessibility-infra-setup/evals/evals.json:43

  • What: The Cypress regression eval has no Cypress project fixture. Why: Nothing can verify that Cypress config, specs, and dependencies remain byte-for-byte untouched while Playwright files are added. How to fix: Add and reference a minimal Angular+Cypress fixture whose before/after diff can be evaluated.
      "files": [],

skills/accessibility-infra-setup/evals/evals.json:69

  • What: The do-not-clobber regression has no package or Playwright config fixture containing the scripts/settings it claims to preserve. Why: The eval cannot detect loss of existing scripts or config fields, which is the behavior this case is meant to guard. How to fix: Add and reference fixtures with sentinel scripts and non-default Playwright settings, then assert they survive.
      "files": [],
  • Files reviewed: 8/8 changed files
  • Comments generated: 5
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread skills/accessibility-infra-setup/SKILL.md Outdated
Comment thread skills/accessibility-infra-setup/SKILL.md
Comment thread skills/accessibility-infra-setup/evals/evals.json Outdated
Comment thread skills/accessibility-infra-setup/SKILL.md Outdated
Comment thread skills/accessibility-infra-setup/assets/accessibility-README.md Outdated
@lukasmatta
lukasmatta marked this pull request as ready for review September 3, 2026 14:31
@fateeand

fateeand commented Sep 3, 2026

Copy link
Copy Markdown

I think it would be great if this skill could also set up pa11y-ci, as it sometimes identifies accessibility issues that Playwright misses, and vice versa. Even though both tools use axe-core under the hood, they can surface different findings and therefore complement each other well. We already use this setup in the components library.

@lukasmatta

Copy link
Copy Markdown
Collaborator Author

I think it would be great if this skill could also set up pa11y-ci, as it sometimes identifies accessibility issues that Playwright misses, and vice versa. Even though both tools use axe-core under the hood, they can surface different findings and therefore complement each other well. We already use this setup in the components library.

Makes sense. Do we have a concrete example where Pa11y catches something Playwright + axe-core misses? If so, it’d be useful to check whether that’s just down to configuration/page state, and whether we could get the same coverage in Playwright without adding another tool.

@fateeand

fateeand commented Sep 4, 2026

Copy link
Copy Markdown

I think it would be great if this skill could also set up pa11y-ci, as it sometimes identifies accessibility issues that Playwright misses, and vice versa. Even though both tools use axe-core under the hood, they can surface different findings and therefore complement each other well. We already use this setup in the components library.

Makes sense. Do we have a concrete example where Pa11y catches something Playwright + axe-core misses? If so, it’d be useful to check whether that’s just down to configuration/page state, and whether we could get the same coverage in Playwright without adding another tool.

Yes, you can find them under "Accessibility fixes found via pa11y" in PR #698.

I'm not sure why Playwright didn't detect them. Pa11y is configured for WCAG 2.0 AA, which is a subset of the Playwright accessibility configuration, and it only scans application routes, which Playwright is doing as well.

@miroslavpojer

Copy link
Copy Markdown
Contributor

The PR miss its guide *.md file and entire documentation connection with repo. Pls add it.
See other skills as example.

@lukasmatta

Copy link
Copy Markdown
Collaborator Author

Yes, you can find them under "Accessibility fixes found via pa11y" in PR #698.

I'm not sure why Playwright didn't detect them. Pa11y is configured for WCAG 2.0 AA, which is a subset of the Playwright accessibility configuration, and it only scans application routes, which Playwright is doing as well.

Thanks for pointing out PR #698. I went through it and found there's nothing pa11y can offer on top of Playwright here. Every* finding in that PR comes down to how each tool treats axe-core's "incomplete" results (things axe can't 100% verify, e.g. some color-contrast and aria-controls cases) - pa11y fails on those by default, Playwright ignores them (with the current setup). So it comes down to a config difference not a gap in their capabilities.

I'd rather not hard-fail on incomplete results, since axe itself isn't sure about them... Some might be real, some false positives, and either way they need a human to confirm. I'd set Playwright to include incomplete findings in the report (but not fail on them). What do you think?

As for the component library, I'd suggest adjusting PW and removing pa11y to reduce tooling complexity.


*one item, the loader animation, probably wasn't caught directly by pa11y either, I tried reverting that bug and it changed nothing in pa11y's output

@lukasmatta

Copy link
Copy Markdown
Collaborator Author

The PR miss its guide *.md file and entire documentation connection with repo. Pls add it. See other skills as example.

Thanks, resolved, added the missing docs.

@miroslavpojer miroslavpojer 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.

Is possible to add more then one dummy scan?
It would increase usability.

Copilot AI 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.

🟡 Changes recommended

Routing, existing-config integration, warning compatibility, and evaluation fixtures contain correctness gaps.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

  • skills/accessibility-infra-setup/evals/files/fresh-angular/package-lock.json: Generated file
  • Files reviewed: 25/26 changed files
  • Comments generated: 11
  • Review effort level: Balanced

Comment thread skills/accessibility-infra-setup/SKILL.md Outdated
Comment thread skills/accessibility-infra-setup/SKILL.md Outdated
Comment thread skills/accessibility-infra-setup/SKILL.md Outdated
Comment thread skills/accessibility-infra-setup/assets/playwright.config.ts Outdated
Comment thread skills/accessibility-infra-setup/assets/playwright.config.ts Outdated
Comment thread skills/accessibility-infra-setup/evals/evals.json Outdated
Comment thread skills/accessibility-infra-setup/evals/evals.json Outdated
Comment thread skills/accessibility-infra-setup/evals/evals.json Outdated
Comment thread skills/accessibility-infra-setup/evals/evals.json Outdated
Comment thread skills/accessibility-infra-setup/assets/accessibility-README.md Outdated
@lukasmatta

Copy link
Copy Markdown
Collaborator Author

Is possible to add more then one dummy scan? It would increase usability.

The skill is on purpose just the infrastructure setup. This way it's more compact and verifiable. Setting up an infrastructure and adding actual tests are two different tasks imo.

Copilot AI 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.

🟡 Changes recommended

Generated integration can fail with custom test directories, ESM packages, non-npm managers, and project-less Playwright configurations.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

  • skills/accessibility-infra-setup/evals/files/fresh-angular/package-lock.json: Generated file

Suppressed comments (4)

Previously missed (2) — in code that hasn't changed since the last review.

skills/accessibility-infra-setup/SKILL.md:82

  • What: The merge path assumes an existing projects array and only explains how to modify existing projects. Why: A valid project-less Playwright config currently has one implicit functional project; adding only the new accessibility project makes ordinary playwright test stop running all functional specs. How to fix: Define how to preserve the implicit functional project before adding accessibility, and cover that configuration in an eval.
    skills/accessibility-infra-setup/SKILL.md:88
  • What: The skill says to reuse any existing axe fixture, while the copied example imports four specific exports from this template. Why: A pre-existing fixture that only exposes an Axe builder will make the generated example fail to compile, despite following these instructions. How to fix: Verify compatible exports/tag policy and otherwise extend the existing fixture in place before copying or adapting the example.

skills/accessibility-infra-setup/assets/accessibility-README.md:53

  • What: The scan-creation instruction again permits any filename containing accessibility. Why: Following that wording can create a spec that the configured suffix regex never executes. How to fix: Require the exact .accessibility.spec.ts suffix here as well.
1. Create a spec whose filename contains `accessibility`, e.g.
   `playwright/a11y/checkout.accessibility.spec.ts`.

skills/accessibility-infra-setup/SKILL.md:172

  • What: Validation remains hard-coded to npm after Step 1 detects npm, Yarn, or pnpm. Why: In setups such as Yarn PnP, npm run cannot resolve the package-manager-managed Playwright binary, so a correct setup fails the required final check. How to fix: Invoke the script through the package manager detected in Step 1 throughout this validation step.
```bash
npm run test:a11y

- **Files reviewed:** 41/42 changed files
- **Comments generated:** 3
- **Review effort level:** Balanced
</details>

Comment thread skills/accessibility-infra-setup/SKILL.md Outdated
Comment thread skills/accessibility-infra-setup/assets/run-a11y-incomplete.js Outdated
Comment thread skills/accessibility-infra-setup/assets/accessibility-README.md Outdated

Copilot AI 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.

🔵 Needs a closer look

The environment-dependent workflow modifies varied existing test configurations and still has unresolved merge and collision behavior.

Review details

Files not reviewed (1)

  • skills/accessibility-infra-setup/evals/files/fresh-angular/package-lock.json: Generated file

Suppressed comments (4)

Previously missed (3) — in code that hasn't changed since the last review.

skills/accessibility-infra-setup/SKILL.md:91

  • The existing-Playwright merge is not safe outside the explicit-project happy path. If projects is absent, adding only accessibility removes Playwright's implicit functional run. Re-running can also duplicate an existing accessibility project, and replacing a project's current testIgnore re-enables tests it intentionally excluded. Make this merge idempotent: create a functional project when needed, update an existing accessibility project instead of duplicating it, and append the suffix rule while preserving every current ignore.
    skills/accessibility-infra-setup/SKILL.md:99
  • The fresh/merge decision checks only for a conventionally named playwright.config.*. A repository can already run Playwright with its default configuration or pass a differently named config through an npm script. Treating that repository as fresh installs a config with testDir: './playwright', so its existing tests are no longer discovered. Inspect Playwright scripts, explicit --config paths, and existing test roots before choosing the fresh path; preserve the current discovery root when Playwright is already in use.
    skills/accessibility-infra-setup/SKILL.md:159
  • The step says not to clobber scripts but does not define what to do when a test:a11y* key already exists. Blindly assigning these keys can replace an application's existing public command; simply skipping them can leave the new setup wired incorrectly. Preserve equivalent commands, and ask before integrating, renaming, or replacing a conflicting command.

docs/guides/accessibility-infra-setup.md:38

  • The expected-layout tree is stale in two ways: it presents playwright/ as universal even though existing setups retain their testDir, and it names print-a11y-warnings.js while omitting the generated run-a11y-incomplete.cjs. This sends users to the wrong directory and filenames. Qualify the tree as the fresh layout and list both .cjs helpers used by the current scripts.
playwright/
├── fixtures/
│   └── axe-helpers.ts                  # Shared axe fixture (WCAG 2.2 AA tags) + assertion/incomplete-warning helpers
├── print-a11y-warnings.js              # Prints tests carrying incomplete-result warnings
└── a11y/
    └── example.accessibility.spec.ts   # ONE dummy scan of static, known-compliant HTML — passes
playwright.config.ts                    # `accessibility` project (Desktop Chrome, testMatch /\.accessibility\.spec\.ts$/)
docs/accessibility.md                   # How to run, where reports land
  • Files reviewed: 41/42 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add skill - Accessibility checks infrastructure setup

4 participants