From adc8dabda1ebcd886c5733cf6782bf50f32bb56c Mon Sep 17 00:00:00 2001 From: Amit Haridas Date: Wed, 25 Mar 2026 22:20:34 +0530 Subject: [PATCH] fix: resolve modal stacking, animation, and close cleanup bugs - Set backdrop z-index:0 and content z-index:1 to fix backdrop covering modal content within the stacking context - Force reflow between removing hidden and adding open class so CSS opacity transition fires correctly - Add transitionend listener + setTimeout fallback to restore hidden class after close animation completes - Override flex:1 on modal footer buttons to prevent full-width stretch - Add min-width to modal size variants for consistent sizing - Add 23 tests covering open/close lifecycle, keyboard, and destroy Amit Haridas --- src/styles/modal.css | 11 +- src/utils/ModalManager.js | 26 +++- tests/modal-manager.test.js | 280 ++++++++++++++++++++++++++++++++++++ 3 files changed, 314 insertions(+), 3 deletions(-) create mode 100644 tests/modal-manager.test.js diff --git a/src/styles/modal.css b/src/styles/modal.css index 299a3f0..38fc52f 100644 --- a/src/styles/modal.css +++ b/src/styles/modal.css @@ -14,7 +14,7 @@ background: rgba(0, 0, 0, 0.4); backdrop-filter: blur(4px); -webkit-backdrop-filter: blur(4px); - z-index: var(--z-modal, 200); + z-index: 0; cursor: pointer; } @@ -46,6 +46,9 @@ * ============================================ */ .modal-content { + position: relative; + z-index: 1; + width: 100%; background: hsl(var(--background, 0 0% 100%)); border-radius: var(--radius-lg, 0.5rem); box-shadow: var(--shadow-xl, 0 20px 25px -5px rgb(0 0 0 / 0.1), 0 8px 10px -6px rgb(0 0 0 / 0.1)); @@ -139,6 +142,9 @@ .modal-footer .btn { min-width: 80px; + /* styles-modern.css sets flex:1 on .btn-primary/.btn-secondary globally + * which causes footer buttons to stretch to full width. Override here. */ + flex: none; } /* ============================================ @@ -208,15 +214,18 @@ .modal-content.small { max-width: 400px; + min-width: 320px; } .modal-content.large { max-width: 800px; + min-width: 560px; } .modal-content.full { max-width: 95vw; max-height: 95vh; + min-width: min(95vw, 700px); } /* ============================================ diff --git a/src/utils/ModalManager.js b/src/utils/ModalManager.js index 2daf1a8..b837fda 100644 --- a/src/utils/ModalManager.js +++ b/src/utils/ModalManager.js @@ -124,8 +124,12 @@ class ModalManager { // Track open modals ModalManager.#openModals.push(this); - // Show modal (remove hidden, add open) + // Show modal: remove hidden first, force a reflow so the browser + // records opacity:0 as the start state, then add 'open' to trigger + // the CSS transition. Without the reflow, both class changes are + // batched into one style recalculation and the transition is skipped. this.#modal.classList.remove('hidden'); + void this.#modal.offsetHeight; // Force reflow — do not remove this.#modal.classList.add('open'); this.#isOpen = true; @@ -165,10 +169,28 @@ class ModalManager { ModalManager.#openModals.splice(index, 1); } - // Hide modal + // Start hide transition this.#modal.classList.remove('open'); this.#isOpen = false; + // Re-add 'hidden' after the CSS transition completes so the modal + // is fully removed from rendering (display:none), not just invisible. + // We use both transitionend and a setTimeout fallback because + // transitionend never fires when prefers-reduced-motion disables transitions. + let hidden = false; + const addHidden = () => { + if (hidden || this.#isOpen) return; + hidden = true; + this.#modal.removeEventListener('transitionend', onTransitionEnd); + this.#modal.classList.add('hidden'); + }; + const onTransitionEnd = (e) => { + if (e.target !== this.#modal) return; + addHidden(); + }; + this.#modal.addEventListener('transitionend', onTransitionEnd); + setTimeout(addHidden, 250); // fallback: slightly longer than 200ms transition + // Restore body scroll if no modals open if (ModalManager.#openModals.length === 0) { document.body.style.overflow = ''; diff --git a/tests/modal-manager.test.js b/tests/modal-manager.test.js new file mode 100644 index 0000000..fc92509 --- /dev/null +++ b/tests/modal-manager.test.js @@ -0,0 +1,280 @@ +/** + * Tests for ModalManager + * Covers the three bugs fixed in modal refactor: + * 1. open() animation: reflow between hidden removal and open class add + * 2. close() cleanup: 'hidden' class restored after transition + * 3. State management: isOpen() accuracy + */ + +const { ModalManager } = require('../src/utils/ModalManager'); + +function createModalElement(id = 'test-modal') { + const modal = document.createElement('div'); + modal.id = id; + modal.className = 'modal hidden'; + modal.setAttribute('role', 'dialog'); + modal.setAttribute('aria-modal', 'true'); + + const backdrop = document.createElement('div'); + backdrop.className = 'modal-backdrop'; + backdrop.setAttribute('data-close', ''); + + const content = document.createElement('div'); + content.className = 'modal-content'; + + const header = document.createElement('div'); + header.className = 'modal-header'; + + const closeBtn = document.createElement('button'); + closeBtn.className = 'modal-close'; + closeBtn.setAttribute('aria-label', 'Close'); + + const body = document.createElement('div'); + body.className = 'modal-body'; + + const input = document.createElement('input'); + input.type = 'text'; + + body.appendChild(input); + header.appendChild(closeBtn); + content.appendChild(header); + content.appendChild(body); + modal.appendChild(backdrop); + modal.appendChild(content); + document.body.appendChild(modal); + + return modal; +} + +describe('ModalManager', () => { + let modal; + let manager; + + beforeEach(() => { + modal = createModalElement(); + manager = new ModalManager(modal); + }); + + afterEach(() => { + manager.destroy(); + while (document.body.firstChild) { + document.body.removeChild(document.body.firstChild); + } + document.body.style.overflow = ''; + }); + + // ========================================================= + // open() + // ========================================================= + + describe('open()', () => { + test('removes hidden class and adds open class', () => { + expect(modal.classList.contains('hidden')).toBe(true); + expect(modal.classList.contains('open')).toBe(false); + + manager.open(); + + expect(modal.classList.contains('hidden')).toBe(false); + expect(modal.classList.contains('open')).toBe(true); + }); + + test('sets isOpen to true', () => { + expect(manager.isOpen()).toBe(false); + manager.open(); + expect(manager.isOpen()).toBe(true); + }); + + test('prevents body scroll', () => { + manager.open(); + expect(document.body.style.overflow).toBe('hidden'); + }); + + test('does not open again if already open', () => { + manager.open(); + manager.open(); // second call should be no-op + expect(manager.isOpen()).toBe(true); + }); + + test('calls onOpen callback', () => { + const onOpen = jest.fn(); + manager.destroy(); + manager = new ModalManager(modal, { onOpen }); + manager.open(); + expect(onOpen).toHaveBeenCalledTimes(1); + }); + + test('dispatches modal:open custom event', () => { + const handler = jest.fn(); + modal.addEventListener('modal:open', handler); + manager.open(); + expect(handler).toHaveBeenCalledTimes(1); + }); + }); + + // ========================================================= + // close() + // ========================================================= + + describe('close()', () => { + beforeEach(() => { + manager.open(); + }); + + test('removes open class', () => { + expect(modal.classList.contains('open')).toBe(true); + manager.close(); + expect(modal.classList.contains('open')).toBe(false); + }); + + test('sets isOpen to false immediately', () => { + manager.close(); + expect(manager.isOpen()).toBe(false); + }); + + test('restores body scroll when no modals remain open', () => { + manager.close(); + expect(document.body.style.overflow).toBe(''); + }); + + test('adds hidden class after transitionend event fires', () => { + manager.close(); + + // Immediately after close(): hidden should NOT yet be added — + // the close animation is still in progress. + // (This is the bug that existed before the fix.) + expect(modal.classList.contains('hidden')).toBe(false); + + // Simulate the CSS transition completing + const event = new Event('transitionend'); + Object.defineProperty(event, 'target', { value: modal, writable: false }); + modal.dispatchEvent(event); + + expect(modal.classList.contains('hidden')).toBe(true); + }); + + test('adds hidden class via 250ms timeout fallback when transitionend never fires', () => { + jest.useFakeTimers(); + + manager.close(); + expect(modal.classList.contains('hidden')).toBe(false); + + // Advance past the fallback timeout (250ms) + jest.advanceTimersByTime(300); + + expect(modal.classList.contains('hidden')).toBe(true); + + jest.useRealTimers(); + }); + + test('does not add hidden class if modal is reopened before timeout fires', () => { + jest.useFakeTimers(); + + manager.close(); + jest.advanceTimersByTime(100); // halfway through timeout + + // Re-open the modal before the timeout fires + manager.open(); + + jest.advanceTimersByTime(200); // past original timeout expiry + + // Modal was reopened, so hidden must NOT have been added + expect(modal.classList.contains('hidden')).toBe(false); + expect(modal.classList.contains('open')).toBe(true); + + jest.useRealTimers(); + }); + + test('calls onClose callback', () => { + const onClose = jest.fn(); + manager.destroy(); + manager = new ModalManager(modal, { onClose }); + manager.open(); + manager.close(); + expect(onClose).toHaveBeenCalledTimes(1); + }); + + test('dispatches modal:close custom event', () => { + const handler = jest.fn(); + modal.addEventListener('modal:close', handler); + manager.close(); + expect(handler).toHaveBeenCalledTimes(1); + }); + + test('is a no-op when modal is already closed', () => { + manager.close(); // close from open + const onClose = jest.fn(); + manager.destroy(); + manager = new ModalManager(modal, { onClose }); + manager.close(); // call close on an already-closed modal + expect(onClose).not.toHaveBeenCalled(); + }); + }); + + // ========================================================= + // Keyboard interaction + // ========================================================= + + describe('keyboard shortcuts', () => { + test('Escape key closes an open modal', () => { + manager.open(); + document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape', bubbles: true })); + expect(manager.isOpen()).toBe(false); + }); + + test('Escape key does nothing when modal is already closed', () => { + expect(() => { + document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape', bubbles: true })); + }).not.toThrow(); + }); + + test('closeOnEscape: false prevents Escape from closing', () => { + manager.destroy(); + manager = new ModalManager(modal, { closeOnEscape: false }); + manager.open(); + document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape', bubbles: true })); + expect(manager.isOpen()).toBe(true); + }); + }); + + // ========================================================= + // Close triggers + // ========================================================= + + describe('close triggers', () => { + test('clicking the × close button closes the modal', () => { + manager.open(); + modal.querySelector('.modal-close').click(); + expect(manager.isOpen()).toBe(false); + }); + + test('clicking backdrop (data-close) closes the modal', () => { + manager.open(); + modal.querySelector('.modal-backdrop').click(); + expect(manager.isOpen()).toBe(false); + }); + + test('closeOnBackdrop: false prevents backdrop from closing', () => { + manager.destroy(); + manager = new ModalManager(modal, { closeOnBackdrop: false }); + manager.open(); + modal.querySelector('.modal-backdrop').click(); + expect(manager.isOpen()).toBe(true); + }); + }); + + // ========================================================= + // destroy() + // ========================================================= + + describe('destroy()', () => { + test('closes modal if open', () => { + manager.open(); + manager.destroy(); + expect(manager.isOpen()).toBe(false); + }); + + test('does not throw when destroying a closed modal', () => { + expect(() => manager.destroy()).not.toThrow(); + }); + }); +});