feat: let overlay actions hand focus off to the surface they open - #1327
Open
tenphi wants to merge 7 commits into
Open
feat: let overlay actions hand focus off to the surface they open#1327tenphi wants to merge 7 commits into
tenphi wants to merge 7 commits into
Conversation
MenuTrigger and DialogTrigger restored focus to their trigger unconditionally on close, so an item or button whose action opened a panel/dialog/inline editor lost focus to the trigger a tick later. Consumers had to out-race the overlay by re-focusing on every animation frame over a several-hundred-millisecond window. Both now follow the rule react-aria's own FocusScope already uses: take focus back only when it is still inside the closing overlay or has been lost to <body>. Anything else means an action claimed focus, so leave it alone. A single focus() from the opened surface holds. Also adds shouldRestoreFocus (default true) for a surface that claims focus later than the restore, and removes the Tabs rename refocus pass that only existed to survive this. Closes CUB-3962 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tenphi
had a problem deploying
to
Chromatic staging
August 17, 2026 17:12 — with 
GitHub Actions
Failure
GitHub Actions
Failure
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: b36518c 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 |
…let-an-action-hand-focus-off-to-the-surface-it-opens
tenphi
had a problem deploying
to
Chromatic staging
August 17, 2026 17:32 — with
GitHub Actions
Failure
GitHub Actions
Failure
Contributor
📦 NPM canary releaseDeployed canary version 0.0.0-canary-e5b4f54. |
Contributor
🧪 Storybook is successfully deployed!
|
The size job aborted whenever `gh run download` could not fetch the baseline artifact, so a transient 503 from GitHub's artifact endpoint reported the library as over budget when it had never been measured. Four consecutive red checks on #1327 were this, not the bundle. The baseline only feeds the delta column, and measure-size.js already degrades to absolute sizes with a visible warning when it cannot read one. So retry with linear backoff, then continue without a comparison instead of failing. The target directory is wiped between attempts so a half-extracted archive is never read as a valid baseline, and the Statoscope step now gates on the download having landed rather than on the baseline merely having been resolved. Commenting is tolerated for the same reason: a 503 from the issues API says nothing about the bundle. The `Throw error` gate still decides the job, so a genuine overage stays red (verified: exit 1 with "Size limit has been exceeded"). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s-off-to-the-surface-it-opens
tenphi
had a problem deploying
to
Chromatic staging
August 17, 2026 17:52 — with
GitHub Actions
Failure
GitHub Actions
Failure
Contributor
🏋️ Size limit report
Compared against main at f3d9a1f — run 32052281350, 2026-08-17T17:52:39Z.To see which modules changed, download the size-limit-statoscope-report artifact from this run and open report.html. |
…on-hand-focus-off-to-the-surface-it-opens' into andrew/cub-3962-menu-let-an-action-hand-focus-off-to-the-surface-it-opens
The size comment linked to `steps.deploy_report.outputs.preview-url`. That step deployed the report to Netlify and was removed in #108, but the link was left behind, so every PR comment since has carried a dead link to a bare `/report.html`. The report was unreachable regardless: it was written into REPORT_FOLDER *after* that folder had already been uploaded, so it never entered an artifact and was discarded with the runner. Every PR with a baseline has been spending time generating a file nobody could open. It now builds into its own directory and uploads as its own artifact, which keeps the baseline artifact — downloaded by every later run — small. ~180 MB on the runner compresses to ~5 MB stored, with 14-day retention since nothing later depends on it. The comment points at that artifact, and says why there is no report when the baseline is missing. Also documents the `place` and `scrollMargin` style props on MenuTrigger, SubMenuTrigger and DialogTrigger, which `pnpm audit-docs` was flagging on all three. 55 and 67 other docs files respectively are still missing them; those are left for a separate sweep. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tenphi
had a problem deploying
to
Chromatic staging
August 17, 2026 18:17 — with
GitHub Actions
Failure
GitHub Actions
Failure
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ed2c0c9. Configure here.
| with: | ||
| name: size-limit-statoscope-report | ||
| path: ./__report/report.html | ||
| retention-days: 14 |
Contributor
There was a problem hiding this comment.
Statoscope upload can fail size check
Medium Severity
The new Upload Statoscope report step has no continue-on-error, so a transient artifact upload failure (or timeout on the large inlined HTML) fails the whole size-limit job even when the bundle is within budget. That contradicts the nearby stance that the module report is not the check — the same class of false red tick the baseline-download retry was added to avoid.
Reviewed by Cursor Bugbot for commit ed2c0c9. Configure here.
Review catch: the new `Upload Statoscope report` step could fail the size-limit job on a transient artifact failure, which is the same false red tick the baseline retry was added to prevent. Uploading ~180 MB is the step most likely to time out, so it was the worst place to leave untolerated. `Build Statoscope Report` had the same exposure, and so did `Resolve baseline run` — several unguarded GitHub API calls that would fail the job in front of the download retry and defeat the point of it. All three are now tolerated. Losing baseline resolution leaves `found` unset, which skips the download and reports absolute sizes with a "No baseline found" note; losing the report leaves `built` unset, which skips the upload and says so in the comment. The job's verdict now rests on the build and size-limit's own measurement, which is what it is supposed to check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tenphi
had a problem deploying
to
Chromatic staging
August 17, 2026 18:27 — with
GitHub Actions
Failure
GitHub Actions
Failure
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe changes
MenuTriggerandDialogTriggerrestored focus to their trigger unconditionally on close. An item or button whose action opened a panel, a dialog or an inline editor lost focus to the trigger a tick later, so consumers had to out-race the overlay by re-focusing on every animation frame over a several-hundred-millisecond window — the shape the Cloud review ruleset callsui-kit-workaround-in-app-code.Reproduced before fixing: a menu item whose action mounts a panel that focuses itself in a mount effect has focus back on the trigger within 50ms and never regains it.
Only the manual restore was at fault. The popover's
<FocusScope restoreFocus>is innocent — react-aria already guards it (restores only when focus is still inside the scope or has fallen to<body>, and re-checksactiveElement === bodyinside arequestAnimationFrame). The fix applies that same rule to our own restore: the trigger takes focus back only when focus is still inside the closing overlay (the pressed control holds it through the ~350ms exit animation) or has been lost to<body>. Anything else means an action claimed focus, so it is left alone.Behaviour is unchanged when an action moves focus nowhere. A clicked
data-popover-dismisscontrol outside the overlay also keeps focus now instead of having it yanked to the trigger — a latent sibling case of the same bug.New
shouldRestoreFocusprop (defaulttrue) on both triggers, for a surface that claims focus later than the restore (async load, entry animation) where the trigger would otherwise take focus first and flash.Workaround deleted:
Tabs'scheduleRenameRefocus— 58 lines re-focusing the rename input on rAF and again at 50/200/400ms purely to survive the closing menu. The input's ownFocusScope autoFocusis enough now.Notes for the reviewer
DialogTrigger's manual restore is dead code fortype="popover".PressRespondergetsref={ref}and then{...triggerProps}, which carries its ownrefand overrides it — soref.currentstays null. I traced who actually re-focuses the trigger: popovers get it fromDialog'sFocusScope, modals fromDialogTriggerBase. Only modal/tray/fullscreen/panel had the bug. Noted in a comment so nobody revives the ref without the guard.shouldRestoreFocushad to reachDialog'sFocusScope, or it would silently do nothing for popovers. It travels throughDialogContextwith a?? truefallback, so aDialogrendered outside a trigger keeps restoring focus.useDialogfilters context throughfilterDOMProps, so the extra key cannot leak onto the DOM (verified in the react-aria source). Stashing that one change fails the opt-out test for both types, so it is load-bearing and tested.The race is order-dependent, which is why this reads as intermittent in an app: whether the opened surface's mount effect runs before or after the restore depends on tree order, and it flips between runs. My first
DialogTriggertest batch only caught the bug in one of four configurations per run, so I added a deterministic case — the action closes the dialog and synchronously focuses an existing element inside its own handler, so focus is provably outside before the restore effect runs. Verified by stashing the fix: that one fails every run.InlineInput's grace period is kept, and this is the one judgement call worth a second opinion. Its comment blamed the closingMenu, but removing it breaksInlineInput's own unit test (no Menu involved), and theTabsfailure it prevents arrives through a dummyMenuTriggerwhere the restore never ran. It is host-agnostic defence for the imperativestartEditing()path. I rewrote its comments (plus two test comments and theInlineInputdocs paragraph) to say what it actually guards instead of naming Menu.Verification gap, stated plainly. I verified the Menu fix in a real browser (
ActionOpensPanelstory: panel opens, holds focus, menu closes). I could not do the same for theTabsrename flow — the automation pane runs withdocument.hidden, which throttles timers and stopsrequestAnimationFrame, so the 500ms grace window elapses. I A/B'd it with the workaround restored under identical conditions and got identical results, so the removal changes nothing there, but the browser evidence is "no difference" rather than "confirmed working". The jsdom tests are what cover it. Worth one manual click-through of tab rename in Storybook.Checklist
Closes: CUB-3962
Other information
New tests: 3 for
MenuTrigger(hand-off holds; restore still works; opt-out never restores), 10 forDialogTriggeracrosspopoverandmodal(the trigger had no focus-restore coverage before this), and anActionOpensPanelstory with aplayfunction.pnpm sizeat 498.78 kB against the 501 kB limit — tight, but within.A marker comment is on the Cloud PR that carries the polling workaround, naming the exact code to delete: cubedevinc/cubejs-enterprise#13967 (comment 5317241822).
Note
Medium Risk
Changes overlay focus restoration used across menus, dialogs, and tab rename—accessibility-sensitive—but behavior is narrowly scoped with extensive tests and a backward-compatible default.
Overview
Focus hand-off (CUB-3962):
MenuTriggerandDialogTriggerno longer unconditionally re-focus their trigger on close. Restore runs only when focus is still inside the closing overlay or was lost tobody/detached nodes; if an action already focused a panel, dialog, or inline editor, that focus is left alone. Both triggers gainshouldRestoreFocus(defaulttrue), wired throughMenuTrigger's popoverFocusScopeand, for dialogs,DialogContext→Dialog'sFocusScopeso popovers honor opt-out too. Modal/tray/fullscreen/panel types use the same guard inDialogTriggerBase's manual restore.Tabs: Removes
scheduleRenameRefocus(~58 lines of rAF/timeout refocus); rename from the tab menu relies onInlineInputautoFocusplus the new menu behavior.InlineInputgrace-period logic is unchanged; comments/docs now describe host-agnostic blur defense rather than blaming the closing menu.CI (
size-limit.yml): Baseline resolve/download tolerates API/503 failures with retries andcontinue-on-error; Statoscope report is built/uploaded separately from the baseline artifact; PR comment links to the Statoscope artifact instead of a dead Netlify URL.Adds unit tests, an
ActionOpensPanelstory, and documentation for focus restoration.Reviewed by Cursor Bugbot for commit b36518c. Bugbot is set up for automated code reviews on this repo. Configure here.