Repository navigation
WP Build Polyfills: continue the admin menu color behind the boot frame - #51619
Conversation
sample the wp-admin menu color behind the boot single-page layout
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Backup plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Boost plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Search plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Social plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Protect plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Videopress plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Mu Wpcom plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Wpcomsh plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Premium Analytics plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 1 file.
1 file is newly checked for coverage.
|
boot 0.21 (gutenberg#81756) renames boot-layout--single-page
the second register() call is the idempotence assertion
kangzj
left a comment
There was a problem hiding this comment.
Verdict: approving — thorough code review found no blockers, and I reproduced both the bug on trunk and the fix on this branch in a live docker WP env (WP 7.1, Gutenberg 23.7.1), including the boot 0.21 CSS Modules selector path.
Blockers
None.
Suggestions (non-blocking)
- The failing Code coverage (JS) check is
extensions/shared/test/execution-lock-test.jsinplugins/jetpack— a JS test this PHP-only PR never touches, and the regular JS-tests check passes. Looks like a flake; a re-run of the failed job should clear it and unblock the coverage gate.
Code review notes
WP_Build_Admin_Frame::register()is genuinely idempotent (same static callback + priority → one$wp_filterentry), so arming it from everyWP_Build_Polyfills::register()call is safe; the test covers the double-registration case.- Both printed blocks are static strings (nowdoc, no interpolation), so there's no escaping surface, and
wp_print_inline_script_tag()keeps the script CSP/nonce-filterable. - All
register()consumers I checked (forms, activity-log, my-jetpack) scope the call to their own page load, which is why Dashboard/Posts stay clean — confirmed live below. - Changelog coverage is complete: entries in the package plus the 10 dependent plugins where the change is user-reachable.
plugins/statsandplugins/starter-pluginalso bundle the package (via my-jetpack) but ship no boot single-page layout, so skipping them is correct. - The one new Phan suppression (
PhanPluginDuplicateAdjacentStatementin the test) carries a justification and suppresses a style rule, not a defect.
Testing performed
Live docker WP env, plugins/jetpack + affected packages built from this branch (PHP is source-mounted; the "before" state is the merge-base 0a3e1ac33a9 of the package source). Navy third-party scheme registered exactly per the testing instructions.
| Step | Result |
|---|---|
Navy · Forms (jetpack-forms-responses-wp-admin) |
✅ property = rgb(2, 57, 92), both ids present (2), layout + body backgrounds = menu color |
| Navy · Activity Log | ✅ same values |
Boot 0.21 simulation (layout root renamed to _a1b2c3__layout-single-page) |
✅ layout + body keep rgb(2, 57, 92) via the attribute selector |
| Core scheme Blue | ✅ rgb(36, 82, 120) continues behind the frame |
| Core scheme Light | ✅ rgb(229, 229, 229) continues behind the frame |
| Dashboard + Posts (no boot layout) | ✅ neither id in the DOM (0), no property set, pages unchanged |
| Narrow viewport (600px) | ✅ page renders, no horizontal scroll, nothing broken |
| Before state (merge-base) | ✅ bug reproduced: ids absent, layout/body = rgb(30, 30, 30) against the navy menu |
jp test php packages/wp-build-polyfills |
✅ pass (CI also green across the PHP 7.4–8.5 matrix) |
jp phan |
Crashed locally (env issue parsing wordpress-stubs); CI Static analysis is green, which I'm treating as authoritative |
Evidence
Before/after under the navy scheme, Forms page — zoomed crops of the bottom band and the notch under the admin bar (2.5×):
… asked for Three cases where the tool produced a clean result it had not earned: * The prompt and progress lines went to stdout, so the documented `run … > report.md` wrote the "flip the flag" prompt into the report and hid it from the terminal. stdout now carries the report alone. * `run` accepted a bare Enter, so EOF (a pipeline, a non-TTY shell) exited 0 with no report, and a keypress buffered during the first capture answered the prompt early and captured flag-off twice. It now refuses a non-TTY, asks for the flag name, and fails if input closes first. * The report judged every run against the default targets, where `control` is optional and has no selector. A control that a port renamed away read as "ok (not present, optional)". Captures now record the targets they used, and a control with a selector is required. Also from that review: record `requestfailed`, so a bad host or a CSP block is not invisible; use `checkVisibility` so an ancestor's `display:none` is a visibility change rather than four px deltas; report when a control selector matches more than one element; keep rects unrounded so a sub-pixel tolerance means something; assert the landed URL still matches the requested path, with `--autologin-url` for hosts whose link logs you in; add `--load-state` for a page whose requests never settle; apply `--wait-selector` to the flag-on capture only and require its absence flag-off, which proves the flip landed; keep the host in the request key and drop per-event hosts such as `pixel.wp.com`; report per-side request totals and count an empty side; warn when the two snapshots are of different pages. Docs and wiring: `.gitignore` now matches the filenames the README uses; the colour-scheme regression is #51619, not #52096; root `composer.json` `test-js` runs these tests, the hook `tools/performance` already used. `capture.js` is no longer untested. `test/stub-wp-admin.js` is a `node:http` stub of wp-login.php, a Dashboard and one flag-dependent admin page, and `pnpm test:smoke` drives real headless Chromium against it — 11 tests covering login, the buffer clear, both redirect guards, ancestor-aware hidden detection and the failed-request path. It skips itself when no browser is installed. The README's claim that this needed a GPU was wrong. 70 unit tests, up from 55, plus the 11 smoke tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Script the wp-build port verification check's geometry and network diffs JETPACK-2685 splits the scriptable half of the port verification rule (JETPACK-2573) out of its manual checklist: step 2 (computed styles and geometry) and step 3 (the network panel), compared with a port's feature flag off versus on. Adds tools/port-verification, a standalone CLI (`verify-port run|capture| diff`) built on Playwright. `run` captures a page with the flag off, pauses for the reviewer to flip the flag on the site, captures again, and prints a Markdown report to paste into the PR. The diff and report logic (src/diff.js, src/report.js) is pure and unit-tested against fixtures here; the Playwright capture half (src/capture.js) needs a live site and is documented as unverified in this sandbox (no GPU). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix wpfooter false positive, redact nonces, and narrow the ignore list PR review on #52579 found three issues in the port verification script: - Blocker: boot hides #wpfooter by design (see projects/js-packages/base-styles/admin-page-layout.scss, so every live run reported it CHANGED with bogus zero-rect deltas -- getBoundingClientRect() on a display:none element is all zeros. capture.js now records a per-target hidden flag; diff.js reports a visibility change as its own hidden-changed status instead of diffing a meaningless rect; report.js renders it as OK (hidden by design) only for targets whose selectors.js entry sets allowHidden (just the footer). An unexpected visibility change on any other target still surfaces as a CHANGED finding. - report.js printed the raw request URL in the network section, so a nonce value survived into Markdown meant to be pasted into a public PR. Added diff.js redactUrl(), which strips the same ignored query params used for matching, and used it for display too. - Dropped v and t from DEFAULT_IGNORED_QUERY_PARAMS -- generic enough to carry real state (an API version, a tab filter) that stripping them could hide a genuine step-3 difference. Added --ignore-query-param for a site that needs them ignored anyway. Also hardened capture.js login(): it now waits for #dashboard-widgets/#wpbody after submit and fails with a clear message instead of silently proceeding to capture wp-login.php own failure page on a wrong password (mirrors tools/performance/scripts/measure-lcp.js). 12 new tests (25 to 37), covering the hidden/missing distinction, the nonce redaction, and the v/t opt-in. node --test and eslint --max-warnings=0 both clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> EOF ) * Address review: don't diff login traffic, don't collapse repeat requests Review of #52579 found three ways the tool reports a clean pass on a page it should flag: * The response listener started before `login()`, so step 3 diffed wp-login.php and the Dashboard alongside the target page. Clear the buffer after login. * `diffNetwork` keyed a Map by method + path, so a repeated request kept only its last occurrence -- a 404 refetched into a 200 reported nothing. Group every occurrence, compare the distinct statuses, and report a changed occurrence count on its own. * A non-numeric `--tolerance` became `NaN`, and `delta > NaN` is false for every delta, so step 2 reported OK for every target. Reject it instead. Also: abort when a capture lands on wp-login.php, since a fresh browser profile has no cookies to arrive authenticated with; add `_ajax_nonce` and `_nonce` to the ignored params, so admin-ajax calls match between loads and their nonces stay out of a report pasted into a public PR; give an unmeasured target its own row, so a skipped `--control-selector` no longer reads as a passing check. Option parsing moves to `src/options.js` to be testable without running `main()`. 55 tests, up from 37. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Address dhasilva review: keep stdout for the report, enforce what was asked for Three cases where the tool produced a clean result it had not earned: * The prompt and progress lines went to stdout, so the documented `run … > report.md` wrote the "flip the flag" prompt into the report and hid it from the terminal. stdout now carries the report alone. * `run` accepted a bare Enter, so EOF (a pipeline, a non-TTY shell) exited 0 with no report, and a keypress buffered during the first capture answered the prompt early and captured flag-off twice. It now refuses a non-TTY, asks for the flag name, and fails if input closes first. * The report judged every run against the default targets, where `control` is optional and has no selector. A control that a port renamed away read as "ok (not present, optional)". Captures now record the targets they used, and a control with a selector is required. Also from that review: record `requestfailed`, so a bad host or a CSP block is not invisible; use `checkVisibility` so an ancestor's `display:none` is a visibility change rather than four px deltas; report when a control selector matches more than one element; keep rects unrounded so a sub-pixel tolerance means something; assert the landed URL still matches the requested path, with `--autologin-url` for hosts whose link logs you in; add `--load-state` for a page whose requests never settle; apply `--wait-selector` to the flag-on capture only and require its absence flag-off, which proves the flip landed; keep the host in the request key and drop per-event hosts such as `pixel.wp.com`; report per-side request totals and count an empty side; warn when the two snapshots are of different pages. Docs and wiring: `.gitignore` now matches the filenames the README uses; the colour-scheme regression is #51619, not #52096; root `composer.json` `test-js` runs these tests, the hook `tools/performance` already used. `capture.js` is no longer untested. `test/stub-wp-admin.js` is a `node:http` stub of wp-login.php, a Dashboard and one flag-dependent admin page, and `pnpm test:smoke` drives real headless Chromium against it — 11 tests covering login, the buffer clear, both redirect guards, ancestor-aware hidden detection and the failed-request path. It skips itself when no browser is installed. The README's claim that this needed a GPU was wrong. 70 unit tests, up from 55, plus the 11 smoke tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Fix two defects a live Jurassic Ninja run found First run of capture.js against a real site, on Jetpack 16.3-a.1: * Login never worked on a site with Jetpack SSO, which is most Jetpack sites. SSO leaves the classic form in the DOM but hidden behind "Log in with username and password", so filling it timed out. Click the toggle first. Pick it by visibility, not by class: the visible one on the test site was `.jetpack-sso-toggle.wpcom`, which is not the one its name suggests. * `_cacheBuster` was not an ignored query param, so every `wp-json` call appeared under both "only with flag off" and "only with flag on". On the Search dashboard that was 4 of 24 network findings. Verified against the `search-wp-build` flag on a real Search dashboard: the four default selectors all resolve, geometry comes back clean on all four frame targets, and step 3 shows the port swapping `jp-search-dashboard.js` for boot's module graph. After the `_cacheBuster` fix, 20 findings, all real. 71 unit tests, 11 smoke tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Stop the login flaking on networkidle Re-running the live check at 86f9772 failed with "Login did not land in wp-admin", then passed twice. About one run in four. `login()` gated its navigation on the caller's --load-state, which defaults to `networkidle`. wp-admin never really goes idle -- heartbeat, JITM -- so the wait sometimes timed out, and the catch reported it as bad credentials, which is a misleading error on top of a flaky one. The login hop now uses `domcontentloaded` and leans on the selector waits, which are the real evidence of being logged in. --load-state still applies to the page capture, where settling is the point. 8 of 8 live logins clean afterwards, against roughly 1 in 4 failing before. That is evidence rather than proof at this sample size, but the mechanism matches: a page that never goes idle cannot be waited on that way. Adding a `#loginform` wait then broke 9 of the 11 smoke tests, because the stub's form had no `id`, which real wp-login.php does have. Fixed the stub rather than dropping the wait -- that gap is the same kind of divergence that hid the SSO bug until a real site ran it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Own tools/port-verification in CODEOWNERS @Automattic/jetpack-monorepo owns /tools by default and does not have the bandwidth for a special-purpose tool, so name an owner explicitly. Follows the @jeherve precedent for a single-owner tool; last match wins, so this takes it off the monorepo team. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Proposed changes
On trunk, every admin page generated by
@wordpress/build(Forms, Premium Analytics, Newsletter, Backup, Scan, SEO, Social, VideoPress, Podcast, Activity Log, site setup in jetpack-mu-wpcom) renders inside the@wordpress/bootsingle-page layout: a white stage with rounded corners over a backdrop that is meant to continue the wp-admin menu.@wordpress/admin-uimaps nine core color schemes to theme seeds and falls back tomodernfor anything else, so on every WordPress.com scheme (nightfall,aquatic,classic-blue,classic-bright,classic-dark,contrast,powder-snow,sakura,sunset) and on any third-party scheme registered withwp_admin_css_color()the backdrop is near-black (#1e1e1e) next to a menu of another color: a black notch under the admin bar and black bands in the bottom and right margins of the frame.With this PR the backdrop follows the menu on every scheme, with no changes in the consuming packages:
WP_Build_Polyfills::register()also arms a newWP_Build_Admin_Frameclass inpackages/wp-build-polyfills, the one runtime every wp-build page already loads.admin_headit prints a stylesheet that paints.boot-layout--single-pageand the body with--wp-build-admin-menu-background, falling back to boot's own colors when the property is not set.in_admin_header, right after#adminmenubackis printed and before the layout mounts, it prints an inline script that samples that element's computed background into the custom property on the root element.How it works
The layout rule boot injects at runtime is
.boot-layout { background: var(--wpds-color-background-surface-neutral-weak) }, specificity (0,1,0), appended to<head>after the page stylesheets.#wpcontent .boot-layout--single-pageis (1,1,0), so the override wins on specificity rather than order. The second selector covers boot 0.21 (WordPress/gutenberg#81756, Gutenberg 23.9), where the layout styles are CSS Modules and the root class is_<hash>__layout-single-page: wp-build keeps the local name in production builds (checked on@wordpress/boot@0.21.0), so[class*="__layout-single-page"]matches whatever the hash is, at the same specificity.body:has(.boot-layout--single-page)keeps the elastic-scroll area consistent without relying on boot's body sync; it is a separate rule so browsers without:has()still apply the layout rule.Sampling the rendered menu instead of mapping schemes is the point: one mechanism covers core schemes on older cores, WordPress.com schemes and third-party schemes, and the property is only set when the sample is a real color. Pages without a boot layout are unaffected by construction: the selectors match nothing.
The fix lives in PHP because on WordPress 7.0+ the boot module that runs is core's bundled copy.
WP_Script_Modules::register()ignores an id that is already registered, so bumping@wordpress/bootin a package changes nothing on WordPress.com. Core already made its nine schemes' menu color equal the ramp output (r62454), which is why only unknown schemes break.Notes for reviewers
src/class-wp-build-admin-frame.php) so it can be removed in one commit once wp-build or boot ship the same behavior. The upstream direction is structured per-scheme seeds inwp_admin_css_color(), see Allow third-party color schemes to apply to Site Editor screens WordPress/gutenberg#80766 and Centralize WordPress admin color scheme settings WordPress/gutenberg#81980.--wp-admin-theme-color, primary buttons) still comes from themodernseed on unknown schemes. Same upstream problem, out of scope here.@wordpress/bootmodule through the wp-build module registration (deregister, then register), so WordPress.com runs boot 0.21 once it moves from Gutenberg 23.8 to 23.9. Jetpack's own builds register only their@jetpack-*modules and keep core's boot. Both class forms were checked on a live page by renaming the layout root to the 0.21 class names: layout and body keep the menu color, and any other local name does not match.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
Register a throwaway third-party scheme in a mu-plugin, then pick Navy in Users → Profile → Admin Color Scheme:
with
navy/colors.csscontaining#adminmenu, #adminmenuback, #adminmenuwrap { background: #02395c; }. Any Masterbar scheme on a WordPress.com site behaves the same way.Unknown scheme
admin.php?page=jetpack-forms-responses-wp-admin), Jetpack → Stats (admin.php?page=jetpack-premium-analytics-wp-admin) and Jetpack → Activity Log. The corner under the admin bar and the bottom and right margins around the frame must be#02395c, on load and after scrolling. On trunk they are near-black.getComputedStyle( document.documentElement ).getPropertyValue( '--wp-build-admin-menu-background' )returnsrgb(2, 57, 92)anddocument.querySelectorAll( '#wp-build-admin-frame-css, #wp-build-admin-frame-js' ).lengthreturns2.Boot 0.21
_<hash>__layout-single-pageand the frame must still be#02395c.Core schemes
rgb(36, 82, 120)andrgb(229, 229, 229)), so nothing regresses on the schemes that already worked.Pages without a boot layout
querySelectorAllabove returns0).Narrow viewport
Tests
jp test php packages/wp-build-polyfills