Create right-click menu #284

fkneist · 2022-02-12T17:44:57Z

2nd try, because of messed up git history in last PR

maximbaz

Tested in Chromium and Firefox, works for me! Nicely done 🙂

src/background.js

maximbaz · 2022-02-12T19:05:53Z

src/background.js

+
+    await chrome.contextMenus.create({
+        ...menuEntryProps,
+        title: `Browserpass - ${numberOfLoginsForThisHost} ${singularOrPlural}`,


Although it's cool to be able to have a dynamic title, honestly I don't see how it is useful - except to indicate that there are 0 entries, in which case I think it's better to not take up space in context menu at all... I'd just prefer a static minimalistic "Browserpass" title...

I can live with that :) In combination with the not showing if 0 matches, the presence of the menu entry now indicates that there is at least one match

Cool :) Would that be too much to ask to rerecord the gif? Not important as you'll change UI in the next PR anyway but if it's super quick for you...

here we go:

maximbaz · 2022-02-12T19:54:25Z

src/background.js

+ * @return void
+ */
+async function clickMenuEntry(settings, login) {
+    await handleMessage(settings, { action: "fill", login }, () => {});


The last parameter to handleMessage currently does nothing but should at least log some error in background page's console. Above, on line 1186, you throw an error and it does end up in background page's console (easy to test by swapping the condition to == 'ok', it's something to test here as well, perhaps just throwing an error would be sufficient?

maximbaz · 2022-02-12T19:55:00Z

src/background.js

+ * @since 3.8.0
+ *
+ * @param object    settings    Full settings object
+ * @param array     login       Filtered and sorted list of logins


I believe this comment is not true, it's not a list of logins right?

Yes, you're right. Btw that reminds me of an idea that I had: what would you think about porting the project to TypeScript, then part of the documentation would become unnecessary and wouldn't need to be manually maintained + it would maybe be a bit easier to understand input/output parameters quickly.

Let me know what you think about that, I'd be interested to look into it.

what would you think about porting the project to TypeScript

I'd prefer not to do that at this point. It's a discussion for another time perhaps, but in the more immediate term I'd rather get the current backlog of stuff cleared and ready for release.

maximbaz · 2022-02-12T21:10:26Z

Btw that reminds me of an idea that I had: what would you think about porting the project to TypeScript, then part of the documentation would become unnecessary and wouldn't need to be manually maintained + it would maybe be a bit easier to understand input/output parameters quickly.

Extracting the question for visibility. @erayd is our main frontend person so I'll let him properly answer this, I can say this was considered back when we created the project, but also I'm led to believe that typescript has improved a lot since 2018, so maybe it's good to spend at least 10 minutes and reconsider again, even if we end up deciding to stick with javascript.

Ideally it would be good for him to also review this PR, but he is busy in the next couple of weeks and you are eager to contribute and we all appreciate that and I want to support you, so let's agree that I'll test it later today and if everything works well I'll merge, so that you can continue working on the next improvements, and we might have to fix some additional code review comments in a separate PR later when @erayd gets time to look at your contributions? 😉

maximbaz · 2022-02-12T23:41:31Z

src/background.js

@@ -41,6 +41,8 @@ chrome.browserAction.setBadgeBackgroundColor({
 chrome.tabs.onUpdated.addListener((tabId, info) => {
    // unregister any auth listeners for this tab
    if (info.status === "complete") {
+        createContextMenu();


While testing I managed to get some errors in the console when I was opening a new tab, which is because of concurrent attempts to recreate context menu. Here's what happens:

createContextMenu is async, so by calling it here we don't really block browser, there's no way of knowing when the function completes. In the beginning of createContextMenu we first remove the context menu and then try to recreate it. At the same time, chrome.tabs.onActivated will get called, in turn calling createContextMenu in parallel. The previous run has just created the parent menu, this run removes it, and then the previous run tries to create children to a now gone parent, and crashes.

Of course one way to reduce the chances of a crash is to move chrome.contextMenus.removeAll() from the beginning of the function to as much later as possible - but this will not entirely eliminate the problem, but rather just reduce its chances.

Another problem is that here you actually call createContextMenu only on complete event - I understand why you did it, but it presents a problem on slow websites. I open github.com and my context menu is filled with entries. I then go in this tab to some very slow website, which loads a ton of images, and so complete event will only fire in say 5 seconds. During the first 5 seconds if I right click, I will still see entries for github.

I propose to repeat what updateMatchingPasswordsCount function does:

be called on all statuses, not just on complete

use some kind of analogue to isRefreshing variable to prevent concurrent runs

probably you won't need chrome.tabs.onActivated below, because badge seems to not need it...

Let me know if it makes sense and if you have any questions

Thanks for the thorough review, I can reproduce but haven't figured out how to solve it completely yet. I'll keep you posted.

I think I figured it out 🎉. I call createContextMenu now on every status, but I need chrome.tabs.onActivated to "kill" callbacks that are being called from an "old" tab.

It's a bit hard to formulate, so here's an example: I'm in tab A with a slow page, I switch to tab B before tab A finished loading, when tab A finally finished loading I don't want it to mess up the context menu.

@fkneist Would simply including the origin in the callback be sufficient? That way you can just discard any calls that have a different origin than the currently active one.

fkneist · 2022-02-21T20:58:16Z

Another thing I noticed while testing is, that the order of the child entries is not always the same (multiple accounts per host). I think in the popup this is intended, as to move frequently used entries up, but in the right-click menu I find it a bit confusing. What would you guys think of alphabetical sorting in the right-click menu?

erayd · 2022-02-21T21:00:41Z

@fkneist

What would you guys think of alphabetical sorting in the right-click menu?

I think it would be great for instances where there are only a small number of entries - but where there are large numbers, we should stick to frequency. As an example, I know someone who has over 200 microsoft.com accounts in their password manager - in that kind of scenario, alphabetised sorting would render the right-click menu unusable.

…o feature/right-click-menu-2

fkneist · 2023-03-28T14:22:40Z

Hi there, it's been a while 😃

I had some time and tinkered on the solution in the last few days:

Not using chrome.contextMenus.removeAll() anymore, rather switching visibility of parent item and remove child items on demand one by one.
Credentials are cached per tab with a TTL now
Unfortunately the solution feels way longer, in terms of LOC, now, probably more surface area for bugs 😒

Some scenarios I tested:

Visit a page that I know I have credentials for. Also the opposite.
Visit a page that I don't have credentials for after I visited one that I have credentials for. And vice versa.
Visit a internal browser page and then above scenarios. And vice versa

Known limitation:

when I have two tabs open, site A and site B. I site A is a slow site and I press refresh and quickly change to site B, it can happen even though I have credentials for site B that the context menu doesn't show for a short time

Let me know what you think @maximbaz and @erayd

maximbaz · 2023-03-28T21:15:55Z

Lovely, thanks for your time and contribution!

There are a couple of other items we need to deal with first (e.g. #290 has a lot of work that's been put in and is very close to being merged, #320 has a deadline in June or the extension will be delisted from Chrome Web Store), so I think it's most realistic that we'll look at this PR afterwards. But if we have the time to add additional items in the release, I would love this PR to be one of them!

erayd · 2023-10-06T01:14:08Z

src/background.js

+ * @return void
+ */
+async function clickMenuEntry(settings, login) {
+    await handleMessage(settings, { action: "fill", login }, (response) => {


This just triggers the standard 'fill' action, however given that it's for a context menu, there needs to be additional logic that ensures the field that has been clicked on is the one that gets filled. Some pages have multiple candidate forms, and the one that Browsewrpass picks by default for the fill action may not be the one that the user right-clicked for the context menu.

Create right-click menu

0d52930

fkneist mentioned this pull request Feb 12, 2022

Enable right-click menu in chrome #282

Closed

maximbaz reviewed Feb 12, 2022

View reviewed changes

fkneist added 3 commits February 12, 2022 21:07

Don't show menu entry when there are zero matching logins

8874b79

Static menu title

646c7e5

trow error + fix docs

3a7f61d

maximbaz reviewed Feb 12, 2022

View reviewed changes

fkneist added 2 commits February 21, 2022 21:27

Fix slow load & tab switch issue

025c98e

Update GIF

fcc3d39

fkneist added 3 commits March 13, 2023 19:43

Merge branch 'master' of github.com:fkneist/browserpass-extension int…

7aaf9c4

…o feature/right-click-menu-2

Cover edge cases for context menu creation

48a3d35

Handle cache expired

434affd

fkneist added 2 commits March 28, 2023 16:31

Refactor

f880d43

Adjust inline documentation

f9b23ee

erayd requested changes Oct 6, 2023

View reviewed changes

erayd assigned fkneist Oct 6, 2023

erayd added the author-action-needed Awaiting action by the PR author label Oct 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create right-click menu #284

Create right-click menu #284

fkneist commented Feb 12, 2022

maximbaz left a comment

maximbaz Feb 12, 2022

fkneist Feb 12, 2022 •

edited

Loading

maximbaz Feb 12, 2022

fkneist Feb 21, 2022

maximbaz Feb 12, 2022

maximbaz Feb 12, 2022

fkneist Feb 12, 2022

erayd Feb 21, 2022 •

edited

Loading

maximbaz commented Feb 12, 2022 •

edited

Loading

maximbaz Feb 12, 2022

fkneist Feb 13, 2022

fkneist Feb 21, 2022

erayd Feb 21, 2022 •

edited

Loading

fkneist commented Feb 21, 2022 •

edited

Loading

erayd commented Feb 21, 2022

fkneist commented Mar 28, 2023

maximbaz commented Mar 28, 2023

erayd Oct 6, 2023

Create right-click menu #284

Are you sure you want to change the base?

Create right-click menu #284

Conversation

fkneist commented Feb 12, 2022

maximbaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fkneist Feb 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erayd Feb 21, 2022 • edited Loading

Choose a reason for hiding this comment

maximbaz commented Feb 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erayd Feb 21, 2022 • edited Loading

Choose a reason for hiding this comment

fkneist commented Feb 21, 2022 • edited Loading

erayd commented Feb 21, 2022

fkneist commented Mar 28, 2023

maximbaz commented Mar 28, 2023

Choose a reason for hiding this comment

fkneist Feb 12, 2022 •

edited

Loading

erayd Feb 21, 2022 •

edited

Loading

maximbaz commented Feb 12, 2022 •

edited

Loading

erayd Feb 21, 2022 •

edited

Loading

fkneist commented Feb 21, 2022 •

edited

Loading