Merge remote-tracking branch 'origin/main' into feat/249-system-appearance

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; }
@@ -131,7 +131,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.
@@ -158,15 +160,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;
@@ -182,7 +192,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; }
@@ -192,10 +202,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);
                 });
@@ -204,7 +216,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) {