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

[WIP] Refactor background, increase version, remove permission #2

Merged
merged 1 commit into from
Jun 16, 2023

Conversation

Mastercuber
Copy link

@Mastercuber Mastercuber commented Mar 15, 2023

This pull request refactors the background script, so that this can be used in firefox and chromium based browsers like Chrome, Opera, Edge.

Also a manifest file for deploying as a chrome extension is created witrh this PR.
The version of the manifest itself is increased to 3. In the README file you can find an answer to why using v3.

In the README also links to the Chrome Webstore and how to publish to the Webstore was added.

I have tested functioning of the plugin successfully with firefox and chrome. Opera is not working with manifest version 3 for now. Another manifest for Opera should do the trick, but seems a bit strange, since another manifest is needed for version 3 but one is enough for version 2.

All in all this PR refactors the background script, increases the manifest version, removes unnecessary permissions and add some Chrome webstore and manfest version related links to README.

To load the plugin manually into the browsers for testing, you can use:

  • For Chrome: chrome://extensions/
  • For Firefox: about:debugging#/runtime/this-firefox
  • For Opera: opera://extensions

For Chrome and Opera the manifest file must be renamed to manifest.json and the current one be stashed or removed.

@Mastercuber Mastercuber changed the title Refactor background, increase version, remove permission [WIP] Refactor background, increase version, remove permission Mar 15, 2023
@Mastercuber
Copy link
Author

This PR also relates to 1 since the PR introduces a fetch from the /wb/typeahed endpoint. Since the endpoints are not reachable from the plugins, the suggestions are mocked data. Before it was the actual search term expanded to a URL. When the endpoint is reachable, the suggestions are the same as when typing on the website search bar itself.

@adbar
Copy link
Member

adbar commented Jun 5, 2023

Thank you very much for your suggestions, we are currently also interested in improving the addon and we will have a look at it.

@adbar
Copy link
Member

adbar commented Jun 5, 2023

Just to make it clear @gremid, my assumption is that the PR uses the polyfill as we just planned:

@Mastercuber
Copy link
Author

Mastercuber commented Jun 5, 2023

@adbar I also spotted the "Building a cross browser extension" on MDN. In this article at the very top is a note, stating, that this is for Manfiest v2.
Chrome is not even accepting add-ons uploaded with manifest v2 anymore.
This is one reason, why there are 2 manifests in the PR. Another reason is, that chromes manifest v3 needs background.service_worker whereas firefox is printing a message, that Service Workers are disabled.. (see issue). Also the permissions in the manifest differ a bit (for chrome contextMenus and for firefox menus is needed), though they could be included both in one manifest - as I discovered so far.

I've also tried to use web-ext and the polyfill without success..
Having 2 different manifests (both version 3) with nearly the same content and just use the pluginApi kind of "own" polyfill seems a pretty good starting point for an cross browser add-on.
There are anyway only 2 big namespaces: chrome and browser. All browsers use one of them.
Then, the most API's are named the same (of course, some a different)

When f.e. the polyfill is used, the search API is almost only usable in firefox and there must still be differentiated if the add-on is executed in firefox or a chrome like browser in f.e. line 45 in the background script.

@Mastercuber
Copy link
Author

After some local testing, I see that I have no problems with Chrome and Opera (suggestions enabled) but with firefox. Immediately after the refactoring of the add-on, there were no problems. With firefox only the context menu works correctly. The omnisearch API seems to get fully ignored (addListener).

@Mastercuber
Copy link
Author

Mastercuber commented Jun 5, 2023

I've web-ext linted the project..

web-ext is indeed helpful (npm i -g web-ext) linting and building the project. The built file is actually a simple ZIP file containing the add-on directory structure with the manifest.json at the root.

Building a add-on version for firefox and one for chrome/opera/edge can be done with the following script:

cd dwds-addon/src
rm -R web-ext-artifacts/
web-ext build
mv web-ext-artifacts/im_dwds_nachschlagen-0.1.4.zip ../im_dwds_nachschlagen-0.1.4-firefox.zip
mv manifest.json manifest-firefox.json
mv manifest-chrome.json manifest.json
web-ext build 
mv web-ext-artifacts/im_dwds_nachschlagen-0.1.4.zip ../im_dwds_nachschlagen-0.1.4-chrome.zip

I've pushed a parameterized (version) script to the PR.
For signing the add-on an api-key and api-secret is needed.

Here is what you get when running web-ext docs.
web-ext run is not working for me (profile could not be loaded) but the add-on can still be loaded manually like mentioned in the initial description.

@Mastercuber
Copy link
Author

I've added you all @adbar @haoess @gremid @ulf1 to the list of contributors of the fork, so if you want, you can commit to the PR. If not, just ignore..

src/manifest.json Outdated Show resolved Hide resolved
src/manifest-chrome.json Outdated Show resolved Hide resolved
src/manifest.json Outdated Show resolved Hide resolved
@adbar
Copy link
Member

adbar commented Jun 7, 2023

Thanks for the changes, feel free to add an AUTHORS.md to the repo and to add yourself to it.

Let's give everyone time to review the PR if necessary and aim for a release next week. At least for Firefox I can test and update the existing extension.

@Mastercuber
Copy link
Author

Mastercuber commented Jun 7, 2023

Ok, the newest commits are now also in this PR included.. Issues resolved..

@Mastercuber
Copy link
Author

Mastercuber commented Jun 7, 2023

@adbar did you get the omnibox to work with firefox? (@dwds + Space in the address bar)
Like mentioned above, in chrome and opera I have no problems (a bit of delay for the suggestions, but HTTP requests are included..) but in firefox I have..

I have no idea what to use instead of omnibox. The API seems to be used correctly and Firefox version 113 is supported.

In the contextMenu.onClicked handler I can set a breakpoint and the debugger stops there. But a breakpoint in the other handlers (omnibox.onInputChanged, omnibox.onInputEntered) doesn't make the debugger stop there, when the specific event should get dispatched.

For an example usage of the omnibox API see here

@adbar
Copy link
Member

adbar commented Jun 8, 2023

No, the omnibox is not working consistently in the current version so it is no downgrade if it isn't working after your changes.

AUTHORS.md Show resolved Hide resolved
@Mastercuber
Copy link
Author

Mastercuber commented Jun 8, 2023

No, the omnibox is not working consistently in the current version so it is no downgrade if it isn't working after your changes.

One other thing I tried to get the suggestions to work in firefox is the suggest_url in the manifest, but also without success.
Unfortunately there is no hint in the docs what a format must be returned from the suggest endpoint (maybe OpenSearch?).

Like you already stated, this is not a downgrade, since the add-on is fully working for chrome and opera.
Actually the keyword is recognized and can be used in the address bar, and also when hitting enter, the SearchTerm will be opened with dwds, but the suggestions are missing.

Here is a bugzilla issue about the omnibox API.

@adbar
Copy link
Member

adbar commented Jun 15, 2023

@Mastercuber Thanks, I was able to build and test the extensions, the build script is really helpful. There are still parts missing, here what I expected and what I found, feel free to complete or comment.

I'm using the current add-on as a reference, the working functions are shown by the screenshots.

  • Add DWDS to the list of search engines
    • Firefox ✔️
    • Chromium ❓ (only present at the bottom in "Some extensions can add search engines to Chrome")
  • Right-click on a word and "search with DWDS"
    • Firefox 🚫
    • Chromium 🚫
  • Omnibox, i.e. shortcuts in address bar
    • Firefox ✔️ (maybe unexpectedly... it actually was already working, I just checked)
      both @dwds + SPACE + word and dwds + SPACE + drop down menu
    • Chromium ❓
      only dwds + SPACE + drop down menu

Before releasing a new version for Firefox I hope we can solve the right-click issue.

Maybe I did something wrong with the tests on Chromium, the add-on builds and installs correctly but the functionality currently isn't there (at least on my computer). Is that normal?

@adbar adbar assigned adbar and unassigned gremid Jun 15, 2023
@adbar adbar added the enhancement New feature or request label Jun 15, 2023
@Mastercuber
Copy link
Author

Add DWDS to the list of search engines

When I change the keywords (omnibox = dwds1; search_provider = dwds2) I can't find the search engine anywhere in the settings, only the dwds1 keyword is visible (and working) like mentioned by you.

It seems like the chrome_settings_overrides.search_provider is not even recognized 🤔

Right-click on a word and "search with DWDS"

Right clicking on a word is not enough, since the context menu item only gets added when text is selected (docs).

For me it seems like it would be possible somehow to implement this functionality, but with a non small time consumption.. Processing the selected text, instead of figuring out, what text is actually meant to be searched with DWDS is certainly the better way to go.

Omnibox, i.e. shortcuts in address bar

In firefox the keyword specified in chrome_settings_overrides.search_provider is getting prefixed with "@" as well. Would the search provider keyword be dwds1 and the omnibox keyword dwds2, then dwds1 and @dwds1 would trigger a request to the provided search_url (with no suggestions as drop down for now). With dwds2 the omnibox API would be invoked and so the event handlers (onInputChanged, onInputEntered) would get executed appropriately (suggestions as drop down visible).

So in firefox - since both keywords are the same and the search provider keyword gets prefixed with @ - @dwds triggers the search_provider request (no extension code is executed) and dwds triggers the event handlers in the background script.

Like mentioned in section 1, in chrome the search_provider seems to get ignored. The omnibox API is working (also with suggestions)

Before releasing a new version for Firefox I hope we can solve the right-click issue.
Maybe I did something wrong with the tests on Chromium, the add-on builds and installs correctly but the functionality currently isn't there (at least on my computer). Is that normal?

Section 2 should clarify this.

@adbar
Copy link
Member

adbar commented Jun 16, 2023

Thanks for the details, here is the new status according to my tests:

  • Add DWDS to the list of search engines
    • Firefox ✔️
    • Chromium ❓ (only present at the bottom in "Some extensions can add search engines to Chrome")
  • Select text + right-click and "search with DWDS"
    • Firefox ✔️
    • Chromium ✔️
  • Omnibox, i.e. shortcuts in address bar (details can be discussed in another PR)
    • Firefox ✔️
    • Chromium ✔️

I'll merge this PR and publish a new Firefox version, for Chrome I'll need to go through the registration process first.

@adbar adbar merged commit efb8cc8 into zentrum-lexikographie:master Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants