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-2147] [BEEEP] Open login form used to unlock extension in a separate window instead of a tab #5384

Conversation

cagonzalezcs
Copy link
Contributor

@cagonzalezcs cagonzalezcs commented May 5, 2023

Type of change

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

Objective

Reimplementing the workflow for how we handle a user who is attempting an autofill when their vault is locked.

With this implementation, we are now opening the password prompt in a window that will always be focused and presented at the top left corner of the currently active browser window. Users can now enter their password in this window and the vault will unlock and autofill the password in the exact same manner as before.

Anytime a user re-attempts a autofill with a locked vault, we first check for existing windows that contain the login form and close them before re-opening the login prompt in the top left corner of the currently active window.

We now also display a notification to the user in the same UI design that is used to display the "Add/Update Cipher" notification. This notification has the ability to re-open the prompt to login to the vault.

Code changes

  • apps/browser/src/_locales/en/messages.json: Adding translations for the notification that we're sending to users when they attempt to autofill with a lock vault
  • apps/browser/src/autofill/background/notification.background.ts: Added a new notification type UnlockVault and incorporated behavior for showing/hiding this notification when the user attempts to autofill from the extension with a locked vault. Implemented behavior to ensure that this new notification did not appear when a user was adding a new item, or editing an existing one, from a locked vault.
  • apps/browser/src/autofill/notification/bar.html: Adding markup required to display the UnlockVault notification
  • apps/browser/src/autofill/notification/bar.ts: Adding content script logic required to display the UnlockVault notification and to populate translated content within the markup used to display the notification. Also adding click listeners to the notification that re-open the prompt to unlock a user vault
  • apps/browser/src/background/models/add-unlock-vault-queue-message.ts: Adding typing information for the UnlockVault notification queue.
  • apps/browser/src/background/models/lockedVaultPendingNotificationsItem.ts: Updating the type information for LockedVaultPendingNotificationsItem to ensure that the msg value is indicated as an object expecting a command key and an optional data key
  • apps/browser/src/background/models/notificationQueueMessageType.ts: Adding UnlockVault as a notification queue message type
  • apps/browser/src/background/runtime.background.ts: Modifying the runtime message listeners that handle prompting the user to login when a vault is locked.
  • apps/browser/src/browser/browserApi.ts: Added a method for getting a window based on a passed window id. Added a method for opening the Bitwarden login prompt in a new window. Added a method that would close the Bitwarden login prompt.

Screenshots

Screen.Recording.2023-05-19.at.10.06.35.AM.mov

Example of adding/editing vault items from a locked extension

Screen.Recording.2023-06-12.at.9.55.12.AM.mov

Before you submit

  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team
  • Ensure that all UI additions follow WCAG AA requirements

cagonzalezcs and others added 13 commits May 2, 2023 13:59
…lock a locked extension within an incongito browsing session
…tcut-does-not-prompt-to-unlock-account-in-incognito-window
…tcut-does-not-prompt-to-unlock-account-in-incognito-window
…tcut-does-not-prompt-to-unlock-account-in-incognito-window
… logging a user in and attempting to autofill within the Firefox Workspaces addon
…tcut-does-not-prompt-to-unlock-account-in-incognito-window
…tcut-does-not-prompt-to-unlock-account-in-incognito-window
…tcut-does-not-prompt-to-unlock-account-in-incognito-window
…-prompt-to-unlock-account-in-incognito-window' into Client-Integrations/PM-2147-beeep-open-login-form-in-new-window
@github-actions github-actions bot added the needs-qa Marks a PR as requiring QA approval label May 5, 2023
@danielleflinn
Copy link
Member

This is awesome. Thanks for working on this user experience bug.

@cagonzalezcs How did you decide on opening the window in the top left?

Also can you explain more of your thinking behind adding the notification and "Unlock" prompt? Is this in case the user accidentally closes the unlock window, the notification will allow the user to re-open without having to use the shortcut again/re-attempt auto-fill through the context menu?

@cagonzalezcs
Copy link
Contributor Author

cagonzalezcs commented May 15, 2023

@danielleflinn

So the goal with moving the opening position from a new tab to the window at the top left is done to solve two issues:

  1. Ensure that the user's attention is not diverted in as harsh of a manner as opening a new tab and completely changing context.
  2. Ensuring that the context for the user login action, ie. autofilling a password into a specific website, is not lost in some manner.

I opted to open the window in the top left corner of the currently active browser because of how that mechanism works when opening any new window in a browser using a keystroke. For instance, if you press CMD + N on the MacOS version of Chrome/Firefox, a new window opens int the top left corner of the currently active window. As a result, our mechanism for opening this window for users should mimic that default action.

The goal of the notification was to ensure that users had an “anchor” to reference when being required to login when triggering an autofill. This requirement came through a conversation I had with @djsmith85 regarding potential pitfalls surrounding having the login form open in a new window rather than a new tab. He felt that it was too easy for users to potentially lose the login window behind another browser window, and this notification allows the user the ability to quickly recall the login form if that does happen.

Also, yes if the user closes the login window, they can open it either using this notification or another autofill keystroke.

@danielleflinn
Copy link
Member

@cagonzalezcs Thanks for clarifying the window placement. For the save passkey in Bitwarden work, the window opens in the top right–the thinking there was to keep the window near the extension icon where users are already used to looking for it. Initially I thought it might be good to standardize the window placement but perhaps this is not as big of a deal as I originally thought since the passkey popout is not triggered by a keystroke.

@cagonzalezcs
Copy link
Contributor Author

@danielleflinn

Hmm... it'd make sense to keep this consistent with other plans for the extension. Shouldn't be a huge issue to modify it to open in the top right either. I'll do that this Friday when we have some BEEEP time.

@cagonzalezcs cagonzalezcs added the browser Browser Extension label May 19, 2023
@cagonzalezcs cagonzalezcs added the needs-qa Marks a PR as requiring QA approval label Jul 27, 2023
@trmartin4 trmartin4 requested a review from MGibson1 August 2, 2023 19:03
Copy link
Member

@MGibson1 MGibson1 left a comment

Choose a reason for hiding this comment

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

Very clean changes, I like it

@cagonzalezcs cagonzalezcs removed the needs-qa Marks a PR as requiring QA approval label Aug 7, 2023
@cagonzalezcs cagonzalezcs enabled auto-merge (squash) August 7, 2023 21:05
@cagonzalezcs cagonzalezcs merged commit 50b3e40 into master Aug 7, 2023
20 of 22 checks passed
@cagonzalezcs cagonzalezcs deleted the Client-Integrations/PM-2147-beeep-open-login-form-in-new-window branch August 7, 2023 21:06
@Camusensei
Copy link

Hi. Just noticed the change in my extension and could not understand some things. Maybe if I knew how to find the page for PM-2147 so feel free to direct me to it if the answers are there.

First is the reason, it's not explained here. Why the change? Historically it would open the popout, then it was changed to a new tab, now to a new window. I'm fine either way, but why these changes?

And the second one is that I don't see the bitwarden extension tag in firefox like shown in the video, so any extension could be showing me the popup and I wouldn't know (unless I remembered the bitwarden extension tag, and always made the effort of maximizing the window to see the address) . Should I open a bug for that?

Thanks!!!

@cagonzalezcs
Copy link
Contributor Author

@Camusensei Thank you for the feedback.

More context on the "why" for this change can be found with this comment: #6412 (comment)

For your second concern, unfortunately we do not have control over how the tag is displayed to the end user. That said, the relevant information for verifying that this window is showing a Bitwarden extension page should be easily accessible. Unfortunately Firefox truncates the extension tag that precedes the URL, however if you click on it you should see the full context of the tag.

Screenshot 2023-10-05 at 4 40 08 AM

As a result, this is working as intended and does not require a bug report.

If you have any further questions or concerns please feel free to open a ticket with support or an issue on this repository.

@maximevhw
Copy link

Hello, I could not find anything in the setting so i thought i'd ask.
Is there a way to disable the seperate window? I much preferred everything happening withing the extension viewport.
Atl east having a setting would be wonderfull since everyone has different preferences. But for me this is kinda anoying,

@Camusensei
Copy link

Camusensei commented Oct 16, 2023

Hello, I could not find anything in the setting so i thought i'd ask. Is there a way to disable the seperate window? I much preferred everything happening withing the extension viewport. Atl east having a setting would be wonderfull since everyone has different preferences. But for me this is kinda anoying,

Hello. I asked the same question and this was the answer:

More context on the "why" for this change can be found with this comment: #6412 (comment)

The comment contains among other things this explanation:

we are unfortunately limited in how we can programmatically open the extension popup window. If I could facilitate your described workflow, I would definitely have opted to open it in that manner instead of in a new window.

@clemensrieder
Copy link

This is a super-annoying change. I unlock my extension using biometrics (MacBook Air + Chrome browser). The popup window disappears but can be restored with CMD + SHIFT + T. I regularly use this shortcut to reopen tabs. Still, I absolutely never want to reopen my extension in a separate window. Unfortunately, it will reopen the Bitwarden extension looking like this:

Bildschirmfoto 2023-10-17 um 22 37 43

@fiskfisk33
Copy link

Is there any way to revert to the old behaviour in settings somewhere I'm missing?

@Jamie4224
Copy link

Jamie4224 commented Oct 29, 2023

Same question here, I have an issue with this in combination with pinned tabs where the newly opened window does not close after password verification. This is very bothersome as I need to close the window manually every time. It would be nice if it was configurable to use a new window or new tab.

Also see my response here with reproduction steps: #6707 (comment)

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.

None yet