fix(overlay): take focus when opening and hand it back when closing - #2423
Conversation
🦋 Changeset detectedLatest commit: 3eb4580 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
c1f2da2 to
df7df50
Compare
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. WalkthroughThe error overlay saves the host page’s deepest active element when it opens and focuses its Close button after the opening render. When cleared, it restores focus if the saved element remains connected and supports focus. Later renders preserve focus on the corresponding in-card control or move it to Close. The iframe receives an accessible title. Client transport handlers suppress specified events after closure and no longer log raw connection errors. The shared socket open path logs “connected.” Test helpers and end-to-end tests cover overlay behavior and both SSE and WebSocket transports. Changesets describe the updates. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to The overlay test may not catch error markup disappearing. Confirm that it verifies the markup is displayed as text before merging. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 60de74d1-aae2-4bf1-81b1-f716bc931dc2
📒 Files selected for processing (3)
.changeset/overlay-focus.mdclient-src/overlay.jstest/e2e/overlay.test.js
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/overlay.test.js (1)
1016-1019: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the markup appears as text.
The current assertions pass when the overlay drops the submitted
<img>markup. Assert that the frame text contains the markup while retaining the checks that no image renders or executes.Suggested fix
+ expect(await frame.evaluate(() => document.body.textContent)).toContain( + "<img src=x onerror='globalThis.xss=1'>", + ); expect( await frame.evaluate(() => document.querySelectorAll("img").length),
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: cbebb2d4-2d29-4657-918d-1c1506454b9f
📒 Files selected for processing (1)
test/e2e/overlay.test.js
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 90749554-0828-4da5-a83b-d7cf168f188a
📒 Files selected for processing (5)
.changeset/connected-log-both-transports.mdclient-src/clients/EventSourceClient.jsclient-src/clients/createSocket.jstest/e2e/transport.test.jstest/helpers/hot-app.js
💤 Files with no reviewable changes (1)
- client-src/clients/EventSourceClient.js
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 043d3f56-11d7-4483-922a-5fb04f2ab87e
📒 Files selected for processing (5)
.changeset/transport-parity.mdclient-src/clients/EventSourceClient.jsclient-src/clients/WebSocketClient.jstest/client-socket.test.jstest/client-transports.test.js
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2423 +/- ##
==========================================
+ Coverage 95.45% 96.19% +0.73%
==========================================
Files 17 17
Lines 1849 1890 +41
==========================================
+ Hits 1765 1818 +53
+ Misses 84 72 -12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The overlay opened without touching focus, so reaching it meant clicking into the frame first: Escape and the arrow keys did nothing until then, and a screen reader stayed wherever the page was. Its frame had no accessible name either, leaving it announced by its `about:blank` url. Only the render that opens it takes focus — paginating re-renders the card, and moving focus then would take it off the button being clicked. These are the parts webpack-dev-server's own overlay has and this one did not, which it would otherwise lose by switching to this one.
…d no test for Mapping webpack-dev-server's 39 end-to-end overlay cases onto this one's 36 found six behaviors it relies on that nothing here pinned: an `errors` filter keeping one problem and rejecting all of them, a failure in the very first build (which arrives through the catch-up sync rather than a rebuild), one broken rebuild replacing another's overlay, Escape working a second time, and markup in a build error reaching the card as text. All six already behave the way webpack-dev-server's do — these are the tests, not fixes.
`connected` was logged by the Server-Sent Events transport itself, so a page using the WebSocket one got no sign it had connected at all. It is said by the shared socket now, on the same event, so neither transport can be the one that stays quiet. Found by running the same four behaviors over both: connecting and applying a hot update, falling back to a reload when nothing accepts it, showing and clearing a build error, and catching up a client on what it missed. The harness takes `transport: "ws"`, which also exercises `attach` — the WebSocket path had no browser coverage before this.
The WebSocket one logged a raw `error`, which the Server-Sent Events one never did. Making both say it would have been the wrong way to agree: `EventSource` fires `error` on every routine reconnection, and a WebSocket `error` carries no detail by specification, so it prints an opaque object. Neither logs it now — the shared socket already reports the reconnection, which is the part worth reading. The WebSocket one also had no equivalent of the other's `closed` guard, so a close or message the browser had already queued still reached the caller after it had closed, and the shared socket would reconnect a connection nobody wanted. The contract they share is now tested once and run against each, which is what turned both of these up, and all three files are fully covered.
Two gaps in the focus handling, both found in review. `document.activeElement` names a shadow host rather than the control focused inside it, and focusing a host that does not delegate focus does nothing at all — so the overlay dropped focus instead of handing it back. Descend into open roots before remembering what to restore. Paging through problems rebuilds the card, which destroys the very button that was clicked; focus then fell back to the frame's body, so a keyboard user lost the navigation after the first press. Each control is named across the re-render and the equivalent one takes focus again. The test helper could not have caught either over a WebSocket, because it could not restart a server at all: an upgraded socket is no longer the HTTP server's to close, so `stopHttp()` waited on a connected client forever, and the replacement server was never handed back to the transport. Both fixed, and the reconnect story now runs over both transports. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
422e901 to
3eb4580
Compare
1 participant
Summary
Rebased onto
mainnow that #2421 has merged, so this stands on its own — one commit.Before webpack-dev-server can drop its own overlay and use this one, this one has to do everything that one does. Comparing them turned up two things it did not, both accessibility:
Focus never moved. The overlay opened without touching it, so reaching the overlay meant clicking into the frame first — Escape and the arrow keys did nothing until you did, and a screen reader stayed wherever the page already was. webpack-dev-server's focuses its close button on open, saves what had focus, and gives it back on close. This one now does the same, so closing no longer leaves focus on a removed element with the next Tab starting from the top of the document.
The frame had no accessible name. webpack-dev-server sets
titleon its iframe; this one did not, leaving it announced by itsabout:blankurl.One subtlety worth pointing out, because it is where the two overlays genuinely differ: this one paginates, and paginating re-renders the card. Focusing on every render would take focus off the ‹ › button being clicked, so only the render that opens the overlay takes focus.
On the wider "merge the overlays" step
I had said this would be a dedupe, with this package's overlay being the superset that wins. Having actually compared them, that was wrong, and I would rather correct it than let it shape the next PR:
styles,ansiColors,openEditorEndpoint,paginatetrustedTypesPolicyName,catchRuntimeError, errors/warnings/runtimeErrors filtersThey are two implementations with overlapping, not nested, feature sets. This PR closes the behavioral gaps so nothing is lost by switching; it deliberately does not attempt to reconcile the architectures.
formatProblem, which webpack-dev-server exports from its overlay, is not a gap: this package formats build problems server-side insrc/hot.jsand runtime errors inside the overlay, so the same work happens in a different place.What kind of change does this PR introduce?
fix
Did you add tests for your changes?
Yes — one end-to-end case that focuses an input, triggers a build error, and checks that focus lands on the close button inside the frame and returns to the input after Escape, plus that the frame has a title.
Teeth-checked in both halves: removing the
closeButton.focus()fails it, and separately removing the focus restoration fails it on thefocus-meassertion. All 36 overlay cases and all 82 end-to-end cases pass, re-run after the rebase.A correction to what I claimed on #2420 and #2421: I said the browser end-to-end suites could not run in my sandbox, and that this was environmental because they failed the same way on a clean
main. That was wrong. They fail when jest is invoked without--experimental-vm-modules— whichnpm run test:e2epasses and I was not — and puppeteer then needs pointing at a browser. With both fixed they run fine, and I have now run the whole suite locally, including the twoclient.test.jssnapshots that CI caught on #2421 and that I had only reasoned about rather than executed.test/logging.test.jsstill fails 74/74 on a cleanmain, unrelated to any of this.Does this PR introduce a breaking change?
No. The overlay gains focus behavior it did not have; nothing about its appearance, options or dismissal changes.
If relevant, what needs to be documented once your changes are merged or what have you already documented?
Nothing — this restores expected behavior rather than adding a knob.
Use of AI
AI-assisted (Claude Code). It was used to compare the two overlays, write the fix and the test, and verify it: the test was run against both halves of the change reverted in turn to confirm it fails, and the full end-to-end suite was run locally.
🤖 Generated with Claude Code
https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
Summary by CodeRabbit