v1.2.1 - Improve checkbox event. Update example file#16
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
bbarao
left a comment
There was a problem hiding this comment.
🤖 AI-assisted review
The intent of this PR is correct, and it genuinely fixes clicks on nested label text and implicit-wrap labels that the previous code missed. However, one change widens the suppression too far and introduces a regression that silently drops real user clicks from the recording. Because this is a recorder — a missed step produces a quietly incomplete recording that only fails later on replay — I'm requesting changes on that one item before merge. The version bumps and OUTPUT-EXAMPLE.md were verified and look good.
Blocking
1. 🔴 src/pages/Content/modules/collectEvents.js:202 — Clicks on links/buttons inside a checkbox label are dropped (regression)
Switching from nodeName === 'label' to tgt.closest('label') makes the early-return fire for any click inside a label that contains or references a checkbox/radio — including clicks on interactive descendants such as <a> and <button>. The browser does not forward those clicks to the labeled control (per the HTML standard, a label's activation behavior does nothing for clicks targeted at interactive content descendants), so no change event fires and the click is a distinct user action — but it gets silently dropped from the recording.
A common trigger is the consent pattern:
<label><input type="checkbox"> I agree to the <a href="/terms">terms</a></label>Clicking "terms" navigates but is no longer recorded. The previous code did not have this issue (it only matched when the click target was the <label> element itself), so this is a regression — while the rest of the change is a genuine improvement. Behavior across cases:
| Click target | change fires? |
Should record click? | This PR |
|---|---|---|---|
label text / nested <span> (for- or wrap-label) |
yes | no (redundant) | suppressed — correct |
nested <a> / <button> in the label |
no | yes | suppressed — dropped |
| label tied to a text input | no | yes | recorded — correct |
Suggested fix — only suppress when the click is actually forwarded to the labeled control (also makes the for lookup work inside shadow DOM, addressing item 2):
const parentLabel = tgt.closest && tgt.closest('label');
if (parentLabel) {
// Only suppress when the click is actually forwarded to the labeled control.
// Clicks on interactive descendants (links, buttons, other fields) are NOT
// forwarded by the browser and must still be recorded.
const interactiveHit = tgt.closest('a, button, input, select, textarea');
const forwardsToControl = !interactiveHit || !parentLabel.contains(interactiveHit);
if (forwardsToControl) {
const forAttr = parentLabel.getAttribute('for');
if (forAttr) {
const root = tgt.getRootNode();
const lookup = root && root.getElementById ? root : document; // shadow-DOM safe
const forEl = lookup.getElementById(forAttr);
if (forEl && forEl.nodeName.toLowerCase() === 'input' &&
(forEl.type === 'checkbox' || forEl.type === 'radio')) {
return;
}
}
if (parentLabel.querySelector('input[type="checkbox"], input[type="radio"]')) {
return;
}
}
}This keeps the intended suppression for label text / nested non-interactive elements and only stops dropping clicks on interactive descendants. Since test-build/ and test-build-firefox/ are committed, please run ./buildit.sh after the source change so the bundles stay in sync.
Nice to have
2. 🟢 src/pages/Content/modules/collectEvents.js:206 — Shadow DOM for-association uses global document
document.getElementById(forAttr) cannot resolve a checkbox that lives inside a shadow root, so a redundant label click in shadow DOM is not suppressed. Using tgt.getRootNode().getElementById(...) (folded into the fix above) handles both the regular page and shadow DOM. Minor and pre-existing.
Notes on the rest of the PR: version bumps are consistent (package.json, manifest-firefox.json, and both build manifests are 1.2.1; the Chrome manifest.json is intentionally version-less and injected at build time). OUTPUT-EXAMPLE.md is valid and the #outer-iframe >>> #inner-iframe format matches how frame paths are built (outermost to innermost). Both regenerated content-script bundles are identical and reflect the source change.
bbarao
left a comment
There was a problem hiding this comment.
🤖 AI-assisted review
Verified the follow-up commit (a2d258f): the label/checkbox click handler now suppresses only the clicks the browser actually forwards to the labeled control, and records clicks on interactive descendants (links, buttons, selects, text fields). Re-tested the shipped logic across 14 label configurations — for- and wrap-association, nested spans, links, buttons, selects, radios, and shadow DOM — and all behave correctly, so the earlier regression (dropping clicks on links/buttons inside a checkbox label) is resolved. Version is consistent at 1.2.2 and both content-script bundles are correctly regenerated. CI is green.
Non-blocking nit: the PR title still says "v1.2.1" — consider updating it to v1.2.2 for accurate release notes.
No description provided.