Correctly set check mark after accepting a new permission

[aspiers]

When the user requests the extension to be given access to the site and confirms this in the consequent browser dialog, the context menu item wasn't being immediately changed to checked.

We can fix this by ensuring that the updateItem() function which is responsible for updating the checkmark is called with the correct URL for the tab in question, so that it can detect whether the permission is now granted to the site and update the context menu item accordingly.

Note that the browser will automatically enable the checkmark simply by virtue of the context menu item having been clicked. So if the user denies access in the dialog (despite previously requesting it to be given via a user-initiated "gesture", which is the only way the chrome.permissions API allows access to be given), then chrome.contextMenus.update() needs to be called to reverse that enabling of the checkmark. However by calling updateItem() within the finally block with the correct tab URL, we can ensure the checkmark is always correctly set regardless of what happened.

Fixes #29.

[aspiers]

Oops, fixing CI now.

[fregante]

If this only needs to be called on error, then it should be inside the catch statement right? Also I presume that if there's an error the status is "unchecked", which doesn't need a URL check.

On extension with limited permissions, you can't get the tab URL (e.g. I think this fails on chrome://extensions for example)

There's a demo extension in this repo you can use for testing. Use npm run demo:watch to build it.

[fregante]

Rather, I suppose:

• restore the code on line 76
• also add the same exact code before throw error;

right?

[aspiers]

If this only needs to be called on error, then it should be inside the catch statement right?

I did think about that, but I wanted to be able to add handling for other scenarios like this one:

aspiers@412c070

and so it felt like a cleaner separation of concerns to only update the context menu from updateItem() and then have that function be smart enough to handle all scenarios, rather than having multiple parts of the code updating the menu which could potentially interact in unpredictable and undesirable ways (e.g. race conditions from having multiple event handlers collide).

That said, this is just a preference rather than a strong opinion.

Also I presume that if there's an error the status is "unchecked", which doesn't need a URL check.

I wasn't sure about that. Couldn't the extension already be enabled, in which case if there's an error it should stay checked?

On extension with limited permissions, you can't get the tab URL (e.g. I think this fails on chrome://extensions for example)

There are so many pitfalls in this area it's crazy 🤣

There's a demo extension in this repo you can use for testing. Use npm run demo:watch to build it.

Oh thanks, I'll check it out!

Rather, I suppose:

• restore the code on line 76
• also add the same exact code before throw error;

right?

You mean in addition to removing the finally clause? Otherwise there could be duplicate menu updates: in both the catch and finally clauses.

[fregante]

I wasn't sure about that. Couldn't the extension already be enabled, in which case if there's an error it should stay checked?

There shouldn't be an error when removing a permission. However… you can't remove a wide permission like *://*/* this way. If that optional permission is granted, the item should become "✔️ Enabled on all sites". Clicking it would remove that wider permission.

Edit: Opened:

• Can't remove permissions broader than exactly currentOrigin/* (support more wildcards and all_urls) #37

For the intents of this PR, it should assume that removal is successful.

[aspiers]

OK, new version coming shortly.

[aspiers]

TL;DR: after investigation, I recommend keeping the approach in this PR, but the gory details follow.

So I tried this approach:

aspiers@toggle-fix2

and it results in several regressions where the check mark doesn't get set correctly in multiple cases.

For example, switch site access to on click, then enable for a site in the manifest via content menu toggle. Because we're relying on Chrome to update the menu item, it doesn't spot that the site is in the manifest, therefore it (correctly) enables the checkmark but fails to disable (grey out) the menu item. This means that it's now possible to attempt to remove the same permission from the still enabled menu item. In that case, this statement:

There shouldn't be an error when removing a permission.

turns out to be not true, because it results in:

Uncaught (in promise) Error: You cannot remove required permissions.

Even if we add some logic to deal with that case, it feels to me like a very brittle approach to assume that we can ensure chrome.permissions.remove() will only ever be called in a way to ensure its success, because there are so many different combinations of scenarios which we don't fully understand yet - even in Chrome, let alone other browsers.

As well as being brittle, it would basically require either:

• copying logic into togglePermission() from updateItem() which tests URLs against the manifest, which would be a violation of DRY and separation of concerns, or
• calling updateItem() from togglePermissions(), in which case it's not really any different to just keeping updateItem() in the finally block.

IMHO this affirms the point I was making earlier about the benefit of separation of concerns: if we have one function which handles interactions with chrome.permissions (i.e. togglePermission(), and then a separate function which takes care of ensuring that the menu item is always configured correctly (updateItem()), then we don't really need to worry about all the ways they can interact with each other. All we need to do is ensure that updateItem() is called at the appropriate points.

Revisiting your original comments with the benefit of hindsight:

If this only needs to be called on error,

We now know this isn't true, because of the need to disable the menu item in certain cases.

Also I presume that if there's an error the status is "unchecked", which doesn't need a URL check.

We now know this isn't true, since if something prior to this goes wrong, it can allow a misguided attempt to disable a site in the manifest would fail, in which case the status should remain checked.

On extension with limited permissions, you can't get the tab URL (e.g. I think this fails on chrome://extensions for example)

I don't see a problem here, because the code in this PR should still work if the tab URL can't be retrieved. The only minor issue might be that updateItem() will cause the menu item to be enabled, which doesn't make sense for things like chrome://extensions. But that could be easily fixed, e.g. by changing the default to enabled: false.

[fregante]

But that could be easily fixed, e.g. by changing the default to enabled: false.

Nothing is "easily" fixed here obviously haha

If this only needs to be called on error,

We now know this isn't true, because of the need to disable the menu item in certain cases.

I'm only talking about updateItem calls in action.onClicked, not tabs.onActivated

and it results in several regressions where the check mark doesn't get set correctly in multiple cases.

As mentioned elsewhere, I really don't want to get into allow access on click and other Chrome UI stuff. Let's fix #38 first.

• Support for Chrome's native site access UI #39

[fregante]

I'll open a PR

[aspiers]

if the force-pushes cause problems for you

It's mostly because it becomes more difficult to see what changed.

Yes, GitHub is bad in this regard. In fact I documented this in detail a long time ago:

• amending PR branches via force-push causes unreachable commits to be garbage-collected isaacs/github#997
• amending PR branches via force-push causes unreachable commits to vanish from web UI isaacs/github#998
• web UI doesn't allow comparison between versions of a PR branch isaacs/github#999

However, it got a bit better over the years. For example, you can click on either of these links to see the difference:

(Although in this PR, you will see a force-push which didn't change the code at all. That doesn't mean this feature is broken; it's just because I realised I had created the commit with the wrong author and committer emails, so I fixed the commit metadata and force-pushed to prevent invalid email addresses from polluting main forever.)

I know squashing is common in other repos so I generally say it to save contributors from unnecessary work

True, I have noticed that a lot of developers really struggle with cleaning git history. I used to contribute to git, so this kind of stuff has been second nature to me for a long time.

BTW in case you haven't seen it, Gerrit does code review WAY better. After years of working with both, GitHub code review still feels like bashing my head against the wall every time. And for Open Source repositories, it's possible to have the best of both worlds for free via https://gerrithub.io/ (although keeping GitHub Actions is a bit more effort). But I digress 😆

[fregante]

you can click on either of these links to see the difference

I know, but the difference is that with regular commits you don't have to click. In fact GitHub even has a nice commit dropdown in both the Files tab and the Commits tab. 😃

Also re-pushing the same commit with an issue reference causes noise in the referenced issue: #29 (reference)

[fregante]

I still don't understand what's the problem with this line.

1. item is unchecked
2. user clicks it
3. chrome checks it
4. we ask for permission
5. user denies ❌
6. this piece of code is reached and we uncheck it
7. done, no need for finally

Or if successful:

1. item is unchecked
2. user clicks it
3. chrome checks it
4. we ask for permission
5. user accepts ✅
6. this piece of code is NOT reached
7. done, no need for finally wither

I can't think of another scenario in which request() returns true but it should not be (natively) checked.

To put it in other words, we only have to fix the check:

• if the user rejects
• if it throws

The way I see it, the only reason #29 is an issue is because updateItem() is called even if successful. That's it.

[aspiers]

I can't think of another scenario in which request() returns true but it should not be (natively) checked.

I've been through so many scenarios now that my head is exploding, but I'll do my best to respond accurately.

I don't think I ever said there was a problem with these three lines; it's just that my PR proposed another way of tackling the issue via modified usage of updateItem() elsewhere, which made these three lines redundant (since we don't want to be updating the same menu item twice in rapid succession as part of the same event handling).

The problematic situation I described above was not relating to when the user wants to grant permission, but rather when they are trying to remove it:

1. item is checked and in the manifest but not disabled (i.e. greyed out)
2. user clicks it
3. we ask to remove permission
4. browser throws an exception because the URL matches a pattern already in the manifest
5. at this point we need to make sure that the checkmark stays present

Yes it's true that in step 1 here, the item should be disabled, but as I observed above, this is not yet working reliably in all situations, which is why I was proposing a less brittle approach which doesn't assume that everything else is working perfectly.

But anyway, I think your draft approach in #38 is more promising so hopefully that will work well in which case I'm happy to leave this PR closed.

[fregante]

browser throws an exception because the URL matches a pattern already in the manifest

This is where we keep butting heads. This does not happen if you keep the Chrome UI (#7) out of the equation.

[fregante]

doesn't assume that everything else is working perfectly.

The issue with that is that you're already working on a broken piece of UI, so what you're suggesting is focusing on the wrong part of the code. Let's instead make sure that the UI is correct before the click.

[aspiers]

Sure, I've already given up worrying about the Chrome UI for now. However there are still problems, but I'll describe them in the relevant issues and PRs.

[fregante]

Closing this for now, I'm improving the overall logic in a new PR.

the benefit of separation of concerns

I agree with you here. Fixing it in a new PR

All we need to do is ensure that updateItem() is called at the appropriate points.

Yes. I also opened this:

• Update toggle when permissions change via events #36

[aspiers]

I know, but the difference is that with regular commits you don't have to click. In fact GitHub even has a nice commit dropdown in both the Files tab and the Commits tab. 😃

Sure. The problem with that is that it's mixing "private" / unclean history (which isn't intended for inclusion in main) with "public" / clean history (which is). This is a key failing of GitHub's code review system, and again something which Gerrit gets perfectly right by cleanly separating the two and making it easy to review whichever diffs you need to see, both history and "meta-history".

So GitHub forces us to pick one of two suboptimal approaches. You prefer one, I prefer the other, it's all good :-)

Also re-pushing the same commit with an issue reference causes noise in the referenced issue

Yes that's an annoying downside. It could be worked around by only referencing the issue in the PR and not in the commits. Also technically speaking, fixups commits if written correctly should also reference the issue they are relating to, so that the reviewer knows which issue they are fixing up. For small PRs and small repos like here, this is less relevant because typically the PR only targets a single issue. I normally work in much larger repos with larger teams, where writing comprehensive messages for every single commit and polishing history prior to merge are much more important, so that's where my preference comes from.

Anyway, back to the main topic ;-)