fix(overlay): take focus when opening and hand it back when closing - #2423
alexander-akait wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: df7df50 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 |
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.
c1f2da2 to
df7df50
Compare
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. WalkthroughThe error overlay now saves the host page’s 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. The iframe receives an accessible title. An end-to-end test covers focus capture and restoration, and a changeset declares a patch release. Merge Risk: 🟡 Moderate · up to Keyboard focus can be lost while navigating build problems, and focus may not return to a control inside a web component when the overlay closes. Fix these focus paths 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.
|
|
||
| // Whatever the page had focused, so it can be given back — the overlay | ||
| // takes focus to be reachable by keyboard, and is rude if it keeps it. | ||
| state.previousActiveElement = document.activeElement; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '435,520p' client-src/overlay.js
sed -n '175,235p' test/e2e/overlay.test.jsRepository: webpack/webpack-dev-middleware
Length of output: 5181
🏁 Script executed:
set -eu
printf '%s\n' '--- overlay focus-related definitions and callers ---'
rg -n -C 8 'previousActiveElement|activeElement|focusOnRender|function clear|function ensureOverlay|delegatesFocus|shadowRoot' client-src/overlay.js test
printf '%s\n' '--- overlay file outline ---'
ast-grep outline client-src/overlay.js
printf '%s\n' '--- focused test file outline ---'
ast-grep outline test/e2e/overlay.test.jsRepository: webpack/webpack-dev-middleware
Length of output: 14363
Preserve the focused control inside an open shadow root.
When an input inside an open shadow root has focus, document.activeElement returns the shadow host. clear() later calls focus() on that host. Without delegatesFocus, this does not restore focus to the input. Traverse shadowRoot.activeElement before saving the element, including nested open shadow roots.
Suggested fix
- state.previousActiveElement = document.activeElement;
+ let previousActiveElement = document.activeElement;
+ while (previousActiveElement?.shadowRoot?.activeElement) {
+ previousActiveElement = previousActiveElement.shadowRoot.activeElement;
+ }
+ state.previousActiveElement = previousActiveElement;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| state.previousActiveElement = document.activeElement; | |
| let previousActiveElement = document.activeElement; | |
| while (previousActiveElement?.shadowRoot?.activeElement) { | |
| previousActiveElement = previousActiveElement.shadowRoot.activeElement; | |
| } | |
| state.previousActiveElement = previousActiveElement; |
| if (state.focusOnRender) { | ||
| state.focusOnRender = false; | ||
| closeButton.focus(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
printf '%s\n' '--- overlay focus/render paths ---'
sed -n '360,425p;480,525p;590,715p;735,805p' client-src/overlay.js
printf '%s\n' '--- relevant tests ---'
sed -n '180,240p;590,675p;1065,1130p' test/e2e/overlay.test.jsRepository: webpack/webpack-dev-middleware
Length of output: 16961
Restore the focused control from the iframe document after rebuilding the card.
renderProblems() removes the focused control from frameDocument. Restore the matching pagination button after goToPage(). If a changed problem set removes the focused Close button, focus the new Close button. If pagination disappears, use Close as the in-card fallback. Do not restore focus when frameDocument.activeElement is not inside card, because the host page or another iframe owns focus.
Suggested fix
const { type, lines } = state.currentProblems;
const paginated = paginate && lines.length > 1;
+ const previousFocusedElement = frameDocument.activeElement;
+ const previousFocusWasInCard =
+ previousFocusedElement !== null &&
+ card.contains(previousFocusedElement);
+ const previousFocusLabel = previousFocusWasInCard
+ ? previousFocusedElement.getAttribute("aria-label")
+ : null;
// Accent the top bar with the problem color (red for errors, yellow for warnings).
card.style.borderTopColor = `#${problemColor(type)}`;
setHTML(card, ""); if (state.focusOnRender) {
state.focusOnRender = false;
closeButton.focus();
+ } else if (previousFocusWasInCard) {
+ const replacement =
+ previousFocusLabel === "Close"
+ ? closeButton
+ : Array.from(card.querySelectorAll("button")).find(
+ (button) =>
+ button.getAttribute("aria-label") === previousFocusLabel,
+ ) || closeButton;
+
+ replacement.focus();
}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