diff --git a/tests/browser/workspace-images.spec.mjs b/tests/browser/workspace-images.spec.mjs index 4aec524..5727a80 100644 --- a/tests/browser/workspace-images.spec.mjs +++ b/tests/browser/workspace-images.spec.mjs @@ -483,6 +483,30 @@ test('collection viewer leaving an image Viewer.js has not built yet releases it await page.evaluate(()=>{location.hash='/overview';});await expect(page.locator('.viewer-container,#viewer-dialog')).toHaveCount(0); }); +test('collection viewer closed while its image finishes loading never builds Viewer.js afterwards',async({page})=>{ + await page.goto(workspace);await expect(cards(page)).toHaveCount(2); + let held;const waiting=new Promise(resolve=>{held=resolve;}); + // The image is held for the whole test: its late load is delivered below, at the moment Viewer.js cannot cancel. + await page.route('**/images/serve/shared/same.png?*',async route=>{ + const url=new URL(route.request().url()); + if(url.searchParams.get('root')==='0'&&url.searchParams.has('_pm_viewer')) {held();return;} + await route.continue().catch(()=>{}); + }); + await cards(page).first().getByRole('button',{name:'Open same.png',exact:true}).click();await waiting; + const preview=page.locator('dialog#viewer-dialog'); + // Viewer.js's own cancel removes its image listeners only while `image.complete` is false. An image that has + // finished downloading but whose load event is still queued is already complete: close in that state, then deliver it. + await page.evaluate(()=>{window.finishingImage=document.querySelector('#viewer-stage > img');Object.defineProperty(window.finishingImage,'complete',{get:()=>true});}); + await page.keyboard.press('Escape');await expect(preview).toHaveCount(0); + const after=await page.evaluate(async()=>{ + const image=window.finishingImage;image.dispatchEvent(new Event('load')); + // Viewer.js builds an inline viewer in a zero-delay timeout after its image loads; this runs after it. + await new Promise(resolve=>setTimeout(resolve)); + return {src:image.hasAttribute('src'),built:!!image.parentNode?.querySelector('.viewer-container')}; + }); + expect(after).toEqual({src:false,built:false}); +}); + test('collection viewer describes video transforms and releases native playback on navigation',async({page,request})=>{ await expect(await request.post('/__test__/reset',{data:{include_video:true}})).toBeOK();await page.goto(workspace); await page.locator('article[data-image-key="0:clip.webm"] .image-open').click(); diff --git a/tests/js/workspace-media-viewer.test.js b/tests/js/workspace-media-viewer.test.js index efa7d2e..d375903 100644 --- a/tests/js/workspace-media-viewer.test.js +++ b/tests/js/workspace-media-viewer.test.js @@ -124,23 +124,32 @@ test('real inspection navigation preserves slideshow across two advances and def } finally {view.dispose();controller.dispose();parentScope.dispose();globalThis.setTimeout=oldSetTimeout;globalThis.clearTimeout=oldClearTimeout;f.restore();} }); -test('an inline Viewer not yet built is destroyed and its image released at once', async () => { +test('Viewer.js is built only on a loaded image and an unbuilt viewer is released at once', async () => { const {reportedLocationAdapter:adapter} = await import('../../web/js/workspace/history-media.mjs'); const f = mountedDom(), parentScope = createScope(), controller = borrowedController(), items = [occurrenceMedia('1',1,'c'),occurrenceMedia('2',0,'d')]; const instances = []; - // Viewer.js builds an inline viewer only once its image loads; until then `ready` is false and destroy() cancels it. + // On a loaded image Viewer.js schedules its inline build; until it runs `ready` is false and destroy() cancels it. f.win.Viewer = class { constructor(node,options) { this.node = node; this.options = options; this.destroyed = 0; this.ready = false; this.viewed = false; instances.push(this); } destroy() { this.destroyed++; } resize() {} }; const globals = {ResizeObserver:globalThis.ResizeObserver, requestAnimationFrame:globalThis.requestAnimationFrame, cancelAnimationFrame:globalThis.cancelAnimationFrame}; Object.assign(globalThis, {ResizeObserver:class { observe() {} disconnect() {} }, requestAnimationFrame:() => 1, cancelAnimationFrame() {}}); const view = module.createMediaViewer({container:f.container,parentScope,inspectionController:controller,adapter,sender:{open(){}}}); try { - view.open({items,selectedKey:'c'.repeat(64)}); + const stageImage = () => f.walk(f.container,n => n.tagName === 'img')[0]; + view.open({items,selectedKey:'d'.repeat(64)}); + // Viewer.js's own cancel misses an image whose load event is still queued, so none exists before the load. + const unloaded = stageImage(), dropped = [];unloaded.removeAttribute = key => dropped.push(key); + assert.equal(instances.length, 0, 'no Viewer.js before the image loads'); + f.walk(f.container,n => n.attributes['aria-label'] === 'Previous media')[0].fire('click'); + assert.deepEqual(dropped, ['src'], 'an image left before it loads is released at once'); + unloaded.fire('load'); + assert.equal(instances.length, 0, 'its late load builds no Viewer.js'); + stageImage().fire('load'); const [loading] = instances, image = loading.node, removed = []; image.removeAttribute = key => removed.push(key); - // Next drops the still-loading image from the stage; a deferred destroy would let its late load build there. + // Next drops the image whose build is pending; a deferred destroy would let that build run there. f.walk(f.container,n => n.attributes['aria-label'] === 'Next media')[0].fire('click'); - assert.equal(instances.length, 2, 'Next builds the next image'); + stageImage().fire('load');assert.equal(instances.length, 2, 'the next image builds its own Viewer.js once loaded'); assert.equal(loading.destroyed, 1, 'an unbuilt viewer is destroyed at once'); assert.deepEqual(removed, ['src'], 'and its image is released at once'); loading.options.viewed.call(loading); @@ -159,7 +168,7 @@ test('an inline Viewer still loading its image is destroyed only after it has vi Object.assign(globalThis, {ResizeObserver:class { observe() {} disconnect() {} }, requestAnimationFrame:() => 1, cancelAnimationFrame() {}}); const view = module.createMediaViewer({container:f.container,parentScope,inspectionController:controller,adapter,sender:{open(){}}}); try { - view.open({items,selectedKey:'c'.repeat(64)}); + view.open({items,selectedKey:'c'.repeat(64)});f.walk(f.container,n => n.tagName === 'img')[0].fire('load'); const [loading] = instances, image = loading.node, removed = []; image.removeAttribute = key => removed.push(key); f.walk(f.container,n => n.attributes['aria-label'] === 'Close image viewer')[0].fire('click'); @@ -172,7 +181,7 @@ test('an inline Viewer still loading its image is destroyed only after it has vi loading.options.viewed.call(loading); assert.equal(loading.destroyed, 1, 'never destroyed twice'); - view.open({items,selectedKey:'c'.repeat(64)}); + view.open({items,selectedKey:'c'.repeat(64)});f.walk(f.container,n => n.tagName === 'img')[0].fire('load'); const shown = instances[1]; shown.viewed = true; shown.options.viewed.call(shown); f.walk(f.container,n => n.attributes['aria-label'] === 'Close image viewer')[0].fire('click'); assert.equal(shown.destroyed, 1, 'a viewed image is destroyed at once'); @@ -260,7 +269,7 @@ test('viewer uses adapter.mediaUrl for image and video', async () => { const img = f.walk(f.container,n => n.tagName === 'img')[0]; assert.equal(img.src, image.url, 'the blob URL is used exactly'); assert.doesNotMatch(img.src, /\?/); - assert.equal(instances[0].node, img, 'Viewer.js shows the blob image'); + img.fire('load');assert.equal(instances[0].node, img, 'Viewer.js shows the blob image'); const download = f.walk(f.container,n => n.attributes['aria-label'] === 'Download original')[0]; assert.equal(download.href, image.url);assert.equal(download.download, 'a.png'); f.walk(f.container,n => n.attributes['aria-label'] === 'Next media')[0].fire('click'); diff --git a/web/js/workspace/media-viewer.mjs b/web/js/workspace/media-viewer.mjs index 4e58f9e..a0a1ccf 100644 --- a/web/js/workspace/media-viewer.mjs +++ b/web/js/workspace/media-viewer.mjs @@ -151,6 +151,14 @@ export function createMediaViewer({api,container,parentScope,inspectionControlle } node.src = src;stage.append(node); if (!selected.is_video && window.Viewer) { + // Viewer.js is built on an image that has loaded. Its own cancel removes its image listeners only + // while `image.complete` is false, so an image left with its load event still queued would build a + // viewer into the released image after destroy() (#293). Until then no viewer exists to cancel. + let mountedViewer = false; + mediaLocal.listen(node,'load',() => { if (current()) { mountedViewer = true;mountViewer(); } },{once:true}); + mediaLocal.cleanup(() => { if (!mountedViewer) node.removeAttribute('src'); }); + } + function mountViewer() { let fitFrame = 0, fitWidth, fitHeight, viewSettled = false, released = false, fallback = 0; const release = () => { if (released) return; @@ -176,8 +184,8 @@ export function createMediaViewer({api,container,parentScope,inspectionControlle const observer = new ResizeObserver(scheduleFit);observer.observe(stage); viewer = instance;mediaLocal.cleanup(() => { observer.disconnect();cancelAnimationFrame(fitFrame); - // Before Viewer.js has built (its image still loading), destroy() cancels the pending build; - // deferring it would let the late load build into the image the stage has just dropped. + // Before Viewer.js has built (its zero-delay build is pending), destroy() cancels that build; + // deferring it would let the build run into the image the stage has just dropped. // Once built, inline Viewer.js keeps viewing a still-loading image after destroy(), and that // view rebuilds its list from images that still have a src; keep both until the view settles, // fails or times out. @@ -190,7 +198,7 @@ export function createMediaViewer({api,container,parentScope,inspectionControlle } function updateControls(state) { const unavailable = inspectionState.refreshRequired ? inspectionState.message : state.phase === 'error' ? 'Media could not be loaded. Retry this file.' : entry.is_video ? - 'Image transforms are unavailable for video.' : !viewer ? + 'Image transforms are unavailable for video.' : !window.Viewer ? 'Image transforms are unavailable because Viewer.js could not load.' : !state.mediaReady ? 'Image transforms are available when the image is ready.' : ''; reason.textContent = unavailable;reason.hidden = !unavailable; for (const node of transforms) {