Use Bootstrap 5#2550
Conversation
WalkthroughMigrate admin UI assets and markup toward Bootstrap 5: update Popper/Bootstrap script registrations and handles, rename Popper helper methods, adjust script dependencies and versions, convert dropdown/tooltip markup and JS to data-bs-*, update multiselect templates, duplicate tooltip CSS block, and update Cypress selectors/flows. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User / DOM
participant AdminJS as admin JS (js/src/admin/admin.js)
participant Bootstrap as Bootstrap 5
rect rgb(245,250,255)
Note over U,AdminJS: Tooltip on-demand flow
U->>AdminJS: mouseenter / request tooltip
AdminJS->>AdminJS: loadTooltip(element, true)
AdminJS->>Bootstrap: init Tooltip (attach/move to parent if needed)
Bootstrap-->>U: show tooltip
end
rect rgb(245,250,255)
Note over U,AdminJS: Dropdown conversion on load
U->>AdminJS: existing .frm-dropdown-menu elements
AdminJS->>AdminJS: convertOldBootstrapDropdownsToBootstrap5()
AdminJS->>U: add `dropdown-menu`, update to `data-bs-*`
end
rect rgb(255,245,245)
Note over Bootstrap,AdminJS: Dropdown show cleanup
Bootstrap->>AdminJS: show.bs.dropdown event
AdminJS->>AdminJS: delete/hide lingering tooltips
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Potential spots needing extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 6
🧹 Nitpick comments (1)
resources/scss/admin/components/select/_dropdown.scss (1)
54-54: Consider removing redundant min-width declaration.The change from
float: lefttomin-width: 10remaligns with Bootstrap 5's layout model. However, Line 73 already declaresmin-width: 160px(equivalent to 10rem), making this new declaration redundant.Consider removing the duplicate declaration:
.frm_wrap .ui-autocomplete, #frm_adv_info.frm-dropdown-menu, .frm-dropdown-menu { /* Copied from .dropdown-menu rules from Bootstrap */ position: absolute; top: 100%; left: 0; z-index: 1000; display: none; - min-width: 10rem; padding-left: 4px; padding-right: 4px; margin: var(--gap-xs) 0 0; text-align: left; list-style: none; background: none; background-color: #fff; background-clip: padding-box; border: 1px solid var(--grey-100); border-radius: 0.25rem; box-shadow: var(--box-shadow-lg); /* Custom rules */ max-height: 200px; overflow-y: auto; overflow-x: hidden; font-size: var(--text-md); width: auto; min-width: 160px; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
js/addons-page.js.mapis excluded by!**/*.map,!**/*.mapjs/bootstrap.min.jsis excluded by!**/*.min.js,!**/*.min.jsjs/form-templates.js.mapis excluded by!**/*.map,!**/*.mapjs/formidable-settings-components.js.mapis excluded by!**/*.map,!**/*.mapjs/formidable_admin.js.mapis excluded by!**/*.map,!**/*.mapjs/formidable_blocks.js.mapis excluded by!**/*.map,!**/*.mapjs/formidable_dashboard.js.mapis excluded by!**/*.map,!**/*.mapjs/formidable_overlay.js.mapis excluded by!**/*.map,!**/*.mapjs/formidable_styles.js.mapis excluded by!**/*.map,!**/*.mapjs/onboarding-wizard.js.mapis excluded by!**/*.map,!**/*.mapjs/popper.min.jsis excluded by!**/*.min.js,!**/*.min.js
📒 Files selected for processing (8)
classes/controllers/FrmAppController.php(5 hunks)classes/controllers/FrmTestModeController.php(1 hunks)classes/views/frm-forms/add_field.php(1 hunks)css/frm_testing_mode.css(1 hunks)js/admin/dom.js(2 hunks)js/admin/style.js(1 hunks)js/src/admin/admin.js(8 hunks)resources/scss/admin/components/select/_dropdown.scss(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
classes/controllers/FrmAppController.php (2)
classes/helpers/FrmAppHelper.php (2)
plugin_url(65-68)FrmAppHelper(6-4628)stripe/helpers/FrmTransLiteAppHelper.php (1)
plugin_url(23-25)
classes/controllers/FrmTestModeController.php (1)
classes/helpers/FrmAppHelper.php (1)
FrmAppHelper(6-4628)
js/src/admin/admin.js (2)
js/src/frm_testing_mode.js (1)
toggle(37-37)js/admin/style.js (1)
dropdownMenu(684-688)
🪛 Biome (2.1.2)
css/frm_testing_mode.css
[error] 3-3: Unexpected shorthand property background after background-color
(lint/suspicious/noShorthandPropertyOverrides)
[error] 3-3: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
text-align is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
[error] 3-3: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
font-size is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
[error] 3-3: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
border-radius is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
🪛 GitHub Check: Run ESLint
js/src/admin/admin.js
[warning] 460-460:
Expected JSDoc block lines to be aligned
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cypress
🔇 Additional comments (6)
js/admin/style.js (1)
636-637: LGTM! Bootstrap 5 attribute migration.The update from
data-toggleanddata-containertodata-bs-toggleanddata-bs-containercorrectly aligns with Bootstrap 5's namespaced data attributes.classes/controllers/FrmTestModeController.php (1)
368-370: LGTM! Script dependencies updated for Bootstrap 5.The script registrations are correctly updated:
- Popper.js upgraded from v1.16.0 to v2.11.8 with jQuery dependency removed (Popper v2 is standalone)
- Bootstrap upgraded from v4.6.1 to v5.0.2 with proper Popper v2 dependency
- bootstrap-multiselect updated to v2.0 for Bootstrap 5 compatibility
These changes correctly align with Bootstrap 5's dependency requirements.
js/admin/dom.js (2)
154-156: LGTM! Multiselect templates updated for Bootstrap 5.The template updates correctly implement Bootstrap 5 requirements:
popupContainernow includes thedropdown-menuclassbuttontemplate usesdata-bs-toggleinstead ofdata-toggle
208-211: Unable to verify Pro compatibility—Pro plugin code not accessible in repository.The Pro plugin directory is not available in the cloned repository, so I cannot verify whether Pro code relies on the
setupBootstrapDropdownsfunction's callback parameter or side effects. You'll need to manually verify this against your Pro codebase before merging to ensure no breakage.classes/controllers/FrmAppController.php (2)
717-717: LGTM! Correct bootstrap-multiselect upgrade.The upgrade to bootstrap-multiselect v2.0 is correct for Bootstrap 5 compatibility, with appropriate dependencies on the updated Bootstrap and Popper versions.
739-739: LGTM! Correct deregistration calls.The calls to
maybe_deregister_popper1()are correctly placed to ensure outdated Popper v1 is replaced with v2 before dependent scripts are enqueued.Also applies to: 890-890
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
css/frm_testing_mode.css (1)
2-3: Remove duplicate tooltip CSS block (reiterating previous review comment).This matches the issue flagged in the prior review: the Bootstrap tooltip CSS block (
.tooltip,.bs-tooltip-*, and related selectors) is duplicated in this file. The stray closing comment token*/on Line 3 signals incomplete cleanup of the Bootstrap 4 tooltip CSS during the Bootstrap 5 migration.The duplication unnecessarily inflates file size and creates property conflicts flagged by Biome (duplicate
text-align,border-radius,background-colorrules, etc.).Remove the duplicate tooltip CSS block so only one instance of each tooltip rule remains. If this file is intended to include both Bootstrap 4 and Bootstrap 5 tooltip definitions during a transitional period, clarify the strategy and ensure property names and selectors do not conflict. Otherwise, replace the old Bootstrap 4 tooltip rules entirely with their Bootstrap 5 equivalents.
js/src/admin/admin.js (1)
473-475: Nice: removing the child’s title prevents double tooltips.This resolves the “native + Bootstrap tooltip overlap” noted previously.
🧹 Nitpick comments (4)
js/src/admin/admin.js (4)
1415-1426: Use BS5 detection for initialized tooltips.Checking data-original-title is a BS4 pattern. In BS5 it’s data-bs-original-title or better use bootstrap.Tooltip.getInstance.
- element.addEventListener( - 'mouseover', - function() { - if ( null === element.getAttribute( 'data-original-title' ) ) { - jQuery( element ).tooltip(); - } - } - ); + element.addEventListener( + 'mouseover', + function() { + if ( window.bootstrap && window.bootstrap.Tooltip ) { + if ( ! window.bootstrap.Tooltip.getInstance( element ) ) { + new window.bootstrap.Tooltip( element, { container: 'body' } ); + } + return; + } + if ( jQuery.fn && jQuery.fn.tooltip ) { + // BS4 compatibility. + if ( null === element.getAttribute( 'data-original-title' ) && null === element.getAttribute( 'data-bs-original-title' ) ) { + jQuery( element ).tooltip({ container: 'body' }); + } + } + } + );
2782-2785: Also remove legacy data-toggle after mapping to data-bs-toggle.Prevents mixed attributes and future ambiguity.
- field.querySelectorAll( '[data-toggle]' ).forEach( toggle => toggle.setAttribute( 'data-bs-toggle', toggle.getAttribute( 'data-toggle' ) ) ); + field.querySelectorAll( '[data-toggle]' ).forEach( toggle => { + toggle.setAttribute( 'data-bs-toggle', toggle.getAttribute( 'data-toggle' ) ); + toggle.removeAttribute( 'data-toggle' ); + } );
11101-11103: Limit global data-toggle → data-bs-toggle conversion to known Bootstrap plugins.This currently converts every [data-toggle] attribute. Guard against unexpected app-specific uses.
- document.querySelectorAll( '[data-toggle]' ).forEach( toggle => toggle.setAttribute( 'data-bs-toggle', toggle.getAttribute( 'data-toggle' ) ) ); + document.querySelectorAll( '[data-toggle]' ).forEach( toggle => { + const v = toggle.getAttribute( 'data-toggle' ); + // Allowlist common BS values. + if ( [ 'dropdown', 'tooltip', 'popover', 'collapse', 'modal', 'tab' ].includes( v ) ) { + toggle.setAttribute( 'data-bs-toggle', v ); + } + } );
11123-11131: Avoid outerHTML string replaces to convert UL/LI; preserve listeners and data.Replacing outerHTML can drop event handlers and data. Build a new container and move children instead.
- function convertBootstrapUl( ul ) { - let html = ul.outerHTML; - html = html.replace( '<ul ', '<div ' ); - html = html.replace( '</ul>', '</div>' ); - html = html.replaceAll( '<li>', '<div class="dropdown-item">' ); - html = html.replaceAll( '<li class="', '<div class="dropdown-item ' ); - html = html.replaceAll( '</li>', '</div>' ); - ul.outerHTML = html; - } + function convertBootstrapUl( ul ) { + const wrapper = document.createElement( 'div' ); + // Copy classes/attrs from UL to DIV. + wrapper.className = ul.className; + [ ...ul.attributes ].forEach( a => { + if ( a.name !== 'class' ) { + wrapper.setAttribute( a.name, a.value ); + } + } ); + // Move children, wrapping LI as .dropdown-item DIVs. + while ( ul.firstChild ) { + const child = ul.firstChild; + if ( child.nodeType === Node.ELEMENT_NODE && child.tagName === 'LI' ) { + const item = document.createElement( 'div' ); + item.className = 'dropdown-item ' + child.className; + // Move li children into item. + while ( child.firstChild ) { + item.appendChild( child.firstChild ); + } + wrapper.appendChild( item ); + child.remove(); + } else { + wrapper.appendChild( child ); + } + } + ul.replaceWith( wrapper ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
css/frm_testing_mode.css(1 hunks)js/src/admin/admin.js(8 hunks)resources/scss/admin/components/select/_dropdown.scss(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/scss/admin/components/select/_dropdown.scss
🧰 Additional context used
🧬 Code graph analysis (1)
js/src/admin/admin.js (2)
js/src/frm_testing_mode.js (1)
toggle(37-37)js/admin/style.js (1)
dropdownMenu(684-688)
🪛 Biome (2.1.2)
css/frm_testing_mode.css
[error] 3-3: Unexpected shorthand property background after background-color
(lint/suspicious/noShorthandPropertyOverrides)
[error] 3-3: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
text-align is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
[error] 3-3: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
min-width is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
[error] 3-3: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
border-radius is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
🪛 GitHub Check: Run ESLint
js/src/admin/admin.js
[warning] 460-460:
Expected JSDoc block lines to be aligned
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Run PHP Syntax inspection (8.3)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (2)
js/src/admin/admin.js (2)
10157-10161: Good call: clearing lingering tooltips on dropdown show.Prevents stuck tooltips when menus open.
480-485: <!-- [scratchpad_update]
[task verify_tooltip_usage]
[observations]
- Bootstrap 5 is designed to work WITHOUT jQuery (index 2-10)
- Bootstrap 5 provides native JS API: window.bootstrap.Tooltip (index 1-12, 4-18)
- However, if jQuery is present, BS5 adds components to jQuery.fn automatically (index 2-24)
- So jQuery tooltip syntax still works IF jQuery is loaded AFTER Bootstrap
- The review comment mentions concern about jQuery.fn.tooltip not being available
[analysis]- Bootstrap 5 JS tooltip API is available as new window.bootstrap.Tooltip()
- If jQuery is present AND loaded after Bootstrap, jQuery.fn.tooltip() also works (index 2-24: "If Bootstrap detects jQuery in the window object it'll add all of our components in jQuery's plugin system")
- The review concern is technically valid but depends on load order
- Need to check the actual code in admin.js to see current implementation
[pending]- Get lines 480-485 from admin.js
- Check if jQuery is loaded
- Verify current tooltip implementation
[scratchpad_end] --><!-- [scratchpad_update]
[task verify_tooltip_usage]
[observations]- KEY FINDING from index 12-7, 12-32, 20-32: Bootstrap 5 DETECTS jQuery and ADDS components to jQuery.fn automatically
- Bootstrap will detect jQuery if present in window object and adds tooltip() method
- However, Bootstrap doesn't "drop" jQuery tooltip - it just makes jQuery support optional
- If jQuery IS loaded after Bootstrap, $(...).tooltip() will work
- The review comment says "Bootstrap 5 drops jQuery tooltip plugin by default" - this is slightly misleading
- Precise statement: BS5 doesn't include jQuery plugin by default, but if jQuery is loaded, BS5 automatically adds components to jQuery.fn
[critical_question]- Need to examine actual admin.js code to see:
- Is jQuery loaded?
- What bootstrap version is loaded?
- How are tooltips currently being initialized?
[pending]- Get actual lines 480-485 from admin.js
- Check if the code is actually vulnerable or already working
[scratchpad_end] -->Now I need to examine the actual code in the file to see what's happening:<!-- [scratchpad_update]
[task verify_tooltip_usage]
[observations]
- Web searches haven't provided the actual admin.js file from the PR being reviewed
- But we have key information from Bootstrap 5 documentation:
- Bootstrap 5 DOES support jQuery tooltips automatically (index 12-7, 20-7)
- If jQuery is detected, BS5 adds components to jQuery.fn
- However, code calling jQuery.fn.tooltip() will fail if jQuery is NOT loaded or if it's loaded BEFORE Bootstrap
[critical_analysis]
The review comment's concern is NUANCED:- Bootstrap 5 didn't "drop" jQuery plugin - it made it optional
- jQuery tooltips work IF jQuery is loaded AND Bootstrap detects it
- The vulnerability exists IF:
- jQuery is not loaded, OR
- jQuery is loaded but NOT found by Bootstrap (data-bs-no-jquery set)
- Bootstrap is loaded before jQuery
- The proposed solution is defensive programming: check for both APIs before using
[conclusion]
- The review comment is TECHNICALLY CORRECT about the risk
- BUT the precise statement should be: "Bootstrap 5 doesn't guarantee jQuery plugin availability"
- NOT: "Bootstrap 5 drops jQuery tooltip plugin by default"
- However, the suggestion to be defensive IS VALID
- The code should check for availability before calling
[pending]
- Still need to check the actual file to see current implementation
- But we can give preliminary verdict: concern is valid, solution is defensive
[scratchpad_end] -->Based on my research, I can now provide the verification result. Bootstrap 5 is designed to be used without jQuery, but if Bootstrap detects jQuery in the window object it'll add all of our components in jQuery's plugin system; this means you'll be able to do $('[data-bs-toggle="tooltip"]').tooltip() to enable tooltips. However, this means the jQuery tooltip method is only available if jQuery is present and loaded in a specific order.
The review comment's concern is valid: relying on
jQuery(...).tooltip()without verification can fail if jQuery is not loaded, not detected, or loaded in the wrong order. The current code at lines 480-485 calls the tooltip method without checking availability, which creates a risk.However, the precise technical framing needs adjustment: Bootstrap 5 doesn't "drop" the jQuery plugin outright—rather, it makes jQuery support optional and conditional on jQuery's presence. The defensive approach suggested in the review is sound and aligns with modern Bootstrap 5 best practices.
Verify that the codebase has jQuery loaded and Bootstrap is properly initialized to use the jQuery tooltip() syntax, or refactor to use the native Bootstrap JS API.
The review comment's core concern is valid: the code should either verify jQuery availability before calling
.tooltip(), or migrate to the Bootstrap 5 JavaScript API (new window.bootstrap.Tooltip()). The proposed robust initialization pattern is a reasonable defensive solution, though it could be simplified depending on the actual jQuery availability in the codebase.
…y11/formidable-forms into update-tests-bootstrap-5
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
js/src/admin/admin.js (2)
1456-1456: Use Bootstrap 5 alignment class.Replace deprecated dropdown-menu-right with dropdown-menu-end.
- const ul = div( { - className: 'frm-dropdown-menu dropdown-menu dropdown-menu-right' - } ); + const ul = div( { + className: 'frm-dropdown-menu dropdown-menu dropdown-menu-end' + } );
11148-11156: Dropdown item semantics: don’t convertto non-actionable ; ensure .dropdown-item is on / and set role="menuitem".The string replace creates div.dropdown-item wrappers, which can break keyboard navigation and ARIA semantics. Transform the children instead and add roles.
- function convertBootstrapUl( ul ) { - let html = ul.outerHTML; - html = html.replace( '<ul ', '<div ' ); - html = html.replace( '</ul>', '</div>' ); - html = html.replaceAll( '<li>', '<div class="dropdown-item">' ); - html = html.replaceAll( '<li class="', '<div class="dropdown-item ' ); - html = html.replaceAll( '</li>', '</div>' ); - ul.outerHTML = html; - } + function convertBootstrapUl( ul ) { + const menu = document.createElement( 'div' ); + menu.className = ul.className; + menu.classList.add( 'dropdown-menu' ); + menu.setAttribute( 'role', ul.getAttribute( 'role' ) || 'menu' ); + + while ( ul.firstChild ) { + const li = ul.firstChild; + if ( li.nodeName === 'LI' ) { + const child = li.firstElementChild; + if ( child && ( child.tagName === 'A' || child.tagName === 'BUTTON' ) ) { + child.classList.add( 'dropdown-item' ); + child.setAttribute( 'role', 'menuitem' ); + menu.appendChild( child ); + } else { + const divItem = document.createElement( 'div' ); + divItem.className = 'dropdown-item'; + divItem.setAttribute( 'role', 'menuitem' ); + while ( li.firstChild ) { + divItem.appendChild( li.firstChild ); + } + menu.appendChild( divItem ); + } + li.remove(); + } else { + menu.appendChild( li ); + } + } + ul.replaceWith( menu ); + }
♻️ Duplicate comments (4)
classes/controllers/FrmAppController.php (2)
920-939: Function logic correctly identifies and deregisters Popper 1.The renamed function
maybe_deregister_popper1()(formerlymaybe_deregister_popper2()) now correctly checks for Popper versions less than 2.0 (line 935) and deregisters them before registering Popper 2.However, the
@sincetag on line 923 still shows "x.x" and should be updated to the actual release version.
941-953: Popper 2.11.8 registration is correct for Bootstrap 5.The renamed function
register_popper2()(formerlyregister_popper1()) correctly registers Popper 2.11.8, which is required for Bootstrap 5 compatibility.However, the
@sincetag on line 944 still shows "x.x" and should be updated to the actual release version.classes/views/frm-forms/add_field.php (1)
44-44: Complete the Bootstrap 5 attribute migration.This line mixes Bootstrap 4 attributes (
data-toggle="dropdown",data-container="body") with Bootstrap 5 attributes (data-bs-display="static"), which is inconsistent with the Bootstrap 5 upgrade. All data attributes should follow the Bootstrap 5 naming convention.js/src/admin/admin.js (1)
468-486: Reparenting is still too broad; restrict to dropdown triggers and set container on the new host.Currently any element with data(-bs)-toggle is treated as a dropdown trigger, which wrongly reparents pure tooltip triggers. Also ensure the parent gets data-bs-container="body" for consistency.
Apply this diff:
- function loadTooltip( element, show = false ) { - let tooltipTarget = element; - - // Bootstrap 5 does not allow tooltips on dropdown triggers, so move the tooltip to the parent element. - if ( tooltipTarget.hasAttribute( 'data-toggle' ) || tooltipTarget.hasAttribute( 'data-bs-toggle' ) ) { - tooltipTarget.parentElement.setAttribute( 'title', tooltipTarget.getAttribute( 'title' ) ); - tooltipTarget.removeAttribute( 'title' ); - tooltipTarget.classList.remove( 'frm_bstooltip' ); - tooltipTarget.parentElement.classList.add( 'frm_bstooltip' ); - tooltipTarget = tooltipTarget.parentElement; - } - - jQuery( tooltipTarget ).tooltip(); - - if ( show ) { - deleteTooltips(); - jQuery( tooltipTarget ).tooltip( 'show' ); - } - } + function loadTooltip( element, show = false ) { + let tooltipTarget = element; + + // Bootstrap 5 does not allow tooltips on dropdown triggers; move tooltip to the parent only for dropdowns. + const toggleAttr = tooltipTarget.getAttribute( 'data-bs-toggle' ) || tooltipTarget.getAttribute( 'data-toggle' ); + const isDropdownTrigger = tooltipTarget.classList.contains( 'dropdown-toggle' ) || toggleAttr === 'dropdown'; + if ( isDropdownTrigger ) { + const parent = tooltipTarget.parentElement; + if ( parent ) { + parent.setAttribute( 'title', tooltipTarget.getAttribute( 'title' ) || '' ); + tooltipTarget.removeAttribute( 'title' ); + tooltipTarget.classList.remove( 'frm_bstooltip' ); + parent.classList.add( 'frm_bstooltip' ); + if ( ! parent.hasAttribute( 'data-bs-container' ) ) { + parent.setAttribute( 'data-bs-container', 'body' ); + } + tooltipTarget = parent; + } + } + + jQuery( tooltipTarget ).tooltip(); + + if ( show ) { + deleteTooltips(); + jQuery( tooltipTarget ).tooltip( 'show' ); + } + }
🧹 Nitpick comments (3)
js/src/admin/admin.js (3)
2794-2797: Also remove legacy data-toggle after copying to data-bs-toggle.Prevents future mis-detection and keeps markup clean.
- field.querySelectorAll( '[data-toggle]' ).forEach( toggle => toggle.setAttribute( 'data-bs-toggle', toggle.getAttribute( 'data-toggle' ) ) ); + field.querySelectorAll( '[data-toggle]' ).forEach( toggle => { + toggle.setAttribute( 'data-bs-toggle', toggle.getAttribute( 'data-toggle' ) ); + toggle.removeAttribute( 'data-toggle' ); + } );
10182-10186: Also clear tooltips on hidden.bs.dropdown.Covers the case where a dropdown is closed without opening another one.
document.addEventListener( 'show.bs.dropdown', function() { // Fixes issues with tooltips lingering after a dropdown is shown. deleteTooltips(); } ); + document.addEventListener( 'hidden.bs.dropdown', function() { + deleteTooltips(); + } );
11126-11128: Remove legacy data-toggle after migration.Same rationale as above; avoids stale attributes.
- document.querySelectorAll( '[data-toggle]' ).forEach( toggle => toggle.setAttribute( 'data-bs-toggle', toggle.getAttribute( 'data-toggle' ) ) ); + document.querySelectorAll( '[data-toggle]' ).forEach( toggle => { + toggle.setAttribute( 'data-bs-toggle', toggle.getAttribute( 'data-toggle' ) ); + toggle.removeAttribute( 'data-toggle' ); + } );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
classes/controllers/FrmAppController.php(5 hunks)classes/views/frm-forms/add_field.php(1 hunks)js/src/admin/admin.js(8 hunks)tests/cypress/e2e/Entries/EntriesPageDataValidations.cy.js(1 hunks)tests/cypress/e2e/Forms/fieldsInFormBuilder.cy.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
classes/controllers/FrmAppController.php (1)
classes/helpers/FrmAppHelper.php (2)
plugin_url(65-68)FrmAppHelper(6-4664)
js/src/admin/admin.js (2)
js/src/frm_testing_mode.js (2)
element(49-49)toggle(37-37)js/admin/style.js (1)
dropdownMenu(684-688)
🪛 GitHub Check: Run ESLint
js/src/admin/admin.js
[warning] 460-460:
Expected JSDoc block lines to be aligned
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (6)
tests/cypress/e2e/Entries/EntriesPageDataValidations.cy.js (1)
76-76: Selector change correctly updated for Bootstrap 5 migration.Verification confirms the change at line 76 is correct. The selector now uses the standard HTML
titleattribute instead of Bootstrap 4'sdata-original-title, which aligns with Bootstrap 5. No remainingdata-original-titlereferences exist in the test suite, indicating the migration is complete.tests/cypress/e2e/Forms/fieldsInFormBuilder.cy.js (2)
24-38: The updated dropdown interaction pattern aligns with Bootstrap 5 behavior.The test now uses a hover-based approach to open the dropdown menu (lines 24-25), clicks the dropdown icon (lines 27-28), then clicks the delete menu item (lines 30-32), and finally confirms deletion (lines 35-38). This multi-step interaction pattern is appropriate for Bootstrap 5 dropdowns, which require explicit user actions to display menu items.
256-256: Timeout increase is appropriate for Bootstrap 5.The timeout increased from 5000ms to 10000ms, which provides additional buffer for Bootstrap 5 rendering and animations that may take longer than Bootstrap 4.
classes/controllers/FrmAppController.php (3)
677-678: Bootstrap 5 and Popper 2 registration is correct.The code correctly registers Popper 2 (line 677) and updates Bootstrap from 4.6.1 to 5.0.2 (line 678), which is required for the Bootstrap 5 migration.
717-717: Bootstrap-multiselect upgraded to version 2.0.The upgrade from bootstrap-multiselect 1.1.1 to 2.0 aligns with the Bootstrap 5 migration, and the updated dependencies (including
popper) are correct.
739-739: Deregistration logic correctly targets Popper 1.The function calls to
maybe_deregister_popper1()are appropriately placed to ensure that outdated Popper 1 installations are removed before loading Bootstrap 5-compatible assets.Also applies to: 890-890
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
classes/controllers/FrmAppController.php (1)
675-718: Bootstrap 5 migration incomplete: 6 files still usedata-toggle="dropdown"instead ofdata-bs-toggle.The verification found remaining Bootstrap 4-era patterns that will break with the registered Bootstrap 5.0.2 bundle:
classes/helpers/FrmStylesHelper.php:179classes/helpers/FrmFormsHelper.php:147classes/views/frm-forms/add_field.php:44classes/views/frm-forms/actions-dropdown.php:8classes/views/frm-forms/add_field_links.php:103classes/views/onboarding-wizard/steps/consent-tracking-step.php:38All instances use
data-toggle="dropdown"(Bootstrap 4) and must be updated todata-bs-toggle="dropdown"(Bootstrap 5) for the dropdowns to function after the bundle switch.
♻️ Duplicate comments (2)
classes/controllers/FrmAppController.php (1)
896-901: Popper helper logic is correct; fill in real@sinceversions.
maybe_deregister_popper1()safely checks the registeredpopperhandle and replaces anything older than v2.0 with your own Popper 2.11.8 viaregister_popper2(), and reusesshould_register_popper()so Popper stays scoped to Formidable screens. The control flow itself looks good.The remaining issue is documentation: both new helpers still have
@since x.xplaceholders in their docblocks. Please update those to the actual release version before merging to keep API history accurate.Also applies to: 929-962
js/src/admin/admin.js (1)
460-486: Limit tooltip reparenting to actual dropdown toggles; also resolve the JSDoc lint warning
- The reparenting condition in
loadTooltipcurrently fires for any element withdata-toggleordata-bs-toggle, which includes pure tooltip and other toggles (e.g. collapse). That will incorrectly move non‑dropdown tooltips to the parent and can break their behavior. This was already flagged in a past review and is still present. Restrict this logic to real dropdown triggers (e.g.data-*-toggle === 'dropdown'or.dropdown-toggle), and guard against missing parents. After reparenting, it’s also safer to ensure the new host has adata-bs-containerset if you rely on body‑scoped tooltips. For example:- // Bootstrap 5 does not allow tooltips on dropdown triggers, so move the tooltip to the parent element. - if ( tooltipTarget.hasAttribute( 'data-toggle' ) || tooltipTarget.hasAttribute( 'data-bs-toggle' ) ) { - tooltipTarget.parentElement.setAttribute( 'title', tooltipTarget.getAttribute( 'title' ) ); - tooltipTarget.removeAttribute( 'title' ); - tooltipTarget.classList.remove( 'frm_bstooltip' ); - tooltipTarget.parentElement.classList.add( 'frm_bstooltip' ); - tooltipTarget = tooltipTarget.parentElement; - } + // Bootstrap 5 does not allow tooltips on dropdown triggers, so move the tooltip to the parent element. + const toggleAttr = tooltipTarget.getAttribute( 'data-bs-toggle' ) || tooltipTarget.getAttribute( 'data-toggle' ); + const isDropdownTrigger = tooltipTarget.classList.contains( 'dropdown-toggle' ) || toggleAttr === 'dropdown'; + if ( isDropdownTrigger && tooltipTarget.parentElement ) { + const title = tooltipTarget.getAttribute( 'title' ); + if ( title ) { + tooltipTarget.parentElement.setAttribute( 'title', title ); + tooltipTarget.removeAttribute( 'title' ); + } + tooltipTarget.classList.remove( 'frm_bstooltip' ); + tooltipTarget.parentElement.classList.add( 'frm_bstooltip' ); + if ( ! tooltipTarget.parentElement.hasAttribute( 'data-bs-container' ) ) { + tooltipTarget.parentElement.setAttribute( 'data-bs-container', 'body' ); + } + tooltipTarget = tooltipTarget.parentElement; + }
- ESLint is still complaining about “Expected JSDoc block lines to be aligned” at this block. Please run the linter and adjust the
/** ... */indentation (and replace@since x.xwith the concrete version) so it passes.Also applies to: 505-508
🧹 Nitpick comments (4)
css/frm_testing_mode.css (1)
2-3: Verify tooltip CSS compatibility with Bootstrap 5 and clean up duplicate properties.The tooltip block here is explicitly Bootstrap 4.6.1 (
.tooltip,.bs-tooltip-*,[x-placement^=…]) while JS is now using Bootstrap 5/Popper 2. Bootstrap 5’s compiled CSS targetsdata-popper-placement, so please double‑check this block is still correct for how tooltips are initialized now, or update it to the Bootstrap 5 tooltip CSS so behavior stays in sync.Also, within the combined
.frm_wrap .ui-autocomplete,#frm_adv_info.frm-dropdown-menu,.frm-dropdown-menu{…}rule there are duplicate/overridden declarations (backgroundvsbackground-color, twomin-widthvalues, and repeated properties liketext-align/border-radius). It would be good to remove the earlier values or consolidate them so the linter errors go away and future maintainers aren’t surprised by silently overridden properties.js/admin/dom.js (1)
207-213: Consider markingsetupBootstrapDropdownsas deprecated now that it’s a no‑op.Leaving a no‑op
setupBootstrapDropdowns()keeps Pro from breaking, which is good, but it may confuse callers who still expect it to wire dropdowns. Adding a short@deprecated‑style comment with the version and new preferred entry point (e.g., where dropdowns are now initialized) would make this transition clearer for downstream code.js/src/admin/admin.js (2)
1415-1417: Make tooltip init guard compatible with Bootstrap 5’s data attributesIn
addTooltip, the guard still checks onlydata-original-title:if ( null === element.getAttribute( 'data-original-title' ) ) { jQuery( element ).tooltip(); }Bootstrap 5’s tooltip implementation commonly uses
data-bs-original-titleinstead, so this condition may never become false andjQuery( element ).tooltip()could be called on every mouseover. That risks repeated initialization and unexpected behavior.Consider updating the guard to handle both attributes (or to check the plugin instance instead), e.g.:
- function() { - if ( null === element.getAttribute( 'data-original-title' ) ) { - jQuery( element ).tooltip(); - } - } + function() { + if ( + ! element.getAttribute( 'data-original-title' ) && + ! element.getAttribute( 'data-bs-original-title' ) + ) { + jQuery( element ).tooltip(); + } + }Also applies to: 1443-1451
2574-2575: Scope field-action dropdown menu lookup to the specific dropdown being shown
onFieldActionDropdownShownow finds the menu via:ul = document.querySelector( '.dropdown .frm-dropdown-menu.show' );This always returns the first matching open menu in the document. If more than one
.dropdown .frm-dropdown-menu.showever exists at once (for example, another admin dropdown using the same classes), this code can end up labeling/offsetting the wrong menu.Since this handler is wired to
show.bs.dropdownfor field action dropdowns, it would be more robust to scope the query to the dropdown associated with the event (orthis), falling back to the global query only if needed.Also applies to: 2580-2606
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
js/addons-page.js.mapis excluded by!**/*.map,!**/*.mapjs/formidable_overlay.js.mapis excluded by!**/*.map,!**/*.mapjs/formidable_styles.js.mapis excluded by!**/*.map,!**/*.mapjs/onboarding-wizard.js.mapis excluded by!**/*.map,!**/*.map
📒 Files selected for processing (5)
classes/controllers/FrmAppController.php(5 hunks)css/frm_testing_mode.css(1 hunks)js/admin/dom.js(2 hunks)js/src/admin/admin.js(8 hunks)tests/cypress/e2e/Entries/EntriesPageDataValidations.cy.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/cypress/e2e/Entries/EntriesPageDataValidations.cy.js
🧰 Additional context used
🧬 Code graph analysis (2)
js/src/admin/admin.js (2)
js/src/frm_testing_mode.js (2)
element(49-49)toggle(37-37)js/admin/style.js (1)
dropdownMenu(684-688)
classes/controllers/FrmAppController.php (1)
classes/helpers/FrmAppHelper.php (2)
plugin_url(65-68)FrmAppHelper(6-4666)
🪛 Biome (2.1.2)
css/frm_testing_mode.css
[error] 3-3: Unexpected shorthand property background after background-color
(lint/suspicious/noShorthandPropertyOverrides)
[error] 3-3: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
text-align is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
[error] 3-3: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
min-width is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
[error] 3-3: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
border-radius is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
🪛 GitHub Check: Run ESLint
js/src/admin/admin.js
[warning] 460-460:
Expected JSDoc block lines to be aligned
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (3)
js/admin/dom.js (1)
152-167: Multiselect templates look Bootstrap 5‑ready; keep an eye on plugin expectations.The updated templates (extra
dropdown-menuonpopupContaineranddata-bs-toggle="dropdown"on the button) are consistent with Bootstrap 5 dropdowns and should work with bootstrap‑multiselect 2.0, assuming its templates are still override‑compatible.I’d suggest quickly verifying against the current bootstrap‑multiselect 2.0 docs/demos that these class names and attributes match their expected structure, especially if the plugin changed any default hooks between v1 and v2.
js/src/admin/admin.js (2)
2794-2797: LGTM: normalizing new field dropdowns to Bootstrap 5Converting any
[data-toggle]within a newly added field todata-bs-toggleand ensuring.frm-dropdown-menuelements also carry thedropdown-menuclass is a good safety net for mixed legacy/new markup when fields are inserted via AJAX.
10203-10207: Centralized tooltip cleanup on dropdown showHooking a global
show.bs.dropdownlistener to calldeleteTooltips()is a sensible way to prevent lingering tooltips once any Bootstrap dropdown is opened. This should eliminate the “stuck tooltip over dropdown” issue without coupling to specific dropdown instances.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/cypress/e2e/Applications/validateApplicationsPage.cy.js(3 hunks)tests/cypress/e2e/Forms/formPageDataValidation.cy.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (2)
tests/cypress/e2e/Applications/validateApplicationsPage.cy.js (2)
78-81: LGTM - Selector updates are consistent.The three selector updates from
c-template-required-plan__texttoc-template-info__textare applied consistently across all affected test cases (Business Directory, Letter of Recommendation, and Testimonials). The test logic and assertions remain unchanged, which is appropriate.Also applies to: 206-209, 323-326
78-81: Confirm external site deployment before merging; selector changes are otherwise correct.The selector update from
c-template-required-plan__texttoc-template-info__textat lines 78, 206, and 323 is isolated and consistent. However, these tests depend on formidableforms.com already having the updated class names deployed. If the external site hasn't been updated, these tests will fail.Please confirm the formidableforms.com site has been deployed with the new
c-template-info__textclass before merging this PR.
2 participants
Currently we're on Bootstrap 4.x, Popper 1.x, and Bootstrap Multiselect 1.x.
This update uses Bootstrap 5.x, Popper 2.x, and Bootstrap Multiselect 2.0.
This update touches a fair bit of admin functionality that we want to test.
We really use Bootstrap for a few things.
Or the ones used for field actions.
And the Permissions in Global Settings.
Things to look out for