🐛 Fixed keyboard focus trapping in empty comment threads - #29724
Conversation
fixes #29708 Kept the form open while focus moves through its controls so keyboard navigation can continue beyond the comments iframe.
WalkthroughThe comments form now tracks whether focus remains within its descendants. Its open state includes internal focus, and blur handling clears that state only when focus leaves the form. The form ref uses Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 21s | View ↗ |
nx run-many -t lint -p @tryghost/comments-ui,gh... |
✅ Succeeded | 7s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 12s | View ↗ |
nx run-many -t test:unit -p @tryghost/comments-ui |
✅ Succeeded | 14s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | 2s | View ↗ |
nx run @tryghost/comments-ui:test:acceptance |
✅ Succeeded | 45s | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-08-03 15:49:24 UTC
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/comments-ui/test/e2e/main-form.test.ts`:
- Around line 58-63: Replace the arbitrary page.waitForTimeout(200) after
focusing the editor in the test flow with a condition-based assertion or wait
that verifies expertiseButton has received focus. Keep the existing editor and
expertiseButton locators unchanged, and preserve the Tab navigation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cfdf1cd2-8768-45cf-8fac-adc34d25f94b
📒 Files selected for processing (2)
apps/comments-ui/src/components/content/forms/main-form.tsxapps/comments-ui/test/e2e/main-form.test.ts
| const editor = frame.getByTestId('editor'); | ||
| const expertiseButton = frame.getByTestId('expertise-button'); | ||
|
|
||
| await editor.focus(); | ||
| await page.keyboard.press('Tab'); | ||
| await page.waitForTimeout(200); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -u
rg -n -C 3 'data-testid=.*(editor|form-editor)' apps/comments-ui
rg -n -C 4 'waitEditorFocused|frame\.getByTestId' apps/comments-ui/test/e2eRepository: TryGhost/Ghost
Length of output: 50371
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf '--- main-form.test.ts relevant slice ---\n'
sed -n '1,90p' apps/comments-ui/test/e2e/main-form.test.ts | cat -n
printf '\n--- e2e utils relevant definitions ---\n'
rg -n -C 5 'waitEditorFocused|getModifierKey|selectText|setClipboard|function|const|export' apps/comments-ui/test/e2e/utils/e2e.ts
printf '\n--- editor.test.ts relevant slice ---\n'
sed -n '1,120p' apps/comments-ui/test/e2e/editor.test.ts | cat -n
printf '\n--- form component relevant slice ---\n'
sed -n '120,155p' apps/comments-ui/src/components/content/forms/form.tsx | cat -nRepository: TryGhost/Ghost
Length of output: 11360
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf '%s\n' '--- locate e2e utils files ---'
fd -t f 'e2e\.(ts|js)$' apps/comments-ui/test/e2e apps/comments-ui | sed -n '1,120p'
printf '%s\n' '--- read candidate e2e utils ---'
for f in $(fd -t f 'e2e\.(ts|js)$' apps/comments-ui/test/e2e apps/comments-ui | sed -n '1,5p'); do
echo "--- $f ---"
wc -l "$f"
sed -n '1,220p' "$f" | cat -n
doneRepository: TryGhost/Ghost
Length of output: 9019
Use condition-based focus synchronization.
frame.getByTestId('editor') can resolve from the existing EditorContent test ID; it is not wrong just because form-editor is visible. The test should still avoid the arbitrary page.waitForTimeout(200) and wait for expertiseButton to become focused instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/comments-ui/test/e2e/main-form.test.ts` around lines 58 - 63, Replace
the arbitrary page.waitForTimeout(200) after focusing the editor in the test
flow with a condition-based assertion or wait that verifies expertiseButton has
received focus. Keep the existing editor and expertiseButton locators unchanged,
and preserve the Tab navigation behavior.
1 participant
Summary
Root cause
The form was considered open only while the editor was focused or contained text. Tabbing from the editor to the dynamically rendered Add your expertise button blurred the editor, which immediately unmounted the focused button. With no comment controls following it, Firefox restarted sequential focus navigation at the beginning of the iframe.
Testing
pnpm exec playwright test test/e2e/main-form.test.ts --project=chromium --workers=1 --reporter=linepnpm --filter @tryghost/comments-ui test:typespnpm --filter @tryghost/comments-ui lint:codeFixes #29708