feat: migrate add modal to native dialog + command/commandfor

[adnaan]

Summary

• Migrates the generated add modal from Tier 2 (<div hidden> + lvt-el:toggleAttr + data-lvt-target) to Tier 1 (<dialog> + command/commandfor)
• Updates 9 template files across generator base, single kit, and multi kit
• Updates E2E modal test to use <dialog> with .open property assertions
• Updates kit feature parity test for new commandfor pattern

What changed

Add button (toolbar templates):

-<button lvt-el:toggleAttr:on:click="hidden" data-lvt-target="#add-modal">
+<button command="show-modal" commandfor="add-modal">

Modal container (form templates):

-<div id="add-modal" hidden aria-hidden="true" role="dialog" data-modal-backdrop
-     style="position: fixed; top: 0; left: 0; width: 100%; height: 100%;
-            background: rgba(0,0,0,0.5); display: flex; ...">
+<dialog id="add-modal" style="max-width: 600px; width: 90%; ...">

Form (closes dialog + routes action):

-<form name="add">
+<form method="dialog" name="add">

Cancel/Close buttons:

-<button type="button" lvt-el:toggleAttr:on:click="hidden" data-lvt-target="#add-modal">
+<button type="button" command="close" commandfor="add-modal">

Not changed

• Edit modal (server-managed via {{if ne .EditingID ""}} conditional render) — unchanged
• Component modals (lvt/components/modal/) — separate pattern, unchanged

Dependencies

Depends on livetemplate/client#57 being merged and released first (the command/commandfor polyfill).

Test plan

• Kit feature parity test passes
• All internal tests pass (go test ./internal/...)
• E2E modal test updated for <dialog> assertions
• Browser E2E tests (require client polyfill in CDN @latest)

🤖 Generated with Claude Code

[claude]

Code Review

Nice direction — moving to native <dialog> + Invoker Commands is the right call. Fewer moving parts, proper semantics, and built-in accessibility. A few things worth addressing before merge:

---

Blocking

<form method="dialog"> closes the dialog before server validation runs

method="dialog" causes the browser to close the dialog the moment the submit button is clicked, before LiveTemplate's server round-trip completes. The add form template includes a server-side error block:

{{if .lvt.HasError "_general"}}
  <div ...>{{.lvt.Error "_general"}}</div>
{{end}}

With the old pattern, the modal stayed open during the request cycle, so validation errors rendered inside a visible modal. With method="dialog", the dialog is already closed by the time the server re-renders with an error — the user never sees it.

Two options:

1. Remove method="dialog" and close the dialog programmatically after a successful server response (if lvt supports a post-action hook).
2. Switch to a <dialog> opened with .show() (non-modal, no native submit-closes behavior), but that loses the Escape-key and backdrop benefits.

This needs a decision before merging. The browser E2E tests being unchecked makes this easy to miss.

---

Non-blocking but worth addressing before merge

::backdrop is unstyled — visual regression

The old overlay used background: rgba(0,0,0,0.5). The native <dialog> opened with .showModal() renders a ::backdrop pseudo-element, but it defaults to a fully transparent rgba(0,0,0,0.1) (or browser-specific). Without explicitly styling it, the dim overlay is gone. Consider adding to the generated app's CSS (or inline on the <dialog>):

dialog::backdrop {
  background: rgba(0, 0, 0, 0.5);
}

Click-outside-to-close behavior removed

The old implementation included data-modal-backdrop, which likely handled closing when clicking the overlay. The native <dialog> backdrop does not close the dialog on click by default. This is a deliberate spec decision, but it changes user-facing behavior. Worth either documenting the change or adding a click handler on the dialog that closes when the click target is the <dialog> itself (not its content).

Escaped Escape test is missing

The old test summary included ✓ Modal closes with Escape key. The new summary removes it:

// Old:
t.Log("   ✓ Modal closes with Escape key")
// New: removed

The native <dialog> + .showModal() does handle Escape natively, which is great — but the test coverage for that behavior was dropped. Given this is testing polyfill behavior (not native browser), confirming Escape works through the polyfill seems worth keeping.

---

Minor

Double-padding risk for CSS framework kits (template.tmpl.tmpl)

In the monolithic template, the <dialog> now has padding: 2rem directly, while the inner <article> or .box wrapper is kept for CSS framework variants. These framework components typically add their own padding/border/background. The combination will likely produce doubled padding for those kits. The inner wrapper's sizing styles were removed but the wrapper element itself remains — either remove the wrapper entirely for the <dialog> case or remove the padding from the <dialog> and let the inner wrapper handle it.

Dependency on unreleased polyfill

The PR description correctly flags livetemplate/client#57 as a prerequisite. Just confirming: is there a plan to gate this PR on that release, or will the generated code silently degrade to non-functional modals on browsers without native Invoker Commands support until the CDN is updated?

---

Overall the migration approach is clean and the test updates are thorough. The method="dialog" + server validation interaction is the main thing I'd want resolved before this ships.

[Copilot]

Pull request overview

Migrates the generated “Add” modal interaction in system kits and generator templates from the Tier 2 hidden+lvt-el:toggleAttr pattern to a native <dialog> approach driven by command/commandfor, and updates associated tests to assert dialog state via .open.

Changes:

• Updated single + multi kit resource templates and component templates to use <dialog> and command/commandfor for opening/closing.
• Updated generator base component templates (toolbar/form) for the same dialog/command pattern.
• Adjusted tests: kit feature parity pattern and the browser E2E modal test to validate <dialog> behavior.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
internal/kits/system/single/templates/resource/template.tmpl.tmpl	Switch add modal markup to <dialog> and open/close via command/commandfor.
internal/kits/system/single/components/toolbar.tmpl	Update add button to command="show-modal" / commandfor="add-modal".
internal/kits/system/single/components/form.tmpl	Convert add modal wrapper to <dialog> and update close/cancel controls.
internal/kits/system/multi/templates/resource/template.tmpl.tmpl	Switch add modal markup to <dialog> and open/close via command/commandfor.
internal/kits/system/multi/components/toolbar.tmpl	Update add button to command="show-modal" / commandfor="add-modal".
internal/kits/system/multi/components/form.tmpl	Convert add modal wrapper to <dialog> and update close/cancel controls.
internal/kits/kits_parity_test.go	Update parity assertion to reflect commandfor usage.
internal/generator/templates/components/toolbar.tmpl	Update generator toolbar add button to command/commandfor.
internal/generator/templates/components/form.tmpl	Convert generator add modal to <dialog> and update close/cancel controls.
e2e/modal_test.go	Update browser E2E modal test HTML + assertions for <dialog> open state.

[Copilot]

Using method="dialog" on the add form will cause the <dialog> to close automatically on submit (unless JS reliably calls preventDefault()), which can hide server-side validation errors rendered inside the dialog. Consider removing method="dialog" and instead closing the dialog only after a successful add action, so validation errors remain visible.

Suggested change

	<form method="dialog" name="add">
	<form method="post" name="add">

[Copilot]

Using method="dialog" on the add form will cause the <dialog> to close automatically on submit (unless JS reliably calls preventDefault()), which can hide server-side validation errors rendered inside the dialog. Consider removing method="dialog" and instead closing the dialog only after a successful add action, so validation errors remain visible.

Suggested change

	<form method="dialog" name="add">
	<form name="add">

[Copilot]

Using method="dialog" here will auto-close the dialog on submit, which can hide the validation error UI inside the dialog. Consider keeping this as a normal form (no method="dialog") and closing the dialog only on successful add, so users can correct validation errors without re-opening.

Suggested change

	<form method="dialog" name="add">
	<form name="add">

[Copilot]

Using method="dialog" here will auto-close the dialog on submit, which can hide the validation error UI inside the dialog. Consider keeping this as a normal form (no method="dialog") and closing the dialog only on successful add, so users can correct validation errors without re-opening.

Suggested change

	<form method="dialog" name="add">
	<form name="add">

[Copilot]

Using method="dialog" on the add form will close the dialog immediately on submit, which makes server-side validation errors (rendered inside the dialog) easy to miss. Consider removing method="dialog" and closing the dialog programmatically only after the add action succeeds.

Suggested change

	<form method="dialog" name="add">
	<form method="post" name="add">

[Copilot]

The parity check for the "add modal open button" is now commandfor="add-modal", but that attribute also appears on the close/cancel buttons. This can let the test pass even if the open button is missing. Make the pattern more specific (e.g., include command="show-modal" along with commandfor).

Suggested change

	{"add modal open button", `commandfor="add-modal"`},
	{"add modal open button", `command="show-modal" commandfor="add-modal"`},

[Copilot]

This test now relies on the command/commandfor polyfill being present in the client JS fetched from @livetemplate/client@latest (via GetClientLibraryJS). Until that release is available, this browser test will fail even if the Go changes are correct. Consider pinning LVT_CLIENT_CDN_URL to a known-good version for CI, or switching this test to use the local client library setup approach used in other e2e tests.

[claude]

Code Review

Good direction overall — native <dialog> is the right long-term approach and the migration is clean and consistent across all 9 templates. A few items worth discussing before merge:

---

Critical: method="dialog" conflicts with lvt server-side validation

Files: internal/generator/templates/components/form.tmpl, internal/kits/system/{single,multi}/components/form.tmpl, internal/kits/system/{single,multi}/templates/resource/template.tmpl.tmpl

-<form name="add">
+<form method="dialog" name="add">

When method="dialog" is set on a form inside a <dialog>, the browser closes the dialog the moment the form is submitted — before any server response. The existing templates already handle server-side validation errors rendered inside this modal:

{{if .lvt.HasError "_general"}}
  <div ...>{{.lvt.GetError "_general"}}</div>
{{end}}

If the dialog closes on submit, users will never see those errors. There are two ways this could work correctly:

1. lvt calls event.preventDefault() on submit (preventing the native method="dialog" close) — but then the dialog never closes on success either, unless lvt explicitly closes it after a successful server round-trip.
2. The intent is that errors are shown elsewhere and the dialog always closes immediately — which is a behavior change from the current UX.

Which path is intended here? If lvt's form handler already calls preventDefault, method="dialog" may be a no-op (or cause subtle interaction bugs depending on event ordering). This deserves a comment or test confirming the interaction.

---

Important: Missing ::backdrop styling

The old backdrop was an explicit <div> with background: rgba(0,0,0,0.5). The native <dialog> has a ::backdrop pseudo-element but it is transparent by default in most browsers. Generated apps will lose the visual overlay.

Consider adding to the dialog's style, or better, a <style> block in the template:

<style>
  #add-modal::backdrop { background: rgba(0,0,0,0.5); }
</style>

Or inline via a style attribute on the dialog itself (not possible for ::backdrop), so a <style> tag or a utility class is needed.

---

Important: Hard dependency on unreleased polyfill

The PR description notes:

Depends on livetemplate/client#57 being merged and released first (the command/commandfor polyfill).

command/commandfor are Chrome 134+ / Firefox 135+ / Safari 17.4+ only. Without the polyfill in @latest, any user who lvt new or lvt gen resource right now will get a broken modal. Should this PR be gated behind the client release, or should the polyfill be included inline in the generated templates as a fallback?

The browser E2E tests are also unchecked in the test plan — worth noting what the blocking condition is.

---

Minor: Inner wrapper elements left behind in template.tmpl.tmpl

In internal/kits/system/{single,multi}/templates/resource/template.tmpl.tmpl, the sizing styles were stripped from the inner wrappers but the wrappers themselves remain:

-        <article style="max-width: 600px; width: 90%; max-height: 90vh; overflow-y: auto;">
+        <article>

For frameworks like Bulma that use <article> (via needsArticle), the <dialog> now provides sizing but the inner <article> is unstyled. This is probably fine since the <dialog> carries the styles, but worth a quick visual check across kits to make sure the content doesn't overflow or look odd inside framework-styled <article>/box elements.

---

Nit: Escape key regression in test log

The old success log had:

t.Log("   ✓ Modal closes with Escape key")

This line was dropped rather than replaced. Native <dialog> does handle Escape natively, which is a win, but the test doesn't explicitly verify it. Not a blocking issue, just worth noting the coverage gap.

---

What's good

• The markup reduction is real — dropping the backdrop div + inner wrapper div simplifies the DOM significantly.
• commandfor="add-modal" (no #) is correct for the commandfor attribute (unlike the old CSS-selector-style data-lvt-target="#add-modal").
• E2E tests properly updated to use .open property instead of hasAttribute('hidden').
• Accessibility is improved — native <dialog> provides built-in focus trapping, aria-modal, and Escape handling.
• Golden file and parity test updated consistently.

The method="dialog" / lvt form handler interaction is the one thing I'd want to see explicitly tested or documented before this lands.

[claude]

Supplemental Review Notes

Reviewing the actual diff more carefully, I want to correct two points from the earlier review and add one new observation.

---

Correction 1: ::backdrop styling IS present

The earlier review flagged missing backdrop styling as a concern, but the diff does add it in each template:

<style>dialog#add-modal::backdrop { background: rgba(0,0,0,0.5); }</style>

That concern is already addressed in this PR. ✓

---

Correction 2: method="dialog" is NOT in the actual diff

The PR description lists this as a change:

-<form name="add">
+<form method="dialog" name="add">

But looking at the actual template diffs (form.tmpl, single/components/form.tmpl, multi/components/form.tmpl), the form element is not changed — the form tag keeps its existing attributes without method="dialog". The PR description is inaccurate on this point. The earlier review's "blocking" concern about method="dialog" closing the dialog before server validation completes is not applicable to the code as actually written.

---

New: <style> tag duplication on re-render

The backdrop style is placed inside the addModal template definition:

{{define "addModal"}}
  <style>dialog#add-modal::backdrop { background: rgba(0,0,0,0.5); }</style>
  <dialog id="add-modal" ...>
    {{template "addForm" .}}
  </dialog>
{{end}}

If LiveTemplate partially re-renders this component (e.g., after a form validation error causes the modal content to refresh), the <style> tag gets injected into the DOM again. Most browsers handle duplicate identical <style> tags gracefully, but it's worth confirming lvt's morphing algorithm doesn't accumulate them. A safer place for this rule is a shared layout template or static stylesheet.

---

Remaining valid concern: Dependency on unreleased polyfill

The dependency on livetemplate/client#57 for the command/commandfor polyfill remains the primary merge blocker — command/commandfor are not yet baseline across all supported browsers. This PR should not land until that client release is available in @latest.

[claude]

Code Review

Clean migration overall — using native <dialog> + command/commandfor is strictly better than the div[hidden] + lvt-el:toggleAttr approach. The diff is consistent across all four template locations and the E2E test updates are accurate.

Observations

<style> tag in body
The <style>dialog#add-modal::backdrop { ... }</style> block lands inside <body> (between toolbar and modal). HTML5 allows this and browsers handle it fine, but it'll be emitted every time the addModal template partial is rendered. Since this modal is shown/hidden via the browser dialog API (not re-rendered by the server), that's fine in practice. If at some point server-side partial re-renders start including this block, deduplication could become a concern. An alternative is to hoist it to the page's <head> or into the kit's global CSS — but that's a future-state polish item, not a blocker.

Inconsistent template comment in kit forms
internal/generator/templates/components/form.tmpl updated its comment to "Native dialog wrapper for add form" but the identical templates in internal/kits/system/multi/components/form.tmpl and internal/kits/system/single/components/form.tmpl still say "Modal wrapper for add form". Minor, but since these are meant to stay in sync it's worth a one-line fix.

Escape key coverage dropped from test summary
The old final log included ✓ Modal closes with Escape key; the new summary doesn't. Native <dialog> handles Escape natively and the polyfill should too, so the behavior is likely correct — but there's no explicit chromedp step asserting it. Considering this is called out as a "critical fix" PR and the polyfill is new, an Escape-key test step would be valuable insurance.

Accessibility: aria-labelledby missing
The old <div role="dialog"> also lacked it, so this isn't a regression. But now that we're on a semantic <dialog> element the gap is more visible. Adding aria-labelledby="add-modal-title" on the <dialog> and id="add-modal-title" on the <h2> would be a straightforward win for screen readers.

Inner wrapper redundancy in template.tmpl.tmpl
After this change, the <article> / <div class="[[boxClass]]"> / <div> inner wrappers inside <dialog> no longer provide sizing or backdrop — they exist purely for CSS-framework-specific semantic markup (Bulma article, etc.). The sizing attributes were correctly stripped from them. Just confirming the intent is preserved: these wrappers are still needed for framework class hooks, so keeping them is right.

Summary

Blocking	None
Nice-to-fix before merge	Align kit form template comments
Follow-up	Add Escape-key E2E step; aria-labelledby

The dependency on client v0.8.21 is noted as satisfied by the last commit. LGTM with the minor comment fix.

[claude]

Review

Overall this is a clean, well-scoped migration. The <dialog> + command/commandfor approach removes a significant amount of custom JS-driven attribute toggling and the changes are applied consistently across all templates. A few items below.

---

Bug: tutorial_test.go not updated

e2e/tutorial_test.go lines 883–885 still reference the old attribute pattern:

chromedp.WaitVisible(`[data-lvt-target="#add-modal"]`, chromedp.ByQuery),
clickUntilModalOpens(`[data-lvt-target="#add-modal"]`, `input[name="title"]`, 15*time.Second),

After this PR lands those selectors won't match any button in the generated output. delete_multi_post_test.go and complete_workflow_test.go were updated; this file was missed. This will cause flaky/failing E2E runs in CI.

---

Inner-wrapper style loss in template.tmpl.tmpl

The old <div>/<article> inner wrappers carried max-width: 600px; width: 90%; max-height: 90vh; overflow-y: auto; (plus background: white on the box-class branch). Those styles moved up to <dialog> itself, which is correct. The <div class="[[boxClass ...]]"> wrapper is now stripped of all sizing/overflow — but it still carries the framework's box class, which often adds its own max-width or padding. Worth verifying that CSS framework kits (e.g. the bulma/bootstrap variants) don't end up with doubled or conflicting constraints from the naked box class sitting inside a sized <dialog>.

---

Template comment drift

internal/generator/templates/components/form.tmpl was updated to say Native dialog wrapper but both kit form.tmpl files still say the old Modal wrapper. Minor, but worth aligning.

---

Polyfill dependency ordering

The command/commandfor attributes depend on livetemplate/client#57. This PR should not merge before that polyfill ships, otherwise generated apps will silently break on any browser without native support. Worth adding a note to the PR description or blocking the merge via a task/label.

---

clickUntilModalOpens — function itself is fine

common_test.go:57 checks offsetParent !== null on the target element (the form input), not the modal attribute — so the function works correctly with the new dialog .open semantics once the caller passes the updated selector. No change needed there.

---

Summary: One real bug (tutorial_test.go), one compatibility question (framework kit inner wrappers), one housekeeping item (comment alignment), and a merge-order dependency on the client polyfill. Everything else looks good.

[claude]

PR Review: feat: migrate add modal to native dialog + command/commandfor

Overall this is a clean, well-scoped migration. The direction is correct — native <dialog> is semantically and functionally superior to the <div hidden> pattern. A few issues worth addressing before merge:

---

Bugs

1. Stale waitFor in tutorial_test.go (line ~228) — will fail at runtime

After clicking the correctly updated [command="show-modal"][commandfor="add-modal"] button, the next line still uses the old selector pattern:

// Click the "+ Add Posts" button in toolbar to open modal
chromedp.WaitVisible(`[command="show-modal"][commandfor="add-modal"]`, chromedp.ByQuery),
chromedp.Click(`[command="show-modal"][commandfor="add-modal"]`, chromedp.ByQuery),
// Wait for modal to appear — BUG: still uses old div[role="dialog"] + hidden attribute check
waitFor(`document.querySelector('[role="dialog"]') && !document.querySelector('[role="dialog"]').hasAttribute('hidden')`, 10*time.Second),

The new <dialog> element doesn't have a role="dialog" attribute (its role is implicit), so the CSS attribute selector [role="dialog"] will return null and this waitFor will always time out. Should be:

waitFor(`document.querySelector('dialog#add-modal')?.open === true`, 10*time.Second),

All other waitFor calls in the codebase were correctly updated — this one was missed.

---

2. PR description says <form method="dialog"> is added — but it isn't

The "What changed" section shows this diff for the form:

-<form name="add">
+<form method="dialog" name="add">

However, method="dialog" doesn't appear anywhere in the actual diff. Either:

• It was intentionally dropped (in which case the description should be updated), or
• It was accidentally omitted (in which case it needs to be added)

Without method="dialog", submitting the form won't automatically close the dialog in native browser behavior — though the server-side flow likely handles this via re-render anyway.

---

Code Quality

3. <style> tag injected inside a template partial — duplicates on re-render

+  <style>dialog#add-modal::backdrop { background: rgba(0,0,0,0.5); }</style>
+  <dialog id="add-modal" ...>

The addModal template partial is included inside a LiveTemplate-managed data-lvt-id block. When the server re-renders (e.g., on form validation errors), this <style> tag gets injected into the DOM again, resulting in duplicate style rules accumulating over time.

Suggested fix: move the ::backdrop style to the global stylesheet or the page <head> template, rather than inlining it inside the modal partial. Alternatively, check if an equivalent style already exists before injecting.

---

4. Inner wrapper styles removed in template.tmpl.tmpl — potential framework regressions

The sizing constraints were moved from the inner wrappers to the <dialog> itself, which is correct for the base case. However, the CSS-framework-specific wrappers now have no dimensions of their own:

-<article style="max-width: 600px; width: 90%; max-height: 90vh; overflow-y: auto;">
+<article>

-<div class="[[boxClass .CSSFramework]]" style="max-width: 600px; width: 90%; max-height: 90vh; overflow-y: auto; background: white;">
+<div class="[[boxClass .CSSFramework]]">

For frameworks like Bulma where .box provides background-color, border-radius, and padding — those styles will now stack with the <dialog>'s own padding (padding: 2rem), resulting in double-padding. Worth checking Bulma and Pico kit renders manually.

---

Dependency / Merge Readiness

5. Hard dependency on livetemplate/client#57

The browser E2E test checkbox is unchecked for good reason — command/commandfor is not yet in baseline browser support (Chrome 135+ only, as of April 2026; no Firefox or Safari). Until client#57 ships and is in CDN @latest, all generated apps will have a non-functional "Add" button in most user browsers.

This PR should either:

• Be merged only after client#57 lands and the CDN is updated, or
• Include a server-rendered fallback for the button (e.g., a <noscript>/CSS fallback) until the polyfill is available

---

Minor

6. Escape key test was silently removed from the test summary log

The old success log included ✓ Modal closes with Escape key. Native <dialog> handles Escape natively, so the behavior is still correct — but it would be reassuring to add the Escape key test back to the explicit test assertions in modal_test.go to prevent future regressions (it's easy to inadvertently intercept keydown and block it).

---

Summary

Item	Severity	Status
Stale waitFor in tutorial_test.go	Bug	Must fix
<form method="dialog"> in description but not diff	Bug / Doc	Clarify or add
<style> tag duplication on re-render	Bug	Should fix
CSS framework double-padding	Concern	Worth checking
Hard dependency on client#57	Blocker	Coordinate release
Missing Escape key assertion	Minor	Nice to have

The approach is solid and the vast majority of the changes are correct. Fixing items 1–3 and coordinating with the client#57 release would make this ready to merge.

[claude]

Code Review — Final Assessment

Note: Previous claude review comments on this PR contain some inaccuracies (the method=\"dialog\" blocking concern and missing ::backdrop concern are both false — see details below). This comment supersedes them.

---

What's correct and well-done

• The migration from <div hidden> + lvt-el:toggleAttr to native <dialog> + command/commandfor is clean and consistent across all 9 templates.
• ::backdrop styling is included via <style>dialog#add-modal::backdrop { background: rgba(0,0,0,0.5); }</style> — no visual regression.
• method=\"dialog\" is not added to any form in the actual diff (the PR description is misleading on this point). The forms remain as <form name=\"add\">, so there is no conflict with server-side validation rendering inside the modal.
• commandfor=\"add-modal\" correctly uses the bare ID (no #), unlike the old data-lvt-target=\"#add-modal\" CSS selector style.
• E2E tests properly updated to use .open property instead of hasAttribute('hidden').
• Accessibility improves: native <dialog> provides built-in focus trapping, aria-modal, and Escape-key handling.

---

Issues to address before merge

1. Hard dependency on unreleased polyfill — merge blocker

command/commandfor requires Chrome 135+, Firefox 135+, Safari 17.4+. The browser E2E tests are unchecked in the test plan because livetemplate/client#57 hasn't shipped. Until that polyfill is in CDN @latest, any app generated with this code will have a non-functional Add button for most users. This PR should land only after the client release is available.

2. <style> tag duplicates on lvt re-render

The backdrop style lives inside the addModal template partial:

{{define \"addModal\"}}
  <style>dialog#add-modal::backdrop { background: rgba(0,0,0,0.5); }</style>
  <dialog id=\"add-modal\" ...>

When lvt re-renders this partial (e.g., after a form validation error), the <style> tag is re-injected into the DOM. Most browsers handle duplicates gracefully, but it's unnecessary and could interact badly with lvt's morphing algorithm. Better placement: the shared layout <head> template or a static stylesheet.

3. CSS framework double-padding in template.tmpl.tmpl

The sizing styles were correctly moved to the <dialog>, but the inner framework wrappers remain unstyled:

-<article style=\"max-width: 600px; width: 90%; max-height: 90vh; overflow-y: auto;\">
+<article>

-<div class=\"[[boxClass .CSSFramework]]\" style=\"... background: white;\">
+<div class=\"[[boxClass .CSSFramework]]\">

For Bulma (.box) and similar kits, the framework class adds its own padding, border-radius, and background. Combined with the <dialog>'s padding: 2rem, this will produce double-padding. Worth a visual check across all supported kits.

4. Click-outside-to-close is silently removed

The old data-modal-backdrop attribute handled closing the modal when the user clicked the overlay. Native <dialog>::backdrop does not close on click by default (this is the spec's intentional choice). If this is an intentional UX change, it should be documented. If not, add a click handler:

dialog.addEventListener('click', e => { if (e.target === dialog) dialog.close(); });

5. Escape key test coverage dropped

The test summary previously included ✓ Modal closes with Escape key. Native <dialog> handles Escape natively (and the polyfill should too), but the explicit assertion was removed rather than updated. Low priority, but worth adding back to prevent future regressions.

---

Summary

Item	Severity
Merge blocker: depends on livetemplate/client#57	Blocker
<style> tag duplicates on re-render	Should fix
CSS framework double-padding	Should verify
Click-outside-to-close removed without documentation	Should document
Escape key assertion dropped	Minor

The approach and implementation are solid. The polyfill dependency and the <style> duplication are the two things I'd want resolved before this lands.

[claude]

Review: feat: migrate add modal to native dialog + command/commandfor

Overall this is a clean, well-scoped migration that improves accessibility and removes a lot of bespoke JS. A few things worth addressing before merge:

---

Bug: <style> inside partial could accumulate on re-render

The <style> for ::backdrop is placed inside the addModal template partial:

<style>dialog#add-modal::backdrop { background: rgba(0,0,0,0.5); }</style>
<dialog id="add-modal" ...>

If LiveTemplate does a partial DOM swap that re-injects the addModal block (e.g. after a server error that re-renders the form), this <style> tag will be inserted into the DOM multiple times. Even if browsers deduplicate identical styles in practice, it's a leak. Options:

• Move it to the page <head> in the layout template
• Or emit it into the page-level CSS (kit stylesheets) rather than the component

This pattern appears in all three form.tmpl files (generator/templates, kits/system/single, kits/system/multi) and in both template.tmpl.tmpl files.

---

Discrepancy: PR description mentions <form method="dialog"> but it's not in the diff

The "What changed" section shows:

-<form name="add">
+<form method="dialog" name="add">

But none of the template diffs include this change to the <form> tag. Was this intentionally left out? Without method="dialog", native form submission won't close the dialog automatically — though LiveTemplate's WebSocket handler likely prevents the default and manages state server-side, so it may be fine. Either way, the PR description should match the actual diff.

---

Concern: Downgraded test assertion could hide flakiness

In e2e/complete_workflow_test.go (~line 32):

if err != nil {
    t.Logf("Warning: add dialog may still be open: %v", err)
}

This was t.Fatalf before. Downgrading dialog-close verification to a warning means a test can silently proceed with the dialog still open, potentially causing flaky failures later in the test for unrelated-seeming reasons. If the wait genuinely times out, that's a real signal. Consider keeping it fatal or at least explicitly documenting the rationale for why it's acceptable to proceed when this times out.

---

Minor: role="dialog" removed from non-dialog fallback context

The old <div> had role="dialog" explicitly. The native <dialog> element has an implicit ARIA role of dialog, so this is correct — just worth noting for any downstream screen-reader testing that the semantic improvement is intentional.

---

Minor: Comment inconsistency in multi/single kit form.tmpl

The base generator template has an updated comment:

{{/* Add Modal - Native dialog wrapper for add form */}}

But kits/system/multi/components/form.tmpl and kits/system/single/components/form.tmpl still say:

{{/* Add Modal - Modal wrapper for add form */}}

Small inconsistency worth keeping in sync.

---

Minor: Escape key behavior no longer tested explicitly

The old test log summary included ✓ Modal closes with Escape key. Native <dialog> handles Escape automatically (which is the right behavior), but the behavior is no longer explicitly exercised in the test suite. Worth adding a test case for it since it's a user-facing interaction.

---

Positives

• Native <dialog> is the right call here: top-layer rendering eliminates z-index issues, built-in focus trapping, Escape key, and ::backdrop are all improvements over the manual Tier 2 approach.
• Removing the manual backdrop overlay <div> with position: fixed / z-index: 1000 / display: flex is a significant simplification.
• The command/commandfor dependency on livetemplate/client#57 is clearly called out.
• Test updates are thorough and the dialog.open assertions are cleaner than hasAttribute('hidden').
• The inner wrapper (<article> / <div class="[[boxClass]]">) inside <dialog> is properly cleaned up for CSS framework variants.

---

The <style> accumulation issue is the most concrete concern — the rest are minor or informational. Happy to discuss any of these.