Repository navigation
feat: a11y testing setup - #3177
matthewlipski wants to merge 47 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:
|
@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
| 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.
There was a problem hiding this comment.
I believe we decided to keep a test for each button now that we have a clear distinction between the responsibilities of different tests, correct me if I'm wrong @YousefED
| } | ||
| } | ||
|
|
||
| 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.
nperez0111
left a comment
There was a problem hiding this comment.
Overall, I think we are 95% the way there @matthewlipski just some minor cleanup and I need some clarification on the tests/a11y folder of files and I think we are good.
Please cleanup any files that you feel are no longer necessary to the PR
| } | ||
| .bn-editor p { margin-block-end: 2em !important; } | ||
| `); | ||
| await screenshotFull(document.body, "default-schema-text-spacing.png", { |
There was a problem hiding this comment.
I'm a little unsure what this screenshot test actually ends up proving or not
There was a problem hiding this comment.
A11y tools can change text spacing/sizing and cause overflows that may not be covered by CSS, causing content to get cut off. E.g. we had a related issue to this with equation blocks initially, where we didn't realize that longer equations would get cut off and the block wasn't scrollable.
| await openEditor(defaultSchemaDocument); | ||
| await waitForMedia(); | ||
| resizeText(2); | ||
| await screenshotFull( |
YousefED
left a comment
There was a problem hiding this comment.
I think there's been a great cleanup and the organization into groups also really helps.
Left some inline comments, my main two questions are:
- How would we test behavior and what's the current policy for this? (see my comment on the skill file
- Some tests are 90% the same (tab to element), but test something slightly different. I assume this is a best practice, but let's quickly tradeoffs at standup
|
|
||
| | Group | Actions and assertions | | ||
| | ---------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | | ||
| | `behavior` | Navigate using the keyboard and activate with Enter. Assert the component's functional effect. For document edits, subscribe with `editor.onChange(vi.fn())` after navigation and before activation, register the unsubscribe with `onTestFinished`, and assert the callback ran. View-only changes may need another observable effect: toggle-heading tests check persisted expansion state. Keep detailed editor-command unit tests colocated with those commands rather than duplicating them here. | |
There was a problem hiding this comment.
so I understand correctly, this means that in the .browser.test.ts files we don't check that an action did the "correct" thing, but just that it did "something"?
There was a problem hiding this comment.
I assume (also based on standup discussions) that "full behavior" tests are then out of scope for now, which is fair. But maybe we should do the thought exercise of where the full behavior tests would / should live if / when we'd invest in those. In the same file under a different group? would they replace the behavior group? Or, the philosophy is that we should always test them at a lower level?
There was a problem hiding this comment.
Note from our discussion: full integration style tests would override the need for "behavior" tests in the case of the formatting toolbar, but there are cases for other UI elements where both test types are relevant. @nperez0111 will provide examples of this.
| await pressKey("Enter"); | ||
| const menu = page.getByRole("menu"); | ||
| await expect.element(menu).toBeVisible(); | ||
| // Mantine initially focuses a placeholder before the first menu item. |
There was a problem hiding this comment.
do we agree to explicitly only test mantine now? ok to leave other skins out of scope, but maybe good to document this as possible follow up work later
There was a problem hiding this comment.
Will add to PR description, though I don't think it's gonna make sense to worry about the other frameworks until we are really happy with Mantine, if at all.
|
|
||
| exports[`ARIA > formatting toolbar: Align text center > after activation 1`] = ` | ||
| "- button "Align text center" [pressed]: | ||
| - img" |
There was a problem hiding this comment.
fyi (for later, once we do fixing). The best practice is to make the icons aria-hidden, right?
There was a problem hiding this comment.
Yeah though it's very minor. The buttons already have accessible names so the images are ignored by screen readers in any testing I've done.
|
|
||
| exports[`ARIA > formatting toolbar: Colors > before activation 1`] = `"- button "Colors": A"`; | ||
|
|
||
| exports[`ARIA > formatting toolbar: Italic > after activation 1`] = ` |
There was a problem hiding this comment.
nitpick? should we snapshot the entire toolbar, or just the button?
The test is for the entire FormattingToolbar, I think it might be more consistent to validate the a11y-tree of the entire component?
There was a problem hiding this comment.
Logically that makes sense, practically I'd rather not do that since the snapshots are harder to check when all of them are 90% the same.
Summary
This PR shows a vertical slice of the a11y areas we need to test. Below is the full list of what we need ot test, which what's included in this PR marked as checked.
The approach to testing is based on the a11y study done earlier. It's recommended to get familiarised with that first.
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 what works and what doesn't at the moment. For each type of test:
🟢 Tests are implemented and passing.
🔴 Tests are skipped as the functionality being tested is missing, or are implemented but failing or having incorrect snapshots.
Editor Body
BlockNoteView.browser.test.ts<body>, it's not quite what we want as they cannot e.g. press Enter to then refocus the editor.BlockNoteView.screen-reader.test.tsFormatting Toolbar
FormattingToolbar.browser.test.tsFormattingToolbar.screen-reader.test.tsToggle Heading
block.browser.test.tsRationale
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
N/A
Impact
N/A
Testing
See above.
Screenshots/Video
N/A
Checklist
Additional Notes
Summary by CodeRabbit