fix scroll position in browser vault

apps/browser/src/platform/popup/layout/popup-page.component.html

@@ -23,6 +23,7 @@
     640px in width (equivalent to tailwind's `sm` breakpoint)
   -->
   <div
+    #scrollRegion
     class="tw-size-full tw-styled-scrollbar"
     data-testid="popup-layout-scroll-region"
     (scroll)="handleScroll($event)"

apps/browser/src/platform/popup/layout/popup-page.component.ts

@@ -3,9 +3,13 @@ import {
   booleanAttribute,
   ChangeDetectionStrategy,
   Component,
+  computed,
+  ElementRef,
   inject,
   input,
+  Signal,
   signal,
+  viewChild,
 } from "@angular/core";
 
 import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
@@ -36,6 +40,13 @@ export class PopupPageComponent {
   /** Accessible loading label for the spinner. Defaults to "loading" */
   readonly loadingText = input<string | undefined>(this.i18nService.t("loading"));
 
+  private readonly scrollRegionRef = viewChild<ElementRef<HTMLElement>>("scrollRegion");
+
+  /** The actual scroll container element for the page (null until view init). */
+  readonly scrollElement: Signal<HTMLElement | null> = computed(() => {
+    return this.scrollRegionRef()?.nativeElement ?? null;
+  });
+
   handleScroll(event: Event) {
     this.scrolled.set((event.currentTarget as HTMLElement).scrollTop !== 0);
   }

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

@@ -1,4 +1,3 @@
-import { CdkVirtualScrollableElement } from "@angular/cdk/scrolling";
 import { ChangeDetectionStrategy, Component, input, NO_ERRORS_SCHEMA } from "@angular/core";
 import { TestBed, fakeAsync, flush, tick } from "@angular/core/testing";
 import { By } from "@angular/platform-browser";
@@ -394,21 +393,28 @@ describe("VaultV2Component", () => {
     expect(values[values.length - 1]).toBe(false);
   });
 
-  it("ngAfterViewInit waits for allFilters$ then starts scroll position service", fakeAsync(() => {
+  it("passes popup-page scroll region element to scroll position service", fakeAsync(() => {
+    const fixture = TestBed.createComponent(VaultV2Component);
+    const component = fixture.componentInstance;
+
+    const readySubject$ = component["readySubject"] as unknown as BehaviorSubject<boolean>;
+    const itemsLoading$ = itemsSvc.loading$ as unknown as BehaviorSubject<boolean>;
     const allFilters$ = filtersSvc.allFilters$ as unknown as Subject<any>;
 
-    (component as any).virtualScrollElement = {} as CdkVirtualScrollableElement;
-
-    component.ngAfterViewInit();
-    expect(scrollSvc.start).not.toHaveBeenCalled();
-
-    allFilters$.next({ any: true });
+    fixture.detectChanges();
     tick();
 
-    expect(scrollSvc.start).toHaveBeenCalledTimes(1);
-    expect(scrollSvc.start).toHaveBeenCalledWith((component as any).virtualScrollElement);
+    const scrollRegion = fixture.nativeElement.querySelector(
+      '[data-testid="popup-layout-scroll-region"]',
+    ) as HTMLElement;
 
-    flush();
+    // Unblock loading
+    itemsLoading$.next(false);
+    readySubject$.next(true);
+    allFilters$.next({});
+    tick();
+
+    expect(scrollSvc.start).toHaveBeenCalledWith(scrollRegion);
   }));
 
   it("showPremiumDialog opens PremiumUpgradeDialogComponent", () => {

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

@@ -1,7 +1,7 @@
 import { LiveAnnouncer } from "@angular/cdk/a11y";
-import { CdkVirtualScrollableElement, ScrollingModule } from "@angular/cdk/scrolling";
+import { ScrollingModule } from "@angular/cdk/scrolling";
 import { CommonModule } from "@angular/common";
-import { AfterViewInit, Component, DestroyRef, OnDestroy, OnInit, ViewChild } from "@angular/core";
+import { Component, DestroyRef, effect, OnDestroy, OnInit, viewChild } from "@angular/core";
 import { takeUntilDestroyed } from "@angular/core/rxjs-interop";
 import { Router, RouterModule } from "@angular/router";
 import {
@@ -119,10 +119,8 @@ type VaultState = UnionOfValues<typeof VaultState>;
   ],
   providers: [{ provide: VaultItemsTransferService, useClass: DefaultVaultItemsTransferService }],
 })
-export class VaultV2Component implements OnInit, AfterViewInit, OnDestroy {
-  // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals
-  // eslint-disable-next-line @angular-eslint/prefer-signals
-  @ViewChild(CdkVirtualScrollableElement) virtualScrollElement?: CdkVirtualScrollableElement;
+export class VaultV2Component implements OnInit, OnDestroy {
+  private readonly popupPage = viewChild(PopupPageComponent);
 
   NudgeType = NudgeType;
   cipherType = CipherType;
@@ -308,16 +306,33 @@ export class VaultV2Component implements OnInit, AfterViewInit, 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!);
-      });
+  private scrollWired = false;
+
+  private readonly _scrollPositionEffect = effect((onCleanup) => {
+    if (this.scrollWired) {
+      return;
     }
-  }
+
+    const popupPage = this.popupPage();
+    const scrollEl = popupPage?.scrollElement();
+
+    if (!scrollEl) {
+      return;
+    }
+
+    const sub = combineLatest([this.allFilters$, this.loading$])
+      .pipe(
+        filter(([, loading]) => !loading),
+        take(1),
+        takeUntilDestroyed(this.destroyRef),
+      )
+      .subscribe(() => {
+        this.scrollWired = true;
+        this.vaultScrollPositionService.start(scrollEl);
+      });
+
+    onCleanup(() => sub.unsubscribe());
+  });
 
   async ngOnInit() {
     this.activeUserId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId));

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

@@ -1,4 +1,3 @@
-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";
@@ -66,21 +65,18 @@ describe("VaultPopupScrollPositionService", () => {
   });
 
   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;
+    let scrollElement: HTMLElement;
+
+    beforeEach(() => {
+      scrollElement = document.createElement("div");
+
+      (scrollElement as any).scrollTo = jest.fn(function scrollTo(opts: { top?: number }) {
+        if (opts?.top != null) {
+          (scrollElement as any).scrollTop = opts.top;
+        }
+      });
+      (scrollElement as any).scrollTop = 0;
+    });
 
     afterEach(() => {
       // remove the actual subscription created by `.subscribe`
@@ -89,47 +85,55 @@ describe("VaultPopupScrollPositionService", () => {
 
     describe("initial scroll position", () => {
       beforeEach(() => {
-        (virtualElement.scrollTo as jest.Mock).mockClear();
-        nativeElement.querySelector.mockClear();
+        ((scrollElement as any).scrollTo as jest.Mock).mockClear();
       });
 
       it("does not scroll when `scrollPosition` is null", () => {
         service["scrollPosition"] = null;
 
-        service.start(virtualElement);
+        service.start(scrollElement);
 
-        expect(virtualElement.scrollTo).not.toHaveBeenCalled();
+        expect((scrollElement as any).scrollTo).not.toHaveBeenCalled();
       });
 
-      it("scrolls the virtual element to `scrollPosition`", fakeAsync(() => {
+      it("scrolls the element to `scrollPosition` (async via setTimeout)", fakeAsync(() => {
         service["scrollPosition"] = 500;
-        nativeElement.scrollTop = 500;
 
-        service.start(virtualElement);
+        service.start(scrollElement);
         tick();
 
-        expect(virtualElement.scrollTo).toHaveBeenCalledWith({ behavior: "instant", top: 500 });
+        expect((scrollElement as any).scrollTo).toHaveBeenCalledWith({
+          behavior: "instant",
+          top: 500,
+        });
+        expect((scrollElement as any).scrollTop).toBe(500);
       }));
     });
 
     describe("scroll listener", () => {
       it("unsubscribes from any existing subscription", () => {
-        service.start(virtualElement);
+        service.start(scrollElement);
 
         expect(unsubscribe).toHaveBeenCalled();
       });
 
-      it("subscribes to `elementScrolled`", fakeAsync(() => {
-        virtualElement.measureScrollOffset = jest.fn(() => 455);
+      it("stores scrollTop on subsequent scroll events (skips first)", fakeAsync(() => {
+        service["scrollPosition"] = null;
 
-        service.start(virtualElement);
+        service.start(scrollElement);
 
-        elementScrolled$.next(null); // first subscription is skipped by `skip(1)`
-        elementScrolled$.next(null);
+        // First scroll event is intentionally ignored (equivalent to old skip(1)).
+        (scrollElement as any).scrollTop = 111;
+        scrollElement.dispatchEvent(new Event("scroll"));
+        tick();
+
+        expect(service["scrollPosition"]).toBeNull();
+
+        // Second scroll event should persist.
+        (scrollElement as any).scrollTop = 455;
+        scrollElement.dispatchEvent(new Event("scroll"));
         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

@@ -1,8 +1,7 @@
-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";
+import { filter, fromEvent, Subscription } from "rxjs";
 
 @Injectable({
   providedIn: "root",
@@ -31,24 +30,25 @@ export class VaultPopupScrollPositionService {
   }
 
   /** Scrolls the user to the stored scroll position and starts tracking scroll of the page. */
-  start(virtualScrollElement: CdkVirtualScrollableElement) {
+  start(scrollElement: HTMLElement) {
     if (this.hasScrollPosition()) {
       // Use `setTimeout` to scroll after rendering is complete
       setTimeout(() => {
-        virtualScrollElement.scrollTo({ top: this.scrollPosition!, behavior: "instant" });
+        scrollElement.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;
-      });
+    let skipped = false;
+    this.scrollSubscription = fromEvent(scrollElement, "scroll").subscribe(() => {
+      if (!skipped) {
+        skipped = true;
+        return;
+      }
+      this.scrollPosition = scrollElement.scrollTop;
+    });
   }
 
   /** Stops the scroll listener from updating the stored location. */