You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Don't lock page scrolling when opening a popup that isn't on the page - #1430
In 1.25.0, calling open on a popup whose markup is not on the page (for example PUM.open( id ) for a missing popup, or .popmake( 'open' ) on an empty collection) adds pum-open, pum-open-overlay and pum-open-scrollable to <html>, then reposition() throws TypeError: can't access property "getBoundingClientRect", n[0] is undefined. The popup never shows, and html.pum-open.pum-open-overlay { overflow: hidden } leaves the page unable to scroll.
This is a 1.25.0 regression. In 1.24.0 the same call threw earlier, in initializeState() ($popup[ 0 ].offsetHeight), before any <html> classes were added, so scrolling kept working. 1.25.0 removed that forced reflow, so execution now reaches the scroll-lock classes before failing.
Change
open() returns early when the popup collection is empty, before the scroll-lock classes are added.
Real popups are unaffected; the new unit test includes a positive case.
Evidence
New tests/unit/site/pum-open.test.js: 2 of 3 cases fail on the current code with the exact getBoundingClientRect error; all 3 pass with the fix. Full JS unit suite passes.
Firefox 153 on WordPress 7.1.3, logged-in admin with toolbar, in an isolated local site:
1.25.0: missing-popup open locks scrolling and throws the reported error, with stack reposition / popmake / open / popmake / open.
1.24.0: throws in initializeState, scrolling still works.
Patched build: no error, no <html> classes, scrolling works; a normal 500 ms auto-open popup still opens for visitors and logged-in users.
An unpatched production build of this branch is byte-identical to the released 1.25.0 dist/assets/site.js; the patched build differs only by these guards.
Not covered
What makes the popup missing on the reporting site (they saw it only for logged-in users with the toolbar) was not reproduced. A plain logged-in auto-open popup works. This PR stops the scroll lock; it does not explain why that site's open call targets a missing popup.
…e page
open() on an empty collection added the html scroll-lock classes, then
reposition() threw on getBoundingClientRect. Bail early in open() and skip
the clipping check when there is no container.
Fixes#1429
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reviewing files that changed from the base of the PR and between acd2dcc and 37474cf.
Files selected for processing: 2
assets/js/src/site/plugins/pum.js
tests/unit/site/pum-open.test.js
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.
Walkthrough
The popup open path now returns when its element is missing. The reposition path checks for a container before reading its bounding rectangle. Unit tests cover missing popup elements and opening a configured popup.
Changes
Missing popup handling
Layer / File(s)
Summary
Missing-element guards and test coverage assets/js/src/site/plugins/pum.js, tests/unit/site/pum-open.test.js
open returns when the popup element is missing. reposition skips bounding-rectangle checks when no container exists. Tests cover missing popup IDs, empty collections, and a configured popup.
The change prevents missing popups from locking page scrolling. No actionable merge-blocking issue remains.
Pre-merge checks: 5
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title clearly and concisely describes the main change: preventing page scroll locking when the targeted popup is absent.
Linked Issues check
Issue #1429 requires missing-popup opens to avoid overlay classes and scroll locking, while normal popups continue to open. open() now returns before events and HTML scroll-lock classes when `$popup…
Out of Scope Changes check
The changes remain within issue #1429. The open() guard prevents the reported scroll-lock regression. The reposition() guard prevents the related empty-container error. The tests verify both regre…
Docstring Coverage
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Full details: Linked Issues check
Explanation
Issue #1429 requires missing-popup opens to avoid overlay classes and scroll locking, while normal popups continue to open. open() now returns before events and HTML scroll-lock classes when $popup.length is zero. reposition() now skips the container clipping check when no container exists. The new tests cover a missing ID, an empty collection, and a normal popup. The positive test verifies the normal popup receives the lock classes and pum-active. The reported cause of missing markup is not a coding requirement.
Full details: Out of Scope Changes check
Explanation
The changes remain within issue #1429. The open() guard prevents the reported scroll-lock regression. The reposition() guard prevents the related empty-container error. The tests verify both regression cases and normal popup behavior. No unrelated change is shown.
Full details: Docstring Coverage
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 2 files.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
ESLint
If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.
assets/js/src/site/plugins/pum.js
ESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox.
tests/unit/site/pum-open.test.js
ESLint skipped: the matched ESLint configuration already failed (missing-dependency).
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.
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
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.
Fixes #1429
Problem
In 1.25.0, calling
openon a popup whose markup is not on the page (for examplePUM.open( id )for a missing popup, or.popmake( 'open' )on an empty collection) addspum-open,pum-open-overlayandpum-open-scrollableto<html>, thenreposition()throwsTypeError: can't access property "getBoundingClientRect", n[0] is undefined. The popup never shows, andhtml.pum-open.pum-open-overlay { overflow: hidden }leaves the page unable to scroll.This is a 1.25.0 regression. In 1.24.0 the same call threw earlier, in
initializeState()($popup[ 0 ].offsetHeight), before any<html>classes were added, so scrolling kept working. 1.25.0 removed that forced reflow, so execution now reaches the scroll-lock classes before failing.Change
open()returns early when the popup collection is empty, before the scroll-lock classes are added.reposition()skips the viewport clipping check (Change vertical alignment of too high popups #1227) when there is no container element.Real popups are unaffected; the new unit test includes a positive case.
Evidence
tests/unit/site/pum-open.test.js: 2 of 3 cases fail on the current code with the exactgetBoundingClientRecterror; all 3 pass with the fix. Full JS unit suite passes.reposition / popmake / open / popmake / open.initializeState, scrolling still works.<html>classes, scrolling works; a normal 500 ms auto-open popup still opens for visitors and logged-in users.dist/assets/site.js; the patched build differs only by these guards.Not covered
What makes the popup missing on the reporting site (they saw it only for logged-in users with the toolbar) was not reproduced. A plain logged-in auto-open popup works. This PR stops the scroll lock; it does not explain why that site's open call targets a missing popup.
🤖 Generated with Claude Code
Summary by CodeRabbit