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

Enable right-click menu in chrome #282

Closed
wants to merge 201 commits into from

Conversation

fkneist
Copy link

@fkneist fkneist commented Feb 5, 2022

I saw issue #195 and thought that must be quite doable!

Currently, the behavior looks like below in the video, and only works in Google Chrome.

You can click anywhere on the page and all entries that match the host get listed in the submenu. When you select an entry, that one gets automatically filled in.

There would be more sophisticated things thinkable, like selecting single fields, like only password. But I'd prefer to first agree on some basic functionality and get that working on all browsers.

This was documentation page was helpful:
https://developer.chrome.com/docs/extensions/reference/contextMenus/

browserpass-right-click.mov

erayd and others added 30 commits April 9, 2018 09:36
* Handle settings response properly
* Change location of default store settings
* Generate chrome extension
* Move popup into its own folder
* Move distribution javascript into new folder
* Remove checkpoint
* Ignore dist JS files
* Rename background source file
* Update clean rule
* Fix make rule for generated files
* Also tidy CSS files
* Implement searching
* Add icon to search bar
* Add copy-password action
* Add copy-user action
* Add launch action
* Button styling
* Send action to backend
* Set targets to .PHONY
* Highlight the first entry
* Allow disabling fuzzy search by starting search with a space
* Implement fetch & launch action
* Fix scroll behavior
* Remove horizontal scrollbar
* Add stubs for filling username / password
* Don't pre-filter for chrome:// URLs
* Add inject.js
* Port Maxim Baz's injected form-fill logic
* Add hotkey to manifest
* Add button to remove domain filter

* Fix removing hint via backspace when search text is present

* Move trim & fuzzy-enabled into search function

* Simplify code

* Use a more descriptive name
* Fix indentation & add permissions for basic auth

* Implement basic auth

* Remove debug statement

* s/browser/chrome/g

* Use one global tab-update listener

* Make webRequest permissions on-demand
* Add copy actions
* Add hotkeys to search box
* Track recently used logins & display first in popup

Results are sorted by:
 - Most recent store first
 - Within each store, login with highest use count first
 - Within same usage count, most recent first

There is a 60-sec debounce on logins, to avoid excessive incrementing of
the counter when a login is used multiple times in rapid succession
(e.g. for copying user / pass etc.).
* Handle no-url-in-file case
* Handle null case
* Fix split (use lookbehind)
* Get user from path
* Sort full list
* Make code clearer
Sort behavior is now as follows:
 * The single most recently used login is first
 * All used logins are next, ordered by usage count from most to least
 * All remaining logins follow, ordered alphabetically
Solves updating recent info when popup gets prematurely closed, e.g. by the "foreign origin" confirmation dialog

Fixes browserpass#30
…owserpass#34)

also use deepCopy when creating copies of objects, and make sure
recent data is only being filled after stores are defined
@maximbaz
Copy link
Member

maximbaz commented Feb 5, 2022

This looks interesting! I suppose it solves the popup-windows usecase from #195. Firefox seems to have a similar API, so that shouldn't hopefully be an issue. @erayd what do you think about this all? 🙂

@erayd
Copy link
Collaborator

erayd commented Feb 8, 2022

I like the general idea, particularly as a mechanism to fill fields that currently aren't fillable, or when there are multiple instances of a login form on the page and the user wants to fill whichever one Browserpass is not automatically selecting.

I do think that feature parity between Chrome and Firefox is important. @fkneist What is the barrier that's preventing this from working in non-Chrome browsers?

@fkneist
Copy link
Author

fkneist commented Feb 9, 2022

Thanks for both your feedback!

@erayd: I was simply wrong about Firefox, turns out it works, when I set the respective permission in the manifest. I simply assumed it would only work in chrome because I used chrome.contextMenus.<someFunction>(...) in the code.

I searched a bit and I guess the answer can be found here: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Chrome_incompatibilities#firefox_supports_both_the_chrome_and_browser_namespaces

So I see at least three use cases now for the right-click menu:

  1. the one I implemented

  2. [...] fill fields that currently aren't fillable [...]

  3. [...] fill whichever one Browserpass is not automatically selecting.

I could imagine the right-click menu looking something like this, covering all three use cases:

parent menu entry         1st child                                  2nd child
-------------------------------------------------------------------------------
browserpass  - 3 entries  > fill automatically: username & pasword
                          > fill selected field: username
                          > fill selected field: password           > account 1
                                                                    > account 2
                                                                    > account 3

The user selected the parent menu entry, then the third option (fill selected field: password) and then can choose one of the matched accounts. What do you think that?

I'd prefer to split the work into two PRs. This one for finishing the "fill automatically" and the next one for doing the manual filling. What I'm still missing for my use case is a good indication when there is no matched account but I think I can work on that in the upcoming evenings.

I can also add the documentation to the function as in the rest of the project. Anyway, please let me know what you think and what's still missing!

@maximbaz
Copy link
Member

maximbaz commented Feb 9, 2022

As long as it works in all browsers, I'm fine with having the work split into multiple PRs, whatever makes it easier for you! It would indeed be good to keep docs up-to-date, which means every PR documenting its part.

For the right-click menu proposal you have above, there's one confusing scenario: suppose you have two login forms and Browserpass annoyingly fills the wrong one; I right click on login field in the desired form and pick "fill automatically: username & pasword", hoping that Browserpass will do the rest - but instead once again it fills the wrong form!

Having said that, lets leave this for the future PR since you prefer to split the work, and focus here on the current proposal. It lays the ground work for the future improvements, it helps with popup window where toolbar is invisible, I see no problem merging this when you are ready and working on improvements separately.

@fkneist
Copy link
Author

fkneist commented Feb 10, 2022

Alright, I've added documentation, handled the case of zero matched entries and bumped the version a MINOR.

If you guys like to include the screenshot, that could be copied to the other assets, currently I just put it on imgur. I saw that the other assets are stored at https://user-images.githubusercontent.com/<...>, that's probably in a repo by one of you(?). If we want to include something animated, I'd record that when the other features are there.

@maximbaz: concerning the proposal above, maybe the first entry on the 1st child level could be renamed to: detect fields & fill credentials. And what also should be possible: that the second and third entries on the 1st child level only appear, when a field is actually right-clicked (so not on a right-click outside a form field)

Bonus idea: there could be a toggle to deactivate the right-click menu in the extension's options. Please let me know what you think has high prio, happy to help on this project that I use on a daily basis!

Copy link
Member

@maximbaz maximbaz left a comment

Choose a reason for hiding this comment

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

Wonderful, thanks!

Bonus idea: there could be a toggle to deactivate the right-click menu in the extension's options.

Let's not, I prefer to make extra effort to make sure the experience is great for everyone, and not create options unless absolutely necessary.

README.md Outdated
@@ -300,6 +306,7 @@ Browserpass extension requests the following permissions:
| `tabs` | To get URL of a given tab, used for example to set count of the matching passwords for a given tab |
| `clipboardRead` | To ensure only copied credentials and not other content is cleared from the clipboard after 60 seconds |
| `clipboardWrite` | For "Copy password" and "Copy username" functionality |
| `contextMenus` | To create a context menu menu, also called right-click menu |
Copy link
Member

Choose a reason for hiding this comment

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

"menu menu"

README.md Outdated
@@ -160,6 +160,12 @@ Note: If the cursor is located in the search input field, every shortcut that wo
| <kbd>Ctrl+Shift+G</kbd> | Open URL in the new tab |
| <kbd>Backspace</kbd> (with no search text entered) | Search passwords in the entire password store |

### Usage via right-click menu

You can right-click anywhere a visited website and there will appear a menu with an option `browserpass - <n> entries`, where `n` is the number of entries that match the host of the visited website. When you select an entry, that one gets automatically filled in, equivalent to the behavior when an entry is selected from the browserpass popup. This can be helpful if you want to fill credentials in a browser popup window wighout extension buttons. Selecting single form fields and choosing values to fill in is currently not supported
Copy link
Member

Choose a reason for hiding this comment

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

Lets have a capitalzed "Browserpass" in user-facing places, here and in actual context menus 🙂

README.md Outdated
@@ -160,6 +160,12 @@ Note: If the cursor is located in the search input field, every shortcut that wo
| <kbd>Ctrl+Shift+G</kbd> | Open URL in the new tab |
| <kbd>Backspace</kbd> (with no search text entered) | Search passwords in the entire password store |

### Usage via right-click menu

You can right-click anywhere a visited website and there will appear a menu with an option `browserpass - <n> entries`, where `n` is the number of entries that match the host of the visited website. When you select an entry, that one gets automatically filled in, equivalent to the behavior when an entry is selected from the browserpass popup. This can be helpful if you want to fill credentials in a browser popup window wighout extension buttons. Selecting single form fields and choosing values to fill in is currently not supported
Copy link
Member

Choose a reason for hiding this comment

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

typo in "wighout"

@@ -1156,3 +1162,61 @@ function onExtensionInstalled(details) {
});
}
}

/**
* Create a context menu menu, also called right-click menu
Copy link
Member

Choose a reason for hiding this comment

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

another case of "menu menu" 🙂

@@ -3,7 +3,7 @@
"manifest_version": 2,
"name": "Browserpass",
"description": "Browser extension for zx2c4's pass (password manager)",
"version": "3.7.2",
"version": "3.8.0",
Copy link
Member

Choose a reason for hiding this comment

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

I like to bump version just before the release, could you please revert this? Fine to keep @since 3.8.0 of course.

README.md Outdated

You can right-click anywhere a visited website and there will appear a menu with an option `browserpass - <n> entries`, where `n` is the number of entries that match the host of the visited website. When you select an entry, that one gets automatically filled in, equivalent to the behavior when an entry is selected from the browserpass popup. This can be helpful if you want to fill credentials in a browser popup window wighout extension buttons. Selecting single form fields and choosing values to fill in is currently not supported

![The right-click menu of browserpass](https://i.imgur.com/X2Q5LZn.png)
Copy link
Member

Choose a reason for hiding this comment

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

saw that the other assets are stored at https://user-images.githubusercontent.com/<...>, that's probably in a repo by one of you(?).

Haha, it's simpler than you think, just paste the image in any text area on github (comment here, or in any issue), Github will upload the picture and replace it with markdown, then take the markdown and that's your URL 🙂 Yes lets do via Github, just so we know all the pictures are hosted in the same place.

If we want to include something animated, I'd record that when the other features are there.

Sure! You can keep this picture now (reuploaded on Github) and we can later replace it with an animation if you like 😉

@fkneist
Copy link
Author

fkneist commented Feb 12, 2022

new gif:

browserpass

@fkneist fkneist closed this Feb 12, 2022
@fkneist fkneist force-pushed the feature/right-click-menu branch from 6613de6 to 1b4872d Compare February 12, 2022 17:25
@fkneist
Copy link
Author

fkneist commented Feb 12, 2022

I applied all the PR feedback but then I noticed I had set the wrong email on my commits, ran a script to change the email retrospectively and then force pushed. Later I noticed the script must have changed all commit IDs. But probably the force push was enough to close the PR. Learning by doing :) Let's continue here #284

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

8 participants