Track form submission events #5381

apata · 2025-05-06T15:05:59Z

Changes

Adds config option to track form submission events to tracking script.
In tests, adds utility to create the page HTML on load, so we'd not need so many fixtures.

TODO

Understand why tests are flaky
- Issue 1: test runner submits the form before tracker script has fully initialized
- Issue 2: Playwright Firefox version is less than 133 which implemented fetch with keepalive.
Fix flakiness in tests
Figure out if form submission tracking is too eager, catching forms that haven't passed custom validation
- Fixed with checking checkValidity but also respecting novalidate attribute on the form element. Note: we don't respect the formnovalidate that can be set on the submit input: another limitation.
Figure out what to do to catch forms that are submitted with FormElement.submit(), which doesn't go through onsubmit event handlers
- Decided to highlight it as a limitation in tests. The workaround for users will be to use FormElement.requestSubmit() or to manually track such submits.
Figure out if and how to support forms that are not present on script load (a'la form that is inserted into the DOM when user expands a specific page section)
- Fixed by listening at document level.

TODO in future PRs

Figure out if and how to distinguish forms on websites that are using hash routing
Figure out exact event configuration: what event name to use and what props to include
Allow setting the option in onboarding, autocreate and autodelete related goal

Tests

Automated tests have been added

Changelog

Entry will be added together with main documentation update

Documentation

This change does not need a documentation update: will be released together with new script installation method

Dark mode

No UI changes

github-actions · 2025-05-06T15:07:56Z

Analyzed 1025 tracker script variants for size changes.
The following tables summarize the results, with comparison with the baseline version in parentheses.

Main variants:

	Brotli	Gzip	Uncompressed
plausible-main.js	2319B (+57B / +2.5%)	2650B (+69B / +2.7%)	6152B (+184B / +3.1%)

Important legacy variants:

	Brotli	Gzip	Uncompressed
plausible.js	1117B (+1B / +0.1%)	1293B (-4B / -0.3%)	2731B (+0B / 0%)
plausible.hash.js	1084B (+0B / 0%)	1263B (-4B / -0.3%)	2623B (+0B / 0%)
plausible.pageview-props.tagged-events.js	1825B (+2B / +0.1%)	2106B (-4B / -0.2%)	4663B (+0B / 0%)
plausible.file-downloads.hash.pageview-props.revenue.js	1624B (-1B / -0.1%)	1917B (-4B / -0.2%)	3995B (+0B / 0%)
plausible.compat.exclusions.file-downloads.outbound-links.pageview-props.revenue.tagged-events.js	2226B (+3B / +0.1%)	2615B (-4B / -0.2%)	5793B (+0B / 0%)

Summary:

	Brotli	Gzip	Uncompressed
Largest variant (plausible.compat.exclusions.file-downloads.outbound-links.pageview-props.revenue.tagged-events.js)	2226B (+3B / +0.1%)	2615B (-4B / -0.2%)	5793B (+0B / 0%)
Max change (plausible-main.js)	2319B (+57B / +2.5%)	2650B (+69B / +2.7%)	6152B (+184B / +3.1%)
Min change (plausible.compat.exclusions.local.manual.outbound-links.js)	1313B (-11B / -0.8%)	1586B (-6B / -0.4%)	3184B (+0B / 0%)
Median change	1671B (+0B / 0%)	1973B (-4B / -0.2%)	4182B (+0B / 0%)

In total, 327 variants brotli size increased and 232 variants brotli size decreased.

macobo · 2025-05-08T06:34:38Z

tracker/src/plausible.js

+  function attachFormSubmitTracking() {
+    document.addEventListener('submit', trackFormSubmission, true);
+  }
+
+  attachFormSubmitTracking();


Suggested change

function attachFormSubmitTracking() {

document.addEventListener('submit', trackFormSubmission, true);

}

attachFormSubmitTracking();

document.addEventListener('submit', trackFormSubmission, true);

Unneeded indirection.

macobo · 2025-05-08T06:38:26Z

tracker/src/plausible.js

+if (COMPILE_CONFIG && config.trackFormSubmissions) {
+  function trackFormSubmission(e) {
+    if (e.target.hasAttribute('novalidate') || e.target.checkValidity()) {
+      plausible('WP Form Completions', { props: { path: location.pathname } });


The event name is incorrect. Don't worry about wordpress in the current stage, we'll figure out a way to deal with their legacy events/goals later.

Suggested change

plausible('WP Form Completions', { props: { path: location.pathname } });

plausible('Form submission', { props: { path: location.pathname } });

Went with 'Form Submission' because it's more inline with 'File Download' etc, exact name TBD.

tracker/test/support/server.js

macobo · 2025-05-08T06:43:26Z

tracker/test/support/initialize-page-dynamically.ts

+  bodyContent: string
+}
+
+export function initializePageDynamically(


This function creates yet another way data is passed to the page. Could we get by creating another version of tracker/test/fixtures/plausible-main.html and dynamically set bodyContent based on a query parameter in an already-set div?

This would keep the approach more consistent across all tests and would remove code from both server.js and yet another helper.

The background here is that I found existing tests hard to work on without a direct link between the HTML and actual test. I couldn't easily tell which parts of the fixture were for which test case.

I guess it wasn't clear that I am proposing this (and the accompanying dynamic route) as an iterative improvement over all existing test setups, including plausible-main and openPage.

As it's a proposal and WIP, I didn't yet refactor all other tests, nor give it all the features to be able to unify all tests. Consistency is not yet in place, but would be if we go forward with this.

The benefits of using this is that it brings almost all of the test setup inside the test. I think the diversity in form related tests proves its usefulness.

A downside is that my IDE doesn't help with this dynamic HTML. I believe it's possible to get most IDEs and prettier to work with it though.

const content = /* html */` <form>...</form> `

Good call to investigate the way we deliver bodyContent and the configured script. I didn't try passing the DOM over query param yet, but I don't see why it shouldn't work. I tried header first actually, and when that failed, landed on the Pagewright API that worked well enough for the proposal.

Tried passing this over query params and I got it to render the UI. However, the default form submit action replaces query params with fields from the form, which then results in the express server rendering an empty page in response... The Playwright API that I'm using doesn't have that issue. The Playwright API seems good enough to me, pretty direct as well.

Though on further thought, maybe the ideal would be to prerender the page or pages at the start of the test:

async function setContent(path, { bodyContent, headContent }) { // Sends a POST request to an Express route, which // instructs the server to render the provided HTML content // and save it to the statically served folder on disk, at the `path`. } // Usage in test case: await setContent(`/${testId}/any/url`, { bodyContent: '<form><input type="submit" value="Submit"></form>', headContent: '<script>...script snippet with config injected</script>' }) await page.goto(`/${testId}/any/url`) await page.click('input[type="submit"]') // ...expectations

I see, this makes sense. I didn't consider that form submissions navigate 🤦

I think integrating with the express server dynamically is quite tricky 😅

Regarding setContent idea, perhaps creating a mock route would be simpler route?

E.g something like this.

const HTML_TEMPLATE = fs.readFileSync(...) const ROUTE = "/formSubmissions" async function openFormPage(page, { path, bodyContent, headContent, config }) { const body = HTML_TEMPLATE.replace(...).replace(...) // Ignorning the script-config schenanigans. page.route(ROUTE, (route) => route.fulfill({ status: 200, contentType: 'text/html', body })) await page.goto(ROUTE) }

(I'm not sure whether path is behaviorally important, so made it constant here)

Love the idea of increasing cohesion in tests by inlining the HTML ❤️ I also find it hard to work with fixtures especially when they serve multiple different tests/assertions.

We decided that initializePageDynamically utility will stay in and will be shared between tests that test features in depth.

I refactored this dynamic HTML not to go through our test server. Instead it's declared as a Playwright route. I didn't realize it at first, but route() API actually has a few gotchas. AFAICT they don't apply to our situation yet, so it's mergable, just a bit annoying to create potential debt.

It overlaps a bit with openPage helper in plausible-main.spec.js, but that helper and its accompanying fixture are specifically built to test script initialization, config overrides, and feature toggling, which require much more cleverness than initializePageDynamically. There's a general solution that covers both cases, but decided not to seek it yet.

macobo · 2025-05-08T06:47:22Z

tracker/test/support/test-utils.js

@@ -231,3 +233,13 @@ function checkEqual(a, b) {
 function delay(ms) {
  return new Promise(resolve => setTimeout(resolve, ms))
 }
+
+export async function openPage(page, config, options = {}) {


This 'helper' is still plausible-main specific. Let's move it back.

macobo · 2025-05-08T06:50:17Z

tracker/test/main/form-submissions.spec.ts

+
+  test('limitation: tracks _all_ forms on the same page, but _records them indistinguishably_', async ({
+    page
+  }, { testId }) => {


Why do we need testId in these tests? Looking at the test it is just noise, but I might be missing something.

At the time you commented, it wasn't really needed, but now it is 😅 It ensures there's no interplay between dynamically populated pages and saves us from having to think of a name for the dynamic route.

macobo · 2025-05-08T06:51:28Z

tracker/test/main/form-submissions.spec.ts

+    })
+  })
+
+  test('tracks form submissions triggered with submit button', async ({


Nitpick, not blocking: Move this test first in the describe. Happy path first over 'limitations'

macobo · 2025-05-08T06:52:25Z

tracker/test/support/types.ts

+    fileDownloads: boolean
+    taggedEvents: boolean
+    trackFormSubmissions: boolean
+  }


Nit: Formatting issues in this file.

macobo · 2025-05-08T06:57:21Z

tracker/test/main/form-submissions.spec.ts

+      scriptConfig: { ...DEFAULT_CONFIG, trackFormSubmissions: true },
+      bodyContent: `
+      <div>
+        <form onsubmit="event.preventDefault(); console.log('Form submitted')">


Q: Why do we do the event.preventDefault here and in other tests? Given that it's in the test code I assumed it's important for the correctness of the test, but it doesn't seem so.

Valid question! I agree that the test should contain only that which is strictly necessary. This is to eliminate page reload from form submit actions as the cause of test flakiness.

I think that makes sense, though you still seem to struggle with flakiness. Could we remove it from the inlined html here and move it to the template page?

It being here makes the reader of the test think it's somehow related to the scenario under test (instead of testing harness).

They're still needed to avoid flakiness due to Playwright Firefox version. We can think of them as scenarios where the customer's form has a custom onsubmit handler. Most of them can be deleted when we update playwright to make the test scenarios a bit shorter.

macobo · 2025-05-08T06:57:38Z

tracker/test/main/form-submissions.spec.ts

+    await expectPlausibleInAction(page, {
+      action: async () => {
+        await page.goto(`/dynamic/${testId}`)
+        await page.click('button#trigger')


Suggested change

await page.click('button#trigger')

await page.click('#submit')

Nit: terminology

Do you mean that I should apply this suggestion AND also change the id of the button? I am specifically using an id that makes it distinguishable from submit clicks in other tests.

Ah, makes sense. In that case:

Nit: perhaps #javascript-submit would be a better id?

Went with trigger-FormElement-submit

macobo · 2025-05-22T06:05:17Z

tracker/test/form-submissions.spec.ts

+          // this conditional is needed because fetch with keepalive is not implemented in the version of Firefox used by Playwright:
+          // can be removed once the Firefox version is >= v133
+          browserName === "firefox"
+            ? 'onsubmit="event.preventDefault(); setTimeout(() => {this.submit()}, 200)"'
+            : ""
+        }>


There's a better syntax for skipping tests: https://playwright.dev/docs/test-annotations#conditionally-skip-a-test

test('skip this test', async ({ page, browserName }) => { test.skip(browserName === 'firefox', 'Still working on it'); });

Thanks for the link! I'll definitely use that when I want to skip tests by browser. At the moment, this test is not skipped though, right? It is demonstrating that even on older versions of Firefox without keepalive support, there's a chance that the Form Submission event is sent.

Currently this test is two different tests: one on firefox, one elsewhere.

Given that the expected behavior is broken on our playwright firefox version, I think skipping is a better option and communicates intent better. Both options can also be easily cleaned up in a future PR.

Can you clarify what the intent is and who we are communicating it to?

As I see it, the intent of the test suite is to communicate how the feature works, and we are communicating it to each other and the engineers of our users.

This branched test (which I agree is effectively two tests, just with less repetition) currently communicates more than a skipped test: the feature still works on older versions of Firefox if the submit navigation is not really fast.

macobo · 2025-05-22T06:06:53Z

tracker/test/support/dynamic-page-template.html

+      (window.plausible =
+        window.plausible ||
+        function () {
+          (window.plausible.q = window.plausible.q || []).push(arguments);
+        }),
+        (window.plausible.init = function (i) {
+          window.plausible.o = i || {};
+        });
+      var script = document.createElement("script");
+      (script.type = "text/javascript"),
+        (script.defer = !0),
+        (script.src = "<%= plausible_script_url %>");
+      var r = document.getElementsByTagName("script")[0];
+      r.parentNode.insertBefore(script, r);


non-blocking q: Why is this beautified? It'd be better to take this snippet:

└> node compile.js --web-snippet <script> window.plausible=window.plausible||function(){(window.plausible.q=window.plausible.q||[]).push(arguments)},window.plausible.init=function(i){window.plausible.o=i||{}};var script=document.createElement("script");script.type="text/javascript",script.defer=!0,script.src="<%= plausible_script_url %>";var r=document.getElementsByTagName("script")[0];r.parentNode.insertBefore(script,r); plausible.init() </script>

Ah, I did take that exactly snippet, but when I autoformat the HTML template, it adds whitespace to the script, which makes it look like the snippet source. I didn't see much point in fighting with that, it's just whitespace. The bigger improvement would be to make an actual hard link between the compiled snippet and dynamic page snippet.

I've hard-linked the two in 7622b23

It's not exactly fool-proof in my opinion and I could roll the commit back, but at least it takes the manual diffing out.

Let's roll it back for now, not important and can make more consistent later if needed.

I've rolled it back, but I don't understand your point on making it more consistent later if needed. How can it be any more consistent than using the same function that node compile.js --web-snippet is using?

macobo · 2025-05-22T06:07:54Z

tracker/test/form-submissions.spec.ts

@@ -0,0 +1,306 @@
+import { test, Page } from "@playwright/test";


Nit: Let's remove the semicolons for consistency, most specs don't use them

Well spotted, but are semicolon related comments really worth our time and the CI resource use?

I don't think so, which is why I prepared and shared prettier and linter configurations for the tracker two times in the last 4 weeks, once at the very start of this endeavor. Let's apply them and regain our focus on what matters.

https://github.com/plausible/analytics/compare/script-v2/add-linter-prettier?expand=1

Agreed that linter/prettier is the better way to solve this, but given that the tracker codebase is mostly semi-colon free why add them in this PR? Fixing it later when we introduce linter may lead to merge conflicts whilst removing them now is free.

macobo · 2025-05-22T06:09:28Z

tracker/test/form-submissions.spec.ts

+      scriptConfig: { ...DEFAULT_CONFIG, formSubmissions: true },
+      bodyContent: `
+      <div>
+        <form onsubmit="event.preventDefault(); console.log('Form 1 submitted')">


Q: Is the event.preventDefault needed?

Yep! Otherwise submitting it causes navigation, which in turn sometimes causes the test to be flaky on Firefox because of the older version of Firefox that Playwright uses.

Let's skip the test on firefox instead then and remove the skip once we've upgraded firefox.

What's the benefit of skipping them?

They run on Firefox and other browsers, and prove that the feature works for forms where the onsubmit handler is custom, which is a commonplace in the web apps that I've worked on.

Did you disagree with this take: #5381 (comment)?

macobo · 2025-05-22T10:35:45Z

I think this is ready to be merged after these comments are resolved:

…ges" This reverts commit 7622b23.

apata · 2025-05-22T14:15:20Z

I think this is ready to be merged after these comments are resolved:

Track form submission events #5381 (comment)

Track form submission events #5381 (comment) and Track form submission events #5381 (comment)

@macobo Thanks for highlighting the blockers. I've made changes to the best of my conscience. Looking at the discussions, the arguments put forth, and the resulting changes, I don't think they were worth blocking over.

macobo reviewed May 8, 2025

View reviewed changes

macobo force-pushed the plausible-main-variant branch 4 times, most recently from 2b0f454 to 5d64bc4 Compare May 12, 2025 06:45

apata changed the base branch from plausible-main-variant to master May 15, 2025 14:06

apata force-pushed the script-v2/track-form-submissions branch 3 times, most recently from 3290bae to e5c2440 Compare May 19, 2025 11:25

Track form submissions

67250d2

apata force-pushed the script-v2/track-form-submissions branch from 0427ecf to 67250d2 Compare May 21, 2025 07:50

apata and others added 6 commits May 21, 2025 07:51

chore: Bump tracker_script_version to 11

19edcc0

Check that script loadedin tests

4ba8263

Track form submissions

7bec7b8

Fix format

415abaf

Fix missing form submit handler

9dfd5df

Make id casing consistent

572e346

apata marked this pull request as ready for review May 21, 2025 12:23

apata changed the title ~~WIP: Track form submission events~~ Track form submission events May 21, 2025

macobo reviewed May 22, 2025

View reviewed changes

Automatically sync compiled script snippet in dynamic test pages

7622b23

apata added 3 commits May 22, 2025 16:13

Enforce format manually

79528f8

Skip one test on Firefox, explain reason for stub

01bf857

Revert "Automatically sync compiled script snippet in dynamic test pa…

94914ce

…ges" This reverts commit 7622b23.

apata requested a review from macobo May 22, 2025 14:15

	plausible('WP Form Completions', { props: { path: location.pathname } });
	plausible('Form submission', { props: { path: location.pathname } });

	await page.click('button#trigger')
	await page.click('#submit')

		@@ -0,0 +1,306 @@
		import { test, Page } from "@playwright/test";

Uh oh!

Track form submission events #5381

Are you sure you want to change the base?

Track form submission events #5381

Conversation

apata commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

TODO

TODO in future PRs

Tests

Changelog

Documentation

Dark mode

Uh oh!

github-actions bot commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

macobo May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ukutaht May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apata May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apata May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

apata commented May 6, 2025 •

edited

Loading

github-actions bot commented May 6, 2025 •

edited

Loading

macobo May 9, 2025 •

edited

Loading

ukutaht May 12, 2025 •

edited

Loading

apata May 21, 2025 •

edited

Loading

apata May 22, 2025 •

edited

Loading