Skip to content

fix(uve): scope stopPropagation to clicks that resolve to a link - #37972

Open
gortiz-dotcms wants to merge 1 commit into
mainfrom
issue-37961-uve-stop-propagation-link-scope
Open

gortiz-dotcms wants to merge 1 commit into
mainfrom
issue-37961-uve-stop-propagation-link-scope

Conversation

@gortiz-dotcms

Copy link
Copy Markdown
Member

Summary

Follow-up to #37962 (merged as baeaf675c268). That PR's final commit — scoping stopPropagation() to clicks that resolve to a link, to fix the QA-reported regression where a cookie-consent dialog's own buttons stopped responding inside the editor — was pushed to the branch after #37962 had already been squash-merged, so it never made it into main/trunk. The QA-reported regression is still live right now. This PR carries exactly that one fix forward.

What this fixes

QA verification on #37961 (comment on the issue, 2026-10-09) returned a failed verdict against the version of #37962 that had just merged: the navigation fix worked, but stopping propagation for every click the editor didn't recognize — not just navigation clicks — broke page-owned, non-navigating UI. Repro'd with a real Usercentrics cookie-consent dialog: its Accept/Reject buttons stopped responding, blocking the contentlets beneath it from being edited at all.

Root cause of the original bug, per QA's own repro: a page's own unrelated bubble-phase code (their fixture: a sidebar's onclick="event.stopPropagation()") could stop a click from ever reaching the old bubble-phase listener on window, so preventDefault() never ran and the anchor's native navigation went through unimpeded. The capture-phase listener #37962 already added fixes that on its own — preventDefault() called during capture blocks the native action regardless of what any later node does with the event afterward. The unconditional stopPropagation() was never actually required for that case; it was added for a narrower one (a page script delegating click-to-navigate on a wrapper around a pointer-events: none anchor, with no real <a> at the click target), which resolveClickedAnchor's descendant fallback already classifies as hasLink: true.

The fix: scope stopPropagation() to exactly that — clicks that resolve to a link. Everything else (cookie-consent dialogs, accordions, tabs, carousels) now reaches the page's own handlers exactly as it did before #37962 existed.

Testing

Unit tests: updated the five capture-phase stopPropagation tests in dot-uve-iframe.component.spec.ts. The four exemption tests (block editor, WYSIWYG, TinyMCE toolbar, contentlet selection) now embed a link in each target, since hasLink is now a prerequisite for stopping at all — without an embedded link they'd pass vacuously and stop catching a regression if the exemption logic were ever removed. Added a positive case (plain link click still stops) alongside the regression case (non-link click no longer stops). 451 tests passing, 3 skipped, across the three affected spec files.

Live, against a real running editor:

  • QA's exact root-cause repro (an ancestor with onclick="event.stopPropagation()" wrapping a real link) — still navigates via the SPA, native navigation still correctly prevented.
  • QA's reported regression (a simulated Usercentrics-style consent dialog) — the Accept button now correctly dismisses it.
  • The pointer-events: none delegate-wrapper pattern resolveClickedAnchor was built for — still protected; the page's own delegate router never sees that click.
  • Contentlet selection — still works on a fresh page load.

Fixes #37961

🤖 Generated with Claude Code

QA verification on #37961 failed this PR: the navigation
fix worked, but stopping propagation for every unrecognized click —
not just link clicks — broke page-owned, non-navigating UI inside the
editor. Confirmed with a real Usercentrics cookie-consent dialog: its
Accept/Reject buttons stopped responding, blocking the contentlets
beneath it from being edited at all.

Root cause of the original bug (per QA's repro): a page's own
unrelated bubble-phase code (e.g. a sidebar's
`onclick="event.stopPropagation()"`) could stop a click from ever
reaching the old bubble-phase listener on `window`, so
preventDefault() never ran and the anchor's native navigation went
through unimpeded. The capture-phase listener added earlier in this
PR already fixes that on its own — preventDefault() called during
capture blocks the native action regardless of what any later node
does with the event. The *unconditional* stopPropagation() was never
required for that case; it was added for a narrower one (a page
script delegating click-to-navigate on a wrapper around a
`pointer-events: none` anchor, with no real `<a>` at the click
target), which resolveClickedAnchor's descendant fallback already
classifies as `hasLink: true`.

Scope stopPropagation() to exactly that: clicks that resolve to a
link. Everything else — cookie-consent dialogs, accordions, tabs,
carousels — now reaches the page's own handlers exactly as it did
before this PR existed.

Verified live against the real running editor (not just unit tests):
- QA's exact root-cause repro (ancestor stopPropagation + real link)
  still navigates via the SPA, native navigation still prevented.
- The reported regression (a simulated consent dialog's Accept button)
  now correctly dismisses.
- The pointer-events:none delegate-wrapper pattern is still protected
  (stopPropagation still fires, the delegate router never sees it).
- Contentlet selection still works on a fresh page load.

Updated the five capture-phase stopPropagation tests: the four
exemption tests now use a link embedded in each target type (block
editor, WYSIWYG, TinyMCE toolbar, contentlet selection), since hasLink
is now a prerequisite for stopping at all — without an embedded link
they'd pass vacuously and stop catching a regression if the exemption
logic were ever removed. Added a positive case (plain link click DOES
stop) alongside the QA regression case (non-link click does NOT stop).
@claude

claude Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @gortiz-dotcms's task in 1m 23s —— View job


Code Review — scope stopPropagation to clicks that resolve to a link

  • Read dot-uve-iframe.component.ts diff and surrounding filter logic
  • Verify resolveClickedAnchor / classifyUveClickTarget semantics
  • Check for duplicate declarations / lint fallout from hoisting href/hasLink
  • Review the five updated capture-phase spec tests

New Issues

No bugs found.

The change is correct and tightly scoped. linkElement/href/hasLink are now computed once at the top of the capture-phase filter (dot-uve-iframe.component.ts:290-292) and the later duplicate declarations were removed cleanly — the const href at line 390 is in the separate bubble-phase listener, so there's no shadowing/redeclaration. Gating stopPropagation() on hasLink (line 308) is the right narrowing: preventDefault() on the capture phase (via the hash branch and handleInternalNav) is what actually blocks native anchor navigation, so stopping propagation was never load-bearing for the #37961 race — only for the delegate-wrapper-around-a-pointer-events:none-anchor case, which resolveClickedAnchor's descendant fallback (utils/index.ts:148-164) still classifies as hasLink: true.

The spec changes correctly embed a real <a> in each exemption target so hasLink is satisfied — without that, those four tests would pass vacuously and stop guarding the exemption logic. The classification still resolves because classifyUveClickTarget uses closest() ancestor walks (dot-uve-iframe.component.ts:215-238), so a link nested inside a [data-mode] / [data-block-editor-content] / [id^="mceu_"] / [data-dot-object] wrapper is still matched.

Note (non-blocking, intentional)

One behavioral narrowing worth stating explicitly for reviewers: a page that drives navigation purely in JS (onclick → location.href) on an element with no <a> anywhere — not even a pointer-events:none descendant anchor — resolves to hasLink: false and is no longer stopped. That click can still reach a page-owned bubble handler that navigates the iframe natively. This is a conscious trade-off: it restores pre-#37962 behavior for all page-owned non-navigating UI (cookie dialogs, accordions, tabs), and the only JS-delegate navigation pattern dotCMS commits to protecting is the pointer-events:none anchor one, which remains covered. The capture-phase comment (dot-uve-iframe.component.ts:294-306) documents this reasoning well. No action needed — flagging only so the scope is on the record.

The PR body claims 451 passing / 3 skipped across the affected specs; I reviewed the diff for correctness rather than re-running the suite.

· branch issue-37961-uve-stop-propagation-link-scope

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area : Frontend PR changes Angular/TypeScript frontend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

UVE canvas still blanks / loses inline-edit wiring on internal-nav click after PR #37750 fix

1 participant