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.

Sign up for GitHub

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

Correctly set check mark after accepting a new permission #33

Closed
wants to merge 1 commit into from
+1 −4
Closed

Correctly set check mark after accepting a new permission #33

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,6 @@ async function togglePermission(tab: chrome.tabs.Tab, toggle: boolean): Promise<

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

@fregante fregante Jan 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

Expand Down Expand Up @@ -117,7 +114,7 @@ async function handleClick(

throw error;
} finally {
void updateItem();
void updateItem(tab?.id ? await getTabUrl(tab?.id) : '');
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

Rather, I suppose:

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

right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

@fregante fregante Jan 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, new version coming shortly.

Copy link
Contributor Author

@aspiers aspiers Jan 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Owner

@fregante fregante Jan 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

I'll open a PR

}
}

Expand Down