Repository navigation
fix: List item screen reader announcement for read-only editor (BLO-1217) - #3223
matthewlipski wants to merge 15 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
matthewlipski
changed the title
matthewlipski
changed the title
# Conflicts: # packages/react/src/editor/a11y/BlockNoteView.screen-reader.test.ts
|
@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: |
| function visit(parent: Node, start: number) { | ||
| type Item = { node: Node; pos: number }; | ||
| function* siblings(): Iterable<ListSibling<Item>> { | ||
| let pos = start; | ||
| for (let i = 0; i < parent.childCount; i++) { | ||
| const node = parent.child(i); | ||
| const type = | ||
| node.type.name === "blockContainer" | ||
| ? node.firstChild?.type.name | ||
| : undefined; | ||
| yield { | ||
| item: { node, pos }, | ||
| type, | ||
| start: node.firstChild?.attrs.start, | ||
| listItem: !!( | ||
| type && | ||
| editor.schema.blockSpecs[type]?.implementation.meta.listItem | ||
| ), | ||
| }; | ||
| if (!node.isLeaf && !node.isTextblock) { | ||
| visit(node, pos + 1); | ||
| } | ||
| pos += node.nodeSize; | ||
| } | ||
| } | ||
|
|
||
| walkListSiblings( | ||
| siblings(), |
There was a problem hiding this comment.
I wonder about the layers of abstraction here. I think they might accidentally doing O(N^2) work for something that should be O(N). I also don't think it helps to make it readable, maybe just inline it and go for simplicity
| const indices = DecorationSet.create(doc, decorations); | ||
| return { | ||
| decorations: indices, | ||
| readOnlyDecorations: indices.add(doc, accessibilityDecorations), |
There was a problem hiding this comment.
let's be careful about introducing more work than needed here. While editing a large doc numbering is actually somewhat expensive (since it needs to scan the whole doc).
The previous version of this (IndexingPlugin) was actually very optimized to pass in the transaction so it only had to regenerate what it needed to. I would really want to keep that behavior because I introduced that change after a user reported slow documents when they get larger.
| attributes: (): Record<string, string> => | ||
| store.state.isEditable && store.state.enabledSet.size === 0 | ||
| ? { | ||
| role: "textbox", | ||
| "aria-autocomplete": "list", | ||
| "aria-haspopup": "listbox", | ||
| } | ||
| : { role: "document" }, |
There was a problem hiding this comment.
This is very smart love it
There was a problem hiding this comment.
Numbering is wrong here no? I think something messed up
There was a problem hiding this comment.
Numbering looks ok to me, the fixture itself was changed intentionally though to have nested list items
| const indices = DecorationSet.create(doc, decorations); | ||
| return { | ||
| decorations: indices, | ||
| readOnlyDecorations: indices.add(doc, accessibilityDecorations), |
There was a problem hiding this comment.
let's be careful about introducing more work than needed here. While editing a large doc numbering is actually somewhat expensive (since it needs to scan the whole doc).
YousefED
left a comment
YousefED
left a comment
There was a problem hiding this comment.
@matthewlipski I haven't reviewed the code yet, but didn't we decide at standup that this would be valuable for editable documents as well?
|
Yeah still gonna look into that |
|
Update r.e. having the a11y changes when editable too: The issue I was running into was that VoiceOver was not announcing lists when navigating using the arrow keys. VoiceOver has its own keyboard navigation which does announce the lists for Chromium, though Webkit's implementation of I originally thought the NVDA does not have this issue. Lists are announced regardless of whether the editor is editable or has I will re-add the decorations to the editor when it's editable as this will still be beneficial to NVDA users, though it doesn't look like there's much we can do about the VoiceOver/Webkit issues atm. I still expect that we will add screen reader announcements for block types in the future which will inform users that they're in a list item, but this is out of scope for this PR ofc. |
|
@nperez0111 I refactored the code to stick to your existing implementation of the |
# Conflicts: # packages/react/src/editor/a11y/BlockNoteView.a11y.test.ts-snapshots/visual/default-schema-text-size-200-percent-chromium-linux.png # packages/react/src/editor/a11y/BlockNoteView.a11y.test.ts-snapshots/visual/default-schema-text-spacing-chromium-linux.png # tests/a11y/commands.ts # tests/a11y/vite.config.ts
Summary
When the editor is read-only, list item blocks aren't properly announced as being list items or belonging to any list. This PR gives list item blocks the
listitemrole, and adds alistItem: booleanfield to themetaproperty so custom blocks can be flagged as list items to receive the role automatically. This lets screen readers know that an element is a list item without it being alielement, so the following HTML snippets are equivalent:However, this doesn't fully solve the issue as list item blocks don't share a common ancestor list block/element, so there is no
ul/role="list"to group list items by. The way around this is thearia-ownsattribute, which tells assistive technologies that an element is semantically the ancestor of some other elements, even though it isn't in the DOM. So the snippet below is also equivalent to the ones above:So this PR also adds a
divwitharia-ownsbefore each each consecutive string of list item blocks of the same type.All of what's described above, i.e. setting
role&idattributes on list item blocks as well as creating an invisibledivwithrole="list"andaria-owns, is done in theListAccessibilityExtensionvia decorations.The editor
rolehas also been changed fromtextboxtodocumentwhen the editor is read-only as the list item changes otherwise cause issues with VoiceOver. This would need further investigation if we wanted to apply this fix always, but #2809 only requires the fix for a read-only editor.Closes #2809
Rationale
List items are not properly read out by screen readers when the editor is read-only.
Changes
See above.
Impact
N/A
Testing
Updated default document for screen reader tests to also test lists. Screen reader tests remain skipped for the time being. Axe scans do not catch this issue as list item blocks are rendered using
pelements, so they are just seen as paragraphs and list item rules don't apply.Screenshots/Video
For the following editor structure:
Below are VoiceOver transcript snippets when announcing the blocks.
Before:
The
"Insertion at beginning of text. list box pop-up edit text"is due to the editor'stextboxrole, which announces that the content is editable even though it is not.After:
The list items are now properly announced as such, and as being part of the same list.
Checklist
Additional Notes
N/A