Repository navigation
ci(editorjs): run the VoiceOver suite on the merge queue - #190
Merged
Merged
Conversation
…eadless Introduce `@editorjs/editorjs`, a batteries-included entry point that composes `@editorjs/core` with the default DOM adapter, collaboration, tools, plugins, and UI, auto-initializes in its constructor, and exposes an `isReady` promise. It honors a v2-style `config.tools` map merged over the defaults with override-by-name. To support it, `Core` becomes a headless engine: - Remove the hardcoded default tool/plugin/infra `use()` calls from the constructor; drop the corresponding dependencies. - `initialize()` now fails loudly: it throws when no rendering adapter is registered or the `defaultBlock` tool is missing, and re-throws init failures instead of swallowing them. `ShortcutsPlugin` stays in `@editorjs/core` (no longer auto-registered); its relocation is handled by a separate change. Free the `@editorjs/editorjs` package name by aliasing the published v2 dependency as `editorjs-v2` (`npm:@editorjs/editorjs@…`) across core, sdk, ui, and tools/paragraph. The playground now consumes `@editorjs/editorjs` instead of wiring `Core` and UI by hand. Specs updated via OpenSpec change `add-editorjs-bundle-package` (archived). Co-Authored-By: Claude Opus 4.8 <[email protected]>
…gration test Core no longer depends on `@editorjs/dom-adapters`, so its dist is not built in CI when checking `@editorjs/core`. The leftover `jest.unstable_mockModule` call still resolved the specifier, failing the suite (and the Stryker dry run) with "Cannot find module '@editorjs/dom-adapters'". Co-Authored-By: Claude Opus 4.8 <[email protected]>
Runs lint, build and unit tests for the new bundle package on pull requests and merge groups, mirroring the other package workflows. Mutation tests are disabled since the package has no Stryker config yet. Co-Authored-By: Claude Opus 4.8 <[email protected]>
`tsc --build` only produces output for TypeScript project references, so `@editorjs/ui` — which is bundled by vite — had no `dist/ui.js` after building `@editorjs/editorjs` from a clean checkout. Jest then failed to resolve `@editorjs/ui` in the bundle test. Build dependencies first via `yarn workspaces foreach`, mirroring how the playground package builds its own dependency graph. Co-Authored-By: Claude Opus 4.8 <[email protected]>
- Core.initialize() no longer logs to console.error before re-throwing (dead try/catch removed) - BlockRenderer/BlockManager import editorjs-v2 types with `import type` instead of pulling it in as a runtime dependency - Playground attaches a .catch() to editor.isReady instead of discarding the promise, avoiding an unhandled rejection - mergeTools() now throws when a config.tools key doesn't match the tool's static name, since Core looks up tools by name, not map key - editorjs-bundle spec: replaced TBD Purpose and documented the new fail-loud mismatch scenario Co-Authored-By: Claude Sonnet 5 <[email protected]>
…hange Adds a Playwright e2e setup for @editorjs/editorjs (Vite-served fixture mounting the real bundle from src/, no build step) covering paragraph rendering, typing, and inline-toolbar bold. Documents a gotcha where locator.press()/selectText() desync Editor.js's native caret tracking. Writing these tests surfaced that @editorjs/ui and the paragraph tool emit no ARIA roles/names anywhere, forcing CSS-class selectors instead of getByRole/getByLabel. Proposes openspec change add-editor-aria-semantics to close that gap. Co-Authored-By: Claude Sonnet 5 <[email protected]>
…ebkit e2e target Replace role="textbox" on the blocks holder with role="group" to avoid nesting a textbox inside another textbox — contenteditable elements implicitly carry role="textbox" via HTML-AAM regardless of explicit role, so putting it on the holder too would be an ARIA anti-pattern. Instead, assign role="textbox" + aria-multiline="true" + aria-label to each block's own contenteditable element (starting with Paragraph). Update proposal, design decisions, specs (ui + tools), and task checklist to reflect the revised approach. Also adds webkit (Desktop Safari) as a second Playwright project alongside chromium in packages/editorjs/playwright.config.ts.
The editor emitted zero roles or accessible names, which made it unusable with a screen reader and forced the e2e suite onto CSS-class selectors. - BlocksUI: role="group" on the holder, so the block textboxes are not nested inside a textbox the holder would otherwise imply - Paragraph: role="textbox", aria-multiline, aria-label, and the configured placeholder as aria-placeholder - ToolbarUI: role="toolbar" with a roving tabindex, named plus button, aria-haspopup/aria-expanded, and focus moved to the button on open so Safari restores it there rather than to <body> - InlineToolbarUI: named toolbar, aria-activedescendant mirrored from ui-kit's highlight and made resolvable with aria-owns, plus a live region announcing the toolbar once per open - Popover item roles and names come from @editorjs/ui-kit 2.0.0, fixed upstream rather than worked around from here Covered by aria.spec.ts, axe.spec.ts and a VoiceOver suite driving a real screen reader (voiceover.spec.ts, macOS-only, excluded from test:e2e). Note: @editorjs/ui-kit 2.0.0 is not published yet, so the lockfile has no entry for it and `yarn install` fails. The local `link:` resolution the work was developed against has been removed deliberately — see tasks.md 5a.2/5a.3. Co-Authored-By: Claude Opus 5 <[email protected]>
The ARIA semantics the UI now depends on are enforced only by Playwright — nothing in the type surface catches a dropped role or accessible name, so until now a regression could land unnoticed. Adds an `include-e2e` input to the reusable package-check workflow and an `e2e-tests` composite action that installs chromium and webkit (cached by lockfile hash) and runs `test:e2e`. Kept as its own job: it needs browsers and takes minutes, and its failures read differently from a unit test's. `voiceover.spec.ts` stays excluded — screen readers cannot run headless. Co-Authored-By: Claude Opus 5 <[email protected]>
main landed the squashed bundle package (#183) plus work this branch predates, so the overlap had to be reconciled by hand: - ShortcutsPlugin: kept main's extraction into @editorjs/shortcuts-plugin — the bundle depends on it, registers it as a default plugin, and core no longer ships it. Added to editorjs's package.json, tsconfigs and index.ts alongside ClipboardPlugin. - ToolConfig now comes from @editorjs/sdk (#188) instead of editorjs-v2; dropped the alias import in sdk entities and paragraph while keeping this branch's ARIA changes in paragraph. - Core.initialize resolves UndoRedoManager after plugins/tools (main's ordering) and registers PluginRegistry. - packages/ui: took main's chokidar dev script and dependency reshuffle, kept this branch's @editorjs/ui-kit ^2.0.0 pin. - openspec: took main's archived bundle change and updated core/editorjs-bundle specs; this branch's add-editor-aria-semantics change is untouched. - Playwright/e2e tooling, .gitignore, eslint config and the e2e CI job are kept from this branch. yarn.lock was regenerated from main's and still has no @editorjs/ui-kit entry: 2.0.0 is not published yet, so `yarn install` fails exactly as it did on this branch before the merge. Verified with the workspace's tsc/jest directly — core (179), editorjs (8) pass; core, paragraph, shortcuts-plugin and editorjs typecheck; packages/ui's three errors are the unpublished ui-kit 2.0.0 APIs. Co-Authored-By: Claude Opus 5 <[email protected]>
…cape Escape closes the popover inside ui-kit, without going through #hide(), so #isVisible stayed true and every later appearance was taken for a reselection while already open: the toolbar was never announced again. Listen for PopoverEvent.Closed to reset it. The listener is removed before the popover is destroyed, since destroy() emits Closed as well and a rebuild would otherwise re-announce the toolbar on every selection change. Co-Authored-By: Claude Opus 5 <[email protected]>
With only the list reporter nothing was written to playwright-report/, so the upload step had nothing to upload and the traces recorded on a retry were lost with the runner. Co-Authored-By: Claude Opus 5 <[email protected]>
Hashing yarn.lock dropped the ~400MB browser cache on any unrelated dependency bump. Each Playwright release pins its own browser builds, so the installed version is all the key needs. Also bump upload-artifact to v7, in line with the other actions. Co-Authored-By: Claude Opus 5 <[email protected]>
@editorjs/ui-kit 2.0.0 depends on @editorjs/dom ^1.2.0, while the workspace packages still asked for ^1.1.0. Yarn will not re-resolve an already-locked entry, so the two ranges resolved separately and node_modules ended up with two copies of @editorjs/dom - including a nested one under @editorjs/caret, which pins ^1.1.0 transitively. Two copies split module state and break any identity check across the boundary. Widen the workspace ranges to ^1.2.0 and run `yarn dedupe @editorjs/dom` so both ranges collapse onto a single 1.2.0 resolution. The lockfile also gains the @editorjs/ui-kit 2.0.0 resolution it was missing. Co-Authored-By: Claude Opus 5 <[email protected]>
`#popoverElement` was only ever written: assigned from `getElement()`, used as the receiver for the id and the append, then nulled in the teardown. A local covers all three, and dropping the field removes a second piece of state that had to be kept in step with `#popover` for no one's benefit. `editor.spec.ts` was the only spec reimplementing the mount-and-wait sequence and the triple-click selection instead of importing `mountDocument` and `selectParagraphText`. The readiness signal it waited on lives in the fixture, so a change there would have fixed the three specs using the helpers and left this one timing out. Co-Authored-By: Claude Opus 5 <[email protected]>
…change The suite drives a real VoiceOver instance one test at a time, so it cannot sit in the path of every PR the way the headless suites do. It is wired as a job condition rather than a `paths:` filter on the workflow: GitHub leaves the checks of a path-filtered workflow Pending forever, which blocks any PR required to pass it, whereas a skipped job reports success and does not. The workflow therefore always runs and the macOS job decides for itself. `should-run-voiceover.sh` answers a narrower question than `should-run-ci.sh`: not "could this affect @editorjs/editorjs" - true of almost every change here - but "could this alter what a screen reader announces". It watches the UI and paragraph sources, the bundle, the suite itself, the lockfile (@editorjs/ui-kit owns the popover roles), and its own CI wiring. It fails open on anything it cannot determine, for the same reason its sibling does. Runs on a standard macos-15 runner, which is free on a public repository, and cancels a superseded run so a stale push does not hold one of the five macOS job slots shared across the account. Co-Authored-By: Claude Opus 5 <[email protected]>
Contributor
Unit Tests
Mutation Tests
|
`npx @guidepup/setup` runs the package's `guidepup` binary, which needs a subcommand - without one it prints its usage and exits 1, which is what failed the job. The correct call is `setup`, plus the `--ci` flag the README calls for in CI so the command does not wait on the manual steps a local machine needs. The change detection goes away with it. Deciding what counts as announcement- relevant turned out to be guesswork that had already missed the inline tools, and it is moot once the suite moves off the PR path onto a schedule. Runs on every PR for now, only so the job can be proven green before that move. Co-Authored-By: Claude Opus 5 <[email protected]>
`setup` and `install` are separate commands and both are required. `setup` configures the OS; `install` puts the preferences disk image into the project. With only the first, every test failed in VoiceOver.start() with "Failed to mount Guidepup preferences" out of resolveDmgPath - the error names both commands, and only one of them had been run. Co-Authored-By: Claude Opus 5 <[email protected]>
The baseline assertion guards the real one, but it fails on an *empty* match list - and the matches alone say nothing in that case. On CI it failed with "Expected: > 0, Received: 0" and no indication whether the cursor never reached the menu or VoiceOver worded the item differently than /menu item/ expects. `collectReachable` now carries the whole sweep alongside the matches so the message can distinguish the two. Retries follow playwright.config.ts, which already uses `isCI ? 2 : 0`. A real screen reader is driven by real keystrokes whose timing shifts under a loaded runner, and resetCursor's own comment records Case 18 as having been non-deterministic before. This does not hide a regression: a case that fails three times running is not a timing artefact, and the message now says what was announced each time. Co-Authored-By: Claude Opus 5 <[email protected]>
The sweep never reached a menu item because it started from the top of the web content. VO+Right moves between siblings rather than into them, and the block actions toolbar is the last sibling, so `next()` returned that same item for all twenty-five stops: ["hello world paragraph...", "block actions toolbar" x24]. The menu was open and correct the whole time - the page snapshot in the failure artifact shows menuitem "Text" present. Walking to the button that opens the toolbox first puts the cursor inside that container, which is where Case 4 reaches menu items from. Worth noting the baseline assertion earned its keep exactly as its comment said it would: without it, "no menu item is reachable after filtering" would have passed because no menu item was reachable before filtering either. Co-Authored-By: Claude Opus 5 <[email protected]>
…open Three CI runs agree on the constraint: once the toolbox menu is open, VoiceOver will not enter the block actions toolbar from the web content root. A forward walk from there reports "block actions toolbar" at all twenty-five stops and never descends, and the previous attempt showed even the button that opened the menu is unreachable that way - "did not reach an item matching /add block/i within 40 next steps". Case 4 reaches the items only because it never leaves the container. So neither sweep resets now. The first runs from where `act()` left the cursor. The second walks backwards to the search field, which re-anchors without going to the root and doubles as the navigation command VoiceOver needs before it will see the `fill()`. Co-Authored-By: Claude Opus 5 <[email protected]>
…reen The investigation notes were useful while the cause was unknown and are noise now that it is settled. What is left is one line per non-obvious decision. Co-Authored-By: Claude Opus 5 <[email protected]>
Eight minutes driving a real screen reader is too slow to sit in front of every PR push. The workflow still runs on pull_request for the rest of its jobs; the VoiceOver job skips there, and a skipped job reports success, so it never blocks a pull request. Inert until a merge queue is enabled on main - `merge_group` does not fire without one. Co-Authored-By: Claude Opus 5 <[email protected]>
5a.3 was still written as the blocking, not-yet-done publish. @editorjs/ui-kit 2.0.0 is published, the lockfile resolves it, and the suite was re-run against the published package: build, 973 unit tests, 80 e2e on both engines, lint. The task now also records the duplicate @editorjs/dom the regeneration surfaced and the dedupe that resolved it, since @editorjs/caret still pins the old range. The verification summary listed the same publish as an outstanding blocker and its verdict turned on it; both now say otherwise. Case 18's comment credited the CSS class with removing filtered items from the accessibility tree. ui-kit sets `hidden` alongside the class and that is what does it - as the spec and aria.spec.ts both already said. Co-Authored-By: Claude Opus 5 <[email protected]>
10.5b, 10.7b and 10.8 were records of pre-existing bugs the change deliberately did not fix, not work items. Every archived change here has zero unchecked tasks, so as encoded they would have blocked archiving and reported 69/72. The findings themselves are preserved outside the repo. What referenced them by number - the proposal, the design note, 10.1c, 10.7a and the verification summary - now describes the bug instead, so nothing points at a task that is gone. Co-Authored-By: Claude Opus 5 <[email protected]>
…gate # Conflicts: # packages/editorjs/e2e/tests/voiceover.spec.ts
gohabereg
changed the title
neSpecc
approved these changes
Oct 1, 2026
# Conflicts: # packages/editorjs/e2e/tests/voiceover.spec.ts # packages/editorjs/playwright.voiceover.config.ts
Main already archives it under archive/2026-10-08-add-editor-aria-semantics; the copies are identical. Co-Authored-By: Claude Opus 5.5 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #187. Runs the 18-case VoiceOver suite in CI, on the merge queue only.
Why the merge queue and not every PR
The suite drives a real VoiceOver instance one test at a time and takes ~8 minutes, which is too
slow to sit in front of every push. It is wired as a job condition, not a
paths:filter onthe workflow, and the distinction matters:
paths:filter leaves its checks Pending, which blocks a PRrequired to pass it. GitHub's own advice is "avoid requiring workflows that can be skipped".
if:reports success and does not block.So
editorjs.ymlstill runs onpull_requestfor the rest of its jobs, and only the macOS jobcarries
if: github.event_name == 'merge_group'.An earlier revision of this PR tried to narrow it with a change-detection script instead. That is
removed: deciding what counts as "announcement-relevant" turned out to be guesswork that had
already missed the inline tools, and it is moot once the suite is off the PR path.
Getting it green
Three rounds, each a real failure rather than a flake:
npx @guidepup/setupneeds a subcommand. Without one it prints usage and exits 1, so everylater step was skipped. It is
setup --ci— the flag keeps it from waiting on prompts for themanual steps a local machine would need.
setupandinstallare both required.setupconfigures the OS;installputs thepreferences disk image into the project. With only the first, all 17 cases failed in
VoiceOver.start()with "Failed to mount Guidepup preferences".the toolbox menu is open VoiceOver will not enter the block actions toolbar from there — a
forward walk reports
"block actions toolbar"at all 25 stops, and even the button that openedthe menu becomes unreachable. Both sweeps now anchor on that button, which is the position
Case 4 reaches the items from.
Diagnosing (3) needed two throwaway diagnostics that are kept, because both gaps would recur:
collectReachablereturns the whole sweep alongside the matches, andwalkTo's error carries theroute it took. The first failure said only
Expected: > 0, Received: 0; the second said only"did not reach it" — neither distinguished "swept past" from "stalled against a wall".
retries: isCI ? 2 : 0matchesplaywright.config.ts. It earned itself immediately: on one runCase 7 failed and passed on retry (a genuine timing flake) while Case 18 failed all three, which
is what separated the two.
Verification
Suite green locally: 18/18, ~6–8 minutes.
CI reached the same point — 16 passed, 1 flaky, 1 failed — before the Case 18 fix, which was
diagnosed from the CI announcement logs and confirmed locally.
Before this gates anything
Two repository settings, neither of which I can make:
main.required_merge_queueisnulltoday, somerge_groupnever fires and this job is inert until it is turned on.
VoiceOverto the required checks. Running in the queue does not gate on its own.Worth considering first:
package-check / e2e-testsis also not required. The headlessaria.spec.ts/axe.spec.tssuite runs on every PR, takes seconds, is deterministic, and coversthe structural half of the same surface — the cheaper gate to require.
🤖 Generated with Claude Code