Skip to content

fix(visual-builder): keep overlayPropagation from piercing the SDK's own toolbar#617

Merged
hitesh-shetty-cstk merged 3 commits into
develop_v4from
fix/overlay-propagation-toolbar-pierce
Jul 8, 2026
Merged

fix(visual-builder): keep overlayPropagation from piercing the SDK's own toolbar#617
hitesh-shetty-cstk merged 3 commits into
develop_v4from
fix/overlay-propagation-toolbar-pierce

Conversation

@hitesh-shetty-cstk

Copy link
Copy Markdown
Contributor

Summary

With overlayPropagation.enable set, clicks that land on the visual builder's own UI (the field toolbar, overlays, add buttons) were resolved through document.elementsFromPoint() and could hit a data-cslp element rendered underneath the SDK's chrome.

In practice: when a page renders a heading from one entry directly above a rich text field from another entry, the focused field's toolbar sits on top of the heading. Clicking the toolbar's edit (pencil) button then also registered as a canvas click on the heading, because the SDK's capture-phase click listener runs before the button's own handler can stop propagation. That sent a focus event for the wrong entry while the edit modal opened for the right one, and the modal's Apply was lost.

Changes

  • getCsDataOfElement skips the elementsFromPoint fallback whenever the click target sits inside .visual-builder__container. The fallback exists to pierce the website's own empty overlapping elements (its original purpose, e.g. CSS grid spacer cells), never the SDK's own UI.

Test plan

  • New unit test: with overlayPropagation enabled, a click on an element inside the visual builder container does not resolve to the data-cslp element underneath, and elementsFromPoint is not called.
  • Existing getCsDataOfElement and listener suites pass (30 tests).
  • Manual: on a page where the field toolbar overlaps a neighboring field of a different entry, clicking the edit (pencil) button no longer flashes the underlying field's label, and the modal's Apply saves to the correct entry.

…own toolbar

With overlayPropagation enabled, clicks landing on the visual builder's
own UI (field toolbar, overlays) were resolved through
document.elementsFromPoint and could hit a data-cslp element rendered
underneath. A click on the edit (pencil) button then also counted as a
canvas click on the overlapped field, sending a focus event for the
wrong entry and breaking the edit modal's apply flow.

Skip the elementsFromPoint fallback whenever the click target sits
inside .visual-builder__container: the fallback exists to pierce the
website's own overlapping elements, never the SDK's chrome.
@hitesh-shetty-cstk hitesh-shetty-cstk requested review from a team as code owners July 8, 2026 09:50
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

🔒 Security Scan Results

ℹ️ Note: Only vulnerabilities with available fixes (upgrades or patches) are counted toward thresholds.

Check Type Count (with fixes) Without fixes Threshold Result
🔴 Critical Severity 0 0 10 ✅ Passed
🟠 High Severity 0 0 25 ✅ Passed
🟡 Medium Severity 0 0 500 ✅ Passed
🔵 Low Severity 0 0 1000 ✅ Passed

⏱️ SLA Breach Summary

✅ No SLA breaches detected. All vulnerabilities are within acceptable time thresholds.

Severity Breaches (with fixes) Breaches (no fixes) SLA Threshold (with/no fixes) Status
🔴 Critical 0 0 15 / 30 days ✅ Passed
🟠 High 0 0 30 / 120 days ✅ Passed
🟡 Medium 0 0 90 / 365 days ✅ Passed
🔵 Low 0 0 180 / 365 days ✅ Passed

✅ BUILD PASSED - All security checks passed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes an interaction bug in the Visual Builder where overlayPropagation.enable could incorrectly “pierce” the SDK’s own chrome (toolbar/overlays) via the elementsFromPoint fallback and resolve clicks to an underlying data-cslp element on the canvas.

Changes:

  • Skip the document.elementsFromPoint() fallback when the click target is inside .visual-builder__container.
  • Add a unit test ensuring toolbar/UI clicks do not resolve to underlying data-cslp elements and that elementsFromPoint is not invoked.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/visualBuilder/utils/getCsDataOfElement.ts Prevents overlay propagation fallback from piercing the Visual Builder UI container.
src/visualBuilder/utils/test/getCsDataOfElement.test.ts Adds coverage for Visual Builder UI clicks with overlayPropagation enabled.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/visualBuilder/utils/getCsDataOfElement.ts
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 67.13% 2506 / 3733
🔵 Statements 65.99% 2544 / 3855
🔵 Functions 64.51% 449 / 696
🔵 Branches 61.46% 1509 / 2455
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
src/visualBuilder/utils/getCsDataOfElement.ts 100% 100% 100% 100%
Generated in workflow #853 for commit e415546 by the Vitest Coverage Report Action

@contentstackMridul

Copy link
Copy Markdown
Contributor

The change itself looks right and low-risk. Everything the SDK renders (toolbar, overlays, add buttons) mounts under .visual-builder__container in initUI, so closest(".visual-builder__container") reliably catches a click on our own chrome, and the guard sits inside the existing overlayPropagation.enable branch so the non-overlay path is untouched.

Two things I'd want to confirm before we treat this as the fix for VB-1958 rather than a good hardening on its own:

  1. I couldn't trace the ticket's actual error, Uncaught TypeError: e.onFocus is not a function, to this path from the SDK source. The only onFocus references are the Start Editing button and Tooltip. Could you point to where the wrong-entry focus event ends up calling onFocus? That closes the loop between this fix and the reported error.
  2. This only engages when overlayPropagation.enable is set, so it's worth confirming SKF actually runs with that config. Otherwise the guard is a no-op for them.

Also, can we add VB-1958 to the description so the two are linked?

On Copilot's note: agree it's worth folding isVisualBuilderUi into the condition since this runs on hover too. One caveat: put the closest() last in the && chain (after overlayPropagation.enable) so it only runs when a hover actually misses every field and the fallback is on.

@contentstackMridul contentstackMridul left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with comment

…dition

Run the closest() check only when the overlayPropagation fallback is
enabled and no field matched, so hover events skip the extra DOM
traversal in the common case.
@hitesh-shetty-cstk

Copy link
Copy Markdown
Contributor Author

Thanks — applied your and Copilot's suggestion: the guard is folded into the fallback condition with closest() last, so hovers only pay for it when the fallback is enabled and nothing matched (e415546).

On 1: you're right, and that's the key finding — the onFocus error never comes from this path. The ticket bundles two independent problems:

  • The console error comes from the rich text editor component itself: venus' HTML RTE editor calls props.onFocus(e) unconditionally when the editable gains focus, and the form controller wired in by form-fields doesn't provide one. That is being fixed separately in the form-fields package (the JSON RTE copy of the same editor already has an onFocus = () => {} default, which is why only HTML RTE throws). It's noisy but harmless to saves — effectively a red herring in the report.
  • The actual save loss is silent and is what this PR fixes: with the fallback on, the pencil click pierces to the overlapped field and the SDK emits a focus event for that field's entry. The app then rebinds its quick form to the wrong entry, the modal's Apply writes the value through that form, the field path doesn't resolve against the wrong content type, and the app-side update throws before the save request is built. No SDK onFocus involved anywhere in that chain.

Reproduced deterministically with a heading from one referenced entry rendered flush above an RTE from another: the field label flashes the heading field's name when clicking the pencil, and removing the fallback flag makes the whole thing vanish.

On 2: agreed, the guard only engages with overlayPropagation.enable set. The pierce is a hard prerequisite for the wrong-entry focus event, and the fallback feature exists precisely for this customer's overlapping-layout site, so it's expected to be on there — but confirming their runtime config with support before closing the loop is the right call.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

🔒 Security Scan Results

ℹ️ Note: Only vulnerabilities with available fixes (upgrades or patches) are counted toward thresholds.

Check Type Count (with fixes) Without fixes Threshold Result
🔴 Critical Severity 0 0 10 ✅ Passed
🟠 High Severity 0 0 25 ✅ Passed
🟡 Medium Severity 0 0 500 ✅ Passed
🔵 Low Severity 0 0 1000 ✅ Passed

⏱️ SLA Breach Summary

✅ No SLA breaches detected. All vulnerabilities are within acceptable time thresholds.

Severity Breaches (with fixes) Breaches (no fixes) SLA Threshold (with/no fixes) Status
🔴 Critical 0 0 15 / 30 days ✅ Passed
🟠 High 0 0 30 / 120 days ✅ Passed
🟡 Medium 0 0 90 / 365 days ✅ Passed
🔵 Low 0 0 180 / 365 days ✅ Passed

✅ BUILD PASSED - All security checks passed

@hitesh-shetty-cstk

Copy link
Copy Markdown
Contributor Author

Also, can we add VB-1958 to the description so the two are linked?

We don't link jira details on public repo to avoid leaking any internal details.

@hitesh-shetty-cstk hitesh-shetty-cstk merged commit fbb7a50 into develop_v4 Jul 8, 2026
9 checks passed
@hitesh-shetty-cstk hitesh-shetty-cstk deleted the fix/overlay-propagation-toolbar-pierce branch July 8, 2026 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants