Prevent CSS leaks by restricting form-builder styles to WPUF pages only #1700
Conversation
arifulhoque7
added
needs: testing
needs: dev review
WalkthroughReplaces minimal admin form-builder CSS with Tailwind Preflight/base and component layering, narrows the white-background selector to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
assets/css/admin/form-builder.css (3)
1008-1027: Duplicate transition declarations — trim to reduce size.This rule repeats transition-property and transition-timing-function multiple times, bloating the CSS. Consider enabling cssnano’s duplicate removal (postcss-merge-rules) in the build to dedupe across the bundle.
6082-6084: Background override scope looks safe; optional extra scoping.#wpcontent { background:#fff } is acceptable since the CSS now loads only on WPUF pages. If you want belt-and-suspenders isolation, add a WPUF body class to the selector.
-#wpcontent { +body[class*="wpuf"] #wpcontent { background: #fff; }
6398-6414: Static analysis: possible duplicate property (outline).Biome flagged a duplicate property around here. I couldn’t pinpoint a visible break, but duplicates do exist elsewhere. Running a dedupe/minify pass in the build would clear this and shrink output.
src/css/admin/form-builder.css (2)
121-129: FontAwesome dependency for selectize arrow.Using content: '\f107' requires FA to be loaded on those screens. If FA isn’t guaranteed, consider a pure-CSS caret or Dashicons for consistency with WP.
-.selectize-control.single .selectize-input:after { - content: '\f107'; - font-family: FontAwesome, serif; +/* Use Dashicons caret-down (ensure dashicons is enqueued) */ +.selectize-control.single .selectize-input:after { + content: "\f140"; + font-family: "dashicons";
188-216: Hard-coded green for range inputs.If this control appears, tie colors to your primary theme token to keep branding consistent.
- background: green; + background: var(--fallback-p, #059669); ... - background: green; + background: var(--fallback-p, #059669);includes/Admin/Forms/Post/Templates/Form_Template.php (2)
85-89: Scoped enqueue is correct; harden screen checks.Guard against null/undefined properties to avoid PHP 8 type errors on unusual screens.
- $screen = get_current_screen(); - if ($screen && (strpos($screen->id, 'wpuf') !== false || strpos($screen->base, 'wpuf') !== false)) { + $screen = function_exists('get_current_screen') ? get_current_screen() : null; + $id = $screen && isset($screen->id) ? (string) $screen->id : ''; + $base = $screen && isset($screen->base) ? (string) $screen->base : ''; + if ( $screen && ( ($id !== '' && strpos($id, 'wpuf') !== false) || ($base !== '' && strpos($base, 'wpuf') !== false) ) ) { wp_enqueue_style( 'wpuf-admin-form-builder' ); }
79-85: Avoid drift: centralize “is WPUF screen?” logic.enqueue_scripts() already uses should_display() for deps; this additional screen sniffing is fine but duplicates logic with Posting::enqueue_script(). Consider extracting a helper (e.g., wpuf_is_wpuf_admin_screen()) to keep criteria in one place.
Also applies to: 85-90
includes/Admin/Posting.php (2)
108-112: Good gating; add defensive casts to prevent notices.Cast screen properties to string before strpos to avoid type errors and align with other usages.
- $screen = get_current_screen(); - if ($screen && (strpos($screen->id, 'wpuf') !== false || strpos($screen->base, 'wpuf') !== false)) { + $screen = function_exists('get_current_screen') ? get_current_screen() : null; + $id = $screen && isset($screen->id) ? (string) $screen->id : ''; + $base = $screen && isset($screen->base) ? (string) $screen->base : ''; + if ( $screen && ( ($id !== '' && strpos($id, 'wpuf') !== false) || ($base !== '' && strpos($base, 'wpuf') !== false) ) ) { wp_enqueue_style( 'wpuf-admin-form-builder' ); }
108-112: Double-check coverage of non-WPUF edit screens.If WPUF meta boxes appear on core post/page edit screens (screen IDs like “post”/“page”), this gate will skip form-builder CSS there. Verify the UI doesn’t regress; if needed, enqueue a smaller, metabox-only stylesheet on those screens instead of the full builder CSS.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
assets/css/admin/form-builder.css(2 hunks)includes/Admin/Forms/Post/Templates/Form_Template.php(1 hunks)includes/Admin/Posting.php(1 hunks)src/css/admin/form-builder.css(2 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
assets/css/admin/form-builder.css
[error] 641-641: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
outline is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
🔇 Additional comments (3)
assets/css/admin/form-builder.css (1)
1-3: Preflight in compiled CSS is acceptable given scoped enqueue.Including Tailwind Preflight here is fine now that the stylesheet is gated to WPUF screens only. No action needed.
src/css/admin/form-builder.css (2)
1-1: Good call switching on Tailwind base in source.This aligns source with the compiled Preflight and keeps intent clear.
53-55: Confirm no clash with admin color schemes.For sites using custom admin color schemes or dark-mode plugins, verify that forcing #fff on #wpcontent is desired on WPUF pages.
Rubaiyat-E-Mohammad
added
QA Approved
| wp_enqueue_style( 'wpuf-admin-form-builder' ); | ||
| // Only load on WPUF admin pages to prevent CSS leaks | ||
| $screen = get_current_screen(); | ||
| if ($screen && (strpos($screen->id, 'wpuf') !== false || strpos($screen->base, 'wpuf') !== false)) { |
There was a problem hiding this comment.
instead of checking the current screen, can we use wpuf_load_post_forms hook? wpuf_load_post_forms hook fires only on wpuf post form page
includes/Admin/Posting.php
Outdated
| wp_enqueue_style( 'wpuf-admin-form-builder' ); | ||
| // Only load on WPUF admin pages to prevent CSS leaks | ||
| $screen = get_current_screen(); | ||
| if ($screen && (strpos($screen->id, 'wpuf') !== false || strpos($screen->base, 'wpuf') !== false)) { |
There was a problem hiding this comment.
instead of checking the current screen, can we use wpuf_load_post_forms hook? wpuf_load_post_forms hook fires only on wpuf post form page
sapayth
added
needs: author reply
arifulhoque7
added
needs: dev review
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
includes/Admin/Forms/Post/Templates/Form_Template.php (2)
36-51: LearnPress deregistration check is ineffective; it never matches the handle.
wp_list_pluck(..., 'src')yields numeric keys, soarray_key_exists('learn-press-admin', $sources)is always false. The conflicting style will not be removed on WPUF screens.Apply:
- $sources = wp_list_pluck( $wp_styles->registered, 'src' ); - - // Look for any pre-existing learn-press style - $result = array_key_exists( 'learn-press-admin', $sources ); - - if ( $result ) { - wp_deregister_style( 'learn-press-admin' ); - } + // Remove LearnPress admin CSS on WPUF screens only. + if ( wp_style_is( 'learn-press-admin', 'enqueued' ) ) { + wp_dequeue_style( 'learn-press-admin' ); + } + if ( wp_style_is( 'learn-press-admin', 'registered' ) ) { + wp_deregister_style( 'learn-press-admin' ); + }
58-72: Harden should_display() against null current screen.On some load paths custom hooks can fire before
get_current_screen()is ready. Accessing->idon null would fatal.- $current_screen = get_current_screen(); + if ( ! function_exists( 'get_current_screen' ) ) { + return false; + } + $current_screen = get_current_screen(); + if ( ! $current_screen ) { + return false; + }
♻️ Duplicate comments (1)
includes/Admin/Forms/Post/Templates/Form_Template.php (1)
15-16: Good switch to a page-scoped hook for enqueues.Moving enqueues to
wpuf_load_post_formsaligns with the goal of preventing global CSS leaks and matches prior feedback. Approved.
🧹 Nitpick comments (1)
includes/Admin/Forms/Post/Templates/Form_Template.php (1)
74-86: Redundant guard; the hook is already page-scoped.
enqueue_scripts()is hooked towpuf_load_post_forms, soshould_display()is unnecessary and can introduce a false negative ifget_current_screen()is unavailable at that moment.- if ( ! $this->should_display() ) { - return; - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
includes/Admin/Forms/Post/Templates/Form_Template.php(1 hunks)includes/Admin/Posting.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- includes/Admin/Posting.php
3 participants
Close #1693
Bug Fix: Prevent WPUF Form Builder CSS from affecting all WP Admin pages
Problem
The
wpuf-admin-form-builder.cssfile contained the@tailwind basedirective, which applies a global CSS reset (e.g.,margin: 0,padding: 0, border resets). Since the stylesheet was being enqueued globally, it caused visual inconsistencies and broke styling on unrelated WordPress admin pages.Root Cause
The CSS was enqueued unconditionally in:
includes/Admin/Posting.php:108→wp_enqueue_style('wpuf-admin-form-builder')includes/Admin/Forms/Post/Templates/Form_Template.php:85→ same enqueue callBecause these enqueue calls had no page restrictions, Tailwind’s reset styles leaked into every admin page, affecting WordPress core UI, plugins, and themes.
Investigation Notes
Solution
get_current_screen().$screen->idor$screen->basecontainswpuf).@tailwind basein the source file since scoping is safely handled at the enqueue level.Testing
✅ This resolves the global CSS leakage issue.
⚡️ Debugging this took much longer than a typical fix because the problem was deeply hidden and only revealed itself after exhaustive analysis.
Summary by CodeRabbit
New Features
Style
Bug Fixes
Refactor