Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
adnaan merged 8 commits into from
Apr 11, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6fb0504
feat: migrate add modal from div+hidden to native dialog+command/comm…
adnaan Apr 11, 2026
c0d773e
fix: update golden file for native dialog template migration
adnaan Apr 11, 2026
c326435
fix: address bot review comments — remove method=dialog, add backdrop
adnaan Apr 11, 2026
6977dc2
ci: re-trigger after client v0.8.21 release (polyfill now in CDN @lat…
adnaan Apr 11, 2026
905a846
fix: update E2E test selectors for native dialog pattern
adnaan Apr 11, 2026
590c9f5
fix: update remaining E2E test selectors for native dialog
adnaan Apr 11, 2026
2fe2424
fix: fix remaining old selectors and edit form focus issue
adnaan Apr 11, 2026
616b596
fix: use JS for edit form submit (focus/click reliability with dialog)
adnaan Apr 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 39 additions & 38 deletions e2e/complete_workflow_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,10 +198,10 @@ func TestCompleteWorkflow_BlogApp(t *testing.T) {
validateNoTemplateExpressions("[data-lvt-id]"),

// Click Add button to open modal
chromedp.WaitVisible(`[data-lvt-target="#add-modal"]`, chromedp.ByQuery),
chromedp.Click(`[data-lvt-target="#add-modal"]`, chromedp.ByQuery),
chromedp.WaitVisible(`[command="show-modal"][commandfor="add-modal"]`, chromedp.ByQuery),
chromedp.Click(`[command="show-modal"][commandfor="add-modal"]`, chromedp.ByQuery),
// Wait for modal to open
waitFor(`document.querySelector('[role="dialog"]') && !document.querySelector('[role="dialog"]').hasAttribute('hidden')`, 3*time.Second),
waitFor(`document.querySelector('dialog#add-modal')?.open === true`, 3*time.Second),

// Fill form
chromedp.WaitVisible(`input[name="title"]`, chromedp.ByQuery),
Expand Down Expand Up @@ -277,19 +277,31 @@ func TestCompleteWorkflow_BlogApp(t *testing.T) {
}
t.Log("✅ Edit form appeared")

// Update title
// Ensure add dialog is fully closed before interacting with edit form
err = chromedp.Run(ctx,
chromedp.Clear(`form[name="update"] input[name="title"]`),
chromedp.SendKeys(`form[name="update"] input[name="title"]`, "My Updated Blog Post", chromedp.ByQuery),
waitFor(`!document.querySelector('dialog#add-modal')?.open`, 3*time.Second),
)
if err != nil {
t.Fatalf("Failed to update title: %v", err)
t.Logf("Warning: add dialog may still be open: %v", err)
}
t.Log("✅ Title updated in form")

// Submit and wait for WebSocket update
// Update title and submit — use JavaScript for reliability
// (chromedp.Clear/Click can fail with "not focusable" when a dialog
// was recently in the top layer, affecting the edit modal overlay)
err = chromedp.Run(ctx,
chromedp.Click(`form[name="update"] button[type="submit"]`, chromedp.ByQuery),
chromedp.Evaluate(`
(() => {
const form = document.querySelector('form[name="update"]');
const input = form?.querySelector('input[name="title"]');
if (!form || !input) return false;
input.focus();
input.value = 'My Updated Blog Post';
input.dispatchEvent(new Event('input', { bubbles: true }));
const submitBtn = form.querySelector('button[type="submit"]');
if (submitBtn) submitBtn.click();
return true;
})()
`, nil),
)
if err != nil {
t.Fatalf("Failed to submit form: %v", err)
Expand Down Expand Up @@ -363,7 +375,7 @@ func TestCompleteWorkflow_BlogApp(t *testing.T) {
// Step 4: Wait for add button
t.Log("[Delete_Post] Step 4: Waiting for add button...")
err = chromedp.Run(ctx,
chromedp.WaitVisible(`[data-lvt-target="#add-modal"]`, chromedp.ByQuery),
chromedp.WaitVisible(`[command="show-modal"][commandfor="add-modal"]`, chromedp.ByQuery),
)
if err != nil {
t.Fatalf("[Delete_Post] Step 4 failed (wait for add button): %v", err)
Expand All @@ -376,14 +388,13 @@ func TestCompleteWorkflow_BlogApp(t *testing.T) {
var openResult map[string]interface{}
err = chromedp.Run(ctx,
chromedp.Evaluate(`(() => {
const modal = document.querySelector('#add-modal');
const modal = document.querySelector('dialog#add-modal');
if (!modal) {
return { success: false, error: 'add-modal not found' };
return { success: false, error: 'add-modal dialog not found' };
}
if (!modal.open) {
modal.showModal();
}
// Open modal the same way the ModalManager does
modal.removeAttribute('hidden');
modal.style.display = 'flex';
modal.setAttribute('aria-hidden', 'false');
return { success: true, modalId: modal.id };
})()`, &openResult),
)
Expand All @@ -393,33 +404,26 @@ func TestCompleteWorkflow_BlogApp(t *testing.T) {
if openResult["success"] != true {
t.Fatalf("[Delete_Post] Step 5 failed: %v", openResult["error"])
}
t.Log("[Delete_Post] Step 5: Add modal opened via DOM manipulation")
t.Log("[Delete_Post] Step 5: Add dialog opened via .showModal()")

// Step 5b: Diagnostic - check modal state after click
time.Sleep(500 * time.Millisecond) // Brief wait for modal to react
var modalState map[string]interface{}
chromedp.Run(ctx,
chromedp.Evaluate(`(() => {
const modal = document.querySelector('[role="dialog"]');
const addModal = document.querySelector('#add-modal');
const addModal = document.querySelector('dialog#add-modal');
const titleInput = document.querySelector('input[name="title"]');
const allForms = document.querySelectorAll('form');
const allModals = document.querySelectorAll('[role="dialog"]');
return {
dialogExists: modal !== null,
dialogHidden: modal?.hasAttribute('hidden'),
addModalExists: addModal !== null,
addModalHidden: addModal?.hasAttribute('hidden'),
addModalDisplay: addModal?.style?.display,
addModalOpen: addModal?.open,
titleInputExists: titleInput !== null,
titleInputVisible: titleInput?.offsetParent !== null,
formCount: allForms.length,
modalCount: allModals.length,
bodyHTML: document.body.innerHTML.substring(0, 3000)
};
})()`, &modalState),
)
t.Logf("[Delete_Post] Step 5b: Modal state after click: %+v", modalState)
t.Logf("[Delete_Post] Step 5b: Dialog state: %+v", modalState)

// Step 6: Wait for form (with shorter timeout for faster failure feedback)
t.Log("[Delete_Post] Step 6: Waiting for form (10s timeout)...")
Expand All @@ -435,10 +439,9 @@ func TestCompleteWorkflow_BlogApp(t *testing.T) {
url: window.location.href,
readyState: document.readyState,
bodyLength: document.body.innerHTML.length,
modalDialogs: Array.from(document.querySelectorAll('[role="dialog"]')).map(m => ({
id: m.id,
hidden: m.hasAttribute('hidden'),
display: m.style.display
dialogs: Array.from(document.querySelectorAll('dialog')).map(d => ({
id: d.id,
open: d.open
}))
};
})()`, &pageState),
Expand Down Expand Up @@ -794,14 +797,12 @@ func TestCompleteWorkflow_BlogApp(t *testing.T) {
chromedp.WaitVisible(`[data-lvt-id]`, chromedp.ByQuery),

// Open add modal via DOM manipulation (more reliable than click event delegation)
chromedp.WaitVisible(`[data-lvt-target="#add-modal"]`, chromedp.ByQuery),
chromedp.WaitVisible(`[command="show-modal"][commandfor="add-modal"]`, chromedp.ByQuery),
chromedp.Evaluate(`
(() => {
const modal = document.querySelector('#add-modal');
if (modal) {
modal.removeAttribute('hidden');
modal.style.display = 'flex';
modal.setAttribute('aria-hidden', 'false');
const modal = document.querySelector('dialog#add-modal');
if (modal && !modal.open) {
modal.showModal();
}
})()
`, nil),
Expand Down
8 changes: 4 additions & 4 deletions e2e/delete_multi_post_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,8 @@ func TestDeleteWithMultiplePosts(t *testing.T) {
chromedp.WaitVisible(`[data-lvt-id]`, chromedp.ByQuery),

// Create first post — retry modal click until event handlers are attached
chromedp.WaitVisible(`[data-lvt-target="#add-modal"]`, chromedp.ByQuery),
clickUntilModalOpens(`[data-lvt-target="#add-modal"]`, `input[name="title"]`, 15*time.Second),
chromedp.WaitVisible(`[command="show-modal"][commandfor="add-modal"]`, chromedp.ByQuery),
clickUntilModalOpens(`[command="show-modal"][commandfor="add-modal"]`, `input[name="title"]`, 15*time.Second),
chromedp.SendKeys(`input[name="title"]`, "First Post", chromedp.ByQuery),
chromedp.SendKeys(`textarea[name="content"]`, "Content of first post", chromedp.ByQuery),
chromedp.Click(`button[type="submit"]`, chromedp.ByQuery),
Expand Down Expand Up @@ -131,8 +131,8 @@ func TestDeleteWithMultiplePosts(t *testing.T) {
chromedp.WaitVisible(`[data-lvt-id]`, chromedp.ByQuery),

// Create second post — retry modal click until event handlers are attached
chromedp.WaitVisible(`[data-lvt-target="#add-modal"]`, chromedp.ByQuery),
clickUntilModalOpens(`[data-lvt-target="#add-modal"]`, `input[name="title"]`, 15*time.Second),
chromedp.WaitVisible(`[command="show-modal"][commandfor="add-modal"]`, chromedp.ByQuery),
clickUntilModalOpens(`[command="show-modal"][commandfor="add-modal"]`, `input[name="title"]`, 15*time.Second),
chromedp.SendKeys(`input[name="title"]`, "Second Post", chromedp.ByQuery),
chromedp.SendKeys(`textarea[name="content"]`, "Content of second post", chromedp.ByQuery),
chromedp.Click(`button[type="submit"]`, chromedp.ByQuery),
Expand Down
2 changes: 1 addition & 1 deletion e2e/embedded_browser_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ func TestToastAutoDismiss(t *testing.T) {

// Open add modal and submit a new item via the submit button (not native form submit)
err = chromedp.Run(bctx,
chromedp.Click(`[data-lvt-target="#add-modal"]`, chromedp.ByQuery),
chromedp.Click(`[command="show-modal"][commandfor="add-modal"]`, chromedp.ByQuery),
chromedp.WaitVisible(`form[name="add"]`, chromedp.ByQuery),
chromedp.SendKeys(`input[name="name"]`, "Test Item", chromedp.ByQuery),
chromedp.Click(`form[name="add"] button[type="submit"]`, chromedp.ByQuery),
Expand Down
137 changes: 67 additions & 70 deletions e2e/modal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,33 +55,31 @@ func TestModalFunctionality(t *testing.T) {
<head>
<meta charset="UTF-8">
<title>Modal Test</title>
<style>dialog#add-modal::backdrop { background: rgba(0,0,0,0.5); }</style>
</head>
<body>
<div data-lvt-id="test-wrapper">
<button id="open-btn" lvt-el:toggleAttr:on:click="hidden" data-lvt-target="#add-modal">Add Product</button>
<button id="open-btn" command="show-modal" commandfor="add-modal">Add Product</button>
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

<!-- Modal -->
<div id="add-modal" hidden aria-hidden="true" role="dialog" data-modal-backdrop data-modal-id="add-modal"
style="position: fixed; top: 0; left: 0; width: 100%; height: 100%; background: rgba(0,0,0,0.5); display: flex; align-items: center; justify-content: center; z-index: 1000;">
<div style="background: white; border-radius: 8px; padding: 2rem; max-width: 600px; width: 90%;">
<div style="display: flex; justify-content: space-between; align-items: center; margin-bottom: 1rem;">
<h2>Add New Product</h2>
<button id="close-x" type="button" lvt-el:toggleAttr:on:click="hidden" data-lvt-target="#add-modal"
style="background: none; border: none; font-size: 1.5rem; cursor: pointer;">&times;</button>
</div>

<form>
<div style="margin-bottom: 1rem;">
<label>Name</label>
<input type="text" name="name" placeholder="Enter name" required>
</div>
<div>
<button type="submit">Add Product</button>
<button id="cancel-btn" type="button" lvt-el:toggleAttr:on:click="hidden" data-lvt-target="#add-modal">Cancel</button>
</div>
</form>
<dialog id="add-modal" style="max-width: 600px; width: 90%; border-radius: 8px; padding: 2rem;">
<div style="display: flex; justify-content: space-between; align-items: center; margin-bottom: 1rem;">
<h2>Add New Product</h2>
<button id="close-x" type="button" command="close" commandfor="add-modal"
style="background: none; border: none; font-size: 1.5rem; cursor: pointer;">&times;</button>
</div>
</div>

<form>
<div style="margin-bottom: 1rem;">
<label>Name</label>
<input type="text" name="name" placeholder="Enter name" required>
</div>
<div>
<button type="submit">Add Product</button>
<button id="cancel-btn" type="button" command="close" commandfor="add-modal">Cancel</button>
</div>
</form>
</dialog>
</div>

<script>
Expand Down Expand Up @@ -139,8 +137,8 @@ func TestModalFunctionality(t *testing.T) {
ctx, timeoutCancel := context.WithTimeout(ctx, getBrowserTimeout())
defer timeoutCancel()

// Shared variable for modal hidden state checks across ActionFuncs
var hidden bool
// Shared variable for modal open state checks across ActionFuncs
var dialogOpen bool

// Run the tests
err = chromedp.Run(ctx,
Expand Down Expand Up @@ -177,15 +175,15 @@ func TestModalFunctionality(t *testing.T) {
// Wait for client to fully initialize
waitFor(`typeof window.liveTemplateClient !== 'undefined'`, 15*time.Second),

// Test 1: Modal should be hidden initially
// Test 1: Dialog should be closed initially
chromedp.ActionFunc(func(ctx context.Context) error {
if err := chromedp.Evaluate(`document.getElementById('add-modal').hasAttribute('hidden')`, &hidden).Do(ctx); err != nil {
return fmt.Errorf("failed to check hidden attribute: %v", err)
if err := chromedp.Evaluate(`document.getElementById('add-modal').open`, &dialogOpen).Do(ctx); err != nil {
return fmt.Errorf("failed to check dialog open state: %v", err)
}
if !hidden {
return fmt.Errorf("modal should be hidden initially")
if dialogOpen {
return fmt.Errorf("dialog should be closed initially")
}
t.Log("✓ Test 1: Modal is hidden initially")
t.Log("✓ Test 1: Dialog is closed initially")
return nil
}),

Expand All @@ -211,18 +209,18 @@ func TestModalFunctionality(t *testing.T) {
t.Log("✓ Clicked open button")
return nil
}),
// Wait for modal to open
waitFor("!document.getElementById('add-modal').hasAttribute('hidden')", 3*time.Second),
// Wait for dialog to open
waitFor("document.getElementById('add-modal').open === true", 3*time.Second),

// Test 3: Verify modal is visible (hidden attribute removed)
// Test 3: Verify dialog is open
chromedp.ActionFunc(func(ctx context.Context) error {
if err := chromedp.Evaluate(`document.getElementById('add-modal').hasAttribute('hidden')`, &hidden).Do(ctx); err != nil {
return fmt.Errorf("failed to check hidden attr: %v", err)
if err := chromedp.Evaluate(`document.getElementById('add-modal').open`, &dialogOpen).Do(ctx); err != nil {
return fmt.Errorf("failed to check dialog open state: %v", err)
}
if hidden {
return fmt.Errorf("modal should not have hidden attr after open")
if !dialogOpen {
return fmt.Errorf("dialog should be open after clicking show-modal button")
}
t.Log("✓ Test 2 & 3: Modal opens (hidden attr removed)")
t.Log("✓ Test 2 & 3: Dialog opens via command='show-modal' polyfill")
return nil
}),

Expand All @@ -246,57 +244,57 @@ func TestModalFunctionality(t *testing.T) {
t.Log("✓ Clicked close button successfully")
return nil
}),
// Wait for modal to close
waitFor("document.getElementById('add-modal').hasAttribute('hidden')", 3*time.Second),
// Wait for dialog to close
waitFor("document.getElementById('add-modal').open === false", 3*time.Second),

// Test 5: Verify modal is hidden after close
// Test 5: Verify dialog is closed after close
chromedp.ActionFunc(func(ctx context.Context) error {
if err := chromedp.Evaluate(`document.getElementById('add-modal').hasAttribute('hidden')`, &hidden).Do(ctx); err != nil {
return fmt.Errorf("failed to get display style: %v", err)
if err := chromedp.Evaluate(`document.getElementById('add-modal').open`, &dialogOpen).Do(ctx); err != nil {
return fmt.Errorf("failed to check dialog open state: %v", err)
}
if hidden != true {
return fmt.Errorf("modal should have hidden attr after close, got hidden=%v", hidden)
if dialogOpen {
return fmt.Errorf("dialog should be closed after clicking close button")
}
t.Log("✓ Test 4 & 5: Modal closes with X button")
t.Log("✓ Test 4 & 5: Dialog closes with X button")
return nil
}),

// Test 6: Reopen modal (critical test - was broken before)
// Test 6: Reopen dialog (critical test - was broken before)
chromedp.ActionFunc(func(ctx context.Context) error {
if err := chromedp.Evaluate(`document.getElementById('open-btn').click()`, nil).Do(ctx); err != nil {
return fmt.Errorf("failed to reopen modal: %v", err)
return fmt.Errorf("failed to reopen dialog: %v", err)
}
return nil
}),
// Wait for modal to reopen
waitFor("!document.getElementById('add-modal').hasAttribute('hidden')", 3*time.Second),
// Wait for dialog to reopen
waitFor("document.getElementById('add-modal').open === true", 3*time.Second),

// Test 7: Verify modal reopened successfully
// Test 7: Verify dialog reopened successfully
chromedp.ActionFunc(func(ctx context.Context) error {
if err := chromedp.Evaluate(`document.getElementById('add-modal').hasAttribute('hidden')`, &hidden).Do(ctx); err != nil {
return fmt.Errorf("failed to get display style: %v", err)
if err := chromedp.Evaluate(`document.getElementById('add-modal').open`, &dialogOpen).Do(ctx); err != nil {
return fmt.Errorf("failed to check dialog open state: %v", err)
}
if hidden != false {
return fmt.Errorf("modal should not have hidden attr on reopen, got hidden=%v", hidden)
if !dialogOpen {
return fmt.Errorf("dialog should be open on reopen")
}
t.Log("✓ Test 6 & 7: Modal REOPENS successfully (critical fix)")
t.Log("✓ Test 6 & 7: Dialog REOPENS successfully (critical fix)")
return nil
}),

// Test 8: Close modal by clicking Cancel button using real browser click
// Test 8: Close dialog by clicking Cancel button using real browser click
chromedp.Click("#cancel-btn", chromedp.ByQuery),
// Wait for modal to close
waitFor("document.getElementById('add-modal').hasAttribute('hidden')", 3*time.Second),
// Wait for dialog to close
waitFor("document.getElementById('add-modal').open === false", 3*time.Second),

// Test 9: Verify modal closed with cancel
// Test 9: Verify dialog closed with cancel
chromedp.ActionFunc(func(ctx context.Context) error {
if err := chromedp.Evaluate(`document.getElementById('add-modal').hasAttribute('hidden')`, &hidden).Do(ctx); err != nil {
return fmt.Errorf("failed to get display style: %v", err)
if err := chromedp.Evaluate(`document.getElementById('add-modal').open`, &dialogOpen).Do(ctx); err != nil {
return fmt.Errorf("failed to check dialog open state: %v", err)
}
if hidden != true {
return fmt.Errorf("modal should close with cancel button")
if dialogOpen {
return fmt.Errorf("dialog should be closed after cancel button")
}
t.Log("✓ Test 8 & 9: Modal closes with Cancel button")
t.Log("✓ Test 8 & 9: Dialog closes with Cancel button")
return nil
}),

Expand Down Expand Up @@ -324,12 +322,11 @@ func TestModalFunctionality(t *testing.T) {
t.Fatalf("Browser automation failed: %v", err)
}

t.Log("\n✅ ALL MODAL TESTS PASSED!")
t.Log(" ✓ Modal opens centered (display: flex)")
t.Log(" ✓ Modal closes with X button")
t.Log(" ✓ Modal closes with Cancel button")
t.Log(" ✓ Modal closes with Escape key")
t.Log(" ✓ Modal can reopen after closing (CRITICAL FIX)")
t.Log("\n✅ ALL DIALOG TESTS PASSED!")
t.Log(" ✓ Dialog opens via command='show-modal' polyfill (.showModal())")
t.Log(" ✓ Dialog closes with X button (command='close')")
t.Log(" ✓ Dialog closes with Cancel button (command='close')")
t.Log(" ✓ Dialog can reopen after closing (CRITICAL FIX)")
t.Log(" ✓ Multiple open/close cycles work")

// Print console logs even on success for debugging
Expand Down
Loading
Loading