diff --git a/static/app.js b/static/app.js index 6ba1dd0bc..02ab5de19 100644 --- a/static/app.js +++ b/static/app.js @@ -1319,8 +1319,7 @@ function initializeEventListeners() { const toolThemeBtn = el('tool-theme-btn'); if (toolThemeBtn) { toolThemeBtn.addEventListener('click', () => { - const tm = document.getElementById('theme-modal'); - if (tm) tm.classList.remove('hidden'); + themeModule.openPopup(); }); } diff --git a/static/js/modalVisibility.js b/static/js/modalVisibility.js new file mode 100644 index 000000000..40fd903af --- /dev/null +++ b/static/js/modalVisibility.js @@ -0,0 +1,42 @@ +// Own both completion paths of an animated close so neither can outlive it. +const _pendingCloses = new WeakMap(); + +function cancelClose(modal) { + const pending = _pendingCloses.get(modal); + if (!pending) return; + clearTimeout(pending.timer); + pending.content.removeEventListener('animationend', pending.onEnd); + pending.content.classList.remove('modal-closing'); + _pendingCloses.delete(modal); +} + +export function showModal(modal) { + if (!modal) return; + cancelClose(modal); + modal.classList.remove('hidden'); +} + +export function hideModal(modal) { + if (!modal) return; + cancelClose(modal); + const content = modal.querySelector('.modal-content, .settings-modal-content'); + // A minimized window has no exit animation. Registering a listener here + // would let its next entrance animation close the newly opened window. + if (!content || modal.classList.contains('hidden')) { + modal.classList.add('hidden'); + return; + } + const pending = { content, timer: null, onEnd: null }; + const finish = () => { + if (_pendingCloses.get(modal) !== pending) return; + cancelClose(modal); + modal.classList.add('hidden'); + }; + pending.onEnd = event => { + if (event.target === content) finish(); + }; + _pendingCloses.set(modal, pending); + content.classList.add('modal-closing'); + content.addEventListener('animationend', pending.onEnd); + pending.timer = setTimeout(finish, 250); +} diff --git a/static/js/settings/lifecycle.js b/static/js/settings/lifecycle.js index 95a1ba86f..530bfe505 100644 --- a/static/js/settings/lifecycle.js +++ b/static/js/settings/lifecycle.js @@ -6,6 +6,8 @@ import { makeWindowDraggable } from '../windowDrag.js'; import { clearDockSide } from '../modalSnap.js'; +import { toggle as restoreMinimizedModal } from '../modalManager.js'; +import { showModal, hideModal } from '../modalVisibility.js'; const _dragBound = new WeakSet(); const _closeBound = new WeakSet(); @@ -147,30 +149,16 @@ export function bindSettingsClose(modalEl, options = {}) { export function showSettingsModal(modalEl) { if (!modalEl) return; + if (restoreMinimizedModal(modalEl.id)) { + showModal(modalEl); + return; + } if (modalEl.classList.contains('hidden')) { resetSettingsWindowPlacement(modalEl); } - modalEl.classList.remove('hidden'); + showModal(modalEl); } export function hideSettingsModal(modalEl) { - if (!modalEl) return; - - const content = modalEl.querySelector('.modal-content, .settings-modal-content'); - if (content && !content.classList.contains('modal-closing')) { - content.classList.add('modal-closing'); - content.addEventListener('animationend', () => { - modalEl.classList.add('hidden'); - content.classList.remove('modal-closing'); - }, { once: true }); - setTimeout(() => { - if (!modalEl.classList.contains('hidden')) { - modalEl.classList.add('hidden'); - content.classList.remove('modal-closing'); - } - }, 250); - return; - } - - modalEl.classList.add('hidden'); + hideModal(modalEl); } diff --git a/static/js/theme.js b/static/js/theme.js index 4cf8b3771..32894ad8d 100644 --- a/static/js/theme.js +++ b/static/js/theme.js @@ -7,6 +7,8 @@ import { initColorPickers, attachColorPicker } from './colorPicker.js?v=20260910 import { hexToRgb } from './color/hex.js'; import { makeWindowDraggable } from './windowDrag.js'; import { snapModalToZone } from './tileManager.js?v=20260910responsivebounds1'; +import { toggle as restoreMinimizedModal } from './modalManager.js'; +import { showModal, hideModal } from './modalVisibility.js'; export const THEMES = { dark: { bg:'#282c34', fg:'#9cdef2', panel:'#111111', border:'#355a66', red:'#e06c75' }, @@ -1570,31 +1572,29 @@ export function makeDraggable(el, handle) { } // Toggle the popup +export function openPopup() { + const modal = document.getElementById('theme-modal'); + if (!modal) return; + restoreMinimizedModal(modal.id); + showModal(modal); +} + export function togglePopup() { const modal = document.getElementById('theme-modal'); if (!modal) return; - const visible = !modal.classList.contains('hidden'); + const visible = !modal.classList.contains('hidden') + && !modal.classList.contains('modal-minimized') + && !modal.querySelector('.modal-closing'); if (visible) { - modal.classList.add('hidden'); + closePopup(); } else { - modal.classList.remove('hidden'); + openPopup(); } } export function closePopup() { const modal = document.getElementById('theme-modal'); - if (!modal) return; - const content = modal.querySelector('.modal-content'); - if (content && !content.classList.contains('modal-closing')) { - content.classList.add('modal-closing'); - content.addEventListener('animationend', () => { - modal.classList.add('hidden'); - content.classList.remove('modal-closing'); - }, { once: true }); - setTimeout(() => { if (!modal.classList.contains('hidden')) { modal.classList.add('hidden'); content.classList.remove('modal-closing'); } }, 250); - } else { - modal.classList.add('hidden'); - } + hideModal(modal); } // Expose for app.js wiring + AI ui_control @@ -2278,7 +2278,7 @@ function _initEmbers() { draw(); } -const themeModule = { initThemeUI, togglePopup, closePopup, makeDraggable, +const themeModule = { initThemeUI, openPopup, togglePopup, closePopup, makeDraggable, THEMES, applyColors, applyFontDensity, applyBgPattern, applyBgEffectColor, applyBgEffectIntensity, applyBgEffectSize, applyBgEffectSpeed, applyFrostedGlass, diff --git a/static/sw.js b/static/sw.js index c0af08e4b..65fd3c012 100644 --- a/static/sw.js +++ b/static/sw.js @@ -38,6 +38,7 @@ const KATEX_FONTS = [ // Both are fetched at install time, in the background. Entries must match the // exact URL the browser requests, query string included. const PRECACHE = [ + '/static/js/modalVisibility.js', '/static/js/compare/sse.js', '/', '/static/css/00-tokens.css?v=20261008confirmwrap1', diff --git a/tests/helpers/test_settings_shell.js b/tests/helpers/test_settings_shell.js index a4494a9b6..b6d273ba7 100644 --- a/tests/helpers/test_settings_shell.js +++ b/tests/helpers/test_settings_shell.js @@ -348,6 +348,8 @@ function moduleSource(relativePath) { }, makeWindowDraggable: (...args) => dragCalls.push(args), clearDockSide: (...args) => dockCalls.push(args), + restoreMinimizedModal: () => false, + clearTimeout() {}, setTimeout: callback => { callback(); return 1; }, WeakSet, }; @@ -356,6 +358,7 @@ function moduleSource(relativePath) { vm.runInContext(moduleSource('search.js'), context, { filename: 'search.js' }); vm.runInContext(moduleSource('sidebar.js'), context, { filename: 'sidebar.js' }); vm.runInContext(moduleSource('navigation.js'), context, { filename: 'navigation.js' }); + vm.runInContext(moduleSource('../modalVisibility.js'), context, { filename: 'modalVisibility.js' }); vm.runInContext(moduleSource('lifecycle.js'), context, { filename: 'lifecycle.js' }); vm.runInContext(moduleSource('dom.js'), context, { filename: 'dom.js' }); diff --git a/tests/helpers/test_settings_shell_coordinator.mjs b/tests/helpers/test_settings_shell_coordinator.mjs index 2cc6ce6b3..d005db748 100644 --- a/tests/helpers/test_settings_shell_coordinator.mjs +++ b/tests/helpers/test_settings_shell_coordinator.mjs @@ -20,6 +20,7 @@ const REAL_MODULES = new Set([ path.join(JS, 'settings/sidebar.js'), path.join(JS, 'settings/navigation.js'), path.join(JS, 'settings/lifecycle.js'), + path.join(JS, 'modalVisibility.js'), path.join(JS, 'settings/api.js'), path.join(JS, 'settings/speech.js'), path.join(JS, 'settings/writingStyle.js'), @@ -869,6 +870,7 @@ const STUBS = new Map([ clearDockSide() {}, }, ], + [path.join(JS, 'modalManager.js'), { toggle() { return false; } }], ]); diff --git a/tests/test_modal_reopen_lifecycle.py b/tests/test_modal_reopen_lifecycle.py new file mode 100644 index 000000000..3d4e050b8 --- /dev/null +++ b/tests/test_modal_reopen_lifecycle.py @@ -0,0 +1,126 @@ +"""Real ESM/DOM regressions for Settings and Theme close/restore races.""" +import json +import subprocess + +import pytest + +from tests.helpers.stylesheets import stylesheet_link_tags + + +@pytest.mark.parametrize('modal_id', ['settings-modal', 'theme-modal']) +@pytest.mark.parametrize('width', [1440, 390]) +def test_modal_reopen_after_dock_close_and_pending_animation(modal_id, width): + script = r""" + import { chromium } from 'playwright'; + const browser = await chromium.launch({headless:true}); + try { + const page = await browser.newPage({viewport:{width:WIDTH,height:1000}}); + await page.route('**/api/**', route => route.fulfill({json:{value:null}})); + await page.goto(process.env.ODYSSEUS_TEST_STATIC_ORIGIN+'/static/js/storage.js'); + await page.setContent('__STYLES__'); + const result = await page.evaluate(async id => { + const html = await (await fetch('/static/index.html')).text(); + const source = new DOMParser().parseFromString(html,'text/html'); + document.body.append(source.getElementById(id)); + const modal = document.getElementById(id); + const content = modal.querySelector('.modal-content'); + const manager = await import('/static/js/modalManager.js'); + let open, close; + if(id === 'settings-modal') { + const lifecycle = await import('/static/js/settings/lifecycle.js'); + open = () => lifecycle.showSettingsModal(modal); + close = () => lifecycle.hideSettingsModal(modal); + } else { + const theme = await import('/static/js/theme.js'); + // The old sidebar simply removed hidden; the public opener now + // also cancels a pending close and restores manager state. + open = theme.openPopup || (() => modal.classList.remove('hidden')); + close = () => theme.closePopup(); + } + const visible = () => getComputedStyle(modal).display !== 'none'; + const wait = () => new Promise(resolve=>setTimeout(resolve,350)); + const states = {}; + if (id === 'theme-modal') { + const theme = await import('/static/js/theme.js'); + theme.togglePopup(); + theme.togglePopup(); + theme.togglePopup(); + await wait(); + states.fastToggleReopens = visible(); + close(); + await wait(); + } + open(); + manager.register(id,{closeFn:close,restoreFn:()=>{}}); + manager.minimize(id); + manager.close(id); + await wait(); + open(); + await wait(); + states.afterDockClose = visible(); + // An opening/descendant animation must not reach an orphan closer. + content.dispatchEvent(new AnimationEvent('animationend',{bubbles:true,animationName:'modal-enter'})); + states.afterOpeningAnimation = visible(); + // Begin a visible close, then reopen before its fallback expires. + close(); + open(); + await wait(); + states.afterFastReopen = visible(); + manager.register(id,{closeFn:close,restoreFn:()=>{}}); + manager.minimize(id); + open(); + states.directOpenRestoresMinimized = visible() && !manager.isMinimized(id); + // A child animation must not terminate a normal parent close. + close(); + content.querySelector('button').dispatchEvent(new AnimationEvent('animationend',{bubbles:true})); + states.childAnimationDoesNotClose = !modal.classList.contains('hidden'); + content.dispatchEvent(new AnimationEvent('animationend',{bubbles:true,animationName:'modal-exit'})); + states.normalCloseCompletes = modal.classList.contains('hidden') && !content.classList.contains('modal-closing'); + // Completing by animation must also remove the timer. + open(); + await wait(); + states.reopenAfterCompletedAnimation = visible(); + close(); + await wait(); + states.fallbackCompletes = modal.classList.contains('hidden') && !content.classList.contains('modal-closing'); + return states; + }, MODAL_ID); + console.log(JSON.stringify(result)); + } finally { await browser.close(); } + """ + script = script.replace('WIDTH', str(width)).replace('MODAL_ID', json.dumps(modal_id)) + script = script.replace('__STYLES__', stylesheet_link_tags()) + result = subprocess.run(['node','--input-type=module','-e',script], capture_output=True, text=True, timeout=60) + assert result.returncode == 0, result.stdout + result.stderr + states = json.loads(result.stdout.strip()) + assert all(states.values()), states + + +def test_modal_visibility_is_available_offline_before_first_open(): + script = r""" + import assert from 'node:assert/strict'; + import { chromium } from 'playwright'; + const browser = await chromium.launch({headless:true}); + try { + const context = await browser.newContext({serviceWorkers:'allow'}); + const page = await context.newPage(); + const origin = process.env.ODYSSEUS_TEST_STATIC_ORIGIN; + await page.goto(origin+'/static/js/storage.js'); + await page.evaluate(async () => { + await navigator.serviceWorker.register('/static/sw.js'); + await navigator.serviceWorker.ready; + if(!navigator.serviceWorker.controller) await new Promise(resolve => + navigator.serviceWorker.addEventListener('controllerchange',resolve,{once:true})); + const response = await caches.match('/static/js/modalVisibility.js'); + if(!response?.ok) throw new Error('Modal helper is missing from install-time precache'); + }); + await context.setOffline(true); + const offline = await context.newPage(); + await offline.goto(origin+'/static/js/storage.js'); + const exports = await offline.evaluate(async () => + Object.keys(await import('/static/js/modalVisibility.js')).sort()); + assert.deepEqual(exports,['hideModal','showModal']); + } finally { await browser.close(); } + """ + result = subprocess.run(['node','--input-type=module','-e',script], capture_output=True, text=True, timeout=90) + assert result.returncode == 0, result.stdout + result.stderr