⚠️ Add middleware/focus props and reference registration to Floating - #3182
Draft
jandrade wants to merge 3 commits into
Draft
⚠️ Add middleware/focus props and reference registration to Floating#3182jandrade wants to merge 3 commits into
jandrade wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: fa574b4 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 |
jandrade
commented
Aug 25, 2026
| import {Arrow, type ArrowStyles} from "./floating-arrow"; | ||
| import {Portal} from "./floating-portal"; | ||
| import {rtlMirror} from "../util/rtl-mirror-middleware"; | ||
| import {acceptsRef} from "../util/accepts-ref"; |
Member
Author
There was a problem hiding this comment.
Floating.
The reference ref is now injected conditionally (only when the trigger can receive one), and five new props are threaded into the floating-ui middleware and FloatingFocusManager config.
Worth scrutinising: a trigger that neither accepts a ref nor calls useFloatingReference now fails silently — no reference element resolves, so open && elements.reference stays false and the floating element simply never renders. Previously React logged a "Function components cannot be given refs" warning in that situation. I left out a dev-mode warning because the check is racy on the first commit (the ref callback and the context registration can both land after the first render); happy to add a deferred check in an effect if you'd prefer the louder failure.
jandrade
commented
Aug 25, 2026
| @@ -0,0 +1,48 @@ | |||
| import * as React from "react"; | |||
Member
Author
There was a problem hiding this comment.
useFloatingReference / FloatingReferenceContext).
The key line to check is where the provider is placed in floating.tsx: it wraps only the trigger, never content. That's what keeps multiple simultaneously open — and nested — floating elements from claiming each other's reference element, since a trigger rendered inside an outer floating element resolves to its own nearest provider.
There are no ids and no document-wide queries involved, so there is nothing to collide across instances or across React roots. Covered by tests for the multi-instance, nested, and "content can't see the setter" cases in floating.test.tsx.
jandrade
commented
Aug 25, 2026
| @@ -0,0 +1,31 @@ | |||
| import * as React from "react"; | |||
Member
Author
There was a problem hiding this comment.
acceptsRef decides whether a ref can be injected by checking $$typeof against Symbol.for("react.forward_ref") / Symbol.for("react.memo") and prototype.isReactComponent. These symbols are stable across React versions, but it is internals-adjacent, so worth confirming the covered cases are the ones we care about.
Unit tests cover host elements, forwardRef, memo(forwardRef(...)), class components, plain function components, and memo(function).
Contributor
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (b8317d2) and published all packages with changesets to npm. You can install the packages in ./dev/tools/deploy_wonder_blocks.js --tag="PR3182"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR3182 |
Contributor
|
Size Change: +321 B (+0.24%) Total Size: 135 kB 📦 View Changed
ℹ️ View Unchanged
|
Contributor
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-acfdgexvsm.chromatic.com/ Chromatic results:
|
jandrade
commented
Aug 26, 2026
| @@ -331,8 +422,6 @@ const styles = StyleSheet.create({ | |||
| background: semanticColor.core.background.base.default, | |||
| border: `solid ${border.width.thin} ${semanticColor.core.border.neutral.subtle}`, | |||
| borderRadius: border.radius.radius_040, | |||
Member
Author
There was a problem hiding this comment.
max-inline-size: 472px cap (carried over from Tooltip). The floating element now sizes to its content, so consumers that want a width limit set one on their own content or via styles.floating. Popover is unaffected — PopoverContentCore already caps at 288px — but any future consumer relying on the implicit tooltip width will now render wider.
added 3 commits
August 27, 2026 10:33
Add new props to support more consumers (e.g. Popover): - `returnFocus`: whether/where focus is returned when the floating element closes. - `closeOnFocusOut`: whether the floating element closes when focus moves outside of it. - `onPlacementChange`: called with the resolved placement (after middleware such as `flip` runs). - `shiftPadding`: padding used by the `shift` middleware. - `rootBoundary`: the boundary used by the `flip` and `shift` middleware. Also add a `useFloatingReference` hook (and the `FloatingReferenceContext` it reads) so a trigger that can't receive a ref (a plain function component) can register its own DOM element as the reference element, without having to forward refs and without Floating rendering a wrapper element around the trigger. Each instance only shares its reference setter with its own trigger, so multiple open (or nested) floating elements stay independent. Triggers that can receive a ref (host elements, forwardRef and class components) keep getting the reference ref injected directly.
The 472px cap was carried over from Tooltip. It forced every Floating consumer into a tooltip-sized bubble, so the floating element now sizes to its content and consumers cap their own width (or use styles.floating).
… ref Floating no longer injects a ref into the trigger. It injects the data-wb-floating-reference attribute (unique per instance) along with the interaction props and resolves the reference element with a DOM lookup, so a trigger of any component type works as long as it spreads the props it is given, which it has to do anyway for the interaction and ARIA props. This removes the acceptsRef helper (which sniffed React internals to decide whether a trigger could receive a ref) and the FloatingReferenceContext / useFloatingReference escape hatch (a trigger picks the element to anchor to by choosing where it spreads the props). Warns in development when the trigger's element can't be found.
jandrade
pushed a commit
that referenced
this pull request
Aug 28, 2026
Migrate `Popover` off PopperJS and the `wonder-blocks-tooltip` dependency to the new `Floating` component (floating-ui). `Floating` now owns positioning, portaling, the tail/arrow, dismiss (Esc/outside click), and focus management. This is stacked on #3182, which adds the `Floating` props this migration needs (`returnFocus`, `closeOnFocusOut`, `onPlacementChange`, `shiftPadding`, `rootBoundary`). Those `Floating` changes are not part of this diff. Popover keeps its public API. `rootBoundary`/`viewportPadding` are remapped onto floating-ui's `flip`/`shift` middleware; `autoUpdate` and `initialFocusDelay` are now deprecated no-ops. Focus management is now floating-ui's non-modal model (focus flows via focus guards) rather than the previous custom circular navigation. The popover "bubble" chrome (background/border/shadow/radius) now comes from `Floating`; `PopoverContentCore` no longer renders its own chrome. The popover uses `strategy="fixed"` so it positions correctly inside clipping/ scrolling ancestors. `Floating` resolves the reference (anchor) element from the DOM rather than via a ref. `PopoverAnchor` is left completely untouched here: passing the reference attribute through to the trigger, and removing the ref plumbing that is no longer needed, are both done in WB-2119.3. That means the popover does not position yet on this commit alone -- the Popover jest suite only goes green once WB-2119.3 is applied on top. The superseded internal helpers (`focus-manager`, `initial-focus`, `popover-event-listener`) and the popper-specific parts of `popover-dialog` are removed. `PopoverDialog` is now a small functional dialog wrapper. The `Placement` and `RootBoundary` types are relocated off the `wonder-blocks-tooltip` and `@popperjs/core` dependencies into a local `util/types.ts` (both unions are unchanged). Issue: WB-2119 Automated: `pnpm typecheck`, `pnpm lint`, and `pnpm build` all pass. The Popover jest suite is expected to fail on this commit alone (see above) and goes green with WB-2119.3 applied on top. Focus-flow behaviors that jsdom can't faithfully run against floating-ui's focus guards are covered by the Floating package's unit tests in #3182. Manual (Storybook — Popover and PopoverContentCore stories): 1. Verify the popover renders with a single border + shadow (no double chrome) across placements, and the arrow/tail connects to the bubble. 2. Verify placement flip near viewport edges, and `rootBoundary="document"` / `viewportPadding` stories still position correctly. 3. Verify dismiss (Esc + outside click), initial focus, return focus, `closedFocusId`, controlled/uncontrolled open, and `portal` on/off. 4. Verify RTL placement (left/right) mirrors correctly. 5. Confirm standalone `PopoverContentCore` stories still look acceptable now that its chrome is provided by the parent (Floating) rather than itself. Please review these risky changes 1. 🚨 `packages/wonder-blocks-popover/src/components/popover.tsx`: Full rewrite of the user-visible Popover to adopt Floating. Changes the positioning engine, focus management, and dismiss behavior. 2.⚠️ `packages/wonder-blocks-popover/src/components/popover-content-core.tsx`: Removes the bubble chrome (border/background/shadow/radius); chrome is now provided by Floating. Affects the appearance of standalone `PopoverContentCore`. 3.⚠️ `packages/wonder-blocks-popover/src/components/popover-dialog.tsx`: Reworked from a class component into a functional dialog wrapper; drops the tail and the `wonder-blocks-tooltip` dependency.
jandrade
pushed a commit
that referenced
this pull request
Aug 28, 2026
Migrate `Popover` off PopperJS and the `wonder-blocks-tooltip` dependency to the new `Floating` component (floating-ui). `Floating` now owns positioning, portaling, the tail/arrow, dismiss (Esc/outside click), and focus management. This is stacked on #3182, which adds the `Floating` props this migration needs (`returnFocus`, `closeOnFocusOut`, `onPlacementChange`, `shiftPadding`, `rootBoundary`). Those `Floating` changes are not part of this diff. Popover keeps its public API. `rootBoundary`/`viewportPadding` are remapped onto floating-ui's `flip`/`shift` middleware; `autoUpdate` and `initialFocusDelay` are now deprecated no-ops. Focus management is now floating-ui's non-modal model (focus flows via focus guards) rather than the previous custom circular navigation. The popover "bubble" chrome (background/border/shadow/radius) now comes from `Floating`; `PopoverContentCore` no longer renders its own chrome. The popover uses `strategy="fixed"` so it positions correctly inside clipping/ scrolling ancestors. `PopoverAnchor` spreads the `FloatingReferenceAttributeName` attribute it is given onto the trigger, so `Floating` can resolve the reference (anchor) element from the DOM. The existing ref plumbing is left in place here and is cleaned up separately in WB-2119.3. The superseded internal helpers (`focus-manager`, `initial-focus`, `popover-event-listener`) and the popper-specific parts of `popover-dialog` are removed. `PopoverDialog` is now a small functional dialog wrapper. The `Placement` and `RootBoundary` types are relocated off the `wonder-blocks-tooltip` and `@popperjs/core` dependencies into a local `util/types.ts` (both unions are unchanged). Issue: WB-2119 Automated: `pnpm typecheck`, `pnpm lint`, and `pnpm build` all pass. The Popover jest suite passes (tests updated for the new focus model). Focus-flow behaviors that jsdom can't faithfully run against floating-ui's focus guards are covered by the Floating package's unit tests in #3182. Manual (Storybook — Popover and PopoverContentCore stories): 1. Verify the popover renders with a single border + shadow (no double chrome) across placements, and the arrow/tail connects to the bubble. 2. Verify placement flip near viewport edges, and `rootBoundary="document"` / `viewportPadding` stories still position correctly. 3. Verify dismiss (Esc + outside click), initial focus, return focus, `closedFocusId`, controlled/uncontrolled open, and `portal` on/off. 4. Verify RTL placement (left/right) mirrors correctly. 5. Confirm standalone `PopoverContentCore` stories still look acceptable now that its chrome is provided by the parent (Floating) rather than itself. Please review these risky changes 1. 🚨 `packages/wonder-blocks-popover/src/components/popover.tsx`: Full rewrite of the user-visible Popover to adopt Floating. Changes the positioning engine, focus management, and dismiss behavior. 2.⚠️ `packages/wonder-blocks-popover/src/components/popover-content-core.tsx`: Removes the bubble chrome (border/background/shadow/radius); chrome is now provided by Floating. Affects the appearance of standalone `PopoverContentCore`. 3.⚠️ `packages/wonder-blocks-popover/src/components/popover-dialog.tsx`: Reworked from a class component into a functional dialog wrapper; drops the tail and the `wonder-blocks-tooltip` dependency.
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.
FloatingAdds the props that consumers like
Popoverneed fromFloating, and changeshow
Floatingresolves its reference (anchor) element so that a trigger nolonger has to forward refs — and so that no wrapper element is added to the DOM.
New props:
returnFocus,closeOnFocusOut,onPlacementChange,shiftPadding,rootBoundary.New export:
useFloatingReference(and theFloatingReferenceContextitreads). A trigger that can't receive a ref (a plain function component) can
attach the returned ref callback to the element it renders, and that element
becomes the reference element. Triggers that can receive a ref (host elements,
forwardRefand class components) keep getting the reference ref injecteddirectly, so existing usage is unchanged.
Each
Floatingprovides its reference setter to its own trigger only —never to the floating content — so multiple simultaneously open (or nested)
floating elements resolve independent reference elements.
Also removes the floating element's
max-inline-size(previously472px,carried over from
Tooltip). That cap forced everyFloatingconsumer into atooltip-sized bubble; the floating element now sizes to its content, and
consumers that need a width cap set one on their own content (or via the
styles.floatingprop).Popoveris unaffected —PopoverContentCorealreadycaps its content at 288px.
Impact:
@khanacademy/wonder-blocks-floatingis not yet released (0.0.1)and currently has no production consumers, so this is additive for external
users. Within this stack,
WB-2119.2(thePopoverrefactor) depends onuseFloatingReferenceand should be rebased on this once it lands.Issue: WB-2119
Test plan:
Automated:
pnpm typecheck,pnpm lint,pnpm jest packages/wonder-blocks-floatingand
packages/wonder-blocks-popoverall pass, plus Storybook story tests(with a11y) for every
Floatingstory and 8Popoverstories.Manual verification (positioning and focus behavior can't be fully asserted in
jsdom):
pnpm startand open Packages / Floating → Custom Trigger Component(http://localhost:6061/?path=/story/packages-floating--custom-trigger-component).
custom component (which uses
useFloatingReference, not a forwarded ref).inspector.
Escand click outside — both should dismiss it, confirming theprops
Floatinginjects still reach the trigger.anchor correctly against
IconButtontriggers, in both LTR and RTL (togglethe direction global).
shift/flip/hidestill behave as before with aViewtrigger.472pxcap isgone: check a long-text story (and a
Popoverstory, which should beunchanged at 288px) in both LTR and RTL.
Review plan:
Please review these risky changes
floating.tsxfloating-reference-context.tsaccepts-ref.tsfloating.tsx(styles) — removes the472pxmax-inline-size.Common patterns:
1 File: New optional props are declared with JSDoc (so they land in the
Storybook props table), defaulted in the destructured signature, and threaded
into the floating-ui config. Five props follow this shape.
3 Files: Logic that doesn't belong in the component was extracted into
src/util/with a colocated test file, then imported byfloating.tsx.