Main CI run 43260797853 (24d9e47) failed in webkit-accessibility: the preferences page nav and source tabs work from the keyboard and a track without audio finds a playable version from the keyboard. The same commit passed on the release PR, so this is a race, not a regression.
Cause: useModalDialog moves focus to the dialog's initial element in a requestAnimationFrame. The tests focused a control inside the dialog right after it opened; when the frame fired later (more often in WebKit), it stole focus back, so ArrowDown/Enter went to the wrong element.
Fix: wait until focus is inside the dialog before moving it, in the three places that did this. Test-only change, no UI change.
This change affects one file: tests/e2e/accessibility/streaming-sources.spec.ts. It adds 13 lines and removes none. The change is test-only. It does not change UI code.
waitForInitialFocus helper. The helper polls until focus is inside the open dialog.waitForInitialFocus. Only then does the test move focus to the dialog's Import button.Intended effect: keyboard steps run after the dialog's initial focus is set. This prevents a pending focus move from taking focus away from a control that the test already focused.
No exported or public declarations change.
Verification: the diff was not confirmed in this session. The results of the diff inspection were not returned. The CI status of the two reported failing tests is not available.
No actionable comments were generated in the recent review.
The accessibility E2E tests add a helper that waits for focus to enter a dialog. Three tests use it before continuing keyboard interactions.
Changes: Dialog focus checks
| Layer / File(s) | Summary |
|---|---|
Wait for dialog focus tests/e2e/accessibility/streaming-sources.spec.ts | A local helper polls until a dialog contains the active element. The preferences, playable-version, and playlist playable-versions tests wait for dialog focus before continuing. |
Estimated code review effort: 1 (Trivial) | ~5 minutes
Change: Other
Possibly related PRs
Suggested labels: javascript
cee01This is a test-only change that makes keyboard-navigation accessibility tests wait for dialog focus before continuing. It has no production impact, and no merge-blocking risk remains.
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | | The title follows Conventional Commits: it uses the allowed test type with the optional e2e scope and no issue number. It names the main change, which adds a wait for dialog initial focus before k… |
| 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. |
| Ipc Contract Update | | The check passes because the diff does not change a public IPC command, payload, event, or source enum. The changed-file inventory for refs/coderabbit/pre-merge-check/base..head lists a single modifie… |
| No Type Escapes | | The PR changes one TypeScript file, tests/e2e/accessibility/streaming-sources.spec.ts. I read the full patch for that file. The added lines are an import type { Locator }, a new helper `waitForIniti… |
| Acceptance Evidence | | The check passes on its first condition: the PR makes no material behavior change. The changed-file inventory shows one file modified, tests/e2e/accessibility/streaming-sources.spec.ts. The patch ad… |
| Do Not Edit Changelog | | The changed-file inventory for the reviewed range (refs/coderabbit/pre-merge-check/base to head) lists one modified file: tests/e2e/accessibility/streaming-sources.spec.ts. The diff for CHANGELOG.md i… |
Explanation
The title follows Conventional Commits: it uses the allowed test type with the optional e2e scope and no issue number. It names the main change, which adds a wait for dialog initial focus before keyboard steps in the accessibility E2E test.
Explanation
The check passes because the diff does not change a public IPC command, payload, event, or source enum. The changed-file inventory for refs/coderabbit/pre-merge-check/base..head lists a single modified file, tests/e2e/accessibility/streaming-sources.spec.ts (+13/-0). The change adds a Playwright helper, waitForInitialFocus, which polls until the dialog contains document.activeElement. It calls that helper in three places before keyboard steps. This is test-only code. No IPC surface is touched, so no docs/references/contracts/ update is required. The contracts directory exists and was not changed, which is consistent with the check.
Explanation
The PR changes one TypeScript file, tests/e2e/accessibility/streaming-sources.spec.ts. I read the full patch for that file. The added lines are an import type { Locator }, a new helper waitForInitialFocus(dialog: Locator) that polls dialog.evaluate((el) => el.contains(document.activeElement)), and three await waitForInitialFocus(dialog); calls. None of the added lines contains as any, [@ts-ignore](/ts-ignore), or [@ts-expect-error](/ts-expect-error). The helper uses the typed Locator parameter, not any. My follow-up grep failed because of a flag error in my own command, so this verdict rests on the direct diff read.
Explanation
The check passes on its first condition: the PR makes no material behavior change. The changed-file inventory shows one file modified, tests/e2e/accessibility/streaming-sources.spec.ts. The patch adds a test-only helper, waitForInitialFocus, which polls until the dialog contains document.activeElement. It calls that helper in three places before keyboard steps. No production code changes, which matches the PR description and the summary. The check's second condition also has supporting evidence. The PR description names the failing CI run (43260797853) and the two failing tests. It also cites the root cause, a requestAnimationFrame focus race in useModalDialog. The PR has no separate acceptance criteria list. The changed test file names the affected tests. Because the check passes on the no-material-change condition, the missing criteria list does not change the result.
Explanation
The changed-file inventory for the reviewed range (refs/coderabbit/pre-merge-check/base to head) lists one modified file: tests/e2e/accessibility/streaming-sources.spec.ts. The diff for CHANGELOG.md is empty, so CHANGELOG.md is unchanged. The check passes because it only fails when CHANGELOG.md is hand-edited.
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.
84f120fc-771f-4bb2-99ab-fcb78acc4338
Reviewing files that changed from the base of the PR and between 24d9e47642fad5acecb6eba303d4fd4449a49d5d and cee01ed2e78fc80e50dda3f1d9a179511bccedff.
tests/e2e/accessibility/streaming-sources.spec.tsIncluded review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
Status: No Issues Found | Recommendation: Merge
tests/e2e/accessibility/streaming-sources.spec.tsReviewed by free · Input: 109.4K · Output: 15.3K · Cached: 690.4K
Sign in to comment.