Repository navigation
Support inline events in the main v12 script - #106
Conversation
Requested: integrate documented inline events without overengineering, keep the script small and website behavior intact, and open a separate PR based on and targeting v12. Use delegated click/submit handlers and keepalive GETs with the existing image fallback. Preserve installed inline helpers and make confirmed 404 detection opt-in. Add focused tests and patch the vulnerable Handlebars build dependency.
| button.form === element && | ||
| (button.tagName === "BUTTON" || | ||
| (button.tagName === "INPUT" && button.type === "submit")) | ||
| ? button.value || button.textContent || "" |
There was a problem hiding this comment.
button.value shadows the label, and the text is neither trimmed nor capped: <button name="action" value="save">Save changes</button> reports button_text: "save", <button>\n Join now\n</button> reports "\n Join now\n ", and a long button pushes the simple.gif URL past 5 KB (verified against dist/latest/latest.dev.js).
| ? button.value || button.textContent || "" | |
| ? (button.textContent || button.value || "") | |
| .replace(/\s+/g, " ") | |
| .trim() | |
| .slice(0, 200) |
With this, <input type="submit" value="Join"> still falls through to value, and all 15 inline-event tests still pass.
There was a problem hiding this comment.
Fixed in 7f712bd with the shorter submitter-only path: prefer textContent, fall back to value for submit inputs, trim outer whitespace and cap the result at 200 characters. Added checks for a label differing from the button value, oversized labels, submit inputs, and missing submitters. All 32 unit tests pass.
| var detect404 = function () { | ||
| if ( | ||
| hasInlineHelper() || | ||
| !/[^0-9.,]404[^0-9.,]/.test(documentElement.innerHTML) |
There was a problem hiding this comment.
Matching 404 against the whole serialized DOM false-positives constantly, so opted-in sites send an extra HEAD request for their own URL on ordinary 200 pages — I confirmed a request fires for <div style="width: 404px">, <script src="/assets/main-404abc.js">, <div class="w-404"> and <div data-id="404">. Narrow the scan to rendered text (e.g. doc.title plus doc.body.textContent) so the added request matches the "existing installs must opt in" cost the comment above promises.
There was a problem hiding this comment.
Changed the probe to title plus body text in 7f712bd. A test containing all four attribute-only examples now confirms that no HEAD request is issued. Confirmed 404 events still work before and after DOM ready in Chromium, and the response status must still be 404.
| run: pnpm run test:matrix | ||
|
|
||
| - name: Test inline events | ||
| run: pnpm run build && pnpm exec mocha test/unit/inline-events.test.js --exit |
There was a problem hiding this comment.
This runs only the new file, so the seven other test/unit/*.test.js suites that cover the same src/default.js this PR rewrites still never run in CI. Widening the glob picks them up (I ran all 31 locally against this branch and they pass):
| run: pnpm run build && pnpm exec mocha test/unit/inline-events.test.js --exit | |
| - name: Run unit tests | |
| run: pnpm run build && pnpm exec mocha "test/unit/*.test.js" --exit |
Worth also adding a test:unit script to package.json — AGENTS.md tells contributors to run npm run test:unit, which does not exist.
There was a problem hiding this comment.
Added test:unit and changed this CI step to run the complete test/unit/*.test.js suite in 7f712bd. All 32 tests pass locally, including the 16 inline-event tests.
Documentation update
|
Claude review checkpointReviewed commit |
|
SDLC label: |
| var hasInlineHelper = function () { | ||
| return doc.querySelector('script[src*="inline-events.js"]'); | ||
| }; |
There was a problem hiding this comment.
This was just a test helper, fine if this collides, it was beta anyway. Remove this check.
There was a problem hiding this comment.
Removed the beta-helper detection from both inline-event handling and 404 detection in 7f712bd. The built-in handlers now run even if the old helper tag is present; the updated PR description says to remove that beta script to avoid duplicate events.
| var detect404 = function () { | ||
| if ( | ||
| hasInlineHelper() || | ||
| !/[^0-9.,]404[^0-9.,]/.test(documentElement.innerHTML) |
There was a problem hiding this comment.
we cna't use a abbrivations of documentElement.innerHTML, right?
There was a problem hiding this comment.
The existing documentElement alias is safe: it holds the DOM object, and reading its .innerHTML later reads the current HTML. Caching the HTML string itself would be stale. In 7f712bd the 404 check instead reads the title and body text, which also prevents attributes such as width: 404px from triggering a HEAD request. The focused attribute test and before/after-DOM-ready Chromium checks pass.
| metadata.button_text = | ||
| button && | ||
| button.form === element && | ||
| (button.tagName === "BUTTON" || | ||
| (button.tagName === "INPUT" && button.type === "submit")) | ||
| ? button.value || button.textContent || "" | ||
| : ""; |
There was a problem hiding this comment.
can we make this less perfect, but a lot shorter?
There was a problem hiding this comment.
Shortened this in 7f712bd: use event.submitter directly and remove the active-element fallback plus the tag/form checks. If the browser provides no submitter, button text is simply empty and the form event is still recorded. The label is preferred over value, trimmed and capped at 200 characters. Form validation, native submitter overrides, and SPA submissions pass the browser checks.
Requested: remove compatibility checks for the beta helper, measure repeated attribute strings, clarify the DOM alias, and shorten submit-button metadata extraction. Keep literals because sharing them increases gzip size. Prefer trimmed, capped submitter labels and scan title/body text for 404s. Address the related bot feedback by running the complete unit suite in CI.
| if (isForm) { | ||
| var button = event.submitter; | ||
| metadata.button_text = button | ||
| ? (button.textContent || button.value || "").trim().slice(0, 200) |
There was a problem hiding this comment.
trim() only strips the ends, so the very common icon/span submit button keeps its markup indentation — I verified <button>\n <svg></svg>\n <span>Save</span>\n <span>changes</span>\n</button> sends button_text: "Save\n changes", and the same button reformatted produces a different value, fragmenting the metadata. Collapse internal whitespace too:
| ? (button.textContent || button.value || "").trim().slice(0, 200) | |
| ? (button.textContent || button.value || "") | |
| .replace(/\s+/g, " ") | |
| .trim() | |
| .slice(0, 200) |
There was a problem hiding this comment.
Fixed in f1b1428. Submit-button labels now collapse internal whitespace before the existing trim and 200-character limit, so compact and indented markup both record Save changes.
Local verification: the new regression fails against the previous compiled tracker; all 33 unit tests, the build, formatting, and diff checks pass after the fix. Remote BrowserStack CI and automated review both passed on this exact commit.
There was a problem hiding this comment.
Confirmed implemented in the current head, f1b1428: .replace(/\s+/g, " ") runs before trimming and the 200-character cap. The regression verifies that compact and indented icon/span button markup both produce Save changes. All 33 unit tests pass locally, and the remote unit-test step has also passed. This costs 20 bytes raw / 8 bytes gzip.
Documentation update
|
Requested: address scripts PR #106 review comment 4282258759 so formatting a button across multiple lines does not fragment button_text metadata. Collapse whitespace before trimming and applying the existing 200-character limit. Add a regression for equivalent compact and indented button markup, and regenerate the affected tracker bundles. Verification: regression fails on the previous compiled tracker; all 33 unit tests pass after the fix. Build, formatting, and diff checks pass.
|
In response to #106 (comment) and #106 (comment): Prepared a patch against current docs/master for the final v12 behavior: built-in inline events, removal of the beta helper, native click/form handling, normalized submit-button labels, and opt-in 404 detection using title/body text. The patch applies cleanly. There is no matching open docs PR to update. I have requested owner approval for a supporting docs PR because this automation is authorized to push to existing PR branches. Publication of these instructions must follow the relevant script PR and the v12 release; the patch remains local pending that approval. |
Summary
Closes #105
Add the documented inline event features to the main tracker (
src/default.js) while keeping the implementation small and preserving website behaviour:document, so dynamically inserted elements and currentdata-simple-event-*metadata are picked up without observers or polling. Every event carries atagfield; form events also carry the submit button label. Form field values are never read.event.submitter, preferringtextContentand falling back tovaluefor<input type="submit">. Internal whitespace is collapsed to single spaces before trimming and capping at 200 characters, so an icon/span button keeps a stablebutton_textwhen its markup is reindented.fetchGET (mode: "no-cors",credentials: "omit") so they survive navigation, falling back to the existing image pixel whenfetchis missing, throws, or rejects. Native navigation, form validation,formactionoverrides and SPA handlers stay in control.inline-events.jshelper is not detected or supported. Remove that extra script when using the built-in handlers, otherwise events are counted twice.data-detect-404="true"orsa_settings.detect404 = true. The tracker scans the page title and body text for "404", confirms with aHEADrequest, and only then sends apage_404event with the query string and fragment stripped from the URL metadata. Existing installations send no extra requests by default.light.jsis unaffected apart from the shared transport refactor.test/unit/inline-events.test.jsplus atest:unitscript, and run the full unit suite in the BrowserStack workflow afterpnpm run build.handlebarsbuild dependency from 4.7.6 to 4.7.10 and addjsdom26.1.0 for the unit tests.Changes:
Security implications
Has security impact - described as: the production tracker starts collecting new end-user interaction data on every install that supports events — event names, element tag,
data-simple-event-*attribute values and submit-button text — all of which are opt-in per element through HTML attributes, and form field values are explicitly excluded. Opt-in 404 detection additionally issues aHEADrequest to the customer's own origin for the current URL; the resultingpage_404metadata strips query strings and fragments. Inline events are sent withcredentials: "omit", and thehandlebarsbuild dependency is upgraded to a patched release.Testing
Run by Claude on this branch (
f1b1428) with Node 22.23.3:npm install— passed.npm run build— passed; the only regenerated-output difference versus the committeddist/is the build date stamp and local minifier output in the untouchedauto-events.jsbundle, so the committeddist/latest/latest.dev.jsis in sync withsrc/.npm run test:unit— 33 passing, 0 failing, including the new submit-label whitespace test.Reported by the author and not re-run here: local Chromium navigation checks, formatting, and bundle-size measurements.
Checklist