After enabling on a domain, context menu item is not immediately ticked

[aspiers]

After granting permission to a domain from the context menu, I would expect a tick to immediately appear to the left of the context menu item. However it only appears after I reload the page.

This has the side effect of making it impossible to disable the extension on that page immediately after enabling; a reload of the page is required first.

There's a chance this is related to my #28 testing, but I have no particular reason to believe that's the case so far.

[fregante]

That behavior was never perfect, I expect MV3 unloading to introduce new race conditions.

It'd be good if you could look into it, adding logging locally and see whether the menu update is called when it's expected. I think it also has to do with tab changes

[aspiers]

Yeah I've already added logging to the background worker, so hopefully I'll get to the bottom of this soon.

BTW I also see weird behaviour with the sites permanently allowed via the static list, although that may have been due to my mistake of including the URLs both with and without the trailing * wildcard in the manifest. I noticed that the code filters out URLs which don't contain * (except <all_urls>), so that might partially explain it.

Part of this weird behaviour was the interaction between the toggle and this submenu of the same context menu:

My current understanding is the following:

• Selecting When you click the extension removes the site from the dynamic allow list if it was previously there. Also sometimes it causes the extension icon to become "embossed" indicating that it's totally disabled on this site and you have to click it and reload the page for it to become enabled. This may or may not achieve the same effect as the next option:
• Selecting On <domain> adds the site to the dynamic allow list if it's not already on the static allow list in the manifest.
• Not sure what selecting On all sites does, but I do know that this option is disabled on my extension, presumably because I have a non-empty list of specific sites on the static allow list in the manifest.

[fregante]

One thing to note is that this has not been tested at all with changes made via that UI, either by clicking "on all sites" or by blocking hosts available in the manifest. So avoid touching that dropdown to ensure it works correctly. A new issue can be opened to resolve specific issues related to changes made via that that UI, including the starting manifest.

However that will eventually need to be adjusted in https://github.com/fregante/webext-permissions

including the URLs both with and without the trailing * wildcard in the manifest

Overlapping patterns shouldn't cause issues because permissions are merged: https://github.com/fregante/webext-permissions#dropoverlappingpermissionspermissions

[aspiers]

Somehow it was allowing URLs of sites on the static list to appear in the dynamic list. Since removing non-wildcarded hosts (which seem prohibited in https://developer.chrome.com/docs/extensions/develop/concepts/match-patterns based on my last reading, even though on previous readings I thought I saw something saying they were allowed), I think this no longer happens.

FYI, I am testing by putting this in my background worker:

import { isContentScriptRegistered } from 'webext-dynamic-content-scripts/utils.js';
import { queryAdditionalPermissions, normalizeManifestPermissions } from 'webext-permissions';

export async function checkPermissions(reason: string): Promise<void> {
  console.log('reason: ', reason);
  const manifest = chrome.runtime.getManifest();
  console.log('manifest: ', manifest);
  const matches = [
    ...manifest.content_scripts[0].matches,
    'https://developer.chrome.com/foo',
    'https://etherscan.io/foo',
  ];
  //console.log('manifest.content_scripts: ', matches);
  for (const match of matches) {
    const mode = await isContentScriptRegistered(match);
    console.log(`${match}: ${mode}`);
  }

  // Returns mix of manifest and new, as explained in
  // https://github.com/fregante/webext-permissions
  // const perms = await chrome.permissions.getAll();
  // console.log('perms: ', perms);

  const newPermissions = await queryAdditionalPermissions();
  console.log('newPermissions.origins: ', newPermissions.origins);

  const manifestPermissions = await normalizeManifestPermissions();
  console.log('manifestPermissions.origins: ', manifestPermissions.origins);
}

chrome.permissions.onAdded.addListener(() => checkPermissions('onAdded'));
chrome.permissions.onRemoved.addListener(() => checkPermissions('onRemoved'));

[fregante]

I confirmed that this is a bug, but it's unrelated to MV3. This line is always called without the url parameter:

https://github.com/fregante/webext-domain-permission-toggle/blob/34b32434e8ce858d6da75e26278638d0f2f48167/index.ts#L122

I think my intent was to call it on error. It needs to be reworked. For example here:

https://github.com/fregante/webext-domain-permission-toggle/blob/34b32434e8ce858d6da75e26278638d0f2f48167/index.ts#L74-L80

The code should probably be

const userAccepted = await chromeP.permissions.request(permissionData);
chrome.contextMenus.update(contextMenuId, {
	checked: userAccepted,
});
if (!userAccepted) {
	return;
}

PR welcome, separately from the MV3 one

[aspiers]

Sorry, I don't quite follow. The call to updateItem() you are referring to is within handleClick() which handles toggling of the option via:

https://github.com/fregante/webext-domain-permission-toggle/blob/0a34065eb86f1f56c9c4f3f3b09481fef98eec8c/index.ts#L104

You wrote:

I think my intent was to call it on error.

but surely it needs to be called whether the domain is being toggled on or off? If there's an error with requesting or removing permissions, then why would it need to be called?

Also I'm not clear if you are saying that handleClick() needs to be changed, or togglePermission(), or something else.

[fregante]

The piece of code I pasted needs to be applied. Other paths need to be reviewed.

I think that the "call on error" was supposed to ensure that the checkbox follows the actual result. This is because, again I think, Chrome actually already toggles the checkmark natively on click:

1. Start unchecked
2. Click item
3. Reject permissions
4. Chrome already checked the menu item
5. Reset the menu item

I wrote this code like 8 years ago so git blame might explain what I was doing then.

[aspiers]

The explicit piece of code I pasted needs to be applied.

Applied where though? Sorry for the dumb questions.

I think that the "call on error" was supposed to ensure that the checkbox follows the actual result. This is because, again I think, Chrome actually already toggles the checkmark natively on click:

1. Start unchecked
2. Click item
3. Reject permissions
4. Chrome already checked the menu item
5. Reset the menu item

Ah right. That's pretty dumb if Chrome interferes with the checkmark in some or all circumstances.

I wrote this code like 8 years ago so git blame might explain what I was doing then.

Good point, will bear that in mind!

[fregante]

I linked to it immediately before

[aspiers]

Ohh I see, I misread "For example" to mean "here's an example of some code on which you could base your fix handleClick()" but you actually meant change that code elsewhere as part of a wider reworking. Makes sense now :)

[aspiers]

I think that the "call on error" was supposed to ensure that the checkbox follows the actual result. This is because, again I think, Chrome actually already toggles the checkmark natively on click:

1. Start unchecked
2. Click item
3. Reject permissions
4. Chrome already checked the menu item
5. Reset the menu item

I confirmed this is correct, which also means that this existing code fragment is correct (although perhaps not optimal; see below):

I think my intent was to call it on error. It needs to be reworked. For example here:

https://github.com/fregante/webext-domain-permission-toggle/blob/34b32434e8ce858d6da75e26278638d0f2f48167/index.ts#L74-L80

It should only run in the scenario described above, to undo Chrome checking the menu item if the permissions aren't granted, but not in the case that permissions are granted.

I tried changing it to this:

The code should probably be

const userAccepted = await chromeP.permissions.request(permissionData);
chrome.contextMenus.update(contextMenuId, {
	checked: userAccepted,
});
if (!userAccepted) {
	return;
}

but that actually had the opposite effect of preventing the item from being checked (although it would be later checked if the tab was activated or updated). I'm not sure why it prevents it; maybe a race condition which somehow interferes with Chrome's native checking of the item.

I confirmed that this is a bug, but it's unrelated to MV3. This line is always called without the url parameter:

https://github.com/fregante/webext-domain-permission-toggle/blob/34b32434e8ce858d6da75e26278638d0f2f48167/index.ts#L122

This was indeed the main problem. And in fact fixing that seems to make the previous call to .update() in the above code unnecessary.

I'll submit a PR in the morning.