feat: polyfill command/commandfor for cross-browser dialog support

[adnaan]

Summary

• Adds a lightweight polyfill for the HTML Invoker Commands API (command/commandfor) so <button command="show-modal" commandfor="dialog-id"> works in Firefox and Safari (not just Chrome 135+)
• The polyfill calls .showModal() / .close() on target <dialog> elements, keeping dialog open/close in Tier 1 (standard HTML, no lvt-* attributes needed)
• Feature detection via commandForElement makes the polyfill a no-op when browsers add native support

Context

The Dialog Routing docs describe command/commandfor as a Tier 1 pattern for opening/closing <dialog>, but it only works natively in Chrome 135+. This was discovered during lvt PR #292 (14 e2e failures in CI Docker). Rather than downgrading dialog routing to Tier 2 (lvt-el:toggleAttr + data-lvt-target), this polyfill makes the documented pattern actually work cross-browser.

The existing commandfor exclusion in event-delegation.ts:136 already prevents these buttons from triggering server actions — this polyfill completes the picture by handling the client-side dialog behavior.

Test plan

• 10 unit tests covering: show-modal, close, child element clicks, non-dialog targets, nonexistent targets, already-open/closed guards, feature detection skip, unknown commands
• E2E test with new dialog-patterns example (separate PR in examples repo, depends on this release)
• Verify polyfill is no-op in Chrome 135+ (feature detection)

🤖 Generated with Claude Code

[claude]

Good polyfill — clean implementation with solid test coverage. Two minor issues worth addressing:

1. Missing disabled button guard

The polyfill handles programmatic button.click() on disabled buttons, but browsers don't fire click events for user-clicks on disabled buttons. Native invoker commands respect the disabled attribute in both cases. Add a guard to stay consistent:

// dom/invoker-polyfill.ts, after finding the button
if (!button || button.disabled) return;

2. e.target null safety

(e.target as Element).closest(...) will throw if e.target is null. It's an unlikely edge case for click events, but easy to guard:

const button = (e.target as Element | null)?.closest(
  "button[commandfor]"
) as HTMLButtonElement | null;

---

Everything else looks good — the closest()-based delegation correctly handles child element clicks (tested), the feature detection via commandForElement is the right check, and the installed guard prevents double-registration. The scope being limited to dialog commands is appropriate and clearly documented.

[Copilot]

Pull request overview

Adds a small client-side polyfill so the documented Tier 1 dialog pattern using the HTML Invoker Commands API (command / commandfor) works in browsers without native support, while remaining a no-op when native support is detected.

Changes:

• Introduces setupInvokerPolyfill() to handle show-modal / close invoker commands for <dialog> targets via document-level click delegation.
• Wires the polyfill into LiveTemplateClient.connect() so it’s enabled by default for client users.
• Adds a dedicated Jest test suite covering the polyfill’s behavior and guardrails.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
dom/invoker-polyfill.ts	New polyfill implementation with feature detection + install/teardown hooks.
livetemplate-client.ts	Calls setupInvokerPolyfill() during client connection initialization.
tests/invoker-polyfill.test.ts	Adds unit tests validating open/close behavior, skipping, and edge cases.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This test mutates HTMLButtonElement.prototype but the cleanup (delete ...commandForElement) is done inline at the end of the test. If an assertion throws before that line runs, the prototype change can leak into later tests and cause confusing failures. Consider capturing the original property descriptor and restoring it in an afterEach (or using a try/finally in the test) so cleanup is guaranteed.

Suggested change

	// Simulate native support by adding commandForElement to prototype
	Object.defineProperty(HTMLButtonElement.prototype, "commandForElement", {
	value: null,
	writable: true,
	configurable: true,
	});
	const dialog = document.createElement("dialog");
	dialog.id = "test-dialog";
	const { showModal } = mockDialogMethods(dialog);
	document.body.appendChild(dialog);
	const button = document.createElement("button");
	button.setAttribute("command", "show-modal");
	button.setAttribute("commandfor", "test-dialog");
	document.body.appendChild(button);
	setupInvokerPolyfill();
	button.click();
	// Polyfill should not have installed its listener
	expect(showModal).not.toHaveBeenCalled();
	// Clean up the mock property
	delete (HTMLButtonElement.prototype as any).commandForElement;
	const originalCommandForElementDescriptor = Object.getOwnPropertyDescriptor(
	HTMLButtonElement.prototype,
	"commandForElement",
	);
	try {
	// Simulate native support by adding commandForElement to prototype
	Object.defineProperty(HTMLButtonElement.prototype, "commandForElement", {
	value: null,
	writable: true,
	configurable: true,
	});
	const dialog = document.createElement("dialog");
	dialog.id = "test-dialog";
	const { showModal } = mockDialogMethods(dialog);
	document.body.appendChild(dialog);
	const button = document.createElement("button");
	button.setAttribute("command", "show-modal");
	button.setAttribute("commandfor", "test-dialog");
	document.body.appendChild(button);
	setupInvokerPolyfill();
	button.click();
	// Polyfill should not have installed its listener
	expect(showModal).not.toHaveBeenCalled();
	} finally {
	if (originalCommandForElementDescriptor) {
	Object.defineProperty(
	HTMLButtonElement.prototype,
	"commandForElement",
	originalCommandForElementDescriptor,
	);
	} else {
	delete (HTMLButtonElement.prototype as any).commandForElement;
	}
	}

[claude]

Good implementation overall — clean feature detection, proper singleton guard, comprehensive tests.

One bug to fix: In handleClick, e.target is cast directly to Element without a null/type check:

const button = (e.target as Element).closest("button[commandfor]") as HTMLButtonElement | null;

If e.target is null (e.g. the element was removed from the DOM mid-dispatch) or is not an Element (e.g. a text node), .closest() will throw a TypeError. Add a guard:

const el = e.target;
if (!el || !(el instanceof Element)) return;
const button = el.closest("button[commandfor]") as HTMLButtonElement | null;

Minor note: The polyfill is only installed once globally (module-level installed flag), which is correct — but if LiveTemplateClient is ever instantiated before DOMContentLoaded, the document.addEventListener call in handleClick references document.getElementById at click time, so that's fine. No issue there.

Otherwise LGTM. The scope limitation to <dialog> elements only is appropriate and well-documented.

[claude]

Good overall — the polyfill is clean and the feature detection approach is correct. Two issues worth fixing before merge:

1. Test cleanup leak (medium)

In the skips polyfill when native support exists test, the commandForElement property is deleted at the bottom of the test body. If the assertion before it throws, the cleanup never runs, leaving a mutated HTMLButtonElement.prototype that could silently break subsequent tests. Wrap in try/finally or move the deletion into the suite's afterEach:

afterEach(() => {
  teardownInvokerPolyfill();
  delete (HTMLButtonElement.prototype as any).commandForElement;
  document.body.innerHTML = '';
});

2. Module singleton + teardown risk (medium)

The module-level installed flag means setupInvokerPolyfill() is a no-op after the first call — fine for normal use. But teardownInvokerPolyfill() removes the global listener unconditionally. If LiveTemplateClient ever wires teardownInvokerPolyfill into a destroy() path (e.g. for hot-reload or multi-instance scenarios), a single client teardown would kill the polyfill for all remaining instances. Either add a ref-count, or document that teardown is only for tests and should not be called from production client cleanup.

Minor note (no action needed): Unlike setupFxLifecycleListeners(this.wrapperElement), the polyfill attaches to document globally — intentional for a polyfill, but worth a brief comment in livetemplate-client.ts near the call site so the asymmetry is obvious.

[claude]

Review: polyfill command/commandfor for cross-browser dialog support

Overall this is well-structured with good test coverage. A few things worth addressing:

---

Bug risk: bubble-phase listener can be silently bypassed (dom/invoker-polyfill.ts:37)

document.addEventListener(click, handleClick);

The polyfill registers on the bubble phase. If any intermediate element calls e.stopPropagation() before the event reaches document, the polyfill silently does nothing. Native invoker commands fire in the capture phase. Consider:

document.addEventListener(click, handleClick, { capture: true });
// and in teardown:
document.removeEventListener(click, handleClick, { capture: true });

---

Docblock overpromises scope (dom/invoker-polyfill.ts:1-8)

The file comment says it polyfills "the HTML Invoker Commands API (command/commandfor)" but the implementation only handles show-modal and close on <dialog> elements. Popover commands (show-popover, hide-popover, toggle-popover) are silently ignored. This is probably intentional given the PR scope, but the docblock should say so explicitly to avoid confusion when someone tries to use it for popovers later.

---

Missing idempotency test

There's no test for calling setupInvokerPolyfill() twice without an intervening teardownInvokerPolyfill(). The installed flag should prevent double-registration, but this isn't exercised. Worth adding given that setupReactiveAttributeListeners() (the call just above it in livetemplate-client.ts) could set a precedent for calling setup functions multiple times.

---

No security concerns. The event target + ID lookup + dialog-method calls path is safe.