Skip to content
This repository has been archived by the owner on May 30, 2024. It is now read-only.

Firefox support #70

Merged
merged 2 commits into from
Sep 6, 2018
Merged

Firefox support #70

merged 2 commits into from
Sep 6, 2018

Conversation

salmanulfarzy
Copy link
Contributor

@salmanulfarzy salmanulfarzy commented Sep 5, 2018

Saw #54 and still making a PR to support Firefox since the changes are minor.

  • Dropped :-webkit-any(a, span) selector which was introduced with 298ab68 and had incompatibility with Firefox. Firefox only supports :-moz-any() and not the newer :matches() pseudo-class.
  • Firefox supports chrome namespace and it was used for storage
  • Storage changed from sync to local (sync wasn't working with testing, Anyway to fix this ?)

Changes tested with Firefox 62 and Chromium 68.

If maintainers don't want to support Firefox because of maintenance burden, I would be happy to Firefox support here or submit it as separate extension on AMO under different name. Really wish this would be accepted here because separate extension will most likely lead to code syncing and update issues.

@salmanulfarzy salmanulfarzy mentioned this pull request Sep 5, 2018
@rugk
Copy link

rugk commented Sep 5, 2018

(sync wasn't working with testing, Anyway to fix this ?)

In general it does work. Maybe you did notw ait long enough or such stuff... In general I can say, it works.

extension/api.js Outdated
}),
set: obj => {
chrome.storage.sync.set(obj);
chrome.storage.local.set(obj);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The selector can change.

This sync cannot change; it breaks the option syncing in Chrome. If it doesn't work open an issue in bugzilla.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted local storage.

Prefixed -any() selectors are incompatible across browsers. Haven't got
a way to make newer :matches() to play nice with nested selectors.
@salmanulfarzy
Copy link
Contributor Author

storage.sync requires add-on id on manifest.json, Will updated with that.

@fregante
Copy link
Collaborator

fregante commented Sep 6, 2018

Excellent! Thanks for finding that info.

I think Sindre's main gripe with Firefox is AMO's strict reviewers, so as long as we don't have to publish it to AMO and don't have to make sacrifices for it we can "support" it.

@sindresorhus what do you say?


Linting errors fixed in #71

@sindresorhus sindresorhus merged commit 6b86d1a into sindresorhus:master Sep 6, 2018
@salmanulfarzy
Copy link
Contributor Author

Can we setup auto publishing to AMO with travis similar to refined-github ?

@fregante
Copy link
Collaborator

fregante commented Sep 6, 2018

I think Sindre's main gripe with Firefox is AMO's strict reviewers, so as long as we don't have to publish it to AMO and don't have to make sacrifices for it we can "support" it.

@rugk
Copy link

rugk commented Sep 6, 2018

You don't have to, if it is done automatically... 😉

Anyway, I think reviews on AMO are quite fast and so on nowadays. (before ff v57 it was sometimes a lot more delayed) So as long as you abine by the rules (and don't include minified custom stuff without prividing corresponding source code), it should be all well.

@salmanulfarzy
Copy link
Contributor Author

AMO review process has improved considerably after webext. I saw you guys encounter some deployment issues with RGH on AMO and solving it. This extension isn't complex as RGH so thought this won't be an issue and published it when this was merged, Excuse me for that.

I'll remove the listing and move to new name ASAP. Hope you would reconsider publishing to Firefox.

@fregante
Copy link
Collaborator

fregante commented Sep 6, 2018

Manual reviews still happen and developers still get bothered by reviewers and the extension can get blocked. Automatic deployment solves absolutely nothing. I don’t blame Sindre for not wanting to put up with it when you can just use Foxified to install from the Chrome Web Store.

@rugk
Copy link

rugk commented Sep 6, 2018

I am glad that these reviews are done as some recent events show. Also, as a dev you can avoid it just by keeping to the rules. Once it is in, it is unlikely to get blocked unless you include malware or such stuff. So automatic deployment makes totally sense, IMHO.

@salmanulfarzy
Copy link
Contributor Author

Understand the sentiment behind not wanting to publish in AMO. Still pushing it because Foxified has been discontinued and not working on latest versions of Firefox.

@matsest
Copy link

matsest commented Nov 22, 2018

I saw the published add-on on Mozilla's site -- is it any reason why it's only for Firefox >= v62? Any way of easily adding it to v60?

@salmanulfarzy salmanulfarzy deleted the firefox branch March 8, 2019 06:47
@fregante fregante mentioned this pull request Mar 19, 2019
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants