Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PM-3914] Refactor Browser Extension Popout Windows #6296

Merged

Conversation

cagonzalezcs
Copy link
Contributor

@cagonzalezcs cagonzalezcs commented Sep 14, 2023

Type of change

- [ ] Bug fix
- [ ] New feature development
- [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Refactor how the browser extension uses pop-out windows, and migrate the logic within BrowserPopoutWindowService and PopupUtilsService to a new utils class BrowserPopupUtils to consolidate behavior. We're also updating any potential popups that open in a tab to open within a new window to facilitate consistency in the implementation.

Code changes

  • apps/browser/src/auth/popup/two-factor.component.ts: As we are updating all "Extension Tab" instances of the popup to a "Window Popout" implementation, the change in this file facilitates closing a potential TwoFactorAuth popout.
  • apps/browser/src/auth/popup/utils/auth-popout-window.spec.ts: Jest tests for the AuthPopoutWindow utils methods used to open popouts for the authentication purposes.
  • apps/browser/src/auth/popup/utils/auth-popout-window.ts: Helper methods that facilitate opening popout windows for authentication purposes.
  • apps/browser/src/autofill/background/notification.background.ts: Updating references in this file to consolidate behavior surrounding unlock popout windows.
  • apps/browser/src/autofill/browser/context-menu-clicked-handler.ts: Updating references in this file to consolidate behavior for how we open master password reprompt popout windows.
  • apps/browser/src/autofill/content/message_handler.ts: Removing unused messages that no longer trigger actionable behavior.
  • apps/browser/src/autofill/notification/bar.ts: Updating naming of unlock popout to reference new naming structure for the unlock popout.
  • apps/browser/src/autofill/services/autofill.service.spec.ts: Adding jest tests to reflect changes within the autofill service.
  • apps/browser/src/autofill/services/autofill.service.ts: Adding a method to facilitate debouncing the opening of master password popouts, and updating references to how we open those popouts.
  • apps/browser/src/background/commands.background.ts: Updating references to the popoutUtilsService class to fit new architecture.
  • apps/browser/src/background/main.background.ts: Removing BrowserPopoutWindowService entirely.
  • apps/browser/src/background/runtime.background.ts: Removing messages that were once used to open popouts in favor of leveraging opening popouts at the logical trigger point.
  • apps/browser/src/platform/browser/browser-api.spec.ts: Adding tests for the added browser api helper methods
  • apps/browser/src/platform/browser/browser-api.ts: Expanding the getWindow method to function with or without a window ID value. If an id is not provided, the method attempts to grab the current window that the user has active.Adding a updateWindowProperties method to facilitate repositioning and focusing any single use popout windows that might exist. Removing openBitwardenExtensionTab and closeBitwardenExtensionTab as those represent deprecated methods for facilitating user interactions with the popup outside of the popup itself.
  • apps/browser/src/platform/popup/abstractions/browser-popout-window.service.ts: Removal of unused abstraction.
  • apps/browser/src/platform/popup/browser-popout-window.service.ts: Removal of unused class.
  • apps/browser/src/platform/popup/browser-popup-utils.spec.ts: Jest tests for the new BrowserPopoutUtils class, centralizing behavior for the BrowserPopoutWindowService and the PopupUtilsService class.
  • apps/browser/src/platform/popup/browser-popup-utils.ts: New class used to centralize behavior for the BrowserPopoutWindowService and the PopupUtilsService classes.
  • apps/browser/src/popup/components/pop-out.component.ts: Updating references to the PopupUtilsService
  • apps/browser/src/popup/components/private-mode-warning.component.ts: Updating references to the PopupUtilsService
  • apps/browser/src/popup/services/init.service.ts: Updating references to the PopupUtilsService
  • apps/browser/src/popup/services/popup-close-warning.service.ts: New class PopupCloseWarningService that handles setting up an onbeforeunload event listener and enabling/disabling warning behavior based on the class instantiation.
  • apps/browser/src/popup/services/popup-utils.service.ts: Removal of this unused class, logic migrated to BrowserPopupUtils.
  • apps/browser/src/popup/services/services.module.ts: Updating references to the PopupUtilsService
  • apps/browser/src/popup/settings/settings.component.ts: Updating references to the PopupUtilsService
  • apps/browser/src/popup/tabs.component.ts: Updating references to the PopupUtilsService
  • apps/browser/src/tools/popup/send/send-add-edit.component.ts: Updating references to the PopupUtilsService
  • apps/browser/src/tools/popup/send/send-groupings.component.ts: Updating references to the PopupUtilsService
  • apps/browser/src/tools/popup/send/send-type.component.ts: Updating references to the PopupUtilsService
  • apps/browser/src/vault/popup/components/vault/add-edit.component.ts: Updating references to the PopupUtilsService
  • apps/browser/src/vault/popup/components/vault/current-tab.component.ts: Updating references to the PopupUtilsService
  • apps/browser/src/vault/popup/components/vault/vault-filter.component.ts: Updating references to the PopupUtilsService
  • apps/browser/src/vault/popup/components/vault/vault-items.component.ts: Updating references to the PopupUtilsService
  • apps/browser/src/vault/popup/components/vault/view.component.ts: Updating references to the PopupUtilsService
  • apps/browser/src/vault/popup/utils/vault-popout-window.spec.ts: Jest tests for the VaultPopoutWindow utils
  • apps/browser/src/vault/popup/utils/vault-popout-window.ts: Represents usage of popout windows specific to use cases where the vault is leveraging opening a popout.
  • apps/browser/test.setup.ts: Jest test setup

Demonstration

Screen.Recording.2023-09-14.at.3.09.43.PM.mp4

@cagonzalezcs cagonzalezcs changed the title [3914] Refactor Browser Extension Popout Windows [PM-3914] Refactor Browser Extension Popout Windows Sep 14, 2023
@github-actions github-actions bot added the needs-qa Marks a PR as requiring QA approval label Sep 14, 2023
@cagonzalezcs cagonzalezcs requested a review from Hinton September 14, 2023 20:01
@bitwarden-bot
Copy link

bitwarden-bot commented Sep 14, 2023

Logo
Checkmarx One – Scan Summary & Detailsee2cf294-36b0-4c87-8119-41be5dea65e9

No New Or Fixed Issues Found

Copy link
Member

@Hinton Hinton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a good start. I would like to see more code owned though.

apps/browser/src/popup/services/popup-utils.service.ts Outdated Show resolved Hide resolved
libs/common/src/enums/browser-popout-type.enum.ts Outdated Show resolved Hide resolved
Copy link
Member

@Hinton Hinton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're a bit back to having two classes again. Albeit one is static and one isn't.

I think we need to write up some guidance on which to use. I think the only stateful portion of PopupUtilsService is knowing if the extension is running in private mode which doesn't feel like something it should know.

cagonzalezcs and others added 13 commits October 25, 2023 12:48
…lose the window instead of attempting to close the tab
…ersist if user opens the extension popout in a new window before locking or logging out
…side of the scope fo the openUlockPopout method to ensure it does not have to be rebuilt each time the method is called
@cagonzalezcs cagonzalezcs removed the needs-qa Marks a PR as requiring QA approval label Nov 8, 2023
@cagonzalezcs cagonzalezcs removed the request for review from Hinton November 8, 2023 17:58
Copy link
Contributor

@JaredSnider-Bitwarden JaredSnider-Bitwarden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@cagonzalezcs cagonzalezcs merged commit cf6ada5 into master Nov 8, 2023
13 of 14 checks passed
@cagonzalezcs cagonzalezcs deleted the autofill/pm-3914-refactor-browser-extension-popouts branch November 8, 2023 18:57
BlackDex pushed a commit to BlackDex/bitwarden-clients that referenced this pull request Nov 21, 2023
* [PM-3914] Refactor Browser Extension Popouts

* [PM-3914] Refactor Browser Extension Popouts

* [PM-3914] Refactor Browser Extension Popouts

* [PM-3914] Adding enums for the browser popout type

* [PM-3914] Making the methods for getting a window in a targeted manner public

* [PM-3914] Refactoing implementation

* [PM-3914] Updating deprecated api call

* [PM-3914] Fixing issues found when testing behavior

* [PM-3914] Reimplementing behavior based on feedback from platform team

* [PM-3914] Adding method of ensuring previously opened single action window is force closed for vault item password reprompts

* [PM-3914] Taking into consideration feedback regarding the browser popup utils service and implementating requested changes

* [PM-3914] Removing unnecesssary class dependencies

* [PM-3914] Adding method for uniquely setting up password reprompt windows

* [PM-3914] Modifying method

* [PM-3914] Adding jest tests and documentation for AuthPopoutWindow util

* [PM-3914] Adding jest tests and documentation for VaultPopoutWindow

* [PM-3914] Adding jest tests for the debouncing method within autofill service

* [PM-3914] Adding jest tests for the new BrowserApi methods

* [PM-3914] Adding jest tests to the BrowserPopupUtils class

* [PM-3914] Updating inPrivateMode reference

* [PM-3914] Updating inPrivateMode reference

* [PM-3914] Modifying comment

* [PM-3914] Moviing implementation for openCurrentPagePopout to the BrowserPopupUtils

* [PM-3914] Applying feedback

* [PM-3914] Applying feedback

* [PM-3914] Applying feedback

* [PM-3983] Refactoring implementation of `setContentScrollY` to facilitate having a potential delay

* [PM-3914] Applying feedback regarding setContentScrollY to the implementation

* [PM-3914] Modifying early return within the run method of the ContextMenuClickedHandler

* [PM-3914] Adding test for VaultPopoutWindow

* [PM-3914] Applying work done within PM-4366 to facilitate opening the popout window as a popup rather than a normal window

* [PM-3914] Updating the BrowserApi.removeTab method to leverage a callback structure for the promise rather than an async away structure

* [PM-3036] Adding jest tests for added passkeys popout windows

* [PM-3914] Adjsuting logic for turning off the warning when FIDO2 credentials are saved

* [PM-3914] Fixing height to design

* [PM-3914] Fixing call to Fido2 Popout

* [PM-3914] Fixing add/edit from fido2 popout

* [PM-3914] Fixing add/edit from fido2 popout

* [PM-3914] Fixing jest tests for updated elements

* [PM-3914] Reverting how context menu actions are passed to the view component

* [PM-3914] Reverting re-instantiation of config service within main.background.ts

* [PM-3914] Adding jest test for BrowserAPI removeTab method

* [PM-3914] Adding method to handle parsing the popout url path

* [PM-3914] Removing JSDOC comment elements

* [PM-3914] Removing await from method call

* [PM-3914] Simplifying implementation on add/edit

* [PM-3032] Adding more direct reference to view item action in context menus

* [PM-3914] Adjusting routing on Fido2 component to pass the singleActionPopout param to the route when opening the add-edit component

* [PM-3914] Adding singleActionPopout param to the fido2 component routing

* [PM-3914] Updating implementation details for how we build the extension url path

* [PM-3914] Reworking implementation for isSingleActionPopoutOpen to clean up iterative logic

* [PM-3914] Merging work from master and fixing merge conflicts

* [PM-3914] Fixing merge conflict introduced from master

* [PM-3914] Reworking closure of single action popouts to ensure they close the window instead of attempting to close the tab

* [PM-3914] Fixing issue within Opera where lock and login routes can persist if user opens the extension popout in a new window before locking or logging out

* [PM-3914] Setting the extensionUrls that are cheked as a variable outside of the scope fo the openUlockPopout method to ensure it does not have to be rebuilt each time the method is called
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
browser Browser Extension
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants