clients icon indicating copy to clipboard operation
clients copied to clipboard

[PS-1184] Remove specific keyboard shortcut logic

Open djsmith85 opened this issue 1 year ago • 0 comments

Type of change

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

Objective

Re-Creating https://github.com/bitwarden/clients/pull/2148 as work was done prior to Prettier and move to the mono-repo

While Oscar reviewed the PR #2132 he had the following comment https://github.com/bitwarden/browser/pull/2132#discussion_r732680650

Looking into the shortcut.ts I realized that, although built for Safari and Vivaldi the code to check for the message 'keyboardShortcutTriggered' would only apply to Vivaldi.

While discussing this further with Oscar via Slack, we found the changes that removed the check for Safari (PR #1491).

At the time I tested the keyboard shortcuts on Vivaldi while Oscar tested on Safari. Conclusion even when removing all logic for the shortcuts in shortcut.ts the keyboard shortcuts would still work as expected.

Through PR #1491 the check for isSafari was removed from commands.background.ts Also with this PR the __bitwardenFrameId was removed all other places but not from shortcut.ts

Other interesting PR's:

  • https://github.com/bitwarden/browser/pull/1450
  • https://github.com/bitwarden/browser/pull/1452

Code changes

  • apps/browser/src/background/commands.background.ts: Removed code and check for command keyboardShortcutTriggered
  • apps/browser/src/content/shortcuts.ts: Deleted shortcut.ts
  • apps/browser/src/manifest.json: Removed loading of shortcut.js
  • apps/browser/src/manifest.v3.json: Removed loading of shortcut.js
  • apps/browser/webpack.config.js: Removed inclusion of shortcut.ts
  • apps/browser/gulpfile.js: Removed function to remove shortcut.js from certain distributions
  • package.json: Removed dependency on mousetrap and @types/mousetrap
  • package-lock.json: Removed dependency on mousetrap and @types/mousetrap

Testing requirements

The following keyboard shortcuts should still work under Safari and Vivaldi (there seems to be an upstream issue, where keyboard shortcuts will only work when set to Global instead of "In Vivaldi" (chrome://extensions/shortcuts) ) Ctrl/CMD + Shift + Y → Activate extension Ctrl/CMD + Shift + L → Autofill, press again to cycle through matching logins Ctrl/CMD + Shift + 9 → Generate a password and copy it to the clipboard

Ctrl/CMD + Shift + N → Lock extension

  • Does not get configured at all, present bug!
  • Need to do some further research
    • manifest.json only allows a max of 4 suggested_key attributes, but because _execute_sidebar_action which is only supported in FF we can't have another entry for lock vault.
    • Ctrl+Shift+N will not work on Firefox as it is a built-in shortcut
    • For the dist builds this would be removable via the gulpfile
    • Currently unable to remove it for npm run build as that just builds without modifying the manifest.json
      • We would need to add flags to the build-command for each browser, change in dev-flow!

Before you submit

- [X] I have checked for **linting** errors (`npm run lint`) (required)
- [ ] I have added **unit tests** where it makes sense to do so (encouraged but not required)
- [ ] This change requires a **documentation update** (notify the documentation team)
- [ ] This change has particular **deployment requirements** (notify the DevOps team)

djsmith85 avatar Jul 25 '22 14:07 djsmith85