From a3b35419961190e9fc80880f7ef958081e26c793 Mon Sep 17 00:00:00 2001 From: prismiwi2015 Date: Sun, 16 Aug 2026 23:40:04 +0200 Subject: [PATCH] Open the import row in the shell, not in a browser tab The row carried a URL and left the opening to the shell, which was the wrong reading of its contract. A constellation routes a row's URL into a window only when there is an admin menu behind it; on a **system** tile there is none, so its last resort for a URL-only row is window.open( sub.url, '_blank', 'noopener,noreferrer' ) -- and clicking "Import forms" threw the page out of the desktop into a browser tab, which is the one thing a desktop exists not to do. The row now carries an `onSelect` that opens the page through `wp.os.windowManager.open()`, the same call the shell makes for an admin URL of its own, under a stable id so a second click focuses the window instead of opening another. Without a shell it navigates, because a pop-up the browser may block is not a fallback. Verified on the desktop with `window.open` instrumented: zero calls, one browser tab, one `allterrain-forms-import` window, and still one after clicking the row again. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_0119U4sRRWGcTQdwYreTwpAp --- assets/js/dock.js | 18 ++++++++++- assets/js/dock.min.js | 2 +- src/dock.ts | 64 ++++++++++++++++++++++++++++++++++----- tests/vitest/dock.test.ts | 57 ++++++++++++++++++++++++++++++---- 4 files changed, 125 insertions(+), 16 deletions(-) diff --git a/assets/js/dock.js b/assets/js/dock.js index 0de9010..969dc16 100644 --- a/assets/js/dock.js +++ b/assets/js/dock.js @@ -5,9 +5,18 @@ var allTerrainFormsDock = function(exports) { const ENTRIES = "allterrain-forms-entries"; const THEMES = "allterrain-forms-themes"; const ANALYTICS = "allterrain-forms-analytics"; + const IMPORT = "allterrain-forms-import"; function open(id) { shell()?.openWindow?.(id, { source: "dock" }); } + function openUrl(id, url, title) { + const manager = shell()?.windowManager; + if (manager?.open) { + void manager.open({ id, url, title, icon: "dashicons-download" }); + return; + } + window.location.assign(url); + } function shell() { return window.wp?.os ?? null; } @@ -49,9 +58,16 @@ var allTerrainFormsDock = function(exports) { }); } if (config2?.canEdit && config2?.adminUrl) { + const url = `${config2.adminUrl}admin.php?page=allterrain-forms-import`; submenu.push({ title: "Import forms", - url: `${config2.adminUrl}admin.php?page=allterrain-forms-import` + // Kept in step with what `onSelect` opens: the shell reads the + // callback, but the URL is what the row means. + url, + onSelect: () => openUrl(IMPORT, url, "Import forms"), + // Declaring it lets the constellation list this row under "Open + // windows" once it is, rather than offering to open a second copy. + windowId: IMPORT }); } if (config2?.canEdit && config2?.devMode) { diff --git a/assets/js/dock.min.js b/assets/js/dock.min.js index 0b1c417..037dbf6 100644 --- a/assets/js/dock.min.js +++ b/assets/js/dock.min.js @@ -1 +1 @@ -var allTerrainFormsDock=function(a){"use strict";const u=window.allTerrainForms,r="allterrain-forms",i="allterrain-forms-entries",s="allterrain-forms-themes",o="allterrain-forms-analytics";function t(e){l()?.openWindow?.(e,{source:"dock"})}function l(){return window.wp?.os??null}function c(e){const n=[];return e?.canEdit&&n.push({title:"Forms",url:"",onSelect:()=>t(r),windowId:r}),e?.canRead&&n.push({title:"Form entries",url:"",onSelect:()=>t(i),windowId:i}),e?.canRead&&n.push({title:"Analytics",url:"",onSelect:()=>t(o),windowId:o}),e?.canEdit&&n.push({title:"Themes",url:"",onSelect:()=>t(s),windowId:s}),e?.canEdit&&e?.adminUrl&&n.push({title:"Import forms",url:`${e.adminUrl}admin.php?page=allterrain-forms-import`}),e?.canEdit&&e?.devMode&&n.push({title:"Demo data",url:"",onSelect:()=>{t(o),document.dispatchEvent(new CustomEvent("atf-open-demo-panel"))},windowId:o}),n}function d(){const e=l();if(!e?.registerSystemTile)return;const n=c(u);try{e.registerSystemTile({id:"allterrain-forms",title:"AllTerrain Forms",icon:"dashicons-feedback",order:5,onOpen:()=>t(u?.canEdit?r:i),isOpen:()=>!!(e.windowManager?.getById?.(r)||e.windowManager?.getById?.(i)||e.windowManager?.getById?.(s)),submenu:n})}catch{}}function m(){const e=l();return e?.ready?(e.ready(d),!0):e?.whenReady?(e.whenReady(d),!0):e?.registerSystemTile?(d(),!0):!1}return m()||document.addEventListener("os-init",()=>void m(),{once:!0}),a.submenuFor=c,Object.defineProperty(a,Symbol.toStringTag,{value:"Module"}),a}({}); +var allTerrainFormsDock=function(l){"use strict";const c=window.allTerrainForms,r="allterrain-forms",o="allterrain-forms-entries",d="allterrain-forms-themes",i="allterrain-forms-analytics",m="allterrain-forms-import";function t(e){a()?.openWindow?.(e,{source:"dock"})}function h(e,n,s){const p=a()?.windowManager;if(p?.open){p.open({id:e,url:n,title:s,icon:"dashicons-download"});return}window.location.assign(n)}function a(){return window.wp?.os??null}function w(e){const n=[];if(e?.canEdit&&n.push({title:"Forms",url:"",onSelect:()=>t(r),windowId:r}),e?.canRead&&n.push({title:"Form entries",url:"",onSelect:()=>t(o),windowId:o}),e?.canRead&&n.push({title:"Analytics",url:"",onSelect:()=>t(i),windowId:i}),e?.canEdit&&n.push({title:"Themes",url:"",onSelect:()=>t(d),windowId:d}),e?.canEdit&&e?.adminUrl){const s=`${e.adminUrl}admin.php?page=allterrain-forms-import`;n.push({title:"Import forms",url:s,onSelect:()=>h(m,s,"Import forms"),windowId:m})}return e?.canEdit&&e?.devMode&&n.push({title:"Demo data",url:"",onSelect:()=>{t(i),document.dispatchEvent(new CustomEvent("atf-open-demo-panel"))},windowId:i}),n}function u(){const e=a();if(!e?.registerSystemTile)return;const n=w(c);try{e.registerSystemTile({id:"allterrain-forms",title:"AllTerrain Forms",icon:"dashicons-feedback",order:5,onOpen:()=>t(c?.canEdit?r:o),isOpen:()=>!!(e.windowManager?.getById?.(r)||e.windowManager?.getById?.(o)||e.windowManager?.getById?.(d)),submenu:n})}catch{}}function f(){const e=a();return e?.ready?(e.ready(u),!0):e?.whenReady?(e.whenReady(u),!0):e?.registerSystemTile?(u(),!0):!1}return f()||document.addEventListener("os-init",()=>void f(),{once:!0}),l.submenuFor=w,Object.defineProperty(l,Symbol.toStringTag,{value:"Module"}),l}({}); diff --git a/src/dock.ts b/src/dock.ts index bced9d7..d613f45 100644 --- a/src/dock.ts +++ b/src/dock.ts @@ -35,7 +35,11 @@ interface ShellDock { whenReady?: ( cb: () => void ) => void; registerSystemTile?: ( item: SystemTile ) => void; openWindow?: ( id: string, opts?: { source?: string } ) => boolean; - windowManager?: { getById?: ( id: string ) => unknown }; + windowManager?: { + getById?: ( id: string ) => unknown; + /** Opens an admin URL as a window. Singleton ids reuse the open one. */ + open?: ( config: { id: string; url: string; title: string; icon?: string } ) => unknown; + }; } interface RuntimeConfig { @@ -52,12 +56,46 @@ const BUILDER = 'allterrain-forms'; const ENTRIES = 'allterrain-forms-entries'; const THEMES = 'allterrain-forms-themes'; const ANALYTICS = 'allterrain-forms-analytics'; +const IMPORT = 'allterrain-forms-import'; /** Opens a window through the shell. */ function open( id: string ): void { shell()?.openWindow?.( id, { source: 'dock' } ); } +/** + * Opens an admin page as a window of its own. + * + * `openWindow()` takes the id of a *native* window, and the import page is not + * one — it is a server-rendered page whose buttons POST, so it belongs in an + * iframe window like any other admin screen. `windowManager.open()` is what + * puts one there. + * + * Doing it here rather than leaving the row's `url` to the shell is the whole + * point of the callback. On a **system tile** the constellation has no menu + * behind it to route a URL through, so its last resort for a row that carries + * only a URL is `window.open( url, '_blank' )` — the import page opening in a + * browser tab, outside the desktop, which is precisely what a desktop is for + * not doing. + * + * @param id Window id. Stable, so a second click focuses the open window. + * @param url The admin page. + * @param title The window's title. + */ +function openUrl( id: string, url: string, title: string ): void { + const manager = shell()?.windowManager; + + if ( manager?.open ) { + void manager.open( { id, url, title, icon: 'dashicons-download' } ); + + return; + } + + // No shell, or one too old to route a URL: navigating beats a dead row, + // and beats a pop-up the browser is entitled to block. + window.location.assign( url ); +} + /** The shell, if there is one on this page. */ function shell(): ShellDock | null { return ( window as unknown as { wp?: { os?: ShellDock } } ).wp?.os ?? null; @@ -131,16 +169,26 @@ export function submenuFor( config: RuntimeConfig | undefined ): SubmenuRow[] { } ); } - // The one row that is a URL rather than a native window, because the import - // page is one: a server-rendered page whose buttons POST to `admin-post.php`. - // The shell opens a row with a real `url` as a window of its own, which is - // the only way this page is reachable on a desktop — with the shell up, the - // Forms admin menu is not registered, so a page with no native window and no - // row here exists at a URL nobody can get to. + // The one row behind an admin page rather than a native window, because the + // import page is one: server-rendered, and its buttons POST. It still opens + // as a window — see `openUrl()` for why the row cannot simply carry the URL + // and leave the opening to the shell. + // + // The row itself is load-bearing. With the shell up the Forms admin menu is + // not registered, so a page with no native window and no row here exists at + // a URL nobody can navigate to. if ( config?.canEdit && config?.adminUrl ) { + const url = `${ config.adminUrl }admin.php?page=allterrain-forms-import`; + submenu.push( { title: 'Import forms', - url: `${ config.adminUrl }admin.php?page=allterrain-forms-import`, + // Kept in step with what `onSelect` opens: the shell reads the + // callback, but the URL is what the row means. + url, + onSelect: () => openUrl( IMPORT, url, 'Import forms' ), + // Declaring it lets the constellation list this row under "Open + // windows" once it is, rather than offering to open a second copy. + windowId: IMPORT, } ); } diff --git a/tests/vitest/dock.test.ts b/tests/vitest/dock.test.ts index 0f7d9c4..7e0c969 100644 --- a/tests/vitest/dock.test.ts +++ b/tests/vitest/dock.test.ts @@ -17,7 +17,7 @@ * UI decision, and a UI decision is never a permission. */ -import { describe, expect, it } from 'vitest'; +import { afterEach, describe, expect, it, vi } from 'vitest'; import { submenuFor } from '../../src/dock'; import type { RuntimeConfig } from '../../src/types'; @@ -101,19 +101,64 @@ describe( 'the rest of the menu', () => { describe( 'the import row', () => { const admin = { canEdit: true, canRead: true, adminUrl: 'http://example.com/wp-admin/' }; + const location = window.location; + + // The two tests below reach into globals the rest of the file leaves alone. + afterEach( () => { + ( window as unknown as { wp?: unknown } ).wp = undefined; + Object.defineProperty( window, 'location', { configurable: true, value: location } ); + } ); it( 'is offered to somebody who may build forms', () => { expect( titles( admin ) ).toContain( 'Import forms' ); } ); - it( 'carries a real URL rather than a callback', () => { - // The import page is a server-rendered page whose buttons POST, not a - // native window — the shell opens a row with a `url` as its own window, - // and that is the only way the page is reachable with the shell up. + it( 'points at the import page', () => { const row = submenuFor( config( admin ) ).find( ( candidate ) => candidate.title === 'Import forms' ); expect( row?.url ).toBe( 'http://example.com/wp-admin/admin.php?page=allterrain-forms-import' ); - expect( row?.onSelect ).toBeUndefined(); + expect( row?.windowId ).toBe( 'allterrain-forms-import' ); + } ); + + it( 'opens the page as a window in the shell', () => { + // The bug this replaces: a row carrying only a URL is, to a *system* + // tile's constellation, a link out — it has no menu behind it to route + // a URL through, so its last resort is `window.open( url, '_blank' )` + // and the import page opened in a browser tab, outside the desktop. + const open = vi.fn(); + + ( window as unknown as { wp: unknown } ).wp = { os: { windowManager: { open } } }; + + const row = submenuFor( config( admin ) ).find( ( candidate ) => candidate.title === 'Import forms' ); + + expect( row?.onSelect ).toBeTypeOf( 'function' ); + + row?.onSelect?.(); + + expect( open ).toHaveBeenCalledWith( + expect.objectContaining( { + id: 'allterrain-forms-import', + url: 'http://example.com/wp-admin/admin.php?page=allterrain-forms-import', + title: 'Import forms', + } ) + ); + } ); + + it( 'navigates rather than dying when there is no shell to open a window', () => { + // A pop-up the browser may block is not a fallback; the page is. + const assign = vi.fn(); + + ( window as unknown as { wp?: unknown } ).wp = undefined; + Object.defineProperty( window, 'location', { + configurable: true, + value: { assign }, + } ); + + submenuFor( config( admin ) ) + .find( ( candidate ) => candidate.title === 'Import forms' ) + ?.onSelect?.(); + + expect( assign ).toHaveBeenCalledWith( 'http://example.com/wp-admin/admin.php?page=allterrain-forms-import' ); } ); it( 'is absent for somebody who may only read entries', () => {