fix(deadclicks): ignore clicks nested inside interactive ancestors#3569
Open
christiaan-ph wants to merge 9 commits into
Open
fix(deadclicks): ignore clicks nested inside interactive ancestors#3569christiaan-ph wants to merge 9 commits into
christiaan-ph wants to merge 9 commits into
Conversation
When a user clicks an element nested inside an anchor or other interactive element (for example an <img> inside an <a href>), the click target reported by the browser is the inner element, not the anchor. The dead-click detector previously only checked the immediate target against autocaptureCompatibleElements, so these clicks entered the candidate queue. Because hard navigation tears down the page rather than producing a DOM mutation within mutation_threshold_ms, the click was then emitted as a false-positive $dead_click. This matches the ancestor walk autocapture already does in getElementAndParentsForElement.
The earlier commit added a comment explaining why the ancestor walk is needed. The reasoning is the same as for the original interactive-tag check that preceded it (which had no comment), so the comment doesn't earn its keep. Folding the check back into the existing tag/element guard also keeps the diff minimal and consistent with the surrounding style.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
.changeset/dead-click-anchor-descendants.md:3
**Spurious `@posthog/types` bump in changeset**
The changeset lists `@posthog/types: patch`, but none of the three files changed in this PR touch the `packages/types/` directory or any exported type from that package. Publishing this changeset as-is will produce an empty patch release for `@posthog/types` with no corresponding code change.
Reviews (1): Last reviewed commit: "chore(deadclicks): drop redundant commen..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Contributor
|
Size Change: -42 B (0%) Total Size: 16 MB
ℹ️ View Unchanged
|
Contributor
|
Reviews (2): Last reviewed commit: "test(deadclicks): cover deep nesting ins..." | Re-trigger Greptile |
Contributor
|
Reviews (3): Last reviewed commit: "Merge branch 'main' into posthog-code/fi..." | Re-trigger Greptile |
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/browser/src/__tests__/entrypoints/lazy-loaded-dead-clicks-autocapture.test.ts:165-181
**Superfluous `_checkClicks()` call in interactive-ancestor tests**
Because `_ignoreClick` is called synchronously at click time (inside `_onClick`), a suppressed click is never pushed to `_clicks`. Advancing the clock and calling `_checkClicks()` in these tests is therefore a no-op — `_checkClicks()` returns immediately on an empty queue. The real assertion (`_clicks.toHaveLength(0)`) already passes right after `triggerMouseEvent`. The extra steps don't add coverage and slightly obscure *where* the filtering happens. The companion "non-interactive ancestor" test — which correctly omits those steps — highlights the inconsistency.
Reviews (4): Last reviewed commit: "Merge branch 'main' into posthog-code/fi..." | Re-trigger Greptile |
_ignoreClick runs synchronously in _onClick, so a suppressed click never enters _clicks. setSystemTime + _checkClicks() were no-ops here.
Contributor
|
Reviews (5): Last reviewed commit: "test(deadclicks): drop no-op _checkClick..." | Re-trigger Greptile |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
$dead_clickevents fire on pages that use<img>(or other non-interactive elements) nested inside<a>tags. Clicking the image navigates fine, but the click target is the<img>, not the anchor, so the dead-click detector's tag check misses the interactive parent and reports a dead click. The pattern shows up frequently on sites built with Framer.Changes
Replaces the leaf-only tag check in
_ignoreClickwith an ancestor walk viaclosest(). The walk uses the existingautocaptureCompatibleElementslist joined into a selector, hoisted toautocaptureCompatibleElementsSelectorfor reuse.Adds parameterized tests covering nested clicks inside each compatible element, plus a regression test for nested clicks inside non-interactive ancestors.
Open questions for reviewers
Selector scope. I reused the full compatible-elements list (
a, button, form, input, select, textarea, label) for the ancestor walk. The motivating bug only needsa/button. A click in the inert padding of a long<form>is arguably a real dead click that this change would now silently suppress. Happy to narrow to['a', 'button']if folks prefer fewer false negatives over consistency with autocapture.Partial parity with autocapture. This mirrors autocapture's ancestor-walk pattern, but autocapture also pierces shadow roots and treats
cursor: pointerparents as interactive. Shadow-root behavior is documented in a code comment; let me know if I should expand the fix to cover those cases or leave them as follow-ups.Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file