fix: lvt-form:action routing, lvt-nav:no-intercept, unreserve action field#45
fix: lvt-form:action routing, lvt-nav:no-intercept, unreserve action field#45
Conversation
- Add lvt-form:action attribute for explicit form routing (highest priority) - Stop excluding 'action' from form data — it's no longer reserved - Rename lvt-no-intercept → lvt-nav:no-intercept on links (semantic separation) - Keep lvt-form:no-intercept for forms (already correct) - Update change-auto-wirer: lvt-no-intercept → lvt-form:no-intercept - Add tests for lvt-form:action routing and action field preservation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Review: lvt-form:action routing, lvt-nav:no-intercept, unreserve action field The changes are clean and the intent is clear. A few things worth flagging: Breaking changes with no backward compatibility Both attribute renames are silent breaking changes:
Any existing HTML using Same applies to the PR description references The action resolution order in the PR body lists Minor: empty
Overall the logic is correct and the tests cover the new cases well. |
There was a problem hiding this comment.
Pull request overview
This PR updates LiveTemplate’s client-side routing/interception behavior to avoid silent data loss for <input name="action"> while introducing an explicit form routing attribute and splitting the “no-intercept” opt-out semantics between forms and links.
Changes:
- Add
lvt-form:actionas the highest-priority form action routing source and stop reserving/excluding"action"from submitted form data. - Rename interception opt-out attributes to be semantic:
lvt-form:no-intercept(forms) andlvt-nav:no-intercept(links). - Update/extend tests to cover
lvt-form:actionpriority and"action"field preservation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
dom/event-delegation.ts |
Adds lvt-form:action routing and preserves "action" as normal form data. |
dom/link-interceptor.ts |
Switches link opt-out attribute to lvt-nav:no-intercept. |
state/change-auto-wirer.ts |
Switches form opt-out attribute to lvt-form:no-intercept for change auto-wiring. |
tests/event-delegation.test.ts |
Adds tests for lvt-form:action routing priority and "action" data preservation. |
tests/change-auto-wirer.test.ts |
Renames tests to use lvt-form:no-intercept for form opt-out coverage. |
Comments suppressed due to low confidence (1)
dom/link-interceptor.ts:66
- Link interception opt-out behavior changed from the old attribute to
lvt-nav:no-intercept, but there are no automated tests coveringLinkInterceptor.shouldSkip()/ click interception. Adding a small unit test for same-origin links with/withoutlvt-nav:no-intercept(and optionally backward-compat if intended) would help prevent regressions.
private shouldSkip(link: HTMLAnchorElement): boolean {
// External links
if (link.origin !== window.location.origin) return true;
// target="_blank" or other targets
if (link.target && link.target !== "_self") return true;
// Download links
if (link.hasAttribute("download")) return true;
// Opt-out attribute for link interception
if (link.hasAttribute("lvt-nav:no-intercept")) return true;
// Hash-only links (scroll anchors)
if (link.pathname === window.location.pathname && link.hash) return true;
// mailto/tel/javascript
const protocol = link.protocol;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Action resolution order: | ||
| // 1. lvt-form:action attribute (explicit routing) | ||
| // 2. submitter.name (button name = action) | ||
| // 3. form.name (form name = action) | ||
| // 4. "" (server defaults to Submit()) | ||
| if (!action && eventType === "submit" && element instanceof HTMLFormElement) { | ||
| if (!element.hasAttribute("lvt-form:no-intercept")) { | ||
| // Check for explicit routing attribute first | ||
| const explicitAction = element.getAttribute("lvt-form:action"); | ||
| const submitter = (e as SubmitEvent).submitter; | ||
| if (submitter instanceof HTMLButtonElement && submitter.name) { | ||
| action = submitter.name; | ||
| } else if (element.name) { | ||
| action = element.name; | ||
| if (explicitAction) { | ||
| action = explicitAction; | ||
| } else { | ||
| action = "submit"; | ||
| if (submitter instanceof HTMLButtonElement && submitter.name) { | ||
| action = submitter.name; | ||
| } else if (element.name) { | ||
| action = element.name; | ||
| } else { | ||
| action = "submit"; | ||
| } |
There was a problem hiding this comment.
The action resolution comment says the final fallback is an empty string (""), but the implementation sets action = "submit". Update the comment (or the fallback value) so docs match behavior; the PR description also states the default should be "submit".
| // Action resolution order: | ||
| // 1. lvt-form:action attribute (explicit routing) | ||
| // 2. submitter.name (button name = action) | ||
| // 3. form.name (form name = action) | ||
| // 4. "" (server defaults to Submit()) | ||
| if (!action && eventType === "submit" && element instanceof HTMLFormElement) { | ||
| if (!element.hasAttribute("lvt-form:no-intercept")) { | ||
| // Check for explicit routing attribute first | ||
| const explicitAction = element.getAttribute("lvt-form:action"); | ||
| const submitter = (e as SubmitEvent).submitter; | ||
| if (submitter instanceof HTMLButtonElement && submitter.name) { | ||
| action = submitter.name; | ||
| } else if (element.name) { | ||
| action = element.name; | ||
| if (explicitAction) { | ||
| action = explicitAction; | ||
| } else { |
There was a problem hiding this comment.
PR description mentions a legacy lvt-action hidden field in the action resolution order, but this client code doesn't read any such field (and it isn't referenced elsewhere in the repo). Either implement that resolution step here or adjust the PR description to avoid advertising unsupported behavior.
dom/event-delegation.ts
Outdated
| // Determine the action routing key to exclude from form data. | ||
| // Only the submitter button's name is excluded (it's the routing key, not data). | ||
| // "action" is NOT excluded — it's a normal data field. | ||
| const submitterForData = (targetElement as any).__lvtSubmitter as HTMLButtonElement | undefined; | ||
| const actionFieldName = submitterForData?.name || "action"; | ||
| const actionFieldName = submitterForData?.name; | ||
|
|
||
| formData.forEach((value, key) => { | ||
| if (key === actionFieldName || key === "action") return; | ||
| if (actionFieldName && key === actionFieldName) return; |
There was a problem hiding this comment.
The comment says the submitter button’s name is excluded because it’s the “routing key”. With lvt-form:action overriding button-name routing, the submitter name may no longer be the routing key, but it’s still being excluded from message.data. Consider clarifying the comment to reflect the actual reason (e.g., always omit the successful submitter control to avoid noisy payloads), or conditionally exclude it only when it’s used for routing.
- Fix action resolution comment: fallback is 'submit' not '' - Add note: lvt-action is server-side only, client doesn't read it - Add comment: empty lvt-form:action='' falls through to submitter - Clarify submitter exclusion: always excluded to avoid noisy payloads Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ReviewBreaking attribute renames (needs migration note)The rename of These are reasonable breaking changes, but the PR description should call them out explicitly, and if there's a changelog/migration guide it should be updated before merge. Minor logic noteIn the data exclusion filter: // Before
const actionFieldName = submitterForData?.name || "action";
if (key === actionFieldName || key === "action") return;
// After
const actionFieldName = submitterForData?.name;
if (actionFieldName && key === actionFieldName) return;This also changes behavior for forms with no submitter button name: previously the No bugs foundLogic, type safety, and test coverage look solid. The |
Summary
Addresses 4 feedback items from the Phase 1A attribute reduction:
1.
actionfield name conflict (data loss fix)actionwas a reserved routing field excluded from ActionData. This silently dropped any legitimate form field namedaction— common in e-commerce checkouts, workflow forms, and multi-step wizards. Nowactionflows through as normal form data.2.
lvt-form:action— explicit routing attributeNew
lvt-form:actionattribute on<form>elements for explicit action routing (highest priority in resolution order). Replaces the old implicitactionfield reservation.Action resolution order (updated):
lvt-form:actionattribute on form element (NEW)lvt-actionhidden field (legacy progressive enhancement)namenameattribute"submit"3.
lvt-nav:no-intercept— semantic link interceptionlvt-form:no-intercepton<a>tags was semantically wrong. Now:lvt-form:no-intercept→ forms onlylvt-nav:no-intercept→ links only4.
lvt-keyclarificationlvt-keystays as-is (notlvt-mod:key): it's an event filter (determines whether an event fires), not a timing modifier like debounce/throttle.Changes
dom/event-delegation.ts: Addlvt-form:actioncheck, stop excludingactionfrom datadom/link-interceptor.ts:lvt-no-intercept→lvt-nav:no-interceptstate/change-auto-wirer.ts:lvt-no-intercept→lvt-form:no-intercept(forms)Testing
All 285 tests pass across 18 suites.
Companion PR: livetemplate/livetemplate (server-side action field fix)