From 3b013b7ea392aeb06d4b69713e75d95235a2a2fc Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Thu, 17 Sep 2026 13:28:26 -0700 Subject: [PATCH] Refactor row editing and insertion to use the shared modal, refs #2790 --- datasette/static/app.css | 88 +---------------------- datasette/static/edit-tools.js | 128 +++++++-------------------------- tests/test_playwright.py | 61 ++++++++++++++++ 3 files changed, 88 insertions(+), 189 deletions(-) diff --git a/datasette/static/app.css b/datasette/static/app.css index 9a57ad90..6971635b 100644 --- a/datasette/static/app.css +++ b/datasette/static/app.css @@ -1272,46 +1272,8 @@ dialog.row-delete-dialog { } dialog.row-edit-dialog { - --ink: #0f0f0f; - --paper: #eef6ff; - --muted: #6b6b6b; - --rule: #d8e6f5; - --accent: #1a56db; - --card: #ffffff; - border: none; - border-radius: var(--modal-border-radius, 0.75rem); - padding: 0; - margin: auto; width: min(720px, calc(100vw - 32px)); - max-width: 95vw; max-height: min(780px, calc(100vh - 32px)); - box-shadow: var(--modal-shadow, 0 20px 25px -5px rgba(0, 0, 0, 0.1), 0 10px 10px -5px rgba(0, 0, 0, 0.04)); - animation: datasette-modal-slide-in var(--modal-animation-duration, 0.2s) ease-out; - overflow: hidden; - font-family: system-ui, -apple-system, sans-serif; - background: var(--card); -} - -dialog.row-edit-dialog[open] { - display: flex; - flex-direction: column; -} - -dialog.row-edit-dialog::backdrop { - background: var(--modal-backdrop-bg, rgba(0, 0, 0, 0.5)); - backdrop-filter: var(--modal-backdrop-blur, blur(4px)); - -webkit-backdrop-filter: var(--modal-backdrop-blur, blur(4px)); - animation: datasette-modal-fade-in var(--modal-animation-duration, 0.2s) ease-out; -} - -.row-edit-dialog .modal-header { - padding: 20px 24px 12px; - border-bottom: 1px solid var(--rule); - display: flex; - align-items: center; - gap: 12px; - flex-shrink: 0; - min-width: 0; } .row-edit-dialog .modal-title { @@ -1320,9 +1282,6 @@ dialog.row-edit-dialog::backdrop { gap: 0.35rem; min-width: 0; max-width: 100%; - font-size: 1rem; - font-weight: 600; - color: var(--ink); } .row-edit-dialog .modal-title .row-dialog-action, @@ -1692,7 +1651,7 @@ textarea.row-edit-input { justify-content: flex-start; } -.row-edit-bulk-actions .btn { +.row-edit-bulk-actions .modal-btn { padding-left: 12px; padding-right: 12px; } @@ -1936,17 +1895,6 @@ datasette-autocomplete input[type="text"], max-width: 46rem; } -.row-edit-dialog .modal-footer { - padding: 14px 20px; - border-top: 1px solid var(--rule); - display: flex; - align-items: center; - justify-content: flex-end; - gap: 10px; - flex-shrink: 0; - background: var(--paper); -} - .row-edit-mode-link { color: var(--accent); font-size: 0.9rem; @@ -1957,39 +1905,7 @@ datasette-autocomplete input[type="text"], display: none; } -.row-edit-dialog .btn { - border: none; - border-radius: 5px; - padding: 9px 20px; - font-size: 0.85rem; - font-weight: 500; - cursor: pointer; - touch-action: manipulation; - font-family: inherit; - transition: background 0.12s; -} - -.row-edit-dialog .btn-ghost { - background: transparent; - color: var(--muted); - border: 1px solid var(--rule); -} - -.row-edit-dialog .btn-ghost:hover { - background: var(--rule); - color: var(--ink); -} - -.row-edit-dialog .btn-primary { - background: var(--accent); - color: #fff; -} - -.row-edit-dialog .btn-primary:hover { - background: #1949b8; -} - -.row-edit-dialog .btn:disabled { +.row-edit-dialog .modal-btn:disabled { opacity: 0.55; cursor: not-allowed; } diff --git a/datasette/static/edit-tools.js b/datasette/static/edit-tools.js index 402af956..edb00c7f 100644 --- a/datasette/static/edit-tools.js +++ b/datasette/static/edit-tools.js @@ -5572,6 +5572,7 @@ function setRowEditDialogLoading(state, isLoading) { function setRowEditDialogSaving(state, isSaving) { state.isSaving = isSaving; + state.modal.busy = isSaving; updateRowEditDialogButtons(state); } @@ -5789,18 +5790,6 @@ function confirmDiscardRowEditChanges(state) { return window.confirm(message); } -function closeRowEditDialogIfConfirmed(state) { - if (!state || state.isSaving) { - return false; - } - if (!confirmDiscardRowEditChanges(state)) { - return false; - } - state.shouldRestoreFocus = true; - state.dialog.close(); - return true; -} - function setRowInsertDialogTitle(state) { var insertData = tableInsertData() || {}; var title = rowEditIsMultipleInsert(state) @@ -6626,38 +6615,6 @@ async function insertBulkPreviewRows(state) { } } -function scheduleCloseRowEditDialogIfConfirmed(state) { - // Fix for an issue in Safari where hitting Esc would show - // the confirm() prompt asking if state should be discarded - // but the Esc key press would then cancel that dialog too. - // Wait for keyup, then move the confirm() to a fresh timer tick. - if (!state || state.isSaving || state.isClosePending) { - return false; - } - if (!rowEditDialogHasChanges(state)) { - state.shouldRestoreFocus = true; - state.dialog.close(); - return true; - } - state.isClosePending = true; - var closeAfterKeyup = function () { - if (!state.isClosePending) { - return; - } - state.isClosePending = false; - closeRowEditDialogIfConfirmed(state); - }; - var onKeyup = function (ev) { - if (ev.key !== "Escape") { - return; - } - document.removeEventListener("keyup", onKeyup, true); - setTimeout(closeAfterKeyup, 0); - }; - document.addEventListener("keyup", onKeyup, true); - return true; -} - function findDataRowElement(root, rowId) { var elements = root.querySelectorAll("[data-row]"); for (var i = 0; i < elements.length; i += 1) { @@ -6747,9 +6704,8 @@ async function saveRowEditDialog(state) { } var formValues = collectRowFormValues(state); if (state.mode === "edit" && !Object.keys(formValues).length) { - state.shouldRestoreFocus = true; hideRowMutationStatus(); - state.dialog.close(); + state.modal.close(); return; } var payload = @@ -6782,9 +6738,8 @@ async function saveRowEditDialog(state) { insertedRowData, insertData.primaryKeys || [], ); - state.shouldRestoreFocus = false; if (!insertedRowId) { - state.dialog.close(); + state.modal.close({ restoreFocus: false }); var missingIdStatus = showRowMutationStatus( state.manager, "Inserted row. Refresh the page to see it.", @@ -6800,7 +6755,7 @@ async function saveRowEditDialog(state) { try { insertedRow = await fetchUpdatedRowElement(state); } catch (_error) { - state.dialog.close(); + state.modal.close({ restoreFocus: false }); var refreshFailedStatus = showRowMutationStatus( state.manager, "Inserted row, but could not refresh the table row. Refresh the page to see it.", @@ -6815,7 +6770,7 @@ async function saveRowEditDialog(state) { rowTitleLabel(insertedRow), ); var addedRow = addInsertedRowToPage(insertedRow); - state.dialog.close(); + state.modal.close({ restoreFocus: false }); showRowMutationStatus(state.manager, insertedStatusMessage, false); if (addedRow) { var insertedFocusTarget = @@ -6824,7 +6779,7 @@ async function saveRowEditDialog(state) { insertedFocusTarget.focus(); } } else { - state.dialog.close(); + state.modal.close({ restoreFocus: false }); var filteredStatus = showRowMutationStatus( state.manager, "Inserted row. It does not match the current filters.", @@ -6836,8 +6791,7 @@ async function saveRowEditDialog(state) { } if (isRowPage()) { - state.shouldRestoreFocus = false; - state.dialog.close(); + state.modal.close({ restoreFocus: false }); location.reload(); return; } @@ -6873,8 +6827,7 @@ async function saveRowEditDialog(state) { ); } - state.shouldRestoreFocus = false; - state.dialog.close(); + state.modal.close({ restoreFocus: false }); if (focusTarget && document.contains(focusTarget)) { focusTarget.focus(); } @@ -7018,7 +6971,8 @@ function ensureRowEditDialog(manager) { return null; } - var dialog = document.createElement("dialog"); + var modal = DatasetteModal.create(); + var dialog = modal.dialog; dialog.id = ROW_EDIT_DIALOG_ID; dialog.className = "row-edit-dialog"; dialog.setAttribute("aria-labelledby", "row-edit-title"); @@ -7048,7 +7002,7 @@ function ensureRowEditDialog(manager) {
- + You can paste the template into Google Sheets or Excel.Paste into Google Sheets or Excel
@@ -7061,14 +7015,15 @@ function ensureRowEditDialog(manager) {
- - + +
`; - document.body.appendChild(dialog); + document.body.appendChild(modal); rowEditDialogState = { + modal: modal, dialog: dialog, form: dialog.querySelector(".row-edit-form"), title: dialog.querySelector(".modal-title"), @@ -7099,7 +7054,6 @@ function ensureRowEditDialog(manager) { singleInsertLink: dialog.querySelector(".row-edit-single-insert"), cancelButton: dialog.querySelector(".row-edit-cancel"), saveButton: dialog.querySelector(".row-edit-save"), - currentButton: null, currentRow: null, currentRowId: null, currentPkPath: null, @@ -7127,9 +7081,7 @@ function ensureRowEditDialog(manager) { manager: manager, isLoading: false, isSaving: false, - isClosePending: false, hasLoaded: false, - shouldRestoreFocus: true, }; rowEditDialogState.form.addEventListener("submit", function (ev) { @@ -7149,10 +7101,7 @@ function ensureRowEditDialog(manager) { rowEditDialogState.bulkInsertTextarea.focus(); return; } - if (!rowEditDialogState.isSaving) { - rowEditDialogState.shouldRestoreFocus = true; - dialog.close(); - } + modal.requestClose("cancel"); }); rowEditDialogState.bulkInsertLink.addEventListener("click", function (ev) { @@ -7271,31 +7220,17 @@ function ensureRowEditDialog(manager) { }, ); - dialog.addEventListener("click", function (ev) { - if (ev.target === dialog) { - closeRowEditDialogIfConfirmed(rowEditDialogState); - } - }); - - dialog.addEventListener("keydown", function (ev) { - if (ev.key !== "Escape") { - return; - } - ev.preventDefault(); - scheduleCloseRowEditDialogIfConfirmed(rowEditDialogState); - }); - - dialog.addEventListener("cancel", function (ev) { - ev.preventDefault(); - scheduleCloseRowEditDialogIfConfirmed(rowEditDialogState); - }); + modal.beforeClose = function (reason) { + return ( + reason === "cancel" || confirmDiscardRowEditChanges(rowEditDialogState) + ); + }; dialog.addEventListener("close", function () { var state = rowEditDialogState; var shouldReloadOnClose = state.shouldReloadOnClose; var redirectOnCloseUrl = state.redirectOnCloseUrl; state.loadId += 1; - state.isClosePending = false; state.bulkInsertLiveValidationError = null; state.shouldReloadOnClose = false; state.redirectOnCloseUrl = null; @@ -7308,13 +7243,6 @@ function ensureRowEditDialog(manager) { destroyRowEditFields(state); setRowEditDialogLoading(state, false); setRowEditDialogSaving(state, false); - if ( - state.shouldRestoreFocus && - state.currentButton && - document.contains(state.currentButton) - ) { - state.currentButton.focus(); - } if (shouldReloadOnClose) { if (redirectOnCloseUrl) { location.href = redirectOnCloseUrl; @@ -7339,7 +7267,6 @@ async function openRowEditDialog(button, manager) { state.manager = manager; state.mode = "edit"; - state.currentButton = button; state.currentRow = row; state.currentRowId = row.getAttribute("data-row") || ""; state.currentPkPath = rowDisplayLabel(row); @@ -7356,7 +7283,7 @@ async function openRowEditDialog(button, manager) { } else { state.form.removeAttribute("action"); } - state.shouldRestoreFocus = true; + state.hasLoaded = false; state.loadId += 1; var loadId = state.loadId; @@ -7375,9 +7302,7 @@ async function openRowEditDialog(button, manager) { state.summary.textContent = ""; syncRowEditInsertModeUi(state); - if (!state.dialog.open) { - state.dialog.showModal(); - } + state.modal.show({ trigger: button }); state.cancelButton.focus(); try { @@ -7417,7 +7342,6 @@ function openRowInsertDialog(button, manager) { state.manager = manager; state.mode = "insert"; - state.currentButton = button; state.currentRow = null; state.currentRowId = null; state.currentPkPath = null; @@ -7432,7 +7356,7 @@ function openRowInsertDialog(button, manager) { state.shouldReloadOnClose = false; state.redirectOnCloseUrl = null; resetBulkInsertPreview(state); - state.shouldRestoreFocus = true; + state.hasLoaded = false; state.loadId += 1; @@ -7454,9 +7378,7 @@ function openRowInsertDialog(button, manager) { state.summary.textContent = ""; syncRowEditInsertModeUi(state); - if (!state.dialog.open) { - state.dialog.showModal(); - } + state.modal.show({ trigger: button }); renderRowInsertFields(state, insertData); } diff --git a/tests/test_playwright.py b/tests/test_playwright.py index d7f6fa5c..753691e4 100644 --- a/tests/test_playwright.py +++ b/tests/test_playwright.py @@ -1761,6 +1761,67 @@ def test_modal_lifecycle(page, datasette_server, shadow): expect(page.locator("#after-save")).to_be_focused() +@pytest.mark.playwright +def test_modal_nested_escape_and_cleanup(page, datasette_server): + from playwright.sync_api import expect + + page.goto(datasette_server + "data/projects") + trigger = page.locator('tr[data-row="1"] button[data-row-action="edit"]') + trigger.click() + dialog = page.locator("#row-edit-dialog") + field = dialog.locator('input[name="title"]') + expect(field).to_be_visible() + field.fill("Unsaved title") + page.evaluate("""() => { + window.confirmations = []; + window.confirm = message => { confirmations.push(message); return false; }; + }""") + # Plugin controls can consume Escape without closing their containing form. + field.evaluate("""node => node.addEventListener('keydown', event => { + if (event.key === 'Escape') event.preventDefault(); + }, {once: true})""") + field.press("Escape") + assert page.evaluate("confirmations") == [] + expect(dialog).to_be_visible() + field.press("Escape") + page.wait_for_function("confirmations.length === 1") + assert page.evaluate("confirmations") == ["Discard unsaved changes to this row?"] + + # A nested native modal closes independently, then returns focus to its field. + field.evaluate("""node => { + node.focus(); + window.nestedModal = DatasetteModal.create(); + nestedModal.dialog.setAttribute('aria-label', 'Nested picker'); + nestedModal.dialog.innerHTML = ''; + node.closest('dialog').append(nestedModal); + nestedModal.show(); + }""") + nested = page.get_by_role("dialog", name="Nested picker") + page.keyboard.press("Escape") + expect(nested).not_to_be_visible() + expect(dialog).to_be_visible() + expect(field).to_be_focused() + assert page.evaluate("confirmations.length") == 1 + + # Closing before keyup cancels the pending confirmation, including on reopen. + page.keyboard.down("Escape") + dialog.locator(".row-edit-cancel").click() + expect(dialog).not_to_be_visible() + trigger.click() + page.keyboard.up("Escape") + expect(field).to_be_visible() + assert page.evaluate("confirmations.length") == 1 + expect(dialog).to_be_visible() + # Native cancel (e.g. an accessibility action) does not wait for keyboard input. + field.fill("Another edit") + dialog.evaluate( + "node => node.dispatchEvent(new Event('cancel', {cancelable: true}))" + ) + assert page.evaluate("confirmations.length") == 2 + dialog.locator(".row-edit-cancel").click() + expect(trigger).to_be_focused() + + @pytest.mark.playwright @pytest.mark.parametrize("name", ["jump", "columns", "type", "mobile"]) def test_modal_consumers_dismiss_and_restore_focus(page, datasette_server, name):