Open the import row in the shell, not in a browser tab

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

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,
 		} );
 	}
 

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', () => {