From 3804640574f015684b7fe1d4f25829dc6dbfd503 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sascha=20I=C3=9Fbr=C3=BCcker?= Date: Fri, 22 Aug 2025 09:57:31 +0200 Subject: [PATCH] Use modal dialog for confirming actions (#1168) * Use modal dialog for confirming actions * cleanup unused state --- .../frontend/behaviors/confirm-button.js | 204 +++++++++++++----- bookmarks/frontend/behaviors/focus-utils.js | 6 + bookmarks/frontend/behaviors/index.js | 2 - bookmarks/frontend/behaviors/modal.js | 20 +- bookmarks/styles/components.css | 25 +-- bookmarks/styles/theme/menus.css | 39 ++++ bookmarks/styles/theme/modals.css | 1 - .../templates/bookmarks/bookmark_list.html | 4 +- .../templates/bookmarks/details/modal.html | 2 +- bookmarks/templates/bookmarks/layout.html | 24 --- .../tests/test_bookmark_details_modal.py | 2 +- .../tests/test_bookmarks_list_template.py | 4 +- .../e2e_test_bookmark_details_modal.py | 6 +- .../e2e_test_bookmark_page_bulk_edit.py | 12 +- .../e2e_test_bookmark_page_partial_updates.py | 18 +- bookmarks/tests_e2e/helpers.py | 3 + 16 files changed, 236 insertions(+), 136 deletions(-) diff --git a/bookmarks/frontend/behaviors/confirm-button.js b/bookmarks/frontend/behaviors/confirm-button.js index fb213ee..fce8db1 100644 --- a/bookmarks/frontend/behaviors/confirm-button.js +++ b/bookmarks/frontend/behaviors/confirm-button.js @@ -1,79 +1,173 @@ import { Behavior, registerBehavior } from "./index"; +import { FocusTrapController, isKeyboardActive } from "./focus-utils"; + +let confirmId = 0; + +function nextConfirmId() { + return `confirm-${confirmId++}`; +} class ConfirmButtonBehavior extends Behavior { constructor(element) { super(element); this.onClick = this.onClick.bind(this); - element.addEventListener("click", this.onClick); + this.element.addEventListener("click", this.onClick); } destroy() { - this.reset(); + if (this.opened) { + this.close(); + } this.element.removeEventListener("click", this.onClick); } onClick(event) { event.preventDefault(); - Behavior.interacting = true; - const container = document.createElement("span"); - container.className = "confirmation"; - - const icon = this.element.getAttribute("ld-confirm-icon"); - if (icon) { - const iconElement = document.createElementNS( - "http://www.w3.org/2000/svg", - "svg", - ); - iconElement.style.width = "16px"; - iconElement.style.height = "16px"; - iconElement.innerHTML = ``; - container.append(iconElement); + if (this.opened) { + this.close(); + } else { + this.open(); } - - const question = this.element.getAttribute("ld-confirm-question"); - if (question) { - const questionElement = document.createElement("span"); - questionElement.innerText = question; - container.append(question); - } - - const buttonClasses = Array.from(this.element.classList.values()) - .filter((cls) => cls.startsWith("btn")) - .join(" "); - - const cancelButton = document.createElement(this.element.nodeName); - cancelButton.type = "button"; - cancelButton.innerText = question ? "No" : "Cancel"; - cancelButton.className = `${buttonClasses} mr-1`; - cancelButton.addEventListener("click", this.reset.bind(this)); - - const confirmButton = document.createElement(this.element.nodeName); - confirmButton.type = this.element.type; - confirmButton.name = this.element.name; - confirmButton.value = this.element.value; - confirmButton.innerText = question ? "Yes" : "Confirm"; - confirmButton.className = buttonClasses; - confirmButton.addEventListener("click", this.reset.bind(this)); - - container.append(cancelButton, confirmButton); - this.container = container; - - this.element.before(container); - this.element.classList.add("d-none"); } - reset() { - setTimeout(() => { - Behavior.interacting = false; - if (this.container) { - this.container.remove(); - this.container = null; - } - this.element.classList.remove("d-none"); + open() { + const dropdown = document.createElement("div"); + dropdown.className = "dropdown confirm-dropdown active"; + + const confirmId = nextConfirmId(); + const questionId = `${confirmId}-question`; + + const menu = document.createElement("div"); + menu.className = "menu with-arrow"; + menu.role = "alertdialog"; + menu.setAttribute("aria-modal", "true"); + menu.setAttribute("aria-labelledby", questionId); + menu.addEventListener("keydown", this.onMenuKeyDown.bind(this)); + + const question = document.createElement("span"); + question.id = questionId; + question.textContent = + this.element.getAttribute("ld-confirm-question") || "Are you sure?"; + question.style.fontWeight = "bold"; + + const cancelButton = document.createElement("button"); + cancelButton.textContent = "Cancel"; + cancelButton.type = "button"; + cancelButton.className = "btn"; + cancelButton.tabIndex = 0; + cancelButton.addEventListener("click", () => this.close()); + + const confirmButton = document.createElement("button"); + confirmButton.textContent = "Confirm"; + confirmButton.type = "submit"; + confirmButton.name = this.element.name; + confirmButton.value = this.element.value; + confirmButton.className = "btn btn-error"; + confirmButton.addEventListener("click", () => this.confirm()); + + const arrow = document.createElement("div"); + arrow.className = "menu-arrow"; + + menu.append(question, cancelButton, confirmButton, arrow); + dropdown.append(menu); + document.body.append(dropdown); + + this.positionController = new AnchorPositionController(this.element, menu); + this.focusTrap = new FocusTrapController(menu); + this.dropdown = dropdown; + this.opened = true; + } + + onMenuKeyDown(event) { + if (event.key === "Escape") { + event.preventDefault(); + event.stopPropagation(); + this.close(); + } + } + + confirm() { + this.element.closest("form").requestSubmit(this.element); + this.close(); + } + + close() { + if (!this.opened) return; + this.positionController.destroy(); + this.focusTrap.destroy(); + this.dropdown.remove(); + this.element.focus({ focusVisible: isKeyboardActive() }); + this.opened = false; + } +} + +class AnchorPositionController { + constructor(anchor, overlay) { + this.anchor = anchor; + this.overlay = overlay; + + this.handleScroll = this.handleScroll.bind(this); + window.addEventListener("scroll", this.handleScroll, { capture: true }); + + this.updatePosition(); + } + + handleScroll() { + if (this.debounce) { + return; + } + + this.debounce = true; + + requestAnimationFrame(() => { + this.updatePosition(); + this.debounce = false; }); } + + updatePosition() { + const anchorRect = this.anchor.getBoundingClientRect(); + const overlayRect = this.overlay.getBoundingClientRect(); + const bufferX = 10; + const bufferY = 30; + + let left = anchorRect.left - (overlayRect.width - anchorRect.width) / 2; + const initialLeft = left; + const overflowLeft = left < bufferX; + const overflowRight = + left + overlayRect.width > window.innerWidth - bufferX; + + if (overflowLeft) { + left = bufferX; + } else if (overflowRight) { + left = window.innerWidth - overlayRect.width - bufferX; + } + + const delta = initialLeft - left; + this.overlay.style.setProperty("--arrow-offset", `${delta}px`); + + let top = anchorRect.bottom; + const overflowBottom = + top + overlayRect.height > window.innerHeight - bufferY; + + if (overflowBottom) { + top = anchorRect.top - overlayRect.height; + this.overlay.classList.remove("top-aligned"); + this.overlay.classList.add("bottom-aligned"); + } else { + this.overlay.classList.remove("bottom-aligned"); + this.overlay.classList.add("top-aligned"); + } + + this.overlay.style.left = `${left}px`; + this.overlay.style.top = `${top}px`; + } + + destroy() { + window.removeEventListener("scroll", this.handleScroll, { capture: true }); + } } registerBehavior("ld-confirm-button", ConfirmButtonBehavior); diff --git a/bookmarks/frontend/behaviors/focus-utils.js b/bookmarks/frontend/behaviors/focus-utils.js index 3851fa3..8e4790c 100644 --- a/bookmarks/frontend/behaviors/focus-utils.js +++ b/bookmarks/frontend/behaviors/focus-utils.js @@ -93,6 +93,12 @@ document.addEventListener("turbo:load", () => { return; } + // Ignore if there is a modal dialog, which should handle its own focus + const modal = document.querySelector("[aria-modal='true']"); + if (modal) { + return; + } + // Check if there is an explicit focus target for the next page load for (const target of afterPageLoadFocusTarget) { const element = document.querySelector(target); diff --git a/bookmarks/frontend/behaviors/index.js b/bookmarks/frontend/behaviors/index.js index 2b14bd5..ab7e13c 100644 --- a/bookmarks/frontend/behaviors/index.js +++ b/bookmarks/frontend/behaviors/index.js @@ -54,8 +54,6 @@ export class Behavior { destroy() {} } -Behavior.interacting = false; - export function registerBehavior(name, behavior) { behaviorRegistry[name] = behavior; } diff --git a/bookmarks/frontend/behaviors/modal.js b/bookmarks/frontend/behaviors/modal.js index bfad237..601b899 100644 --- a/bookmarks/frontend/behaviors/modal.js +++ b/bookmarks/frontend/behaviors/modal.js @@ -23,32 +23,22 @@ export class ModalBehavior extends Behavior { this.closeButton.removeEventListener("click", this.onClose); document.removeEventListener("keydown", this.onKeyDown); - this.clearInert(); + this.removeScrollLock(); this.focusTrap.destroy(); } init() { - this.setupInert(); + this.setupScrollLock(); this.focusTrap = new FocusTrapController( this.element.querySelector(".modal-container"), ); } - setupInert() { - // Inert all other elements on the page - document - .querySelectorAll("body > *:not(.modals)") - .forEach((el) => el.setAttribute("inert", "")); - // Lock scroll on the body + setupScrollLock() { document.body.classList.add("scroll-lock"); } - clearInert() { - // Clear inert attribute from all elements to allow focus outside the modal again - document - .querySelectorAll("body > *") - .forEach((el) => el.removeAttribute("inert")); - // Remove scroll lock from the body + removeScrollLock() { document.body.classList.remove("scroll-lock"); } @@ -85,7 +75,7 @@ export class ModalBehavior extends Behavior { doClose() { this.element.remove(); - this.clearInert(); + this.removeScrollLock(); this.element.dispatchEvent(new CustomEvent("modal:close")); } } diff --git a/bookmarks/styles/components.css b/bookmarks/styles/components.css index 1289230..c589905 100644 --- a/bookmarks/styles/components.css +++ b/bookmarks/styles/components.css @@ -31,22 +31,17 @@ } /* Confirm button component */ -span.confirmation { - display: flex; - align-items: baseline; - gap: var(--unit-1); - color: var(--error-color) !important; +.confirm-dropdown.active { + position: fixed; + z-index: 500; - svg { - align-self: center; - } - - .btn.btn-link { - color: var(--error-color) !important; - - &:hover { - text-decoration: underline; - } + & .menu { + position: fixed; + display: flex; + flex-direction: column; + box-sizing: border-box; + gap: var(--unit-2); + padding: var(--unit-2); } } diff --git a/bookmarks/styles/theme/menus.css b/bookmarks/styles/theme/menus.css index 4fa5aad..4b0002f 100644 --- a/bookmarks/styles/theme/menus.css +++ b/bookmarks/styles/theme/menus.css @@ -87,4 +87,43 @@ border-bottom: solid 1px var(--secondary-border-color); margin: var(--unit-2) 0; } + + &.with-arrow { + overflow: visible; + --arrow-size: 16px; + --arrow-offset: 0px; + + .menu-arrow { + display: block; + position: absolute; + inset-inline-start: calc(50% + var(--arrow-offset)); + top: 0; + width: var(--arrow-size); + height: var(--arrow-size); + translate: -50% -50%; + rotate: 45deg; + background: inherit; + border: inherit; + clip-path: polygon(0 0, 0 100%, 100% 0); + } + + &.top-aligned { + transform: translateY( + calc(calc(var(--arrow-size) / 2) + var(--layout-spacing-sm)) + ); + } + + &.bottom-aligned { + transform: translateY( + calc(calc(calc(var(--arrow-size) / 2) + var(--layout-spacing-sm)) * -1) + ); + + .menu-arrow { + top: auto; + bottom: 0; + rotate: 225deg; + translate: -50% 50%; + } + } + } } diff --git a/bookmarks/styles/theme/modals.css b/bookmarks/styles/theme/modals.css index 102b663..9291deb 100644 --- a/bookmarks/styles/theme/modals.css +++ b/bookmarks/styles/theme/modals.css @@ -106,7 +106,6 @@ & .modal-footer { padding: var(--unit-6); padding-top: 0; - text-align: right; } } diff --git a/bookmarks/templates/bookmarks/bookmark_list.html b/bookmarks/templates/bookmarks/bookmark_list.html index 2e38354..61beed5 100644 --- a/bookmarks/templates/bookmarks/bookmark_list.html +++ b/bookmarks/templates/bookmarks/bookmark_list.html @@ -120,7 +120,7 @@ {% if bookmark_item.show_mark_as_read %} diff --git a/bookmarks/templates/bookmarks/layout.html b/bookmarks/templates/bookmarks/layout.html index 41ff112..3a5bf34 100644 --- a/bookmarks/templates/bookmarks/layout.html +++ b/bookmarks/templates/bookmarks/layout.html @@ -18,18 +18,6 @@ - - - - - - - - - - - @@ -41,18 +29,6 @@ - - - - - - - - - - - diff --git a/bookmarks/tests/test_bookmark_details_modal.py b/bookmarks/tests/test_bookmark_details_modal.py index 8c69876..51a2d85 100644 --- a/bookmarks/tests/test_bookmark_details_modal.py +++ b/bookmarks/tests/test_bookmark_details_modal.py @@ -501,7 +501,7 @@ class BookmarkDetailsModalTestCase(TestCase, BookmarkFactoryMixin, HtmlTestMixin modal = self.get_index_details_modal(bookmark) delete_button = modal.find("button", {"type": "submit", "name": "remove"}) self.assertIsNotNone(delete_button) - self.assertEqual("Delete...", delete_button.text.strip()) + self.assertEqual("Delete", delete_button.text.strip()) self.assertEqual(str(bookmark.id), delete_button["value"]) form = delete_button.find_parent("form") diff --git a/bookmarks/tests/test_bookmarks_list_template.py b/bookmarks/tests/test_bookmarks_list_template.py index cf5d60d..a95bd87 100644 --- a/bookmarks/tests/test_bookmarks_list_template.py +++ b/bookmarks/tests/test_bookmarks_list_template.py @@ -231,7 +231,7 @@ class BookmarkListTemplateTest(TestCase, BookmarkFactoryMixin, HtmlTestMixin): f"""