diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-list-filters/vault-list-filters.component.html b/apps/browser/src/vault/popup/components/vault-v2/vault-list-filters/vault-list-filters.component.html index 56f35c41f6d..c61562f9f90 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-list-filters/vault-list-filters.component.html +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-list-filters/vault-list-filters.component.html @@ -2,8 +2,9 @@
- + - + - + { + return { + organizations, + collections, + folders, + }; + }), + ); + constructor(private vaultPopupListFiltersService: VaultPopupListFiltersService) {} } diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.ts index 7c21c7e6a0c..12952a69c79 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.ts +++ b/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 { filter, map, take } from "rxjs/operators"; +import { combineLatest, filter, map, Observable, shareReplay, switchMap, take } from "rxjs"; 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(); + } } diff --git a/apps/browser/src/vault/popup/components/vault-v2/view-v2/view-v2.component.spec.ts b/apps/browser/src/vault/popup/components/vault-v2/view-v2/view-v2.component.spec.ts index 526ab2e2579..39feb86f4fd 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/view-v2/view-v2.component.spec.ts +++ b/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() }, { provide: PlatformUtilsService, useValue: mock() }, { provide: ConfigService, useValue: mock() }, - { provide: PopupRouterCacheService, useValue: mock() }, + { provide: PopupRouterCacheService, useValue: mock({ 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", + }); + }); + }); + }); + }); }); diff --git a/apps/browser/src/vault/popup/components/vault-v2/view-v2/view-v2.component.ts b/apps/browser/src/vault/popup/components/vault-v2/view-v2/view-v2.component.ts index f3cd713dd5f..378d3251e11 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/view-v2/view-v2.component.ts +++ b/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({ diff --git a/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.ts b/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.ts index 6190d14a6a7..579319c92ab 100644 --- a/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.ts +++ b/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 { await this.filterVisibilityState.update(() => isVisible); diff --git a/apps/browser/src/vault/popup/services/vault-popup-scroll-position.service.spec.ts b/apps/browser/src/vault/popup/services/vault-popup-scroll-position.service.spec.ts new file mode 100644 index 00000000000..562375f8f85 --- /dev/null +++ b/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); + })); + }); + }); +}); diff --git a/apps/browser/src/vault/popup/services/vault-popup-scroll-position.service.ts b/apps/browser/src/vault/popup/services/vault-popup-scroll-position.service.ts new file mode 100644 index 00000000000..5bfe0ec9331 --- /dev/null +++ b/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); + } + } +}