Repository navigation
feat: a11y testing setup - #3177
matthewlipski wants to merge 14 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
||
| - name: Upload browser report and diagnostics | ||
| if: ${{ !cancelled() }} | ||
| uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 |
|
|
||
| - name: Upload VoiceOver report and diagnostics | ||
| if: ${{ !cancelled() }} | ||
| uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 |
@blocknote/ariakit
@blocknote/code-block
@blocknote/core
@blocknote/diagram-block
@blocknote/mantine
@blocknote/math-block
@blocknote/react
@blocknote/server-util
@blocknote/shadcn
@blocknote/xl-ai
@blocknote/xl-docx-exporter
@blocknote/xl-email-exporter
@blocknote/xl-multi-column
@blocknote/xl-odt-exporter
@blocknote/xl-pdf-exporter
@blocknote/xl-typst-exporter
commit: |
|
…e from check list items to toggle headings
| @@ -0,0 +1,61 @@ | |||
| This document covers the main areas that are most relevant to the editor regarding accessibility. These areas are derived from: | |||
There was a problem hiding this comment.
let's make sure this .md's are not merged, because afaik they're 1-off studies for this workstream. Anything in the repo should be long-lived
There was a problem hiding this comment.
Yep, marked in the description as temporary. Just have to remember to remove them later, but still useful atm mainly so you can point agents to them.
| await expect.element(button).toHaveAttribute("aria-pressed", "true"); | ||
| }); | ||
|
|
||
| test("formatting toolbar: Italic", async () => { |
There was a problem hiding this comment.
This repeats the same for every button, which got me thinking we can go in two directions:
- full: Accept that we want to test every user interaction like this. In that case, we might as well extend it to make sure the action (enter) does what's expected (make text bold). This concept would not turn it into an a11y specific test anymore, but sets a pattern where every component must have a complete browser test that covers functionality, a11y, etc. In this case, we should also think if mouse clicks should be handled as well
- minimal: test this for 1 button or at the ToolbarButton level
There was a problem hiding this comment.
I do wonder if we should have screenshots for all possible states though
There was a problem hiding this comment.
I think it's worth having a discussion about this on call. Another solution could be to have even more granular, Storybook-esque UI testing, and test toolbar buttons/dropdowns/menus/etc in isolation. That would be a pretty major refactor though.
| @@ -0,0 +1,44 @@ | |||
| import { expect, test } from "vite-plus/test"; | |||
There was a problem hiding this comment.
This is nice. General note; once we have the patterns, it would be great to set up an agent file with testing rules about file names and testing patterns, to make sure everything stays consistent, and to make sure LLMs / contributors don't pollute the nicely structured files with tests that repro a specific case but go against the pattern
Even better if we could cover this at the type system or lint stage.
There was a problem hiding this comment.
For example, let's make explicit that these .axe files should:
- scan the main compononent
- scan all "hidden" popups owned by the component
| } | ||
| } | ||
|
|
||
| test.describe("text spacing and resizing", () => { |
There was a problem hiding this comment.
Interesting! How did you find this? Are there similar cases that should be tested? Are there tools that can set these variants for us? (so we don't need to hand-roll these document changes)
There was a problem hiding this comment.
I was literally browsing the WCAG 2.0 requirements one by one and stumbled across these. They were the only ones I found that were based on making sure that some external styles can be applied without issue, except animations but we can't really test those as they'd require smth like video snapshots.
| }); | ||
| }); | ||
|
|
||
| test("default schema remains readable at 200% text size", async () => { |
There was a problem hiding this comment.
can't we set this with playwright or sth else instead of hand-rolling this?
There was a problem hiding this comment.
It's not that simple since each font in the editor has to be scaled by 200%, not just the base editor font size. So we have to go through each one and multiple the original size.
| ) | ||
| .toBe(false); | ||
| const results = await axe.run(scope ? { include: [scope] } : document.body); | ||
| expect(results.violations, "axe violations").toEqual([]); |
There was a problem hiding this comment.
how will we go about this in practice? I assume this will report a lot? Are they all relevant? What's a good workflow that allows us to ship continuous improvements?
There was a problem hiding this comment.
Honestly there aren't that many. We can skip the failing axe test files for now and start enabling each one separately after fixing all the issues for that component.
These are all reported violations:
| Test | Rule | Severity | Finding |
|---|---|---|---|
| Default schema | aria-input-field-name |
Serious | The editor’s textbox has no accessible name. |
| Default schema | color-contrast |
Serious | “Quoted text” has 4.29:1 contrast; requires 4.5:1. |
| Default schema | label |
Critical | Both check list checkboxes—checked and unchecked—lack accessible names. |
| Default schema | target-size |
Serious | Both checkboxes measure 12 × 24px, with insufficient surrounding space to satisfy the spacing exception. |
| Formatting toolbar: block type menu | aria-required-children |
Critical | The menu contains a Mantine autofocus div[tabindex="-1"] that axe flags as a disallowed child. |
| Formatting toolbar: colors menu | aria-required-children |
Critical | Same autofocus-element issue as the block type menu. |
There is one incomplete finding requiring manual review:
| Test | Rule | Finding |
|---|---|---|
| Default schema | video-caption |
Check whether the video requires captions and whether suitable captions are available. Axe cannot determine this automatically. |
The formatting toolbar itself and link popover report no violations or incomplete findings.
| import { pressKey, readSelection } from "@root/tests/a11y/helpers.js"; | ||
|
|
||
| // Shared by browser keyboard checks and the native transcript test. | ||
| export async function navigateDefaultSchemaDocument() { |
There was a problem hiding this comment.
- this test could skip over blocks without us detecting it
- we dont test arrowdown and arrowright navigation (not sure if we want, but fyi)
- This feels more like a shared test than a helper. It differs in nature quite a bit from the other helpers
| ); | ||
| } | ||
|
|
||
| export function applyStyles(css: string) { |
There was a problem hiding this comment.
- why is resizeText here, but not increaseLineSpacing? instead, that one uses applyStyles
TLDR; let's make sure all patterns are consistent
| @@ -0,0 +1,119 @@ | |||
| import type { PartialBlock } from "@blocknote/core"; | |||
There was a problem hiding this comment.
I think this file also contains some screenreader specific helpers, and some blocknote-specific
| @@ -0,0 +1,124 @@ | |||
| import type { | |||
There was a problem hiding this comment.
is this different from the default testdocument or can we reuse code?
| output: ["packages/core/README.md", "packages/react/README.md"], | ||
| // Accessibility checks interact with Docker or a native screen reader; | ||
| // replaying cached output must never stand in for running those checks. | ||
| a11y: { |
There was a problem hiding this comment.
tbd; let's see if we can / want to run these tests as part of the regular suite. Makes sense that GuidePup has a different setup, but the other tests could just be run as part of the regular tests I think.
Closely related to https://github.com/TypeCellOS/BlockNote/pull/3177/changes#paneldiscussion_r4255956960
| "no-restricted-properties": [ | ||
| "error", | ||
| { | ||
| property: "getByTestId", |
There was a problem hiding this comment.
this can apply to the entire project?
(general rule; let's try to keep the codebase as consistent as possible)
Summary
This PR shows a vertical slice of the a11y areas we need to test. It's based on the a11y study done earlier. It's recommended to get familiarised with that first. The documents from the study are also temporarily included as Markdown files in the git branch.
The general approach was to colocate a11y test files with the components they test. The tests all require using a browser environment due to one of the following:
The test runner itself is Vitest Browser Mode, which is in line with a lot of our existing browser tests and uses Playwright for the browser.
Below is a summary of the tested a11y areas. For each type of test:
🟢 Tests are implemented and passing.
🟡 Tests are implemented and passing, but snapshots are at least partially incorrect.
🔴 Tests aren't implemented as the functionality being tested is missing, or are implemented but failing.
Editor Content
These tests are for making sure the editor content is broadly accessible, covering keyboard navigation, focus handling, legibility, and screen reader support.
BlockNoteView.a11y.spec.ts<body>and pressing Tab moves focus to the next interactive element in the page. This works for now but will have to be revisited when fixing the selection handling (see below).<body>, it's not quite what we want as they cannot e.g. press Enter to then refocus the editor.Interactive blocks
These tests are for making sure that for any blocks with interactive elements, e.g. check list items and toggle headings, the interactive elements can be manipulated using the keyboard. Currently, only the toggle heading has a test file.
block.a11y.spec.tsBelow is a list of all the other interactive blocks that still need testing & a11y fixes:
Editor UI
These tests verify the accessibility of BlockNote's UI elements like menus and toolbars. These cover basically everything a11y related - keyboard navigation, focus handling, legibility, and screen reader support. Currently, only the formatting toolbar is tested, which fortunately has had some a11y improvements already.
The formatting toolbar tests are also not 100% complete as there are many items which are only visible in specific circumstances. For example, the button for adding a comment is only visible when collaboration is enabled, and media blocks like images/videos/etc have their own set of items.
FormattingToolbar.a11y.spec.tsBelow is a list of all other UI elements that still need testing & a11y fixes:
Rationale
While we have done accessibility work before, it was mainly patching fairly glaring issues like the formatting toolbar being inaccessible via the keyboard. This PR is the start of a longer term commitment to improve a11y, and testing gives us a framework for figuring out where the main issues are and what still needs to be done.
Changes
TODO
Impact
N/A
Testing
See above.
Screenshots/Video
N/A
Checklist
Additional Notes
Broader TODOs: