fix(js-core): use closest() fallback for nested click target matching#7327
Conversation
When a user clicks a child element inside a button or div matched by a CSS selector action (e.g. clicking the <svg> or <span> inside <button class=my-btn>), event.target is the child, not the button. Previously, evaluateNoCodeConfigClick() only called: targetElement.matches(selector) This returned false for child elements even though an ancestor matched, silently dropping the click action. Fix: resolve matchedElement by trying direct .matches() first, then falling back to .closest(cssSelector) to find the nearest ancestor. Only if neither matches does the function return false. Also moved innerHtml check to use matchedElement instead of the raw click target, so element attributes are read from the correct node. Regression tests added for: - Child <span> click inside a matched button → now triggers correctly - Child with no matching ancestor → still returns false - Direct target click → closest() not called (fast path preserved) Fixes: formbricks#7314
WalkthroughThe pull request updates the 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/js-core/src/lib/common/tests/utils.test.ts (1)
1002-1024: Nit: verbose type cast for mock assignment.The existing tests in this file (e.g., Line 864, 889) assign mocks directly without the
as unknown as { ... }cast:targetElement.matches = vi.fn(() => true);The new tests use a heavier pattern:
(icon as unknown as { matches: ReturnType<typeof vi.fn> }).matches = vi.fn(() => false);Both work, but using the simpler pattern would be consistent with the rest of the file.
♻️ Simplify mock assignments
- (icon as unknown as { matches: ReturnType<typeof vi.fn> }).matches = vi.fn(() => false); - (icon as unknown as { closest: ReturnType<typeof vi.fn> }).closest = vi.fn(() => button); + icon.matches = vi.fn(() => false); + icon.closest = vi.fn(() => button);Apply the same simplification to the other two new tests as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/js-core/src/lib/common/tests/utils.test.ts` around lines 1002 - 1024, Replace the verbose "as unknown as { matches: ReturnType<typeof vi.fn> }" style casts used to assign mocks in the new tests with the simpler direct assignment pattern used elsewhere in the file (e.g., targetElement.matches = vi.fn(() => true)); specifically update the mock assignments for matches and closest on the icon/button elements in the tests that call evaluateNoCodeConfigClick so they use direct assignment (icon.matches = vi.fn(...); icon.closest = vi.fn(...);) and apply the same simplification to the other two new tests for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/js-core/src/lib/common/tests/utils.test.ts`:
- Around line 1002-1024: Replace the verbose "as unknown as { matches:
ReturnType<typeof vi.fn> }" style casts used to assign mocks in the new tests
with the simpler direct assignment pattern used elsewhere in the file (e.g.,
targetElement.matches = vi.fn(() => true)); specifically update the mock
assignments for matches and closest on the icon/button elements in the tests
that call evaluateNoCodeConfigClick so they use direct assignment (icon.matches
= vi.fn(...); icon.closest = vi.fn(...);) and apply the same simplification to
the other two new tests for consistency.
|
Hi @Dhruwang , This fixes nested click target matching in evaluateNoCodeConfigClick by adding a closest() fallback when the direct target doesn’t match the selector. All unit tests are passing (232 total), and I added regression tests covering: child click inside matched element no matching ancestor direct match fast path Could you please review when you get a chance? |
|
Thanks for the PR. Could you share an example of a scenario where this issue would occur? As far as I know, this hasn’t been reported before. |
|
Hi @Dhruwang, happy to clarify! The bug reproduces any time a click action's CSS selector targets an element that contains child elements (which is nearly every real-world button). Minimal reproduction: In Formbricks, create a no-code click action with CSS selector: .feedback-btn <button class="feedback-btn">
<svg xmlns="[http://www.w3.org/2000/svg](http://www.w3.org/2000/svg)" width="16" height="16" viewBox="0 0 24 24" fill="none" stroke="currentColor">
<path d="M21 15a2 2 0 0 1-2 2H7l-4 4V5a2 2 0 0 1 2-2h14a2 2 0 0 1 2 2z"/>
</svg>
Give Feedback
</button>Click the SVG icon (left side of the button) → Survey does not trigger ❌ This is especially common with icon buttons (virtually every design system — shadcn, Radix, MUI — uses or inside buttons). Happy to record a short screen demo if that would help! |
…fix/nested-click-target-delegate
…nctions; simplify CSS selector matching logic
What was broken
Formbricks CSS selector click actions silently fail when a user clicks inside a matched element rather than on it directly.
Fixes #7314
Example — you configure a click action targeting .submit-btn:
... SubmitWhen the user clicks the icon, event.target is — not .submit-btn. The SDK called:
ts
targetElement.matches(".submit-btn") // → false ❌ action dropped
The survey never triggers, with no error or warning.
This breaks any button with nested content — icon buttons, buttons with text wrappers, and virtually every component from Radix UI, MUI, shadcn/ui, and similar design systems.
What this PR does
Adds a .closest() fallback when .matches() fails, so the SDK walks up the DOM to find the nearest matching ancestor:// Before
if (!targetElement.matches(selector)) return false;
// After
const matchesDirectly = individualSelectors.every((sel) => targetElement.matches(sel));
if (!matchesDirectly) {
const ancestor = targetElement.closest(cssSelector);
if (!ancestor) return false;
matchedElement = ancestor as HTMLElement; // ✅ use the button, not the
}
The matched element is then used for all downstream checks (including innerHTML comparison), so attribute reads always happen on the correct node.
Performance: .closest() is only called as a fallback — direct target matches take the same fast path as before.
Tests
3 regression tests added that fail on the old code and pass on the new code:
✅ Clicking a child inside .my-btn → action fires correctly
✅ Clicking an element with no matching ancestor → correctly returns false
✅ Clicking the target directly → .closest() is not called (fast path preserved)
Full suite result: 232 tests · 19 files · all passing