[PM-13388] Extension: Persist Scroll from Vault (#12325)

* add service to track scroll position of the vault tab in the popup

* add data attribute to individual vault items
- Allows query selector to focus on the specific element

* stop scroll service when a cipher is deleted

* start scroll listener when the vault page is initialized

* fix strict linting errors

* remove focus reset when navigating back to the vault screen

* skip recording the first scroll from the automatic scroll

* combine filters into a single observable

* do not start the scroll service until filters have loaded in

* refactor allFilters to come from the vault popup list filters service

* use assertion on scroll position

* hide virtual scrolling element while scrolling is restored

* update comments

* fix failing tests to use different matcher

* remove visibility trick for restoring scroll position after chatting with design

---------

Co-authored-by: bnagawiecki <107435978+bnagawiecki@users.noreply.github.com>

apps/browser/src/vault/popup/components/vault-v2/vault-list-filters/vault-list-filters.component.html

@@ -2,8 +2,9 @@
   <form
     [formGroup]="filterForm"
     class="tw-gap-2 tw-mt-2 tw-grid tw-grid-cols-2 sm:tw-grid-cols-3 lg:tw-grid-cols-4"
+    *ngIf="allFilters$ | async as allFilters"
   >
-    <ng-container *ngIf="organizations$ | async as organizations">
+    <ng-container *ngIf="allFilters.organizations as organizations">
       <bit-chip-select
         *ngIf="organizations.length"
         fullWidth
@@ -14,7 +15,7 @@
       >
       </bit-chip-select>
     </ng-container>
-    <ng-container *ngIf="collections$ | async as collections">
+    <ng-container *ngIf="allFilters.collections as collections">
       <bit-chip-select
         *ngIf="collections.length"
         fullWidth
@@ -25,7 +26,7 @@
       >
       </bit-chip-select>
     </ng-container>
-    <ng-container *ngIf="folders$ | async as folders">
+    <ng-container *ngIf="allFilters.folders as folders">
       <bit-chip-select
         *ngIf="folders.length"
         fullWidth

apps/browser/src/vault/popup/components/vault-v2/vault-list-filters/vault-list-filters.component.ts

@@ -1,6 +1,7 @@
 import { CommonModule } from "@angular/common";
 import { Component } from "@angular/core";
 import { ReactiveFormsModule } from "@angular/forms";
+import { combineLatest, map } from "rxjs";
 
 import { JslibModule } from "@bitwarden/angular/jslib.module";
 import { ChipSelectComponent } from "@bitwarden/components";
@@ -20,5 +21,20 @@ export class VaultListFiltersComponent {
   protected folders$ = this.vaultPopupListFiltersService.folders$;
   protected cipherTypes = this.vaultPopupListFiltersService.cipherTypes;
 
+  // Combine all filters into a single observable to eliminate the filters from loading separately in the UI.
+  protected allFilters$ = combineLatest([
+    this.organizations$,
+    this.collections$,
+    this.folders$,
+  ]).pipe(
+    map(([organizations, collections, folders]) => {
+      return {
+        organizations,
+        collections,
+        folders,
+      };
+    }),
+  );
+
   constructor(private vaultPopupListFiltersService: VaultPopupListFiltersService) {}
 }

apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.ts

@@ -1,10 +1,9 @@
-import { ScrollingModule } from "@angular/cdk/scrolling";
+import { CdkVirtualScrollableElement, ScrollingModule } from "@angular/cdk/scrolling";
 import { CommonModule } from "@angular/common";
-import { Component, DestroyRef, OnDestroy, OnInit } from "@angular/core";
+import { AfterViewInit, Component, DestroyRef, OnDestroy, OnInit, ViewChild } from "@angular/core";
 import { takeUntilDestroyed } from "@angular/core/rxjs-interop";
 import { RouterLink } from "@angular/router";
-import { combineLatest, Observable, shareReplay, switchMap } from "rxjs";
+import { combineLatest, filter, map, Observable, shareReplay, switchMap, take } from "rxjs";
-import { filter, map, take } from "rxjs/operators";
 
 import { JslibModule } from "@bitwarden/angular/jslib.module";
 import { CipherId, CollectionId, OrganizationId } from "@bitwarden/common/types/guid";
@@ -19,6 +18,7 @@ import { PopupHeaderComponent } from "../../../../platform/popup/layout/popup-he
 import { PopupPageComponent } from "../../../../platform/popup/layout/popup-page.component";
 import { VaultPopupItemsService } from "../../services/vault-popup-items.service";
 import { VaultPopupListFiltersService } from "../../services/vault-popup-list-filters.service";
+import { VaultPopupScrollPositionService } from "../../services/vault-popup-scroll-position.service";
 
 import { BlockedInjectionBanner } from "./blocked-injection-banner/blocked-injection-banner.component";
 import {
@@ -58,7 +58,9 @@ enum VaultState {
     DecryptionFailureDialogComponent,
   ],
 })
-export class VaultV2Component implements OnInit, OnDestroy {
+export class VaultV2Component implements OnInit, AfterViewInit, OnDestroy {
+  @ViewChild(CdkVirtualScrollableElement) virtualScrollElement?: CdkVirtualScrollableElement;
+
   cipherType = CipherType;
 
   protected favoriteCiphers$ = this.vaultPopupItemsService.favoriteCiphers$;
@@ -88,9 +90,12 @@ export class VaultV2Component implements OnInit, OnDestroy {
 
   protected VaultStateEnum = VaultState;
 
+  private allFilters$ = this.vaultPopupListFiltersService.allFilters$;
+
   constructor(
     private vaultPopupItemsService: VaultPopupItemsService,
     private vaultPopupListFiltersService: VaultPopupListFiltersService,
+    private vaultScrollPositionService: VaultPopupScrollPositionService,
     private destroyRef: DestroyRef,
     private cipherService: CipherService,
     private dialogService: DialogService,
@@ -119,6 +124,17 @@ export class VaultV2Component implements OnInit, OnDestroy {
       });
   }
 
+  ngAfterViewInit(): void {
+    if (this.virtualScrollElement) {
+      // The filters component can cause the size of the virtual scroll element to change,
+      // which can cause the scroll position to be land in the wrong spot. To fix this,
+      // wait until all filters are populated before restoring the scroll position.
+      this.allFilters$.pipe(take(1), takeUntilDestroyed(this.destroyRef)).subscribe(() => {
+        this.vaultScrollPositionService.start(this.virtualScrollElement!);
+      });
+    }
+  }
+
   async ngOnInit() {
     this.cipherService.failedToDecryptCiphers$
       .pipe(
@@ -134,5 +150,7 @@ export class VaultV2Component implements OnInit, OnDestroy {
       });
   }
 
-  ngOnDestroy(): void {}
+  ngOnDestroy(): void {
+    this.vaultScrollPositionService.stop();
+  }
 }

apps/browser/src/vault/popup/components/vault-v2/view-v2/view-v2.component.spec.ts

@@ -21,12 +21,15 @@ import { FakeAccountService, mockAccountServiceWith } from "@bitwarden/common/sp
 import { UserId } from "@bitwarden/common/types/guid";
 import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service";
 import { CipherType } from "@bitwarden/common/vault/enums";
+import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view";
 import { CipherAuthorizationService } from "@bitwarden/common/vault/services/cipher-authorization.service";
+import { DialogService, ToastService } from "@bitwarden/components";
 import { CopyCipherFieldService } from "@bitwarden/vault";
 
 import { BrowserApi } from "../../../../../platform/browser/browser-api";
 import BrowserPopupUtils from "../../../../../platform/popup/browser-popup-utils";
 import { PopupRouterCacheService } from "../../../../../platform/popup/view-cache/popup-router-cache.service";
+import { VaultPopupScrollPositionService } from "../../../services/vault-popup-scroll-position.service";
 
 import { VaultPopupAutofillService } from "./../../../services/vault-popup-autofill.service";
 import { ViewV2Component } from "./view-v2.component";
@@ -44,6 +47,10 @@ describe("ViewV2Component", () => {
   const collect = jest.fn().mockResolvedValue(null);
   const doAutofill = jest.fn().mockResolvedValue(true);
   const copy = jest.fn().mockResolvedValue(true);
+  const back = jest.fn().mockResolvedValue(null);
+  const openSimpleDialog = jest.fn().mockResolvedValue(true);
+  const stop = jest.fn();
+  const showToast = jest.fn();
 
   const mockCipher = {
     id: "122-333-444",
@@ -54,7 +61,7 @@ describe("ViewV2Component", () => {
       password: "test-password",
       totp: "123",
     },
-  };
+  } as unknown as CipherView;
 
   const mockVaultPopupAutofillService = {
     doAutofill,
@@ -68,13 +75,21 @@ describe("ViewV2Component", () => {
   const mockCipherService = {
     get: jest.fn().mockResolvedValue({ decrypt: jest.fn().mockResolvedValue(mockCipher) }),
     getKeyForCipherKeyDecryption: jest.fn().mockResolvedValue({}),
+    deleteWithServer: jest.fn().mockResolvedValue(undefined),
+    softDeleteWithServer: jest.fn().mockResolvedValue(undefined),
   };
 
   beforeEach(async () => {
+    mockCipherService.deleteWithServer.mockClear();
+    mockCipherService.softDeleteWithServer.mockClear();
     mockNavigate.mockClear();
     collect.mockClear();
     doAutofill.mockClear();
     copy.mockClear();
+    stop.mockClear();
+    openSimpleDialog.mockClear();
+    back.mockClear();
+    showToast.mockClear();
 
     await TestBed.configureTestingModule({
       imports: [ViewV2Component],
@@ -84,9 +99,12 @@ describe("ViewV2Component", () => {
         { provide: LogService, useValue: mock<LogService>() },
         { provide: PlatformUtilsService, useValue: mock<PlatformUtilsService>() },
         { provide: ConfigService, useValue: mock<ConfigService>() },
-        { provide: PopupRouterCacheService, useValue: mock<PopupRouterCacheService>() },
+        { provide: PopupRouterCacheService, useValue: mock<PopupRouterCacheService>({ back }) },
         { provide: ActivatedRoute, useValue: { queryParams: params$ } },
         { provide: EventCollectionService, useValue: { collect } },
+        { provide: VaultPopupScrollPositionService, useValue: { stop } },
+        { provide: VaultPopupAutofillService, useValue: mockVaultPopupAutofillService },
+        { provide: ToastService, useValue: { showToast } },
         {
           provide: I18nService,
           useValue: {
@@ -98,7 +116,6 @@ describe("ViewV2Component", () => {
             },
           },
         },
-        { provide: VaultPopupAutofillService, useValue: mockVaultPopupAutofillService },
         {
           provide: AccountService,
           useValue: accountService,
@@ -114,7 +131,13 @@ describe("ViewV2Component", () => {
           useValue: mockCopyCipherFieldService,
         },
       ],
-    }).compileComponents();
+    })
+      .overrideProvider(DialogService, {
+        useValue: {
+          openSimpleDialog,
+        },
+      })
+      .compileComponents();
 
     fixture = TestBed.createComponent(ViewV2Component);
     component = fixture.componentInstance;
@@ -223,4 +246,130 @@ describe("ViewV2Component", () => {
       expect(closeSpy).toHaveBeenCalledTimes(1);
     }));
   });
+
+  describe("delete", () => {
+    beforeEach(() => {
+      component.cipher = mockCipher;
+    });
+
+    it("opens confirmation modal", async () => {
+      await component.delete();
+
+      expect(openSimpleDialog).toHaveBeenCalledTimes(1);
+    });
+
+    it("navigates back", async () => {
+      await component.delete();
+
+      expect(back).toHaveBeenCalledTimes(1);
+    });
+
+    it("stops scroll position service", async () => {
+      await component.delete();
+
+      expect(stop).toHaveBeenCalledTimes(1);
+      expect(stop).toHaveBeenCalledWith(true);
+    });
+
+    describe("deny confirmation", () => {
+      beforeEach(() => {
+        openSimpleDialog.mockResolvedValue(false);
+      });
+
+      it("does not delete the cipher", async () => {
+        await component.delete();
+
+        expect(mockCipherService.deleteWithServer).not.toHaveBeenCalled();
+        expect(mockCipherService.softDeleteWithServer).not.toHaveBeenCalled();
+      });
+
+      it("does not interact with side effects", () => {
+        expect(back).not.toHaveBeenCalled();
+        expect(stop).not.toHaveBeenCalled();
+        expect(showToast).not.toHaveBeenCalled();
+      });
+    });
+
+    describe("accept confirmation", () => {
+      beforeEach(() => {
+        openSimpleDialog.mockResolvedValue(true);
+      });
+
+      describe("soft delete", () => {
+        beforeEach(() => {
+          (mockCipher as any).isDeleted = null;
+        });
+
+        it("opens confirmation dialog", async () => {
+          await component.delete();
+
+          expect(openSimpleDialog).toHaveBeenCalledTimes(1);
+          expect(openSimpleDialog).toHaveBeenCalledWith({
+            content: {
+              key: "deleteItemConfirmation",
+            },
+            title: {
+              key: "deleteItem",
+            },
+            type: "warning",
+          });
+        });
+
+        it("calls soft delete", async () => {
+          await component.delete();
+
+          expect(mockCipherService.softDeleteWithServer).toHaveBeenCalled();
+          expect(mockCipherService.deleteWithServer).not.toHaveBeenCalled();
+        });
+
+        it("shows toast", async () => {
+          await component.delete();
+
+          expect(showToast).toHaveBeenCalledWith({
+            variant: "success",
+            title: null,
+            message: "deletedItem",
+          });
+        });
+      });
+
+      describe("hard delete", () => {
+        beforeEach(() => {
+          (mockCipher as any).isDeleted = true;
+        });
+
+        it("opens confirmation dialog", async () => {
+          await component.delete();
+
+          expect(openSimpleDialog).toHaveBeenCalledTimes(1);
+          expect(openSimpleDialog).toHaveBeenCalledWith({
+            content: {
+              key: "permanentlyDeleteItemConfirmation",
+            },
+            title: {
+              key: "deleteItem",
+            },
+            type: "warning",
+          });
+        });
+
+        it("calls soft delete", async () => {
+          await component.delete();
+
+          expect(mockCipherService.deleteWithServer).toHaveBeenCalled();
+          expect(mockCipherService.softDeleteWithServer).not.toHaveBeenCalled();
+        });
+
+        it("shows toast", async () => {
+          await component.delete();
+
+          expect(showToast).toHaveBeenCalledWith({
+            variant: "success",
+            title: null,
+            message: "permanentlyDeletedItem",
+          });
+        });
+      });
+    });
+  });
 });

apps/browser/src/vault/popup/components/vault-v2/view-v2/view-v2.component.ts

@@ -49,6 +49,7 @@ import { PopOutComponent } from "../../../../../platform/popup/components/pop-ou
 import { PopupRouterCacheService } from "../../../../../platform/popup/view-cache/popup-router-cache.service";
 import { BrowserPremiumUpgradePromptService } from "../../../services/browser-premium-upgrade-prompt.service";
 import { BrowserViewPasswordHistoryService } from "../../../services/browser-view-password-history.service";
+import { VaultPopupScrollPositionService } from "../../../services/vault-popup-scroll-position.service";
 import { closeViewVaultItemPopout, VaultPopoutType } from "../../../utils/vault-popout-window";
 
 import { PopupFooterComponent } from "./../../../../../platform/popup/layout/popup-footer.component";
@@ -113,6 +114,7 @@ export class ViewV2Component {
     private popupRouterCacheService: PopupRouterCacheService,
     protected cipherAuthorizationService: CipherAuthorizationService,
     private copyCipherFieldService: CopyCipherFieldService,
+    private popupScrollPositionService: VaultPopupScrollPositionService,
   ) {
     this.subscribeToParams();
   }
@@ -202,6 +204,7 @@ export class ViewV2Component {
       return false;
     }
 
+    this.popupScrollPositionService.stop(true);
     await this.popupRouterCacheService.back();
 
     this.toastService.showToast({

apps/browser/src/vault/popup/services/vault-popup-list-filters.service.ts

@@ -370,6 +370,9 @@ export class VaultPopupListFiltersService {
     ),
   );
 
+  /** Organizations, collection, folders filters. */
+  allFilters$ = combineLatest([this.organizations$, this.collections$, this.folders$]);
+
   /** Updates the stored state for filter visibility. */
   async updateFilterVisibility(isVisible: boolean): Promise<void> {
     await this.filterVisibilityState.update(() => isVisible);

apps/browser/src/vault/popup/services/vault-popup-scroll-position.service.spec.ts

@@ -0,0 +1,137 @@
+import { CdkVirtualScrollableElement } from "@angular/cdk/scrolling";
+import { fakeAsync, TestBed, tick } from "@angular/core/testing";
+import { NavigationEnd, Router } from "@angular/router";
+import { Subject, Subscription } from "rxjs";
+
+import { VaultPopupScrollPositionService } from "./vault-popup-scroll-position.service";
+
+describe("VaultPopupScrollPositionService", () => {
+  let service: VaultPopupScrollPositionService;
+  const events$ = new Subject();
+  const unsubscribe = jest.fn();
+
+  beforeEach(async () => {
+    unsubscribe.mockClear();
+
+    await TestBed.configureTestingModule({
+      providers: [
+        VaultPopupScrollPositionService,
+        { provide: Router, useValue: { events: events$ } },
+      ],
+    });
+
+    service = TestBed.inject(VaultPopupScrollPositionService);
+
+    // set up dummy values
+    service["scrollPosition"] = 234;
+    service["scrollSubscription"] = { unsubscribe } as unknown as Subscription;
+  });
+
+  describe("router events", () => {
+    it("does not reset service when navigating to `/tabs/vault`", fakeAsync(() => {
+      const event = new NavigationEnd(22, "/tabs/vault", "");
+      events$.next(event);
+
+      tick();
+
+      expect(service["scrollPosition"]).toBe(234);
+      expect(service["scrollSubscription"]).not.toBeNull();
+    }));
+
+    it("resets values when navigating to other tab pages", fakeAsync(() => {
+      const event = new NavigationEnd(23, "/tabs/generator", "");
+      events$.next(event);
+
+      tick();
+
+      expect(service["scrollPosition"]).toBeNull();
+      expect(unsubscribe).toHaveBeenCalled();
+      expect(service["scrollSubscription"]).toBeNull();
+    }));
+  });
+
+  describe("stop", () => {
+    it("removes scroll listener", () => {
+      service.stop();
+
+      expect(unsubscribe).toHaveBeenCalledTimes(1);
+      expect(service["scrollSubscription"]).toBeNull();
+    });
+
+    it("resets stored values", () => {
+      service.stop(true);
+
+      expect(service["scrollPosition"]).toBeNull();
+    });
+  });
+
+  describe("start", () => {
+    const elementScrolled$ = new Subject();
+    const focus = jest.fn();
+    const nativeElement = {
+      scrollTop: 0,
+      querySelector: jest.fn(() => ({ focus })),
+      addEventListener: jest.fn(),
+      style: {
+        visibility: "",
+      },
+    };
+    const virtualElement = {
+      elementScrolled: () => elementScrolled$,
+      getElementRef: () => ({ nativeElement }),
+      scrollTo: jest.fn(),
+    } as unknown as CdkVirtualScrollableElement;
+
+    afterEach(() => {
+      // remove the actual subscription created by `.subscribe`
+      service["scrollSubscription"]?.unsubscribe();
+    });
+
+    describe("initial scroll position", () => {
+      beforeEach(() => {
+        (virtualElement.scrollTo as jest.Mock).mockClear();
+        nativeElement.querySelector.mockClear();
+      });
+
+      it("does not scroll when `scrollPosition` is null", () => {
+        service["scrollPosition"] = null;
+
+        service.start(virtualElement);
+
+        expect(virtualElement.scrollTo).not.toHaveBeenCalled();
+      });
+
+      it("scrolls the virtual element to `scrollPosition`", fakeAsync(() => {
+        service["scrollPosition"] = 500;
+        nativeElement.scrollTop = 500;
+
+        service.start(virtualElement);
+        tick();
+
+        expect(virtualElement.scrollTo).toHaveBeenCalledWith({ behavior: "instant", top: 500 });
+      }));
+    });
+
+    describe("scroll listener", () => {
+      it("unsubscribes from any existing subscription", () => {
+        service.start(virtualElement);
+
+        expect(unsubscribe).toHaveBeenCalled();
+      });
+
+      it("subscribes to `elementScrolled`", fakeAsync(() => {
+        virtualElement.measureScrollOffset = jest.fn(() => 455);
+
+        service.start(virtualElement);
+
+        elementScrolled$.next(null); // first subscription is skipped by `skip(1)`
+        elementScrolled$.next(null);
+        tick();
+
+        expect(virtualElement.measureScrollOffset).toHaveBeenCalledTimes(1);
+        expect(virtualElement.measureScrollOffset).toHaveBeenCalledWith("top");
+        expect(service["scrollPosition"]).toBe(455);
+      }));
+    });
+  });
+});

apps/browser/src/vault/popup/services/vault-popup-scroll-position.service.ts

@@ -0,0 +1,81 @@
+import { CdkVirtualScrollableElement } from "@angular/cdk/scrolling";
+import { inject, Injectable } from "@angular/core";
+import { takeUntilDestroyed } from "@angular/core/rxjs-interop";
+import { NavigationEnd, Router } from "@angular/router";
+import { filter, skip, Subscription } from "rxjs";
+
+@Injectable({
+  providedIn: "root",
+})
+export class VaultPopupScrollPositionService {
+  private router = inject(Router);
+
+  /** Path of the vault screen */
+  private readonly vaultPath = "/tabs/vault";
+
+  /** Current scroll position relative to the top of the viewport. */
+  private scrollPosition: number | null = null;
+
+  /** Subscription associated with the virtual scroll element. */
+  private scrollSubscription: Subscription | null = null;
+
+  constructor() {
+    this.router.events
+      .pipe(
+        takeUntilDestroyed(),
+        filter((event): event is NavigationEnd => event instanceof NavigationEnd),
+      )
+      .subscribe((event) => {
+        this.resetListenerForNavigation(event);
+      });
+  }
+
+  /** Scrolls the user to the stored scroll position and starts tracking scroll of the page. */
+  start(virtualScrollElement: CdkVirtualScrollableElement) {
+    if (this.hasScrollPosition()) {
+      // Use `setTimeout` to scroll after rendering is complete
+      setTimeout(() => {
+        virtualScrollElement.scrollTo({ top: this.scrollPosition!, behavior: "instant" });
+      });
+    }
+
+    this.scrollSubscription?.unsubscribe();
+
+    // Skip the first scroll event to avoid settings the scroll from the above `scrollTo` call
+    this.scrollSubscription = virtualScrollElement
+      ?.elementScrolled()
+      .pipe(skip(1))
+      .subscribe(() => {
+        const offset = virtualScrollElement.measureScrollOffset("top");
+        this.scrollPosition = offset;
+      });
+  }
+
+  /** Stops the scroll listener from updating the stored location. */
+  stop(reset?: true) {
+    this.scrollSubscription?.unsubscribe();
+    this.scrollSubscription = null;
+
+    if (reset) {
+      this.scrollPosition = null;
+    }
+  }
+
+  /** Returns true when a scroll position has been stored. */
+  hasScrollPosition() {
+    return this.scrollPosition !== null;
+  }
+
+  /** Conditionally resets the scroll listeners based on the ending path of the navigation */
+  private resetListenerForNavigation(event: NavigationEnd): void {
+    // The vault page is the target of the scroll listener, return early
+    if (event.url === this.vaultPath) {
+      return;
+    }
+
+    // For all other tab pages reset the scroll position
+    if (event.url.startsWith("/tabs/")) {
+      this.stop(true);
+    }
+  }
+}