Merge remote-tracking branch 'origin/main' into fix/images-collection-viewer-race

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

@@ -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();

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');

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) {