From 0425785b0dbfb6b4e94d8d00d8898a20c898fa52 Mon Sep 17 00:00:00 2001 From: gohabereg Date: Tue, 29 Sep 2026 18:30:11 +0100 Subject: [PATCH 1/8] ci(editorjs): gate PRs on the VoiceOver suite when announcements can 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 --- .github/actions/voiceover-tests/action.yml | 68 +++++++++++++ .github/scripts/should-run-voiceover.sh | 108 +++++++++++++++++++++ .github/workflows/editorjs.yml | 53 ++++++++++ 3 files changed, 229 insertions(+) create mode 100644 .github/actions/voiceover-tests/action.yml create mode 100755 .github/scripts/should-run-voiceover.sh diff --git a/.github/actions/voiceover-tests/action.yml b/.github/actions/voiceover-tests/action.yml new file mode 100644 index 00000000..99e5df28 --- /dev/null +++ b/.github/actions/voiceover-tests/action.yml @@ -0,0 +1,68 @@ +name: "VoiceOver tests" +description: Run a package's Guidepup VoiceOver suite against a real screen reader +inputs: + package-name: + description: 'Name of the package' + required: true + working-directory: + description: 'Package working directory' + required: true +runs: + using: "composite" + steps: + - name: Setup Node.js + uses: actions/setup-node@v6 + with: + node-version-file: .nvmrc + + - name: Setup environment + uses: ./.github/actions/setup + + # Grants the accessibility and automation permissions VoiceOver scripting needs, and + # installs the screen reader assets Guidepup drives. Nothing else can do this: the + # permissions live in the OS TCC database, not in the workspace. + # + # The CLI rather than guidepup/setup-action, which is archived upstream. + - name: Configure the runner for screen reader automation + shell: bash + run: npx --yes @guidepup/setup + + # webkit only, matching playwright.voiceover.config.ts: VoiceOver's support for Safari is + # what the suite's expectations are written against. No --with-deps, which is a Linux-only + # concern. Cached by Playwright version for the same reason as the headless e2e action. + - name: Resolve Playwright version + id: playwright-version + shell: bash + run: echo "version=$(yarn workspace ${{ inputs.package-name }} exec playwright --version | awk '{ print $2 }')" >> "$GITHUB_OUTPUT" + + - name: Restore Playwright browsers + uses: actions/cache@v5 + id: playwright-cache + with: + path: ~/Library/Caches/ms-playwright + key: ${{ runner.os }}-playwright-${{ steps.playwright-version.outputs.version }} + + - name: Install Playwright webkit + if: ${{ steps.playwright-cache.outputs.cache-hit != 'true' }} + shell: bash + run: yarn workspace ${{ inputs.package-name }} exec playwright install webkit + + # The suite's own webServer command builds the workspace dependencies first - the fixture + # reaches @editorjs/ui and the tools through their `dist`, so an unbuilt change is invisible + # to it. Same reasoning as the headless e2e action. + - name: Run VoiceOver tests + shell: bash + run: yarn workspace ${{ inputs.package-name }} run test:e2e:voiceover + + # Guidepup records what VoiceOver actually said. On a failure that transcript is the whole + # diagnosis - the assertion message alone only says the expected phrase never arrived. + - name: Upload VoiceOver transcripts + if: ${{ !cancelled() }} + uses: actions/upload-artifact@v7 + with: + name: voiceover-recordings + path: | + ${{ inputs.working-directory }}/test-results/ + ${{ inputs.working-directory }}/recordings/ + if-no-files-found: ignore + retention-days: 7 diff --git a/.github/scripts/should-run-voiceover.sh b/.github/scripts/should-run-voiceover.sh new file mode 100755 index 00000000..b7bfd79c --- /dev/null +++ b/.github/scripts/should-run-voiceover.sh @@ -0,0 +1,108 @@ +#!/bin/bash + +# Decide whether the VoiceOver suite needs to run for this pull request. +# +# Deliberately narrower than should-run-ci.sh. That script answers "could this change affect +# @editorjs/editorjs at all", which is true of almost every change in the monorepo; this suite +# drives a real screen reader one test at a time and takes tens of minutes, so running it on +# that signal would put it in the path of every PR. This answers the narrower question: could +# this change alter what a screen reader *announces*. +# +# Usage: bash .github/scripts/should-run-voiceover.sh [--verbose] +# Exits 0 when the suite should run, 1 when it can be skipped. + +# Don't exit on error - failures are handled explicitly so the script can fail open +set +e + +BASE_REF="${1}" +VERBOSE="${2}" + +if [ -z "$BASE_REF" ]; then + echo "Usage: bash .github/scripts/should-run-voiceover.sh [--verbose]" + exit 1 +fi + +debug() { + if [ "$VERBOSE" = "--verbose" ] || [ "$VERBOSE" = "-v" ]; then + echo "[DEBUG] $@" >&2 + fi +} + +# Paths whose contents decide what assistive technology announces. +# +# - packages/ui/src roles, accessible names, live regions, the roving tabindex +# - packages/tools/paragraph/src the block's own role/name/aria-placeholder +# - packages/editorjs/src which tools are mounted, hence what is on screen to announce +# - packages/editorjs/e2e the suite, its fixtures and its shared helpers +# - yarn.lock @editorjs/ui-kit owns the popover roles the suite asserts on, +# so a bump to it changes announcements without touching src +# - the CI wiring below so a change to the gate is validated by the gate itself +# +# packages/core is deliberately absent. It drives selection and caret, which the toolbar reacts +# to, but the structural half of that is already covered on every PR by the headless aria.spec.ts +# suite - what this one adds is announcement verification. Add `packages/core/src` here if a core +# regression ever reaches announcements without aria.spec.ts catching it first. +WATCHED_PATHS=( + packages/ui/src + packages/tools/paragraph/src + packages/editorjs/src + packages/editorjs/e2e + packages/editorjs/playwright.voiceover.config.ts + yarn.lock + .github/workflows/editorjs.yml + .github/actions/voiceover-tests + .github/scripts/should-run-voiceover.sh +) + +# Resolve the base ref - same ladder as should-run-ci.sh, for the same reasons +RESOLVED_BASE_REF="" + +if git rev-parse --verify "$BASE_REF" >/dev/null 2>&1; then + RESOLVED_BASE_REF="$BASE_REF" + debug "Resolved base ref as: $BASE_REF" +fi + +if [ -z "$RESOLVED_BASE_REF" ] && git rev-parse --verify "origin/$BASE_REF" >/dev/null 2>&1; then + RESOLVED_BASE_REF="origin/$BASE_REF" + debug "Resolved base ref as: origin/$BASE_REF" +fi + +if [ -z "$RESOLVED_BASE_REF" ]; then + if git rev-parse --verify "origin/main" >/dev/null 2>&1; then + RESOLVED_BASE_REF="origin/main" + debug "Resolved base ref as: origin/main" + elif git rev-parse --verify "main" >/dev/null 2>&1; then + RESOLVED_BASE_REF="main" + debug "Resolved base ref as: main" + fi +fi + +# Fail open: a base ref we cannot resolve means we cannot tell what changed, and a suite that +# silently never runs is worse than one that runs when it did not have to +if [ -z "$RESOLVED_BASE_REF" ]; then + debug "Warning: could not resolve base ref '$BASE_REF'" + debug "Assuming the change is relevant (safe for CI)" + exit 0 +fi + +debug "Base ref: $RESOLVED_BASE_REF" + +CHANGED=$(git diff --name-only "$RESOLVED_BASE_REF...HEAD" -- "${WATCHED_PATHS[@]}" 2>/dev/null) +DIFF_STATUS=$? + +# Fail open again: `git diff` erroring (a bad range, a missing object after a shallow fetch) +# is indistinguishable here from "nothing changed", and the two must not be treated alike +if [ $DIFF_STATUS -ne 0 ]; then + debug "Warning: git diff against $RESOLVED_BASE_REF failed" + debug "Assuming the change is relevant (safe for CI)" + exit 0 +fi + +if [ -n "$CHANGED" ]; then + debug "✓ Announcement-relevant files changed:" + debug "$CHANGED" + exit 0 +fi + +debug "✗ No announcement-relevant changes" +exit 1 diff --git a/.github/workflows/editorjs.yml b/.github/workflows/editorjs.yml index bf423d72..eddd59e5 100644 --- a/.github/workflows/editorjs.yml +++ b/.github/workflows/editorjs.yml @@ -22,3 +22,56 @@ jobs: include-e2e: true secrets: stryker_dashboard_api_key: ${{ secrets.STRYKER_DASHBOARD_API_KEY }} + + # Deliberately a job condition rather than a `paths:` filter on the workflow. A workflow + # skipped by path filtering leaves its checks Pending forever, which blocks a PR that is + # required to pass it; a skipped *job* reports success and does not. So this workflow always + # runs and the expensive job below decides for itself whether it has anything to do. + voiceover-should-run: + name: Check if VoiceOver suite needs to run + runs-on: ubuntu-latest + outputs: + run: ${{ steps.check.outputs.run }} + steps: + - uses: actions/checkout@v6 + with: + fetch-depth: 0 + + - name: Determine if the VoiceOver suite should run + id: check + run: | + # The merge queue is the last gate before main, so it always runs the suite + if [ "${{ github.event_name }}" != "pull_request" ]; then + echo "run=true" >> $GITHUB_OUTPUT + exit 0 + fi + + if bash .github/scripts/should-run-voiceover.sh "${{ github.base_ref }}" --verbose; then + echo "run=true" >> $GITHUB_OUTPUT + else + echo "run=false" >> $GITHUB_OUTPUT + fi + + voiceover: + name: VoiceOver + needs: voiceover-should-run + if: ${{ needs.voiceover-should-run.outputs.run == 'true' }} + # A real screen reader needs a real macOS window server session. Standard runner, which is + # what keeps this free on a public repository - the larger variants are billed regardless. + runs-on: macos-15 + # Seventeen tests at the config's five-minute per-test budget, run one at a time. The cap is + # well above the expected runtime and exists so a wedged VoiceOver fails rather than hangs. + timeout-minutes: 60 + # Only five macOS jobs can run at once across the whole account, so a superseded push must + # not keep one of them occupied for the rest of its run + concurrency: + group: voiceover-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + steps: + - uses: actions/checkout@v6 + + - name: Run VoiceOver tests + uses: ./.github/actions/voiceover-tests + with: + package-name: '@editorjs/editorjs' + working-directory: './packages/editorjs' From b114800e8a33323965905f04e68282785f91e939 Mon Sep 17 00:00:00 2001 From: gohabereg Date: Tue, 29 Sep 2026 19:08:03 +0100 Subject: [PATCH 2/8] ci(editorjs): fix the Guidepup setup invocation and drop the change gate `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 --- .github/actions/voiceover-tests/action.yml | 10 +- .github/scripts/should-run-voiceover.sh | 108 --------------------- .github/workflows/editorjs.yml | 32 +----- 3 files changed, 7 insertions(+), 143 deletions(-) delete mode 100755 .github/scripts/should-run-voiceover.sh diff --git a/.github/actions/voiceover-tests/action.yml b/.github/actions/voiceover-tests/action.yml index 99e5df28..d9600a22 100644 --- a/.github/actions/voiceover-tests/action.yml +++ b/.github/actions/voiceover-tests/action.yml @@ -18,14 +18,16 @@ runs: - name: Setup environment uses: ./.github/actions/setup - # Grants the accessibility and automation permissions VoiceOver scripting needs, and - # installs the screen reader assets Guidepup drives. Nothing else can do this: the - # permissions live in the OS TCC database, not in the workspace. + # Grants the accessibility and automation permissions VoiceOver scripting needs. Nothing + # else can do this: the permissions live in the OS TCC database, not in the workspace. + # + # `--ci` is what keeps the command from prompting for the manual steps a local machine + # would need. Without it the CLI waits for interaction that never comes. # # The CLI rather than guidepup/setup-action, which is archived upstream. - name: Configure the runner for screen reader automation shell: bash - run: npx --yes @guidepup/setup + run: npx --yes @guidepup/setup setup --ci # webkit only, matching playwright.voiceover.config.ts: VoiceOver's support for Safari is # what the suite's expectations are written against. No --with-deps, which is a Linux-only diff --git a/.github/scripts/should-run-voiceover.sh b/.github/scripts/should-run-voiceover.sh deleted file mode 100755 index b7bfd79c..00000000 --- a/.github/scripts/should-run-voiceover.sh +++ /dev/null @@ -1,108 +0,0 @@ -#!/bin/bash - -# Decide whether the VoiceOver suite needs to run for this pull request. -# -# Deliberately narrower than should-run-ci.sh. That script answers "could this change affect -# @editorjs/editorjs at all", which is true of almost every change in the monorepo; this suite -# drives a real screen reader one test at a time and takes tens of minutes, so running it on -# that signal would put it in the path of every PR. This answers the narrower question: could -# this change alter what a screen reader *announces*. -# -# Usage: bash .github/scripts/should-run-voiceover.sh [--verbose] -# Exits 0 when the suite should run, 1 when it can be skipped. - -# Don't exit on error - failures are handled explicitly so the script can fail open -set +e - -BASE_REF="${1}" -VERBOSE="${2}" - -if [ -z "$BASE_REF" ]; then - echo "Usage: bash .github/scripts/should-run-voiceover.sh [--verbose]" - exit 1 -fi - -debug() { - if [ "$VERBOSE" = "--verbose" ] || [ "$VERBOSE" = "-v" ]; then - echo "[DEBUG] $@" >&2 - fi -} - -# Paths whose contents decide what assistive technology announces. -# -# - packages/ui/src roles, accessible names, live regions, the roving tabindex -# - packages/tools/paragraph/src the block's own role/name/aria-placeholder -# - packages/editorjs/src which tools are mounted, hence what is on screen to announce -# - packages/editorjs/e2e the suite, its fixtures and its shared helpers -# - yarn.lock @editorjs/ui-kit owns the popover roles the suite asserts on, -# so a bump to it changes announcements without touching src -# - the CI wiring below so a change to the gate is validated by the gate itself -# -# packages/core is deliberately absent. It drives selection and caret, which the toolbar reacts -# to, but the structural half of that is already covered on every PR by the headless aria.spec.ts -# suite - what this one adds is announcement verification. Add `packages/core/src` here if a core -# regression ever reaches announcements without aria.spec.ts catching it first. -WATCHED_PATHS=( - packages/ui/src - packages/tools/paragraph/src - packages/editorjs/src - packages/editorjs/e2e - packages/editorjs/playwright.voiceover.config.ts - yarn.lock - .github/workflows/editorjs.yml - .github/actions/voiceover-tests - .github/scripts/should-run-voiceover.sh -) - -# Resolve the base ref - same ladder as should-run-ci.sh, for the same reasons -RESOLVED_BASE_REF="" - -if git rev-parse --verify "$BASE_REF" >/dev/null 2>&1; then - RESOLVED_BASE_REF="$BASE_REF" - debug "Resolved base ref as: $BASE_REF" -fi - -if [ -z "$RESOLVED_BASE_REF" ] && git rev-parse --verify "origin/$BASE_REF" >/dev/null 2>&1; then - RESOLVED_BASE_REF="origin/$BASE_REF" - debug "Resolved base ref as: origin/$BASE_REF" -fi - -if [ -z "$RESOLVED_BASE_REF" ]; then - if git rev-parse --verify "origin/main" >/dev/null 2>&1; then - RESOLVED_BASE_REF="origin/main" - debug "Resolved base ref as: origin/main" - elif git rev-parse --verify "main" >/dev/null 2>&1; then - RESOLVED_BASE_REF="main" - debug "Resolved base ref as: main" - fi -fi - -# Fail open: a base ref we cannot resolve means we cannot tell what changed, and a suite that -# silently never runs is worse than one that runs when it did not have to -if [ -z "$RESOLVED_BASE_REF" ]; then - debug "Warning: could not resolve base ref '$BASE_REF'" - debug "Assuming the change is relevant (safe for CI)" - exit 0 -fi - -debug "Base ref: $RESOLVED_BASE_REF" - -CHANGED=$(git diff --name-only "$RESOLVED_BASE_REF...HEAD" -- "${WATCHED_PATHS[@]}" 2>/dev/null) -DIFF_STATUS=$? - -# Fail open again: `git diff` erroring (a bad range, a missing object after a shallow fetch) -# is indistinguishable here from "nothing changed", and the two must not be treated alike -if [ $DIFF_STATUS -ne 0 ]; then - debug "Warning: git diff against $RESOLVED_BASE_REF failed" - debug "Assuming the change is relevant (safe for CI)" - exit 0 -fi - -if [ -n "$CHANGED" ]; then - debug "✓ Announcement-relevant files changed:" - debug "$CHANGED" - exit 0 -fi - -debug "✗ No announcement-relevant changes" -exit 1 diff --git a/.github/workflows/editorjs.yml b/.github/workflows/editorjs.yml index eddd59e5..c033499b 100644 --- a/.github/workflows/editorjs.yml +++ b/.github/workflows/editorjs.yml @@ -23,39 +23,9 @@ jobs: secrets: stryker_dashboard_api_key: ${{ secrets.STRYKER_DASHBOARD_API_KEY }} - # Deliberately a job condition rather than a `paths:` filter on the workflow. A workflow - # skipped by path filtering leaves its checks Pending forever, which blocks a PR that is - # required to pass it; a skipped *job* reports success and does not. So this workflow always - # runs and the expensive job below decides for itself whether it has anything to do. - voiceover-should-run: - name: Check if VoiceOver suite needs to run - runs-on: ubuntu-latest - outputs: - run: ${{ steps.check.outputs.run }} - steps: - - uses: actions/checkout@v6 - with: - fetch-depth: 0 - - - name: Determine if the VoiceOver suite should run - id: check - run: | - # The merge queue is the last gate before main, so it always runs the suite - if [ "${{ github.event_name }}" != "pull_request" ]; then - echo "run=true" >> $GITHUB_OUTPUT - exit 0 - fi - - if bash .github/scripts/should-run-voiceover.sh "${{ github.base_ref }}" --verbose; then - echo "run=true" >> $GITHUB_OUTPUT - else - echo "run=false" >> $GITHUB_OUTPUT - fi - + # Temporarily on every PR, to prove the job green before it moves to a schedule. voiceover: name: VoiceOver - needs: voiceover-should-run - if: ${{ needs.voiceover-should-run.outputs.run == 'true' }} # A real screen reader needs a real macOS window server session. Standard runner, which is # what keeps this free on a public repository - the larger variants are billed regardless. runs-on: macos-15 From db40f6ee1c3c7f84fc8b09e8c9863c9864ad468c Mon Sep 17 00:00:00 2001 From: gohabereg Date: Tue, 29 Sep 2026 19:19:14 +0100 Subject: [PATCH 3/8] ci(editorjs): install the Guidepup screen reader assets `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 --- .github/actions/voiceover-tests/action.yml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.github/actions/voiceover-tests/action.yml b/.github/actions/voiceover-tests/action.yml index d9600a22..8a96899b 100644 --- a/.github/actions/voiceover-tests/action.yml +++ b/.github/actions/voiceover-tests/action.yml @@ -29,6 +29,14 @@ runs: shell: bash run: npx --yes @guidepup/setup setup --ci + # Separate from `setup`, and both are required: `setup` configures the OS, `install` puts + # the preferences disk image into the project. Without it VoiceOver.start() fails in + # resolveDmgPath with "Failed to mount Guidepup preferences". Runs after `yarn install`, + # since it installs into node_modules. + - name: Install screen reader assets + shell: bash + run: npx --yes @guidepup/setup install + # webkit only, matching playwright.voiceover.config.ts: VoiceOver's support for Safari is # what the suite's expectations are written against. No --with-deps, which is a Linux-only # concern. Cached by Playwright version for the same reason as the headless e2e action. From b33e7ac0b5dbf9d1541ddded5b9c0a05ff40e702 Mon Sep 17 00:00:00 2001 From: gohabereg Date: Tue, 29 Sep 2026 19:31:16 +0100 Subject: [PATCH 4/8] test(editorjs): report what VoiceOver said when Case 18's baseline fails 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 --- packages/editorjs/e2e/tests/voiceover.spec.ts | 31 ++++++++++++++++--- .../editorjs/playwright.voiceover.config.ts | 5 +++ 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/packages/editorjs/e2e/tests/voiceover.spec.ts b/packages/editorjs/e2e/tests/voiceover.spec.ts index 32f89c7b..28efa7ec 100644 --- a/packages/editorjs/e2e/tests/voiceover.spec.ts +++ b/packages/editorjs/e2e/tests/voiceover.spec.ts @@ -1117,6 +1117,15 @@ test('Case 17: announces applied links as links', async ({ page, voiceOver }) => expect(insideBlock.join(' | ')).toContain('link'); }); +/** What a sweep found, kept alongside everything it passed through. */ +interface ReachableItems { + /** Distinct items matching the pattern, in the order the cursor met them. */ + matches: string[]; + + /** Every stop the sweep made, matching or not. */ + all: string[]; +} + /** * Every distinct item matching `pattern` that VoiceOver's cursor reaches within `SCAN_STEPS` * forward steps. @@ -1124,13 +1133,24 @@ test('Case 17: announces applied links as links', async ({ page, voiceOver }) => * Returns the matches rather than a boolean so that asserting "nothing is reachable" fails with * the offending announcements in the message. A bare `toBe(false)` says only that something * matched, which leaves you guessing at whether the fault is the page or the pattern. + * + * `all` carries the whole sweep alongside them, because the *baseline* assertion fails the other + * way round - on an empty `matches` - and the matches alone say nothing at all in that case. The + * question it leaves open is whether the cursor never reached the menu or VoiceOver worded it + * differently than the pattern expects, and only the full sweep distinguishes the two. * @param voiceOver - Guidepup VoiceOver controller * @param pattern - matched against the current item's text at each stop */ -async function collectReachable(voiceOver: VoiceOverPlaywright, pattern: RegExp): Promise { +async function collectReachable( + voiceOver: VoiceOverPlaywright, + pattern: RegExp +): Promise { const reachable = await sweep(voiceOver, SCAN_STEPS); - return [...new Set(reachable.filter(item => pattern.test(item)))]; + return { + matches: [...new Set(reachable.filter(item => pattern.test(item)))], + all: reachable, + }; } test('Case 18: does not let the cursor reach toolbox items filtered out by search', async ({ page, voiceOver }) => { @@ -1152,7 +1172,10 @@ test('Case 18: does not let the cursor reach toolbox items filtered out by searc const beforeFiltering = await collectReachable(voiceOver, menuItemPattern); - expect(beforeFiltering.length).toBeGreaterThan(0); + expect( + beforeFiltering.matches.length, + `Nothing matched ${menuItemPattern}. VoiceOver announced: ${JSON.stringify(beforeFiltering.all)}` + ).toBeGreaterThan(0); // Opening the toolbox puts DOM focus in its search field; filling it filters the list. await page.getByRole('searchbox', { name: 'Search' }).fill('no such tool'); @@ -1169,5 +1192,5 @@ test('Case 18: does not let the cursor reach toolbox items filtered out by searc // own `display: flex`), so nothing matching should remain reachable. If something does, the // message below carries its announcement - the answer to "is this the hidden item, or is the // pattern matching something else entirely" is not worth guessing at. - expect(await collectReachable(voiceOver, menuItemPattern)).toEqual([]); + expect((await collectReachable(voiceOver, menuItemPattern)).matches).toEqual([]); }); diff --git a/packages/editorjs/playwright.voiceover.config.ts b/packages/editorjs/playwright.voiceover.config.ts index e3fbebf4..cecf9e91 100644 --- a/packages/editorjs/playwright.voiceover.config.ts +++ b/packages/editorjs/playwright.voiceover.config.ts @@ -24,6 +24,11 @@ export default defineConfig({ testMatch: /voiceover\.spec\.ts/, reporter: 'list', timeout: TEST_TIMEOUT_MS, + // Matches playwright.config.ts. A real screen reader is driven by real keystrokes whose timing + // shifts under a loaded runner, and Case 18 is documented as having been non-deterministic + // before (see resetCursor). A retry distinguishes that from a genuine regression rather than + // hiding one: a case that fails twice in a row is not a timing artefact. + retries: isCI ? 2 : 0, use: { ...screenReaderConfig.use, baseURL: `http://localhost:${PORT}`, From ae2d4e001a59fb74857819673a99ec62c3418fbc Mon Sep 17 00:00:00 2001 From: gohabereg Date: Tue, 29 Sep 2026 19:46:27 +0100 Subject: [PATCH 5/8] test(editorjs): enter the toolbox before sweeping it in Case 18 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 --- packages/editorjs/e2e/tests/voiceover.spec.ts | 27 +++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/packages/editorjs/e2e/tests/voiceover.spec.ts b/packages/editorjs/e2e/tests/voiceover.spec.ts index 28efa7ec..63316234 100644 --- a/packages/editorjs/e2e/tests/voiceover.spec.ts +++ b/packages/editorjs/e2e/tests/voiceover.spec.ts @@ -1117,6 +1117,23 @@ test('Case 17: announces applied links as links', async ({ page, voiceOver }) => expect(insideBlock.join(' | ')).toContain('link'); }); +/** + * Puts VoiceOver's cursor inside the open toolbox after a page-driven change, ready to sweep. + * + * `resetCursor` on its own is not enough, and the way it fails is silent: it leaves the cursor at + * the top of the web content, and a forward walk from there stops dead on the block actions + * toolbar. VO+Right moves between siblings rather than into them, and the toolbar is the last + * sibling, so `next()` returns that same item for the whole sweep without ever reaching the menu + * inside it - twenty-five stops, all of them "block actions toolbar". Walking to the button that + * opens the toolbox puts the cursor within that container first, which is the position Case 4 + * reaches the menu items from. + * @param voiceOver - Guidepup VoiceOver controller + */ +async function resetCursorToToolbox(voiceOver: VoiceOverPlaywright): Promise { + await resetCursor(voiceOver); + await findItem(voiceOver, /add block/i); +} + /** What a sweep found, kept alongside everything it passed through. */ interface ReachableItems { /** Distinct items matching the pattern, in the order the cursor met them. */ @@ -1166,9 +1183,9 @@ test('Case 18: does not let the cursor reach toolbox items filtered out by searc const menuItemPattern = /menu item/; // Baseline. It also guards the real assertion below: if VoiceOver words menu items - // differently than this expects, the test fails here instead of making "nothing reachable" - // pass for the wrong reason. - await resetCursor(voiceOver); + // differently than this expects, or the sweep never reaches them at all, the test fails here + // instead of making "nothing reachable" pass for the wrong reason. + await resetCursorToToolbox(voiceOver); const beforeFiltering = await collectReachable(voiceOver, menuItemPattern); @@ -1182,11 +1199,11 @@ test('Case 18: does not let the cursor reach toolbox items filtered out by searc await expect(menu.getByRole('menuitem')).toHaveCount(0); - // Back to the top rather than continuing from wherever the scan stopped - that item may be + // Back to the toolbox rather than continuing from wherever the scan stopped - that item may be // one of the ones just hidden, and VoiceOver would keep describing it from where it stands. // The round trip inside resetCursor is what makes this reliable: `fill()` is a page-driven // change, so without it VoiceOver can still be describing the unfiltered list. - await resetCursor(voiceOver); + await resetCursorToToolbox(voiceOver); // Filtered items are `display: none` (ui-kit's `--hidden` class, which wins over the item's // own `display: flex`), so nothing matching should remain reachable. If something does, the From 07d74e48309e42c3ca4e69e7dd76727311d57eb4 Mon Sep 17 00:00:00 2001 From: gohabereg Date: Tue, 29 Sep 2026 19:59:03 +0100 Subject: [PATCH 6/8] test(editorjs): keep the cursor inside the toolbox while the menu is 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 --- packages/editorjs/e2e/tests/voiceover.spec.ts | 38 +++++++------------ 1 file changed, 14 insertions(+), 24 deletions(-) diff --git a/packages/editorjs/e2e/tests/voiceover.spec.ts b/packages/editorjs/e2e/tests/voiceover.spec.ts index 63316234..b26236c4 100644 --- a/packages/editorjs/e2e/tests/voiceover.spec.ts +++ b/packages/editorjs/e2e/tests/voiceover.spec.ts @@ -1117,23 +1117,6 @@ test('Case 17: announces applied links as links', async ({ page, voiceOver }) => expect(insideBlock.join(' | ')).toContain('link'); }); -/** - * Puts VoiceOver's cursor inside the open toolbox after a page-driven change, ready to sweep. - * - * `resetCursor` on its own is not enough, and the way it fails is silent: it leaves the cursor at - * the top of the web content, and a forward walk from there stops dead on the block actions - * toolbar. VO+Right moves between siblings rather than into them, and the toolbar is the last - * sibling, so `next()` returns that same item for the whole sweep without ever reaching the menu - * inside it - twenty-five stops, all of them "block actions toolbar". Walking to the button that - * opens the toolbox puts the cursor within that container first, which is the position Case 4 - * reaches the menu items from. - * @param voiceOver - Guidepup VoiceOver controller - */ -async function resetCursorToToolbox(voiceOver: VoiceOverPlaywright): Promise { - await resetCursor(voiceOver); - await findItem(voiceOver, /add block/i); -} - /** What a sweep found, kept alongside everything it passed through. */ interface ReachableItems { /** Distinct items matching the pattern, in the order the cursor met them. */ @@ -1182,11 +1165,16 @@ test('Case 18: does not let the cursor reach toolbox items filtered out by searc const menuItemPattern = /menu item/; + // Swept from where `act()` left the cursor, deliberately without resetting first. Once the + // menu is open VoiceOver will not enter the block actions toolbar from the web content root + // at all: a forward walk from there reports "block actions toolbar" at every one of its stops + // and never descends, and even the button that opened the menu stops being reachable. Staying + // inside the container is the only position the items can be swept from - it is how Case 4 + // reaches them too. + // // Baseline. It also guards the real assertion below: if VoiceOver words menu items // differently than this expects, or the sweep never reaches them at all, the test fails here // instead of making "nothing reachable" pass for the wrong reason. - await resetCursorToToolbox(voiceOver); - const beforeFiltering = await collectReachable(voiceOver, menuItemPattern); expect( @@ -1199,11 +1187,13 @@ test('Case 18: does not let the cursor reach toolbox items filtered out by searc await expect(menu.getByRole('menuitem')).toHaveCount(0); - // Back to the toolbox rather than continuing from wherever the scan stopped - that item may be - // one of the ones just hidden, and VoiceOver would keep describing it from where it stands. - // The round trip inside resetCursor is what makes this reliable: `fill()` is a page-driven - // change, so without it VoiceOver can still be describing the unfiltered list. - await resetCursorToToolbox(voiceOver); + // Re-anchored rather than continuing from wherever the scan stopped - that item may be one of + // the ones just hidden, and VoiceOver would keep describing it from where it stands. Walking + // *backwards* to the search field rather than resetting to the top, for the reason above: the + // cursor has to stay inside the container. The walk is also the navigation command VoiceOver + // needs before it will see a page-driven change at all, so `fill()` becoming visible to it + // comes for free - without one it can still be describing the unfiltered list. + await findItem(voiceOver, /search/i, 'previous'); // Filtered items are `display: none` (ui-kit's `--hidden` class, which wins over the item's // own `display: flex`), so nothing matching should remain reachable. If something does, the From 6560f5cd566984ca4e0b129445cfb61a06430cb5 Mon Sep 17 00:00:00 2001 From: gohabereg Date: Tue, 29 Sep 2026 21:38:40 +0100 Subject: [PATCH 7/8] refactor: trim the comments added while getting the VoiceOver suite green 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 --- .github/actions/voiceover-tests/action.yml | 24 +++---------- .github/workflows/editorjs.yml | 9 ++--- packages/editorjs/e2e/tests/voiceover.spec.ts | 34 +++++++------------ .../editorjs/playwright.voiceover.config.ts | 5 +-- 4 files changed, 22 insertions(+), 50 deletions(-) diff --git a/.github/actions/voiceover-tests/action.yml b/.github/actions/voiceover-tests/action.yml index 8a96899b..e7b03e73 100644 --- a/.github/actions/voiceover-tests/action.yml +++ b/.github/actions/voiceover-tests/action.yml @@ -18,28 +18,16 @@ runs: - name: Setup environment uses: ./.github/actions/setup - # Grants the accessibility and automation permissions VoiceOver scripting needs. Nothing - # else can do this: the permissions live in the OS TCC database, not in the workspace. - # - # `--ci` is what keeps the command from prompting for the manual steps a local machine - # would need. Without it the CLI waits for interaction that never comes. - # - # The CLI rather than guidepup/setup-action, which is archived upstream. + # `--ci` skips the prompts for manual steps a local machine would need. - name: Configure the runner for screen reader automation shell: bash run: npx --yes @guidepup/setup setup --ci - # Separate from `setup`, and both are required: `setup` configures the OS, `install` puts - # the preferences disk image into the project. Without it VoiceOver.start() fails in - # resolveDmgPath with "Failed to mount Guidepup preferences". Runs after `yarn install`, - # since it installs into node_modules. + # Separate from `setup` and also required: it installs the preferences disk image. - name: Install screen reader assets shell: bash run: npx --yes @guidepup/setup install - # webkit only, matching playwright.voiceover.config.ts: VoiceOver's support for Safari is - # what the suite's expectations are written against. No --with-deps, which is a Linux-only - # concern. Cached by Playwright version for the same reason as the headless e2e action. - name: Resolve Playwright version id: playwright-version shell: bash @@ -52,20 +40,18 @@ runs: path: ~/Library/Caches/ms-playwright key: ${{ runner.os }}-playwright-${{ steps.playwright-version.outputs.version }} + # webkit only, matching playwright.voiceover.config.ts. - name: Install Playwright webkit if: ${{ steps.playwright-cache.outputs.cache-hit != 'true' }} shell: bash run: yarn workspace ${{ inputs.package-name }} exec playwright install webkit - # The suite's own webServer command builds the workspace dependencies first - the fixture - # reaches @editorjs/ui and the tools through their `dist`, so an unbuilt change is invisible - # to it. Same reasoning as the headless e2e action. + # The suite's webServer builds the workspace dependencies first; the fixture loads their dist. - name: Run VoiceOver tests shell: bash run: yarn workspace ${{ inputs.package-name }} run test:e2e:voiceover - # Guidepup records what VoiceOver actually said. On a failure that transcript is the whole - # diagnosis - the assertion message alone only says the expected phrase never arrived. + # The transcript of what VoiceOver said is the whole diagnosis on a failure. - name: Upload VoiceOver transcripts if: ${{ !cancelled() }} uses: actions/upload-artifact@v7 diff --git a/.github/workflows/editorjs.yml b/.github/workflows/editorjs.yml index c033499b..24a38478 100644 --- a/.github/workflows/editorjs.yml +++ b/.github/workflows/editorjs.yml @@ -26,14 +26,11 @@ jobs: # Temporarily on every PR, to prove the job green before it moves to a schedule. voiceover: name: VoiceOver - # A real screen reader needs a real macOS window server session. Standard runner, which is - # what keeps this free on a public repository - the larger variants are billed regardless. + # A real screen reader needs a real macOS session; the standard runner is free on a public repo. runs-on: macos-15 - # Seventeen tests at the config's five-minute per-test budget, run one at a time. The cap is - # well above the expected runtime and exists so a wedged VoiceOver fails rather than hangs. + # Well above the ~8 minute runtime, so a wedged VoiceOver fails rather than hangs. timeout-minutes: 60 - # Only five macOS jobs can run at once across the whole account, so a superseded push must - # not keep one of them occupied for the rest of its run + # Only five macOS jobs run at once account-wide, so a superseded push must not hold one. concurrency: group: voiceover-${{ github.event.pull_request.number || github.ref }} cancel-in-progress: true diff --git a/packages/editorjs/e2e/tests/voiceover.spec.ts b/packages/editorjs/e2e/tests/voiceover.spec.ts index b26236c4..b04de650 100644 --- a/packages/editorjs/e2e/tests/voiceover.spec.ts +++ b/packages/editorjs/e2e/tests/voiceover.spec.ts @@ -209,7 +209,11 @@ async function walkTo( } } - throw new Error(`VoiceOver did not reach an item matching ${pattern.toString()} within ${maxSteps} ${direction} steps`); + /** The route separates a cursor that swept past the target from one stalled against a wall. */ + throw new Error( + `VoiceOver did not reach an item matching ${pattern.toString()} within ${maxSteps} ${direction} steps. ` + + `It passed through: ${JSON.stringify(descriptions)}` + ); } /** @@ -1134,10 +1138,7 @@ interface ReachableItems { * the offending announcements in the message. A bare `toBe(false)` says only that something * matched, which leaves you guessing at whether the fault is the page or the pattern. * - * `all` carries the whole sweep alongside them, because the *baseline* assertion fails the other - * way round - on an empty `matches` - and the matches alone say nothing at all in that case. The - * question it leaves open is whether the cursor never reached the menu or VoiceOver worded it - * differently than the pattern expects, and only the full sweep distinguishes the two. + * `all` comes too: the baseline fails on an empty `matches`, which on its own says nothing. * @param voiceOver - Guidepup VoiceOver controller * @param pattern - matched against the current item's text at each stop */ @@ -1165,16 +1166,12 @@ test('Case 18: does not let the cursor reach toolbox items filtered out by searc const menuItemPattern = /menu item/; - // Swept from where `act()` left the cursor, deliberately without resetting first. Once the - // menu is open VoiceOver will not enter the block actions toolbar from the web content root - // at all: a forward walk from there reports "block actions toolbar" at every one of its stops - // and never descends, and even the button that opened the menu stops being reachable. Staying - // inside the container is the only position the items can be swept from - it is how Case 4 - // reaches them too. - // + // Both sweeps anchor here: with the menu open, only the button reaches the items. + await findItem(voiceOver, /add block/i, 'previous'); + // Baseline. It also guards the real assertion below: if VoiceOver words menu items - // differently than this expects, or the sweep never reaches them at all, the test fails here - // instead of making "nothing reachable" pass for the wrong reason. + // differently than this expects, the test fails here instead of making "nothing reachable" + // pass for the wrong reason. const beforeFiltering = await collectReachable(voiceOver, menuItemPattern); expect( @@ -1187,13 +1184,8 @@ test('Case 18: does not let the cursor reach toolbox items filtered out by searc await expect(menu.getByRole('menuitem')).toHaveCount(0); - // Re-anchored rather than continuing from wherever the scan stopped - that item may be one of - // the ones just hidden, and VoiceOver would keep describing it from where it stands. Walking - // *backwards* to the search field rather than resetting to the top, for the reason above: the - // cursor has to stay inside the container. The walk is also the navigation command VoiceOver - // needs before it will see a page-driven change at all, so `fill()` becoming visible to it - // comes for free - without one it can still be describing the unfiltered list. - await findItem(voiceOver, /search/i, 'previous'); + // Back to the anchor; the sweep ends past the popover, and this is what shows VoiceOver the fill(). + await findItem(voiceOver, /add block/i, 'previous'); // Filtered items are `display: none` (ui-kit's `--hidden` class, which wins over the item's // own `display: flex`), so nothing matching should remain reachable. If something does, the diff --git a/packages/editorjs/playwright.voiceover.config.ts b/packages/editorjs/playwright.voiceover.config.ts index cecf9e91..61066d8c 100644 --- a/packages/editorjs/playwright.voiceover.config.ts +++ b/packages/editorjs/playwright.voiceover.config.ts @@ -24,10 +24,7 @@ export default defineConfig({ testMatch: /voiceover\.spec\.ts/, reporter: 'list', timeout: TEST_TIMEOUT_MS, - // Matches playwright.config.ts. A real screen reader is driven by real keystrokes whose timing - // shifts under a loaded runner, and Case 18 is documented as having been non-deterministic - // before (see resetCursor). A retry distinguishes that from a genuine regression rather than - // hiding one: a case that fails twice in a row is not a timing artefact. + // Matches playwright.config.ts: screen reader timing shifts under a loaded runner. retries: isCI ? 2 : 0, use: { ...screenReaderConfig.use, From 09bd946c686a4953ef3c2d5f567c7ad743ba70b7 Mon Sep 17 00:00:00 2001 From: gohabereg Date: Tue, 29 Sep 2026 21:56:15 +0100 Subject: [PATCH 8/8] ci(editorjs): run the VoiceOver suite on the merge queue only 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 --- .github/workflows/editorjs.yml | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/.github/workflows/editorjs.yml b/.github/workflows/editorjs.yml index 24a38478..630014a9 100644 --- a/.github/workflows/editorjs.yml +++ b/.github/workflows/editorjs.yml @@ -23,16 +23,18 @@ jobs: secrets: stryker_dashboard_api_key: ${{ secrets.STRYKER_DASHBOARD_API_KEY }} - # Temporarily on every PR, to prove the job green before it moves to a schedule. + # Merge queue only: ~8 minutes driving a real screen reader is too slow for every PR push. + # A skipped job reports success, so this never blocks a pull request. voiceover: name: VoiceOver + if: ${{ github.event_name == 'merge_group' }} # A real screen reader needs a real macOS session; the standard runner is free on a public repo. runs-on: macos-15 # Well above the ~8 minute runtime, so a wedged VoiceOver fails rather than hangs. timeout-minutes: 60 - # Only five macOS jobs run at once account-wide, so a superseded push must not hold one. + # Only five macOS jobs run at once account-wide, so a superseded entry must not hold one. concurrency: - group: voiceover-${{ github.event.pull_request.number || github.ref }} + group: voiceover-${{ github.ref }} cancel-in-progress: true steps: - uses: actions/checkout@v6