Repository navigation
[3314 by Claude Opus 5.5] Keep task Follow Up focus contained and restore its invoker on close - #3315
Conversation
![propr-dev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…ith Escape, Cancel, Close, the result's Close or a successful post puts focus back on the button that opened it, as long as that button is still on the page. Only `propr-ui/src/components/TaskDetails/FollowupModal.tsx` changed; #3313's To-Do code is untouched. I didn't test on a real Android device or emulator; touch was covered by Playwright's mobile emulation in Chromium. What changed - Remembering the opener: I removed `autoFocus` from the Comment textarea. The dialog now notes which element had focus, then moves focus to Comment itself, so the opener is recorded before focus moves. - Restoring focus: on close, the dialog lifts the inert state from the workspace first, then focuses the opener. It skips this if the opener is gone, or if focus has already moved to something else outside the dialog, so it won't steal focus after an unrelated change. - Tab handling: Tab and Shift+Tab move by hand through the controls that are currently visible and enabled. This works when focus is on the result message or on a button that a submission just disabled. While posting, when every control is disabled, focus stays on the dialog itself. - Window controls: Tab is left alone when focus is on the desktop window controls, so they keep their normal behaviour. - Unchanged: Escape still doesn't close the dialog while a post is in progress. Prefill, destination, duplicate-post guard, submission lock, failure receipts and result focus/scroll all behave as before. Tests - `FollowupModal.focus.test.tsx` (new, 9 tests): the forward and backward Tab loops, focus moving back in from outside, and each way of closing. It also covers a fake successful post, Tab from the failure receipt (skipping the blocked Post Comment), Tab while posting, an opener that has left the page, and not taking focus back from another control. - `e2e/followup-focus.pw.ts` (new, Playwright): opens Follow Up from the keyboard at desktop 1280×820 and on 390 and 320 touch layouts. It runs both Tab loops, every way of closing, a stubbed failure receipt and a stubbed successful post, and checks the inert regions are back to how they were. A Linux desktop case checks Tab still moves between the window controls normally. All post responses are stubbed in the test, so nothing reaches GitHub. - Fails on the old component: with the unchanged `FollowupModal.tsx`, 7 of the 9 new unit tests failed and all 7 Playwright cases failed (focus left the dialog on Tab, or was on BODY after closing). - With the fix: - The new Playwright spec plus the existing `dialog-keyboard-viewport` (Follow Up keyboard band) and `task-dialog-native-chrome` specs: 30 passed. - Vitest over `TaskDetails`, `hooks` and `utils`: 689 passed. - `tsc --noEmit` is clean, and eslint with zero warnings is clean on the changed files. When typing into the prefilled comment, the cursor starts at the beginning of the text. The old `autoFocus` focused the field the same way, so I left that as it was. I also captured four synthetic screenshots for the previews: Shift+Tab wrapping and Escape returning focus on desktop, and Tab from the failure receipt and Close returning focus at 390px.
|
/review codex-astra |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex-astraOverall EvaluationThe PR correctly contains keyboard focus within Follow Up and restores the connected opener on dismissal. It is merge-ready within the reviewed scope, conditional on the remaining checks passing. ✅ Stable focus lifecycle — Captures the opener before focusing Comment, restores background interactivity before returning focus, and avoids taking focus from another connected control. ✅ Dynamic keyboard containment — Handles forward and backward navigation, disabled submission controls, focused feedback, and the no-enabled-controls fallback while preserving native window-control navigation. ✅ Focused regression coverage — Adds component and browser tests for dismissal, successful submission, receipt feedback, mobile action-sheet handoff, and window chrome. This was a static review of the supplied code; no commands or tests were run. The issue description is truncated, so requirements beyond the visible text could not be assessed. The supplied current checks show no failures and four pending checks. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S1: 🟢 Verify real Android keyboard behaviorRepeat the focus and dismissal scenarios on an Android device or emulator with its OS keyboard. The browser tests cover narrow touch layouts and keyboard navigation, but do not establish real virtual-keyboard behavior—the environment that originally exposed the issue. This is useful follow-up validation, not a demonstrated defect or merge blocker. ScoreThe implementation addresses the visible objective with localized changes and substantial regression coverage. No introduced correctness defect was identified; remaining checks are pending. Score: 9/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
Independent exact-head validation found one WebKit test failure at Please address the new regression test's unstable inert-baseline assertion without weakening the actual modal isolation/restoration requirements. In Make the test assess restoration of the workspace regions the modal owns separately from independently changing component state. Also assert the compact header's own inert property agrees with its current aria-hidden/visibility contract after dismissal. Retain strong coverage for a pre-existing inert region (a controlled stable fixture is appropriate), all dismissal paths, keyboard loops, failure receipts, duplicate-post prevention, successful posting, and native window controls. Do not simply drop the inert assertion, force the compact header's state, hardcode timing waits, add retries, or change the product focus code unless inspection reveals an actual product defect. Preserve the original44-case/43+1 evidence; independently retest on WebKit if available and report any environment limit. Keep #3313 To-Do code untouched. Edit/test only; ProPR finalizes. I will review the new exact head, rerun the affected assertions, and request Astra again if needed. Local diagnostic: |
|
✅ Applied the requested follow-up changes in commit a281500 Summary of ChangesI fixed the unstable inert check in How the restore check works now (
The tab loops, receipt and duplicate-post checks, successful-post body and window-control steps are as before. I added no waits, retries or forced header state. Checking that the new assertions catch real breakage (I made each break temporarily, then put the code back):
Test runs
WebKit limit: WebKit here is Playwright's headless WPE build on Debian 12, with its system libraries extracted without root. In it, the old spec at Your diagnostic There's nothing visual to show, so I made no previews. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…ts`; it was the only file I changed. The modal's restore code was not the cause: the compact header's state comes from an IntersectionObserver that runs separately from the modal. So I made no product changes, and #3313's To-Do code is untouched. I fixed the unstable inert check in `e2e/followup-focus.pw.ts`; it was the only file I changed. The modal's restore code was not the cause: the compact header's state comes from an IntersectionObserver that runs separately from the modal. So I made no product changes, and #3313's To-Do code is untouched. **How the restore check works now** (`expectRestored`, called after Escape, Cancel, Close, the receipt's Close, a successful post and the Linux window-controls case): - **Regions the modal owns:** once the dialog is open, the test tags the outermost regions that were made inert. It requires the task workspace to be inside one of them, so isolation itself is still checked. After dismissal, none of the tagged regions may still be inert. - **A region that was inert before:** a fixed fixture, an inert `<div>` with a button, is added next to the app root before the baseline is taken. Every dismissal must leave it inert. The modal's isolation code skips past it, so a restore that cleared too much would be caught. - **Nothing else left inert:** all inert regions except the compact header must exactly match the baseline. Apart from the compact header, the only inert region in the baseline was the fixture, on every layout. - **Compact header:** its `inert`, `aria-hidden="true"` and `invisible` class must agree with each other after dismissal. Whether it is shown or hidden is not asserted, so its scroll-driven state can change freely. The tab loops, receipt and duplicate-post checks, successful-post body and window-control steps are as before. I added no waits, retries or forced header state. **Checking that the new assertions catch real breakage** (I made each break temporarily, then put the code back): | Temporary change | Result | |---|---| | Restore makes every inert element live again | 7/7 fail: fixture `inert` expected true, got false | | Restore leaves one region inert | 7/7 fail: tagged inert regions expected 0, got 1 | | No restore at all | 7/7 fail on the opener's focus | | Header switched to fully visible after dismissal (what the WebKit trace showed) | 7/7 pass | | Header `inert` removed while still `aria-hidden`/invisible | fails on both touch layouts. On desktop this injection didn't change anything, because Chromium already showed the header there. | **Test runs** - `followup-focus.pw.ts`: 7/7 on Chromium and 7/7 on WebKit, plus 35/35 on WebKit with `--repeat-each 5`. - `followup-focus`, `dialog-keyboard-viewport` and `task-dialog-native-chrome` together: 30/30 on Chromium and 30/30 on WebKit. That is 30 cases, not your 44; I don't know which other cases make up the rest of your set. - ESLint (`--max-warnings 0`) and `tsc --noEmit` (which covers `e2e/`) pass. **WebKit limit:** WebKit here is Playwright's headless WPE build on Debian 12, with its system libraries extracted without root. In it, the old spec at `6f7eb29` also passed 35/35 with `--repeat-each 5`, so I couldn't reproduce the failure itself. The fix is backed by the injected header flip above, not a reproduction. Your diagnostic `/tmp/propr-pr3315-6f7eb29-webkit-inert-baseline-diagnostic/evidence.json` isn't in this container, so I worked from the details in your comment; your original report and traces are untouched. There's nothing visual to show, so I made no previews. PR: #3315 Comment by: @integry (ID: 6318894719) Model: Claude Opus 5.5
|
/review codex-astra !gitfix Review exact head a281500 for issue #3314. This is a read-only independent review; implementation remains Claude via ProPR. Check the full focus contract: capture the real connected invoker before Comment focus moves; contain forward/reverse Tab through currently enabled visible controls and dynamic receipt feedback; keep focus inside while all controls are disabled; return focus after Escape, Cancel, Close, receipt Close and successful submission, while avoiding disconnected invokers or stealing another control's focus. Preserve workspace isolation, native window-control behavior, existing prefill, API-resolved destination, duplicate-post guard, submission lock, failure receipts and IME layout. The latest commit changes only followup-focus.pw.ts. Verify that the new isolation checks still catch unreleased modal-owned regions and preserve a controlled pre-existing inert region, while independently checking compact-header inert/aria-hidden/invisible consistency. They must allow the header's legitimate IntersectionObserver/scroll-driven state change. No retries, fixed waits or forced header state were added. Root's original WebKit43pass/1failure trace is retained rather than waived. Independent root validation: current head typecheck/lint/fresh build pass; all14 changed Chromium/WebKit cases pass once without retries/skips; the unchanged seven new tests each fail on focus against the old published5cd UI. Runtime files are byte-identical to6f7eb29, supporting explicitly carried74unchangedbrowser cases/689units and3realAndroidIME typing/draft/dismiss/Cancel-focus journeys. These are operator results, not commands executed by this review. Combined To-Do/FollowUp integration testing is separate and publication remains gated. |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex-astraOverall EvaluationReady to merge within the reviewed scope, conditional on the pending Validate Changes check completing successfully. This is a static review of the supplied diff and context; no commands or tests were run. The issue excerpt is truncated, so unseen requirements could not be assessed. ✅ Stable focus lifecycle — The effect captures the opener before focusing Comment, restores background interactivity before returning focus, and avoids disconnected openers or overriding another control’s focus. ✅ Dynamic Tab containment — Each Tab press recalculates available controls, handles navigation from receipt feedback, and falls back to the dialog when submission disables every control. Native window chrome retains its Tab behavior. ✅ Isolation regression coverage — The revised browser tests check that marked modal-owned regions are released and a controlled pre-existing inert region remains inert. Compact-bar consistency is checked separately, avoiding the previous dependency on its scroll-driven baseline state. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S2: 🟢 Browser-test pending submission focusConsider adding a browser case that holds the submission response pending and exercises both Tab directions while all controls are disabled. The unit test covers forward Tab and Escape during submission, but browser coverage would additionally verify native focus behavior when the active control becomes disabled. This is optional coverage, not a demonstrated defect. ScoreThe implementation is localized, preserves the surrounding submission and layout behavior, and includes substantial focused regression coverage. No merge blockers were identified; the supplied current-head status reports no failed checks and one pending check. Score: 9/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex-astraOverall EvaluationMerge-ready within the reviewed scope, conditional on the pending Validate Changes check completing successfully. No verified blockers were found in the supplied diff for head ✅ Focus lifecycle — The opener is captured before Comment receives focus. Cleanup restores background interactivity before restoring focus, checks that the opener remains connected, and avoids overriding another control’s focus. ✅ Dynamic keyboard containment — Each Tab event recalculates eligible controls, handles navigation from receipt feedback, skips disabled submission controls, and focuses the dialog when no controls remain enabled. Native window chrome retains its Tab behavior. ✅ Isolation regression coverage — The revised browser assertions check tagged modal-isolated regions for unreleased inert state, preserve the controlled pre-existing inert region, and separately compare compact-header The changes preserve the existing submission guard, duplicate-post protection, resolved destination, failure receipts, prefill, and viewport layout logic. This was a static review of the supplied code; no commands were executed. Browser, unit, and Android validation results are operator-reported evidence. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S3: 🟢 Browser coverage during pending submissionConsider holding the fixture submission response open and testing both Tab directions while every dialog control is disabled. The new unit test exercises the dialog-focus fallback, but a browser test would additionally cover native focus behavior when the active button becomes disabled. This is optional additional coverage, not a demonstrated defect. ScoreThe implementation addresses the focus contract with localized changes and targeted regression coverage. The supplied current-head status has no failures and one pending check; reported browser validation supports the revised isolation assertions. Score: 9/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
AI Implementation Summary
Closes #3314
Branch:
3314/claude-opus-5-5-keep-task-follow-up-focus-20261021-1416-tsfCommits: ✅ Changes committed (6f7eb29)
AI Processing Completed
Execution Details:
Summary:
Follow Up now keeps Tab and Shift+Tab inside the dialog. Closing it with Escape, Cancel, Close, the result's Close or a successful post puts focus back on the button that opened it, as long as that button is still on the page. Only
propr-ui/src/components/TaskDetails/FollowupModal.tsxchanged; #3313's To-Do code is untouched. I didn't test on a real Android device or emulator; touch was covered by Playwright's mobile emulation in Chromium.What changed
autoFocusfrom the Comment textarea. The dialog now notes which element had focus, then moves focus to Comment itself, so the opener is recorded before focus moves.Tests
FollowupModal.focus.test.tsx(new, 9 tests): the forward and backward Tab loops, focus moving back in from outside, and each way of closing. It also covers a fake successful post, Tab from the failure receipt (skipping the blocked Post Comment), Tab while posting, an opener that has left the page, and not taking focus back from another control.e2e/followup-focus.pw.ts(new, Playwright): opens Follow Up from the keyboard at desktop 1280×820 and on 390 and 320 touch layouts. It runs both Tab loops, every way of closing, a stubbed failure receipt and a stubbed successful post, and checks the inert regions are back to how they were. A Linux desktop case checks Tab still moves between the window controls normally. All post responses are stubbed in the test, so nothing reaches GitHub.FollowupModal.tsx, 7 of the 9 new unit tests failed and all 7 Playwright cases failed (focus left the dialog on Tab, or was on BODY after closing).dialog-keyboard-viewport(Follow Up keyboard band) andtask-dialog-native-chromespecs: 30 passed.TaskDetails,hooksandutils: 689 passed.tsc --noEmitis clean, and eslint with zero warnings is clean on the changed files.When typing into the prefilled comment, the cursor starts at the beginning of the text. The old
autoFocusfocused the field the same way, so I left that as it was.I also captured four synthetic screenshots for the previews: Shift+Tab wrapping and Escape returning focus on desktop, and Tab from the failure receipt and Close returning focus at 390px.
Detailed Logs:
de219950-71af-48b7-b210-462f0d00aa99Log files stored at:
/usr/src/app/logs/executions/issue-3314-2026-10-21T14-31-02-416Z-conversation.json/usr/src/app/logs/executions/issue-3314-2026-10-21T14-31-02-416Z-output.txtLatest Conversation Messages
This PR was created automatically by ProPR after processing issue #3314.
💡 Need changes?
Comment on this PR to request refinements — the AI agent monitors comments and will update the implementation based on your feedback. Keep iterating until you're satisfied!
Visual preview
390px touch: Close returns focus to Follow Up
Activating Close from the keyboard closes the dialog and focus returns to the Follow Up button.
Original unavailable: managed storage is unavailable or the upload has expired.
390px touch: Tab from the result reaches its Close
Stubbed posted-but-not-submitted receipt (no real write). Tab from the focused result moves to Close; the duplicate-blocked Post Comment is skipped.
Original unavailable: managed storage is unavailable or the upload has expired.
Desktop: Escape returns focus to Follow Up
After Escape the dialog is gone and keyboard focus is back on the Follow Up button that opened it.
Original unavailable: managed storage is unavailable or the upload has expired.
Desktop: Shift+Tab wraps inside Follow Up
1280x820, synthetic task. Shift+Tab from Comment goes to Close and then wraps to Post Comment instead of leaving the dialog.
Original unavailable: managed storage is unavailable or the upload has expired.