Phase 1A: Client attribute reduction — generic event router + removals#44
Phase 1A: Client attribute reduction — generic event router + removals#44
Conversation
Implement the attribute-reduction-proposal Phase 1A changes:
Generic event router:
- Replace 16 individual event attrs (lvt-click, lvt-keydown, etc.) with
lvt-on[:{scope}]:{event} pattern (scopes: window, document)
- Add parseLvtOn() parser with ordered greedy scope consumption
Prefix consolidation:
- lvt-scroll/highlight/animate → lvt-fx:scroll/highlight/animate
- lvt-debounce/throttle → lvt-mod:debounce/throttle
- lvt-preserve → lvt-form:preserve
- lvt-disable-with → lvt-form:disable-with
- lvt-no-intercept → lvt-form:no-intercept (link-interceptor.ts)
- lvt-{action}-on:{event} → lvt-el:{method}:on:{event}
Removals:
- Delete modal-manager.ts — replaced by native <dialog> + command/commandfor
- Delete utils/confirm.ts — remove lvt-confirm and lvt-data-* extraction
- Remove lvt-data-*/lvt-value-* extraction loops
- Remove lvt-change fallback logic
- Remove disable/enable from reactive attributes
New features:
- lvt-el:*:on:click-away — client-side DOM manipulation (not server action)
- CSS custom properties for directive config (--lvt-scroll-behavior, etc.)
- lvtSelector() utility for CSS colon escaping
- livetemplate.css with custom property defaults
FormLifecycleManager:
- Remove ModalManager dependency, use native dialog.close()
- Rename lvt-preserve → lvt-form:preserve
Stats: 15 files changed, -379 net lines
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review: Phase 1A Client Attribute ReductionOverall this is clean and well-structured. A few things worth flagging: Bug:
|
- Add data-* attribute extraction for non-form element events
(fixes asymmetry where window handler extracted data-* but main
handler did not after lvt-data-* removal)
- Scope click-away delegation to targeted attribute selectors instead
of querySelectorAll('*') for better performance on large DOMs
- Restore test for data-* extraction on click events
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Implements Phase 1A of the client-side “attribute reduction” effort by consolidating many directive/event attributes into fewer generic patterns, removing legacy client behaviors (modal manager, confirm/data extraction), and updating tests accordingly.
Changes:
- Introduces a generic
lvt-on[:{scope}]:{event}event routing pattern and updates delegation logic/tests. - Refactors reactive attributes to the
lvt-el:{method}:on:*pattern and removes disable/enable reactive actions. - Migrates UI directives to consolidated prefixes (
lvt-fx:*,lvt-mod:*,lvt-form:*), adds CSS custom property defaults, and removes modal-manager + confirm utilities.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/lvt-selector.ts | Adds a helper for building CSS selectors for colon-containing attribute names. |
| utils/confirm.ts | Removes lvt-confirm and lvt-data-* utilities. |
| dom/event-delegation.ts | Switches to lvt-on:* routing, updates data extraction strategy, adds click-away client-side DOM manipulation. |
| dom/reactive-attributes.ts | Updates reactive attribute grammar to lvt-el:*:on:* and removes disable/enable actions. |
| state/form-lifecycle-manager.ts | Stops using ModalManager; closes native <dialog> on success; switches preserve attr to lvt-form:preserve. |
| dom/link-interceptor.ts | Renames intercept opt-out attribute to lvt-form:no-intercept. |
| dom/directives.ts | Migrates directives to lvt-fx:* and reads configuration from CSS custom properties. |
| livetemplate-client.ts | Removes modal manager wiring and confirm/data exports; updates init comments accordingly. |
| livetemplate.css | Adds default CSS custom properties for lvt-fx:* directives. |
| tests/reactive-attributes.test.ts | Updates tests for new reactive attribute pattern and removed actions. |
| tests/event-delegation.test.ts | Updates tests for lvt-on:* and consolidated modifier/form attributes. |
| tests/directives.test.ts | Updates tests for lvt-fx:* and CSS custom property configuration. |
| tests/modal-manager.test.ts | Removes tests for deleted ModalManager. |
| tests/form-lifecycle-manager.test.ts | Updates tests to use <dialog> behavior and renamed preserve attribute. |
| dom/modal-manager.ts | Removes the legacy modal manager implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dom/reactive-attributes.ts
Outdated
| * New pattern: lvt-el:{method}:on:[{action}:]{state} | ||
| * Also supports legacy: lvt-{method}-on:[{action}:]{state} |
There was a problem hiding this comment.
The docstring says parseReactiveAttribute also supports the legacy lvt-{method}-on: pattern, but the implementation now only matches lvt-el:*:on:* and will always return null for legacy attributes. Please either add the legacy parsing back or update the comment to avoid misleading consumers/tests.
| * New pattern: lvt-el:{method}:on:[{action}:]{state} | |
| * Also supports legacy: lvt-{method}-on:[{action}:]{state} | |
| * Supported pattern: lvt-el:{method}:on:[{action}:]{state} |
dom/event-delegation.ts
Outdated
| const SCOPE_KEYWORDS = new Set(["window", "document"]); | ||
|
|
||
| /** | ||
| * Parse an lvt-on:* attribute name into scope and event. | ||
| * Grammar: lvt-on[:{scope}]:{event} | ||
| * - scope: "window" | "document" | (omitted = "element") | ||
| * - event: any native DOM event name | ||
| */ | ||
| export function parseLvtOn(attr: string): { scope: string; event: string } | null { | ||
| if (!attr.startsWith("lvt-on:")) return null; | ||
| const segs = attr.slice(7).split(":"); | ||
| if (segs.length === 0 || segs[0] === "") return null; | ||
| let scope = "element"; | ||
| if (SCOPE_KEYWORDS.has(segs[0])) scope = segs.shift()!; | ||
| const event = segs.join(":"); | ||
| if (!event) return null; | ||
| return { scope, event }; |
There was a problem hiding this comment.
parseLvtOn (and SCOPE_KEYWORDS) are currently unused in this file. Since they’re exported, this can confuse API consumers about which parsing logic is actually supported; consider either wiring this into the event router or removing it until it’s needed.
| if (existingListener) { | ||
| document.removeEventListener("click", existingListener); | ||
| } | ||
|
|
||
| const listener = (e: Event) => { | ||
| const currentWrapper = this.context.getWrapperElement(); | ||
| if (!currentWrapper) return; | ||
|
|
There was a problem hiding this comment.
setupClickAwayDelegation runs querySelectorAll('*') and then inspects every element’s attributes on every click. In large wrappers this is O(N) per click and can become a noticeable hotspot. Consider maintaining a small registry of click-away-enabled elements (e.g., populate once + update via MutationObserver) so clicks only iterate the relevant elements.
dom/event-delegation.ts
Outdated
| const target = e.target as Element; | ||
| const elements = currentWrapper.querySelectorAll("[lvt-click-away]"); | ||
|
|
||
| elements.forEach((element) => { | ||
| if (!element.contains(target)) { | ||
| const action = element.getAttribute("lvt-click-away"); | ||
| if (!action) return; | ||
|
|
||
| const message: any = { action, data: {} }; | ||
|
|
||
| Array.from(element.attributes).forEach((attr) => { | ||
| if (attr.name.startsWith("lvt-data-")) { | ||
| const key = attr.name.replace("lvt-data-", ""); | ||
| message.data[key] = this.context.parseValue(attr.value); | ||
| } | ||
| }); | ||
|
|
||
| Array.from(element.attributes).forEach((attr) => { | ||
| if (attr.name.startsWith("lvt-value-")) { | ||
| const key = attr.name.replace("lvt-value-", ""); | ||
| message.data[key] = this.context.parseValue(attr.value); | ||
| } | ||
| }); | ||
|
|
||
| this.context.send(message); | ||
| } | ||
| // Pre-filter: only scan elements that have lvt-el: attributes with click-away | ||
| // Use attribute substring selector to avoid scanning all DOM elements | ||
| const clickAwayElements = currentWrapper.querySelectorAll("[lvt-el\\:addclass\\:on\\:click-away], [lvt-el\\:removeclass\\:on\\:click-away], [lvt-el\\:toggleclass\\:on\\:click-away], [lvt-el\\:setattr\\:on\\:click-away], [lvt-el\\:toggleattr\\:on\\:click-away], [lvt-el\\:reset\\:on\\:click-away]"); | ||
| clickAwayElements.forEach((element) => { | ||
| if (element.contains(target)) return; // Click was inside, not away | ||
|
|
||
| Array.from(element.attributes).forEach((attr) => { | ||
| if (!attr.name.includes(":on:click-away")) return; | ||
| const match = attr.name.match(/^lvt-el:(\w+):on:click-away$/); | ||
| if (!match) return; | ||
| const method = match[1].toLowerCase(); | ||
| const param = attr.value; | ||
|
|
||
| switch (method) { | ||
| case "addclass": | ||
| if (param) element.classList.add(...param.split(/\s+/).filter(Boolean)); | ||
| break; | ||
| case "removeclass": | ||
| if (param) element.classList.remove(...param.split(/\s+/).filter(Boolean)); | ||
| break; | ||
| case "toggleclass": | ||
| if (param) param.split(/\s+/).filter(Boolean).forEach(c => element.classList.toggle(c)); | ||
| break; | ||
| case "setattr": | ||
| if (param) { | ||
| const ci = param.indexOf(":"); |
There was a problem hiding this comment.
The click-away DOM manipulation switch duplicates the action logic already implemented in dom/reactive-attributes.ts (parsing method names, splitting class lists, setAttr parsing, etc.). To avoid divergence (e.g., behavior differences between lifecycle-triggered and click-away-triggered actions), consider reusing the shared METHOD_MAP/executeAction logic or extracting a common helper used by both paths.
utils/lvt-selector.ts
Outdated
| /** Escapes colons in attribute names for use in CSS attribute selectors. */ | ||
| export function lvtSelector(attr: string, value?: string): string { | ||
| const escaped = attr.replace(/:/g, "\\:"); | ||
| return value !== undefined ? `[${escaped}="${value}"]` : `[${escaped}]`; |
There was a problem hiding this comment.
lvtSelector() supports an optional value, but the implementation interpolates it directly into a quoted CSS attribute selector without escaping. A value containing " or \ will produce an invalid selector (or select unexpectedly). Consider using CSS.escape (with a jsdom fallback like the existing pattern in state/change-auto-wirer.ts) and/or emitting an unquoted selector form to avoid string-escaping issues.
| /** Escapes colons in attribute names for use in CSS attribute selectors. */ | |
| export function lvtSelector(attr: string, value?: string): string { | |
| const escaped = attr.replace(/:/g, "\\:"); | |
| return value !== undefined ? `[${escaped}="${value}"]` : `[${escaped}]`; | |
| function escapeCssAttributeValue(value: string): string { | |
| if (typeof globalThis !== "undefined" && globalThis.CSS?.escape) { | |
| return globalThis.CSS.escape(value); | |
| } | |
| return value.replace(/(^-?\d)|^-$|[^\u0080-\uFFFF\w-]/g, (match, leadingDigit) => { | |
| if (leadingDigit) { | |
| return `\\${leadingDigit} `; | |
| } | |
| return `\\${match}`; | |
| }); | |
| } | |
| /** Escapes colons in attribute names for use in CSS attribute selectors. */ | |
| export function lvtSelector(attr: string, value?: string): string { | |
| const escaped = attr.replace(/:/g, "\\:"); | |
| return value !== undefined | |
| ? `[${escaped}=${escapeCssAttributeValue(value)}]` | |
| : `[${escaped}]`; |
|
Good cleanup overall — the namespace consolidation is coherent. A few things worth addressing before Phase 1B ships. Data leakage risk The switch from lvt-data-/lvt-value- to standard data-* means all data-* attributes on action elements (except data-key and data-lvt-id) now get forwarded to the server. Third-party libraries, analytics scripts, and UI frameworks routinely write data-* attributes onto DOM elements. This could inadvertently send data that was never intended for the server. Consider a more explicit opt-in — e.g., only forward data-lvt-* — rather than a broad allowlist with a short exclusion list. lvt-confirm removed without replacement checkLvtConfirm is deleted with no migration path. Any template using lvt-confirm on destructive actions will silently skip the confirmation dialog after this ships. Phase 1B should either provide an equivalent or the migration guide needs to call this out explicitly as a hard breaking change. disable/enable reactive actions removed lvt-el:addclass:on:pending is not equivalent to disabling a form control — disabled inputs are excluded from form submission, are keyboard-inaccessible, and skip constraint validation. If the intent is double-submission prevention during pending, a CSS class alone will not achieve that. Recommend keeping disable/enable in ACTION_MAP or documenting a concrete replacement. Hardcoded click-away selector list is fragile setupClickAwayDelegation hardcodes 6 method names in the querySelector string. If a new method is added to ACTION_MAP, the click-away variant silently does nothing. Generating the selector from Object.keys(ACTION_MAP) would remove the maintenance burden — and lvtSelector is already available for the escaping, though it is inconsistently unused here while being used in the window handler. |
- Remove dead code: parseLvtOn() and SCOPE_KEYWORDS (unused exports) - Fix misleading docstring in reactive-attributes.ts (no legacy pattern) - Refactor click-away to reuse executeAction() from reactive-attributes - Generate click-away selectors from method map instead of hardcoding - Escape CSS attribute values in lvtSelector() to prevent injection Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review: Phase 1A Attribute ReductionGood overall structure. A few things worth addressing before Phase 1B lands: Behavioral breaking changes (silent)
click-away semantic change — Old Bug:
|
- Replace querySelectorAll('*') in processReactiveAttributes with targeted
attribute selectors built from known method names and lifecycle events
- Use lvtSelector() in click-away delegation for consistent colon escaping
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Code ReviewOverall this is a well-structured refactor — good test coverage, clean separation of concerns, and the Issues to address before Phase 1B merge1. Click-away no longer sends server actions (behavioral regression) 2. 3. 4. CSS Minor notes
|
- Validate --lvt-scroll-behavior CSS property against known values (auto, smooth, instant) instead of raw cast - Add clarifying comments for data-key/data-lvt-id exclusion Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PR ReviewThis is a large, coherent attribute-namespace refactor with some good improvements (CSS custom props for FX config, native Bugs / Breaking Changes Without Migration1. 2. 3. 4. Performance Concern5. Catch-all fallback in } catch {
candidates = document.querySelectorAll("*");
}If an Minor6. |
- Fix lvt-form:no-intercept on links: use generic lvt-no-intercept for
link interceptor since form: prefix is semantically wrong for <a> tags
- Properly escape CSS-special chars in actionName for attribute selectors
and avoid querySelectorAll('*') fallback on invalid selectors
- Hoist VALID_SCROLL_BEHAVIORS Set to module level to avoid re-allocation
- Validate ScrollBehavior against known values
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review: Phase 1A Attribute ReductionOverall this is a clean, well-scoped refactor. A few issues worth addressing: Bug: Unintended data leakage via The switch from Consider a more targeted namespace ( Modal removal is incomplete
Any element with
Input/search live-action patterns relying on Minor: In the new data extraction block ( |
Summary
Implements Phase 1A of the attribute-reduction-proposal — all client-side TypeScript changes.
Changes
Generic event router — Replace 16 individual event attrs (
lvt-click,lvt-keydown, etc.) withlvt-on[:{scope}]:{event}pattern.Prefix consolidation:
lvt-scroll/highlight/animate→lvt-fx:*lvt-debounce/throttle→lvt-mod:*lvt-preserve/disable-with/no-intercept→lvt-form:*lvt-{action}-on:{event}→lvt-el:{method}:on:{event}Removals:
modal-manager.ts— replaced by native<dialog>+command/commandforutils/confirm.ts— removedlvt-confirmandlvt-data-*lvt-data-*/lvt-value-*extraction,lvt-changefallback,disable/enablereactive attrsNew:
lvt-el:*:on:click-away— client-side DOM manipulationlvtSelector()utility for CSS colon escapinglivetemplate.csswith defaultsStats
15 files changed, 324 insertions(+), 703 deletions(-)
Tests
18 suites, 283 tests — all passing ✅
Phase 1B (server-side changes) must be completed first.