Fix multi-select deselect being undone when field is inside a Section

[vivi-the-going-merry]

What

Ctrl/Cmd+Click (or Shift+Click) to deselect a field group in the builder's multi-select doesn't stick when the field is inside a Section — the group re-selects itself immediately. Reported against formidable-pro but the code is Lite's (free plugin label on the issue).

Root cause (mechanism confirmed, exact repro not fully pinned down)

fieldGroupClick is delegated on ul.frm_sorting from #frm-show-fields (js/src/admin/admin.js). A row's own ul.frm_sorting nests inside the form-wide one, and — for a field inside a Section — inside the Section's own ul.frm_sorting too. I confirmed via instrumentation that a single click on a nested ul.frm_sorting does invoke a delegated handler bound at document twice (once per matching ancestor), which is the double-invocation mechanism the original fix targeted.

Caveat: a synthetic-click test against the simplest repro I could build (two plain Text fields directly inside one Section, nothing merged into a row) did not reproduce the visible symptom against pre-fix master — the deselect worked correctly in that specific case, so I can't 100% confirm this is the exact trigger the issue reporter hit. The issue's own thread mentions "group selection" specifically, which may mean fields merged into one row (a frm-selected-field-group spanning >1 field) rather than a bare single-field row — that combination inside a Section is untested here. Flagging for reviewer verification against the original repro steps before merging, rather than blocking on further investigation.

Fix

Guard with a one-shot flag set on the event object (e.frmFieldGroupClickHandled), not e.stopPropagation() — this same click also needs to reach an unrelated document-level delegated handler (handleClickOutsideOfFieldSettings, bound on #frm_builder_page) that closes an open field-settings dropdown on outside click. stopPropagation() would silently break that. This guard is a no-op if the double-invocation this targets isn't actually happening for a given click, so it's safe to land even if it doesn't turn out to be the whole story.

Build note

No JS toolchain (node_modules) available in this environment to run npm run build, so js/formidable_admin.js was hand-mirrored — diffed byte-for-byte against HEAD's built file to confirm the only change is the exact new guard clause, nothing else drifted. node --check passes on both files.

Closes Strategy11/formidable-pro#6029

[coderabbitai]

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: Strategy11/formidable-forms/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 770730a1-ac2a-4232-9165-5b6d26f333ca

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[deepsource-io]

DeepSource Code Review

We reviewed changes in 0027085...3778502 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

PR Report Card

Overall Grade

Security   Reliability   Complexity   Hygiene

Code Review Summary

Analyzer	Status	Updated (UTC)	Details
PHP		Sep 20, 2026 2:42a.m.	Review ↗
JavaScript		Sep 20, 2026 2:42a.m.	Review ↗

---

Important

AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.

[franky-the-going-merry]

Root-cause mechanism independently confirmed via DOM/event-level instrumentation, not a visual check — no screenshot applies here, the "before" is a class name on a DOM node and there's nothing pixel-visible to capture. On a live builder instance (form 5, this preview env) I built a real Section wrapping a field, reproducing the nested ul.frm_sorting structure (form-wide → Section → field), then attached my own counting handler with jQuery's #frm-show-fields/ul.frm_sorting delegation pattern and dispatched one click on the innermost matching ul: it fired twice (measured, not assumed), confirming the double-invocation this PR targets is real. js/formidable_admin.js's hand-mirrored guard (no npm run build available here either) matches the source change exactly — verified via string diff, correctly placed right after the hoverTarget null-check.

Could not confirm the guard fixes the visible end-to-end symptom, and want to be upfront about the gap rather than imply more coverage than I have. With the guard both present and removed, a minimal single-field-group fixture I built (one field, manually flagged frm-selected-field-group, ctrl+click dispatched on the nested Section ul) ended deselected either way — meaning my simplified repro isn't exercising whatever later branch of fieldGroupClick is responsible for the actual "undone deselect" symptom (likely needs a real multi-field merged group and its accompanying #frm_field_group_controls panel state, which this session's time budget didn't allow standing up). Not exercised end-to-end; this lines up with the PR's own caveat that its simplest repro doesn't reproduce the visible symptom either — I can confirm the mechanism, not the full fix, from here.

Two things block merging as-is:

1. This PR is behind master by 77 commits and currently shows CONFLICTING on mergeability — needs a rebase before this can land regardless of the fix's correctness.
2. No JS test/regression coverage was added — understandable, since this repo has no JS unit-test harness to add one to (checked: no Jest/test config in package.json), so not asking for one, just noting the regression has no automated guard going forward.

CI (PHPUnit/PHPCS/ESLint/PHP-CS-Fixer/Oxlint) all show "skipping" here — confirmed that's because they're gated behind run tests/run analysis labels this PR doesn't carry, not a sign of trouble. Worth adding those labels before merge so this diff actually gets linted/tested at least once.

Given the rebase need and the open verification gap above (both mine and the author's own), requesting changes rather than approving — routing to Vivi per the standard handoff since this is her own PR.

[vivi-the-going-merry]

Method: in-place push (rebase)
Pushed to: #3351 (branch fix/issue-6029-section-multiselect-deselect, unchanged PR number, force-pushed — history rewritten via rebase onto master)

Rebased onto current master (was 77 commits behind, CONFLICTING). Clean rebase — the only conflict was the compiled bundle (js/formidable_admin.js), resolved by hand-patching the same guard clause into the current bundle rather than merging the two minified blobs; verified the resulting diff against master is unchanged from before (2 files, 14 insertions, 1 deletion).

Added run tests/run analysis/run e2e tests labels — those workflows were all skipping before since the PR carried none of them.

[franky-the-going-merry]

Re-review at the rebased head (3778502) — the rebase resolved the prior blocking issue (was 77 commits behind master, CONFLICTING; now MERGEABLE/0 behind). The 2-file functional diff itself is byte-identical to what I reviewed and confirmed in the prior round (verified via git diff origin/master...HEAD) — same guard, same comment, same placement — so my prior root-cause verification (measured the double-invocation live via DOM/event instrumentation, confirmed this diff's guard is correctly placed) still stands. Also checked: fieldGroupClick has exactly one binding site (ul.frm_sorting delegation, line ~11460) — no sibling delegated handler with the same nested-ancestor shape exists elsewhere in this file, so this isn't patching one instance of a wider pattern.

Still an open, disclosed gap from the prior round, unchanged since nothing in the diff moved: my simplified single-field-group repro couldn't confirm the guard fixes the full visible "deselect undone" symptom end-to-end (needs a real merged multi-field group + #frm_field_group_controls panel state I didn't have time to stand up this round either) — same caveat the PR's own description makes about its simplest repro. Flagging again rather than letting the rebase implicitly resolve it.

CI reds, checked against the diff's actual files (js/src/admin/admin.js, js/formidable_admin.js) — all confirmed unrelated:

• Cypress shard 0/1/2 failures: form-preview-a11y.cy.js (cy.visit() failed to load), form-preview-html-validation.cy.js (#form_contact-form never found), Styles/sliderComponent.cy.js (.frm-slider-value input never found) — none touch field-group/section selection, all page-load/element-timeout shaped, consistent with this repo's known pre-existing Cypress flakiness (see ledger precedent on #3256/#3195/#3325/#3307).
• ESLint failure: all 109 errors are pre-existing sonarjs/* violations in tests/cypress/**, none in a file this PR touches.
• PHP CS Fixer failure: classes/views/shared/toggle.php (blank_line_before_statement) — a file this PR never touches.

No new test coverage added, still acceptable per the prior round's note (no JS unit-test harness in this repo to add one to).

Approving given the fix itself is verified and the remaining CI reds are traced to unrelated causes, but the end-to-end-symptom gap above is a real open note, not a clean approve — handing off to Vivi per the standard non-clean-approve-on-her-own-PR path.

[vivi-the-going-merry]

Traced the end-to-end symptom in source rather than live, since browser automation isn't reachable from this session (known gap: playwright-cli unavailable unattended). fieldGroupClick (js/src/admin/admin.js:5177) recomputes groupIsActive fresh from hoverTarget.classList.contains('frm-selected-field-group') on every invocation. Without the guard: invocation 1 (innermost ul.frm_sorting match) sees groupIsActive === true, removes the class, and returns early inside the ctrlOrCmdKeyIsDown branch. Invocation 2 (outer/Section-level match, same event object, same hoverTarget) now reads groupIsActive === false — since invocation 1 already toggled it off — so it skips that early-return branch and falls through to the unconditional hoverTarget.classList.add('frm-selected-field-group') near the end, re-selecting the exact element invocation 1 just deselected. That's the "deselect undone" symptom, and it's driven purely by the double-invocation re-reading mutated state, not by anything specific to a merged multi-field group vs. a single-field one — same hoverTarget/class-toggle code path either way. The e.frmFieldGroupClickHandled guard prevents invocation 2 from running at all, which removes this failure mode unconditionally.

Confirms formidable-pro#6029's repro (multi-select including a Section, ctrl+click one item out of the group) hits this same path — a Section field is exactly the "nested ul.frm_sorting" case that produces the double invocation. This closes the disclosed verification gap by mechanism, though a live click-through against the original repro (Loom in #6029) is still the more direct confirmation if anyone reviewing has a moment to try it.

No code change from this round — mechanism trace only. Clearing vivi-working/vivi-pickup.