Repository navigation
feat: a11y testing setup - #3177
matthewlipski wants to merge 37 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
Merge Risk:
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly identifies the main change: adding an accessibility testing setup. Description check The description is detailed, on-topic, and covers the rationale, impact, testing results, limitations, and follow-up work. The Changes section remains marked TODO, and the checklist is unchecked, but … Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description is detailed, on-topic, and covers the rationale, impact, testing results, limitations, and follow-up work. The Changes section remains marked TODO, and the checklist is unchecked, but the overall description is mostly complete.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 10 functions across 18 files. (12 skipped: 12 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Fix failing CI checks
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit checks the tab key’s flight
Through headings, menus, left and right
Axe and readers join the run
Snapshots note what has been done
CI carries tests along
The bunny thumps a testing song
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
@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?
| resetToggleHeading(); | ||
| await openEditor(toggleHeadingDocument, { editable: false }); | ||
| const toggleButton = await focusToggleHeadingWithAssertions(); | ||
| await expect |
There was a problem hiding this comment.
I'm a bit worried that we will end up with a lot of screenshots (one for proving every element is visually focused). I wonder whether there are heuristics we can use instead of this? Like for example that the element has a new outline or border that was not there before the focusing. It may not capture every regression, but ideally we wouldn't have unrelated changes (like changing colors) affect a11y/behavior tests, because then it becomes noise and visual diffs are the most annoying to review.
There was a problem hiding this comment.
We probably could, I'm surprised you mention that the visual snapshots are the annoying ones to review since I personally feel like they're the easiest. I'll give this a shot on a separate branch either way
There was a problem hiding this comment.
They are somewhat easy to review in a PR since there is UI for doing visual diffing, but otherwise it becomes opening the files and "find the difference between these two images" which is pretty annoying across multiple files
| // Record selection → toolbar → Escape → editor. | ||
| // Individual toolbar item actions and focus rings are covered by the browser tests. | ||
| // Screen reader announcements do not clearly communicate entering the toolbar. | ||
| test.skip("formatting toolbar: open, focus, and close", async () => { |
There was a problem hiding this comment.
I imagine that there is more to this test that I'm missing?
There was a problem hiding this comment.
It's just that focus switches like this can be pretty disorienting. Returning focus to the editor can read out the entire editor content for example, which is not great. So this test is to catch those kinds of issues.
| - heading "Heading level one" [level=1] | ||
| - text: • | ||
| - paragraph: Bullet list item | ||
| - text: "1." |
There was a problem hiding this comment.
oof, yea I can see clearly that this snapshot is messed up haha. Good thing we are doing this work!
# Conflicts: # pnpm-lock.yaml # tests/vite.config.browser.ts
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:
Summary by CodeRabbit