[PM-30127] [Defect] Automatic pop out is not necessary when editing a file send (#19111)

* relax popout requirement on file type Send edit

* remove popout req for text sends on firefox and safari & adjust test coverage

apps/browser/src/popup/app-routing.module.ts

@@ -356,7 +356,7 @@ const routes: Routes = [
   {
     path: "edit-send",
     component: SendAddEditV2Component,
-    canActivate: [authGuard, filePickerPopoutGuard()],
+    canActivate: [authGuard],
     data: { elevation: 1 } satisfies RouteDataProperties,
   },
   {

apps/browser/src/tools/popup/guards/file-picker-popout.guard.spec.ts

@@ -269,23 +269,21 @@ describe("filePickerPopoutGuard", () => {
       inSidebarSpy.mockReturnValue(false);
     });
 
-    it.each([
-      { route: "/import" },
-      { route: "/add-send" },
-      { route: "/edit-send" },
-      { route: "/attachments" },
-    ])("should open popout for $route route", async ({ route }) => {
-      const importState: RouterStateSnapshot = {
-        url: route,
-      } as RouterStateSnapshot;
+    it.each([{ route: "/import" }, { route: "/add-send" }, { route: "/attachments" }])(
+      "should open popout for $route route",
+      async ({ route }) => {
+        const importState: RouterStateSnapshot = {
+          url: route,
+        } as RouterStateSnapshot;
 
-      const guard = filePickerPopoutGuard();
-      const result = await TestBed.runInInjectionContext(() => guard(mockRoute, importState));
+        const guard = filePickerPopoutGuard();
+        const result = await TestBed.runInInjectionContext(() => guard(mockRoute, importState));
 
-      expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#" + route);
-      expect(closePopupSpy).toHaveBeenCalledWith(window);
-      expect(result).toBe(false);
-    });
+        expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#" + route);
+        expect(closePopupSpy).toHaveBeenCalledWith(window);
+        expect(result).toBe(false);
+      },
+    );
   });
 
   describe("Url handling", () => {
@@ -354,30 +352,6 @@ describe("filePickerPopoutGuard", () => {
           expect(result).toBe(true);
         },
       );
-
-      it.each([
-        { deviceType: DeviceType.FirefoxExtension, name: "Firefox" },
-        { deviceType: DeviceType.SafariExtension, name: "Safari" },
-        { deviceType: DeviceType.ChromeExtension, name: "Chrome" },
-        { deviceType: DeviceType.EdgeExtension, name: "Edge" },
-      ])("should allow navigation for editing text Sends on $name", async ({ deviceType }) => {
-        getDeviceSpy.mockReturnValue(deviceType);
-        inPopoutSpy.mockReturnValue(false);
-        inSidebarSpy.mockReturnValue(false);
-
-        const editTextSendState: RouterStateSnapshot = {
-          url: "/edit-send?sendId=abc123&type=0",
-        } as RouterStateSnapshot;
-
-        const guard = filePickerPopoutGuard();
-        const result = await TestBed.runInInjectionContext(() =>
-          guard(mockRoute, editTextSendState),
-        );
-
-        expect(openPopoutSpy).not.toHaveBeenCalled();
-        expect(closePopupSpy).not.toHaveBeenCalled();
-        expect(result).toBe(true);
-      });
     });
 
     describe("File Sends (type=1)", () => {
@@ -487,115 +461,6 @@ describe("filePickerPopoutGuard", () => {
           }
         },
       );
-
-      it.each([
-        {
-          deviceType: DeviceType.FirefoxExtension,
-          name: "Firefox",
-          os: "Mac",
-          userAgent: "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7)",
-          expectPopout: true,
-        },
-        {
-          deviceType: DeviceType.FirefoxExtension,
-          name: "Firefox",
-          os: "Linux",
-          userAgent: "Mozilla/5.0 (X11; Linux x86_64)",
-          expectPopout: true,
-        },
-        {
-          deviceType: DeviceType.FirefoxExtension,
-          name: "Firefox",
-          os: "Windows",
-          userAgent: "Mozilla/5.0 (Windows NT 10.0; Win64; x64)",
-          expectPopout: true,
-        },
-        {
-          deviceType: DeviceType.SafariExtension,
-          name: "Safari",
-          os: "Mac",
-          userAgent: "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7)",
-          expectPopout: true,
-        },
-        {
-          deviceType: DeviceType.ChromeExtension,
-          name: "Chrome",
-          os: "Mac",
-          userAgent: "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7)",
-          expectPopout: true,
-        },
-        {
-          deviceType: DeviceType.ChromeExtension,
-          name: "Chrome",
-          os: "Linux",
-          userAgent: "Mozilla/5.0 (X11; Linux x86_64)",
-          expectPopout: true,
-        },
-        {
-          deviceType: DeviceType.ChromeExtension,
-          name: "Chrome",
-          os: "Windows",
-          userAgent: "Mozilla/5.0 (Windows NT 10.0; Win64; x64)",
-          expectPopout: false,
-        },
-        {
-          deviceType: DeviceType.EdgeExtension,
-          name: "Edge",
-          os: "Mac",
-          userAgent: "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7)",
-          expectPopout: true,
-        },
-        {
-          deviceType: DeviceType.EdgeExtension,
-          name: "Edge",
-          os: "Linux",
-          userAgent: "Mozilla/5.0 (X11; Linux x86_64)",
-          expectPopout: true,
-        },
-        {
-          deviceType: DeviceType.EdgeExtension,
-          name: "Edge",
-          os: "Windows",
-          userAgent: "Mozilla/5.0 (Windows NT 10.0; Win64; x64)",
-          expectPopout: false,
-        },
-      ])(
-        "should require popout for editing a file Send on $name $os",
-        async ({ deviceType, userAgent, expectPopout }) => {
-          getDeviceSpy.mockReturnValue(deviceType);
-          inPopoutSpy.mockReturnValue(false);
-          inSidebarSpy.mockReturnValue(false);
-
-          if (userAgent) {
-            Object.defineProperty(window, "navigator", {
-              value: { userAgent, appVersion: userAgent },
-              configurable: true,
-              writable: true,
-            });
-          }
-
-          const editFileSendState: RouterStateSnapshot = {
-            url: "/edit-send?sendId=abc123&type=1",
-          } as RouterStateSnapshot;
-
-          const guard = filePickerPopoutGuard();
-          const result = await TestBed.runInInjectionContext(() =>
-            guard(mockRoute, editFileSendState),
-          );
-
-          if (expectPopout === false) {
-            expect(openPopoutSpy).not.toHaveBeenCalled();
-            expect(closePopupSpy).not.toHaveBeenCalled();
-            expect(result).toBe(true);
-          } else {
-            expect(openPopoutSpy).toHaveBeenCalledWith(
-              "popup/index.html#/edit-send?sendId=abc123&type=1",
-            );
-            expect(closePopupSpy).toHaveBeenCalledWith(window);
-            expect(result).toBe(false);
-          }
-        },
-      );
     });
 
     describe("Send routes without type parameter", () => {

apps/browser/src/tools/popup/guards/file-picker-popout.guard.ts

@@ -24,9 +24,9 @@ import { SendType } from "@bitwarden/common/tools/send/types/send-type";
  */
 export function filePickerPopoutGuard(): CanActivateFn {
   return async (_route: ActivatedRouteSnapshot, state: RouterStateSnapshot) => {
-    // Check if this is a text Send route (no file picker needed)
-    if (isTextOnlySendRoute(state.url)) {
-      return true; // Allow navigation without popout
+    // Text Sends have no file picker — never require a popout regardless of browser
+    if (isTextSendRoute(state.url)) {
+      return true;
     }
 
     // Check if browser is one that needs popout for file pickers
@@ -81,29 +81,16 @@ export function filePickerPopoutGuard(): CanActivateFn {
 }
 
 /**
- * Determines if the route is for a text Send that doesn't require file picker display.
- *
- * @param url The route URL with query parameters
- * @returns true if this is a Send route with explicitly text type (SendType.Text = 0)
+ * Returns true when the add-send route targets a Text Send (type=0).
+ * Text Sends have no file picker and never require a popout window.
  */
-function isTextOnlySendRoute(url: string): boolean {
-  // Only apply to Send routes
-  if (!url.includes("/add-send") && !url.includes("/edit-send")) {
+function isTextSendRoute(url: string): boolean {
+  if (!url.includes("/add-send")) {
     return false;
   }
-
-  // Parse query parameters to check Send type
-  const queryStartIndex = url.indexOf("?");
-  if (queryStartIndex === -1) {
-    // No query params - default to requiring popout for safety
+  const queryStart = url.indexOf("?");
+  if (queryStart === -1) {
     return false;
   }
-
-  const queryString = url.substring(queryStartIndex + 1);
-  const params = new URLSearchParams(queryString);
-  const typeParam = params.get("type");
-
-  // Only skip popout for explicitly text-based Sends (SendType.Text = 0)
-  // If type is missing, null, or not text, default to requiring popout
-  return typeParam === String(SendType.Text);
+  return new URLSearchParams(url.substring(queryStart + 1)).get("type") === String(SendType.Text);
 }

apps/browser/src/tools/popup/guards/firefox-popout.guard.spec.ts

@@ -1,194 +0,0 @@
-import { TestBed } from "@angular/core/testing";
-import { ActivatedRouteSnapshot, RouterStateSnapshot } from "@angular/router";
-
-import { DeviceType } from "@bitwarden/common/enums";
-
-import { BrowserApi } from "../../../platform/browser/browser-api";
-import BrowserPopupUtils from "../../../platform/browser/browser-popup-utils";
-import { BrowserPlatformUtilsService } from "../../../platform/services/platform-utils/browser-platform-utils.service";
-
-import { firefoxPopoutGuard } from "./firefox-popout.guard";
-
-describe("firefoxPopoutGuard", () => {
-  let getDeviceSpy: jest.SpyInstance;
-  let inPopoutSpy: jest.SpyInstance;
-  let inSidebarSpy: jest.SpyInstance;
-  let openPopoutSpy: jest.SpyInstance;
-  let closePopupSpy: jest.SpyInstance;
-
-  const mockRoute = {} as ActivatedRouteSnapshot;
-  const mockState: RouterStateSnapshot = {
-    url: "/import?param=value",
-  } as RouterStateSnapshot;
-
-  beforeEach(() => {
-    getDeviceSpy = jest.spyOn(BrowserPlatformUtilsService, "getDevice");
-    inPopoutSpy = jest.spyOn(BrowserPopupUtils, "inPopout");
-    inSidebarSpy = jest.spyOn(BrowserPopupUtils, "inSidebar");
-    openPopoutSpy = jest.spyOn(BrowserPopupUtils, "openPopout").mockImplementation();
-    closePopupSpy = jest.spyOn(BrowserApi, "closePopup").mockImplementation();
-
-    TestBed.configureTestingModule({});
-  });
-
-  afterEach(() => {
-    jest.clearAllMocks();
-  });
-
-  describe("when browser is Firefox", () => {
-    beforeEach(() => {
-      getDeviceSpy.mockReturnValue(DeviceType.FirefoxExtension);
-      inPopoutSpy.mockReturnValue(false);
-      inSidebarSpy.mockReturnValue(false);
-    });
-
-    it("should open popout and block navigation when not already in popout or sidebar", async () => {
-      const guard = firefoxPopoutGuard();
-      const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState));
-
-      expect(getDeviceSpy).toHaveBeenCalledWith(window);
-      expect(inPopoutSpy).toHaveBeenCalledWith(window);
-      expect(inSidebarSpy).toHaveBeenCalledWith(window);
-      expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/import?param=value");
-      expect(closePopupSpy).toHaveBeenCalledWith(window);
-      expect(result).toBe(false);
-    });
-
-    it("should not add autoClosePopout parameter to the url", async () => {
-      const guard = firefoxPopoutGuard();
-      await TestBed.runInInjectionContext(() => guard(mockRoute, mockState));
-
-      expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/import?param=value");
-      expect(openPopoutSpy).not.toHaveBeenCalledWith(expect.stringContaining("autoClosePopout"));
-    });
-
-    it("should allow navigation when already in popout", async () => {
-      inPopoutSpy.mockReturnValue(true);
-
-      const guard = firefoxPopoutGuard();
-      const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState));
-
-      expect(openPopoutSpy).not.toHaveBeenCalled();
-      expect(closePopupSpy).not.toHaveBeenCalled();
-      expect(result).toBe(true);
-    });
-
-    it("should allow navigation when already in sidebar", async () => {
-      inSidebarSpy.mockReturnValue(true);
-
-      const guard = firefoxPopoutGuard();
-      const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState));
-
-      expect(openPopoutSpy).not.toHaveBeenCalled();
-      expect(closePopupSpy).not.toHaveBeenCalled();
-      expect(result).toBe(true);
-    });
-  });
-
-  describe("when browser is not Firefox", () => {
-    beforeEach(() => {
-      inPopoutSpy.mockReturnValue(false);
-      inSidebarSpy.mockReturnValue(false);
-    });
-
-    it.each([
-      { deviceType: DeviceType.ChromeExtension, name: "ChromeExtension" },
-      { deviceType: DeviceType.EdgeExtension, name: "EdgeExtension" },
-      { deviceType: DeviceType.OperaExtension, name: "OperaExtension" },
-      { deviceType: DeviceType.SafariExtension, name: "SafariExtension" },
-      { deviceType: DeviceType.VivaldiExtension, name: "VivaldiExtension" },
-    ])(
-      "should allow navigation without opening popout when device is $name",
-      async ({ deviceType }) => {
-        getDeviceSpy.mockReturnValue(deviceType);
-
-        const guard = firefoxPopoutGuard();
-        const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState));
-
-        expect(getDeviceSpy).toHaveBeenCalledWith(window);
-        expect(openPopoutSpy).not.toHaveBeenCalled();
-        expect(closePopupSpy).not.toHaveBeenCalled();
-        expect(result).toBe(true);
-      },
-    );
-  });
-
-  describe("file picker routes", () => {
-    beforeEach(() => {
-      getDeviceSpy.mockReturnValue(DeviceType.FirefoxExtension);
-      inPopoutSpy.mockReturnValue(false);
-      inSidebarSpy.mockReturnValue(false);
-    });
-
-    it("should open popout for /import route", async () => {
-      const importState: RouterStateSnapshot = {
-        url: "/import",
-      } as RouterStateSnapshot;
-
-      const guard = firefoxPopoutGuard();
-      const result = await TestBed.runInInjectionContext(() => guard(mockRoute, importState));
-
-      expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/import");
-      expect(closePopupSpy).toHaveBeenCalledWith(window);
-      expect(result).toBe(false);
-    });
-
-    it("should open popout for /add-send route", async () => {
-      const addSendState: RouterStateSnapshot = {
-        url: "/add-send",
-      } as RouterStateSnapshot;
-
-      const guard = firefoxPopoutGuard();
-      const result = await TestBed.runInInjectionContext(() => guard(mockRoute, addSendState));
-
-      expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/add-send");
-      expect(closePopupSpy).toHaveBeenCalledWith(window);
-      expect(result).toBe(false);
-    });
-
-    it("should open popout for /edit-send route", async () => {
-      const editSendState: RouterStateSnapshot = {
-        url: "/edit-send",
-      } as RouterStateSnapshot;
-
-      const guard = firefoxPopoutGuard();
-      const result = await TestBed.runInInjectionContext(() => guard(mockRoute, editSendState));
-
-      expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/edit-send");
-      expect(closePopupSpy).toHaveBeenCalledWith(window);
-      expect(result).toBe(false);
-    });
-  });
-
-  describe("url handling", () => {
-    beforeEach(() => {
-      getDeviceSpy.mockReturnValue(DeviceType.FirefoxExtension);
-      inPopoutSpy.mockReturnValue(false);
-      inSidebarSpy.mockReturnValue(false);
-    });
-
-    it("should preserve query parameters in the popout url", async () => {
-      const stateWithQuery: RouterStateSnapshot = {
-        url: "/import?foo=bar&baz=qux",
-      } as RouterStateSnapshot;
-
-      const guard = firefoxPopoutGuard();
-      await TestBed.runInInjectionContext(() => guard(mockRoute, stateWithQuery));
-
-      expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/import?foo=bar&baz=qux");
-      expect(closePopupSpy).toHaveBeenCalledWith(window);
-    });
-
-    it("should handle urls without query parameters", async () => {
-      const stateWithoutQuery: RouterStateSnapshot = {
-        url: "/simple-path",
-      } as RouterStateSnapshot;
-
-      const guard = firefoxPopoutGuard();
-      await TestBed.runInInjectionContext(() => guard(mockRoute, stateWithoutQuery));
-
-      expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/simple-path");
-      expect(closePopupSpy).toHaveBeenCalledWith(window);
-    });
-  });
-});

apps/browser/src/tools/popup/guards/firefox-popout.guard.ts

@@ -1,38 +0,0 @@
-import { ActivatedRouteSnapshot, CanActivateFn, RouterStateSnapshot } from "@angular/router";
-
-import { BrowserApi } from "@bitwarden/browser/platform/browser/browser-api";
-import BrowserPopupUtils from "@bitwarden/browser/platform/browser/browser-popup-utils";
-import { BrowserPlatformUtilsService } from "@bitwarden/browser/platform/services/platform-utils/browser-platform-utils.service";
-import { DeviceType } from "@bitwarden/common/enums";
-
-/**
- * Guard that forces a popout window on Firefox browser when a file picker could be exposed.
- * Necessary to avoid a crash: https://bugzilla.mozilla.org/show_bug.cgi?id=1292701
- * Also disallows the user from closing a popout and re-opening the view exposing the file picker.
- *
- * @returns CanActivateFn that opens popout and blocks navigation on Firefox
- */
-export function firefoxPopoutGuard(): CanActivateFn {
-  return async (_route: ActivatedRouteSnapshot, state: RouterStateSnapshot) => {
-    // Check if browser is Firefox using the platform utils service
-    const deviceType = BrowserPlatformUtilsService.getDevice(window);
-    const isFirefox = deviceType === DeviceType.FirefoxExtension;
-
-    // Check if already in popout/sidebar
-    const inPopout = BrowserPopupUtils.inPopout(window);
-    const inSidebar = BrowserPopupUtils.inSidebar(window);
-
-    // Open popout if on Firefox and not already in popout/sidebar
-    if (isFirefox && !inPopout && !inSidebar) {
-      // Don't add autoClosePopout for file picker scenarios - user should manually close
-      await BrowserPopupUtils.openPopout(`popup/index.html#${state.url}`);
-
-      // Close the original popup window
-      BrowserApi.closePopup(window);
-
-      return false; // Block navigation - popout will reload
-    }
-
-    return true; // Allow navigation
-  };
-}