From add035d94061956f957b6533848e00e8dce94d8d Mon Sep 17 00:00:00 2001 From: Vito Date: Fri, 23 Oct 2026 06:08:44 -0700 Subject: [PATCH] fix(images): release a collection viewer image Viewer.js has not built yet (#293) * fix(images): release a collection viewer image Viewer.js has not built yet Leaving a viewer image while it was still loading (Previous, Next, the arrow keys, Retry) deferred the inline Viewer.js destroy() until the image's view settled, failed or a 10 s fallback ran. Before its image loads Viewer.js has not built, so nothing could settle: the stage dropped the image, the late load ran Viewer.js's build() on an image without a parent, and getComputedStyle(null) threw a page error ("Argument 1 is not an object" in Firefox, "must be an instance of Element" in WebKit). Under CI load this failed "collection viewer follows filtered loaded order beyond mounted cards". A viewer that is not ready yet is now destroyed at once, which cancels its pending build, and its image src is released. A built viewer with a still-loading view keeps the existing deferral. * fix(images): build Viewer.js only on a loaded viewer image Destroying an unbuilt inline Viewer.js at once (429b3a8) is not safe in one state. Viewer.js's own cancel removes its image listeners only while `image.complete` is false, so an image that had finished downloading with its load event still queued kept them. Closing the viewer then released the src, the queued load built Viewer.js anyway, and its first view found no listed image: "can't access property querySelector, c is undefined" (workspace-history.spec.mjs:222 on Firefox in CI). The viewer now constructs Viewer.js from its own load listener, which the media scope removes when the image is left. Before the load no Viewer.js exists, so leaving the image only releases its src; after it Viewer.js sees a complete image and only its zero-delay build is pending, which destroy() cancels. Transforms report Viewer.js as unavailable only when it did not load at all. * fix(images): describe transforms by the Viewer.js an image was built with 1086436 creates Viewer.js only once the image has loaded, so the "Viewer.js could not load" reason moved from the instance to the live window.Viewer. An image shown natively while Viewer.js was missing then offered transforms as soon as Viewer.js reappeared, which full-inspector.spec.mjs:854 ("collection viewer preserves original download and rooted retries") caught on every project. Each rebuild now captures the Viewer.js it builds with. The reason, the native fallback and the later construction all use that capture. --- tests/browser/workspace-images.spec.mjs | 44 +++++++++++++++++++++++ tests/js/workspace-media-viewer.test.js | 47 +++++++++++++++++++++---- web/js/workspace/media-viewer.mjs | 34 ++++++++++++------ 3 files changed, 107 insertions(+), 18 deletions(-) diff --git a/tests/browser/workspace-images.spec.mjs b/tests/browser/workspace-images.spec.mjs index 4f544cf..5727a80 100644 --- a/tests/browser/workspace-images.spec.mjs +++ b/tests/browser/workspace-images.spec.mjs @@ -463,6 +463,50 @@ test('collection viewer ignores delayed media callbacks and keeps inspection ret await page.evaluate(()=>{location.hash='/overview';});await expect(page.locator('.viewer-container,#viewer-dialog')).toHaveCount(0); }); +test('collection viewer leaving an image Viewer.js has not built yet releases it at once',async({page})=>{ + await page.goto(workspace);await expect(cards(page)).toHaveCount(2); + let release,held;const waiting=new Promise(resolve=>{held=resolve;});const pending=new Promise(resolve=>{release=resolve;}); + 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();await pending;} + 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'); + await page.evaluate(()=>{window.unbuiltImage=document.querySelector('#viewer-stage > img');}); + await preview.getByRole('button',{name:'Next media',exact:true}).click(); + await expect(preview.getByRole('button',{name:'Zoom in',exact:true})).toHaveAttribute('aria-disabled','false'); + // The left image is off the stage. Viewer.js has not built it, so the viewer is destroyed and the src released at + // once: its late load must never build Viewer.js into an image without a parent (getComputedStyle(null)). + release(); + await expect.poll(()=>page.evaluate(()=>[window.unbuiltImage.isConnected,window.unbuiltImage.hasAttribute('src'),!!window.unbuiltImage.viewer])).toEqual([false,false,false]); + 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 749694a..d375903 100644 --- a/tests/js/workspace-media-viewer.test.js +++ b/tests/js/workspace-media-viewer.test.js @@ -124,18 +124,51 @@ 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 still loading its image is destroyed only after it has viewed it', 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')]; + const f = mountedDom(), parentScope = createScope(), controller = borrowedController(), items = [occurrenceMedia('1',1,'c'),occurrenceMedia('2',0,'d')]; const instances = []; - // Viewer.js's inline destroy() leaves an in-flight view running; its callback then needs a live instance. - f.win.Viewer = class { constructor(node,options) { this.node = node; this.options = options; this.destroyed = 0; this.viewed = false; instances.push(this); } + // 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 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'); + 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); + assert.equal(loading.destroyed, 1, 'never destroyed twice'); + } finally { view.dispose(); parentScope.dispose(); f.restore(); Object.assign(globalThis, globals); } +}); + +test('an inline Viewer still loading its image is destroyed only after it has viewed it', async () => { + const {reportedLocationAdapter:adapter} = await import('../../web/js/workspace/history-media.mjs'); + const f = mountedDom(), parentScope = createScope(), controller = borrowedController(), items = [occurrenceMedia('1',1,'c')]; + const instances = []; + // A built (`ready`) inline Viewer.js's destroy() leaves an in-flight view running; its callback then needs a live instance. + f.win.Viewer = class { constructor(node,options) { this.node = node; this.options = options; this.destroyed = 0; this.ready = true; 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)});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'); @@ -148,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'); @@ -236,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 4427dd6..bd1b323 100644 --- a/web/js/workspace/media-viewer.mjs +++ b/web/js/workspace/media-viewer.mjs @@ -35,7 +35,7 @@ export function createMediaViewer({api,container,parentScope,inspectionControlle if (scope.signal.aborted) return; active?.close(false); const {items,selectedKey,trigger = document.activeElement} = options; - let mounted = false, initial, entry, currentKey, viewer = null, mediaScope = null, closed = false; + let mounted = false, initial, entry, currentKey, viewer = null, withoutViewer = false, mediaScope = null, closed = false; let originalDownload = null, inspectionState = inspectionController.state; const session = createViewerSession({adapter,items,initialKey:selectedKey,onChange:(state,selected) => { if (!mounted) { initial = [state,selected]; return; } @@ -115,7 +115,9 @@ export function createMediaViewer({api,container,parentScope,inspectionControlle mediaScope?.dispose();mediaScope = createScope();viewer = null; for (const child of [...stage.children]) if (child !== failure) child.remove(); failure.hidden = true; - const mediaLocal = mediaScope, key = session.state.key; + const mediaLocal = mediaScope, key = session.state.key, ViewerJs = window.Viewer; + // Transforms follow the Viewer.js this media was built with, which builds once the image has loaded. + withoutViewer = !ViewerJs; const current = () => !local.signal.aborted && !mediaLocal.signal.aborted && session.state.key === key; // An adapter's own media URL (a session-owned blob:) is used exactly, so a retry reuses it; a server // original gets a fresh cache-buster on every build. @@ -142,15 +144,23 @@ export function createMediaViewer({api,container,parentScope,inspectionControlle },{once:true}); } else { node.alt = selected.filename;node.decoding = 'async'; - node.hidden = !!window.Viewer; + node.hidden = !!ViewerJs; // With Viewer.js the src is released after the viewer (see release below). - if (!window.Viewer) { + if (!ViewerJs) { mediaLocal.listen(node,'load',() => {if (current()) session.ready(key);}); mediaLocal.cleanup(() => node.removeAttribute('src')); } } node.src = src;stage.append(node); - if (!selected.is_video && window.Viewer) { + if (!selected.is_video && ViewerJs) { + // 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; @@ -166,7 +176,7 @@ export function createMediaViewer({api,container,parentScope,inspectionControlle fitWidth = width;fitHeight = height;instance.resize(); }); }; - const instance = new window.Viewer(node,{inline:true,toolbar:false,navbar:false,title:false,button:false, + const instance = new ViewerJs(node,{inline:true,toolbar:false,navbar:false,title:false,button:false, keyboard:false,fullscreen:false,transition:false,viewed() { viewSettled = true; if (mediaLocal.signal.aborted) { release(); return; } @@ -176,10 +186,12 @@ export function createMediaViewer({api,container,parentScope,inspectionControlle const observer = new ResizeObserver(scheduleFit);observer.observe(stage); viewer = instance;mediaLocal.cleanup(() => { observer.disconnect();cancelAnimationFrame(fitFrame); - // 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. - if (viewSettled) { release(); return; } + // 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. + if (viewSettled || !instance.ready) { release(); return; } node.addEventListener('error',release,{once:true}); fallback = setTimeout(release,PENDING_VIEW_DESTROY_MS); }); @@ -188,7 +200,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.' : withoutViewer ? '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) {