refactor(core)!: centralize popup positioning - #1904
Merged
Conversation
Move popup geometry, observers, style updates, and cleanup into a shared DOM positioner. Use thin HTML and React adapters around the shared lifecycle. BREAKING CHANGE: Low-level positioning helpers and CSS variable key types are no longer exported.
✅ Deploy Preview for vjs10-site ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📦 Bundle Size Report🎨 @videojs/html
Presets (7)
Media (12)
Players (5)
Skins (30)
UI Components (39)
Sizes are marginal over the root entry point. ⚛️ @videojs/react
Presets (7)
Media (11)
Skins (27)
UI Components (33)
Sizes are marginal over the root entry point. 🧩 @videojs/core
Entries (75)
🏷️ @videojs/element — no changesEntries (2)
📦 @videojs/store — no changesEntries (3)
🔧 @videojs/utils — no changesEntries (13)
📦 @videojs/media — no changesEntries (14)
📦 @videojs/spf — no changesEntries (5)
ℹ️ How to interpretJS sizes are initial static graph totals (minified + brotli). Lazy dynamic chunks are shown separately when present.
Run |
mihar-22
marked this pull request as ready for review
August 3, 2026 18:43
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 94d5164. Configure here.
| }); | ||
|
|
||
| return () => positioner.cleanup(); | ||
| }, [open, anchorName, position, trigger, popupRef, boundary, container, cssVars, onSideChange, positioner]); |
There was a problem hiding this comment.
Stale trigger blocks initial positioning
High Severity
usePopupPosition passes the render-time trigger into positioner.sync, but on defaultOpen that value is still null because trigger refs have not run yet. The old layout effect read triggerElement after refs attached. Sync bails out, so the first paint(s) skip anchor setup and measurement until a later transition re-render happens to recover.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 94d5164. Configure here.
| this.#capturePopupStyles(options.popup, options.cssVars ?? PopoverCSSVars); | ||
| applyStyles(options.popup, style); | ||
| options.onSideChange?.(side); | ||
| } |
There was a problem hiding this comment.
Menu skips sync remeasure pass
Medium Severity
PopupPositioner.#position applies geometry once, then onSideChange runs syncMenuViewRoot, which can change the menu box via --media-menu-width. The old menu path remeasured and reapplied after that sync for the JS fallback. Alignment for center/end can stay wrong until a later ResizeObserver tick, and stays wrong if ResizeObserver is missing.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 94d5164. Configure here.
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
2 participants
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.
Summary
Centralize popup positioning in
@videojs/core/domand migrate Tooltip, Popover, and root Menu positioning in HTML and React to thin adapters. This is the first menu-simplification step and deliberately leaves menu navigation, viewport transitions, and skin behavior unchanged.Changes
PopupPositionerfor CSS anchor setup, boundary geometry, CSS variable output, observers, scroll handling, and cleanupPositionControllerand ReactusePopupPositionintegration to lifecycle adaptersImplementation details
The core DOM positioner owns all structural style writes and restores consumer-authored inline styles during cleanup. HTML discovers elements and forwards options; React owns refs and synchronizes the positioner in a layout effect without keeping geometry in React state.
This intentionally narrows the public surface by removing low-level positioning helpers and CSS variable key types from
@videojs/core/dom.Testing
pnpm -F @videojs/core test src/dom/ui/popover/tests/popover-positioning.test.ts src/dom/ui/popover/tests/popup-positioner.test.ts— 43 testspnpm -F @videojs/html test src/ui/menu/tests/menu-element.test.ts src/ui/popover/tests/popover-element.test.ts src/ui/tooltip/tests/tooltip-element.test.ts— 28 testspnpm -F @videojs/react test src/ui/menu/tests/menu.test.tsx src/ui/popover/tests/popover.test.tsx src/ui/tooltip/tests/tooltip.test.tsx— 37 testspnpm -F @videojs/core buildpnpm -F @videojs/html buildpnpm -F @videojs/react buildpnpm build:sandboxpnpm typecheckpnpm lintpnpm check:workspacegit diff --checkNote
Medium Risk
Cross-cutting UI positioning refactor with a breaking
@videojs/core/domexport change; behavior is covered by new/updated tests but regressions in flip/sizing or style restoration are possible.Overview
Introduces a shared
PopupPositionerin@videojs/core/domthat owns anchor setup, boundary measurement, inline style writes, scroll/resize/ResizeObservertracking, and cleanup (including restoring consumer-authored inline styles).HTML
PositionControllerand ReactusePopupPositionare reduced to lifecycle adapters; popover, tooltip, and root menu elements stop duplicating measurement/getAnchorPositionStylelogic and callsyncwith boundary, container, and optionalcssVars/onSideChangeinstead.The public DOM surface is narrowed:
getAnchorNameStyle,getPopoverCSSVars, andPopoverCSSVarKey/TooltipCSSVarKeyare removed;ManualOffsetsis renamed toPositioningOffsets. Trigger/popup anchor-name handling moves into the positioner (merging with existinganchor-namevalues when supported).Reviewed by Cursor Bugbot for commit 94d5164. Bugbot is set up for automated code reviews on this repo. Configure here.