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

Add Safari support (done) #10

Closed
fregante opened this issue Nov 30, 2020 · 4 comments
Closed

Add Safari support (done) #10

fregante opened this issue Nov 30, 2020 · 4 comments
Labels
help wanted safari Related to Safari only

Comments

@fregante
Copy link
Owner

Safari has its own permission system which would replace this module

However this module should be updated to avoid adding a non-working context menu item

@fregante fregante added help wanted safari Related to Safari only labels Nov 30, 2020
@fregante
Copy link
Owner Author

fregante commented Dec 19, 2020

I found out why: tab.url is an empty string for some reason. activeTab should fill that in.

https://github.com/fregante/webext-domain-permission-toggle/blob/2aacb195e7fbaaf04e23c32b074b5bbd10651732/index.ts#L88-L92

If this can be fixed, I think this module is still the easiest way to build cross-browser extensions since we don't have to customize the Safari build.

@fregante
Copy link
Owner Author

fregante commented Jan 7, 2021

tab.url is empty because apparently context menu clicks don't "activate" the tab (activeTab permission). Left-clicking the browserAction first and then clicking the context menu item works, but that's awkward.

Screen Shot 2

This bug might be solved by Safari in the near future or it might not.


I have 2 "solutions" for this:

  • Detect Safari and just hide the context menu item. The developer would have to document this manually.
  • If tab.url is missing from the event handler, instruct the user to click the icon first. This frees the developer from having to document this behavior.

Perhaps I use both:

  • By default, keep the menu item and inform the user
  • Offer an option to detect and exclude Safari, like addDomainPermissionToggle({safari: false})

@fregante

This comment has been minimized.

@fregante
Copy link
Owner Author

fregante commented Jan 7, 2021

I think I found a workable solution!!

Step 1: Ask the user to try again.

if (!tab.url) {
    chrome.tabs.executeScript({
        code: `alert("Try again pretty please?")` // Alert doesn't work from background in Safari
    });
    return;
}

Step 2: Bind the request/remove functions to chrome.permissions because that's how Safari likes them.

Step 3: Make sure that the menuitem checkmark is updated correctly when it fails

🥳

However webext-dynamic-content-scripts also needs to be fixed.

@fregante fregante changed the title Doesn't work on Safari (but doesn't need to) Add Safari support Jan 8, 2021
@fregante fregante pinned this issue Jan 8, 2021
@fregante fregante changed the title Add Safari support Add Safari support (done) Jan 8, 2021
fregante added a commit that referenced this issue Jan 8, 2021
@fregante fregante unpinned this issue Apr 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted safari Related to Safari only
Projects
None yet
Development

No branches or pull requests

1 participant