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

Provide Firefox version of Gitcoin Chrome Extension #1

Open
gasolin opened this Issue Sep 28, 2017 · 44 comments

Comments

Projects
None yet
10 participants
@gasolin
Copy link
Collaborator

gasolin commented Sep 28, 2017

Firefox now provide web extension API which is similar to Chrome
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Porting_a_Google_Chrome_extension

And MetaMask already provided the Firefox extension
https://addons.mozilla.org/en-US/firefox/addon/ether-metamask/?src=api

@owocki

This comment has been minimized.

Copy link
Member

owocki commented Sep 28, 2017

this is a great idea. if anyone wants to fund a bounty for it or dev it, i'm all 👂 s

@owocki owocki referenced this issue Nov 27, 2017

Merged

Browser Extension Beta #5

7 of 7 tasks complete
@owocki

This comment has been minimized.

Copy link
Member

owocki commented Dec 19, 2017

is this easy to do?

@owocki owocki changed the title provide Firefox version Provide Firefox version of Gitcoin Chrome Extension Dec 20, 2017

@owocki

This comment has been minimized.

Copy link
Member

owocki commented Dec 20, 2017

funding this now.

requirements:

  • a way (preferably an automated script) to build the current codebase for firefox.
  • (just porting the existing codebase to firefox is not acceptable, as ongoing changes to upstream chrome extension will need to be merged downstream into firefox on an ongoing basis)
  • deliver all assets for submission to firefox add-on store ( https://addons.mozilla.org/en-US/firefox/ )

if anyone feels like the issue is underpriced leave a comment and we can work it out

@gitcoinbot

This comment has been minimized.

Copy link
Member

gitcoinbot commented Dec 20, 2017

This issue now has a funding of 0.05 ETH (39.7 USD) attached to it.

  • If you would like to work on this issue you can claim it here.
  • If you've completed this issue and want to claim the bounty you can do so here
  • Questions? Get help on the Gitcoin Slack
  • $11187.5 more Funded OSS Work Available at: https://gitcoin.co/explorer

@owocki owocki added the ports label Dec 20, 2017

@Kielek

This comment has been minimized.

Copy link

Kielek commented Dec 20, 2017

I suppose not all functions are compatible with Firefox, see compatibility report based on: https://www.extensiontest.com/test/b105f3f0-e5b8-11e7-8ba1-af95702cad20

{ "compat": [ { "message": "extension.onMessage is not supported", "locations": [ { "file": "script/background.js", "line": 1 } ] }, { "message": "extension.sendMessage is not supported", "locations": [ { "file": "script/pageload/functions.js", "line": 9 } ] } ], "errors": [ { "message": "Banned 3rd-party JS library", "locations": [ { "file": "script/popup/jquery.js" } ] } ], "warnings": [ { "message": "Unsafe assignment to innerHTML", "locations": [ { "file": "script/pageload/functions.js", "line": 4 } ] } ] }

@owocki

This comment has been minimized.

Copy link
Member

owocki commented Dec 21, 2017

http://bits.owocki.com/1e1z0o0f2E0a/Screen%20Shot%202017-12-20%20at%205.06.36%20PM.png

i get a 404

based upon the result you posted, it seems that theres a few things we'll have to update :)

@gasolin

This comment has been minimized.

Copy link
Collaborator

gasolin commented Dec 21, 2017

Finally got some time to deal with this...
Sent PR to make the extension compatible with Firefox Extension #19

And uploaded it to mozilla addon
https://addons.mozilla.org/en-US/firefox/addon/gitcoin/

@owocki

This comment has been minimized.

Copy link
Member

owocki commented Dec 22, 2017

@gasolin thank you!

https://addons.mozilla.org/en-US/firefox/addon/gitcoin/ returns a 404 for me. is it live yet? i'd love to review the listing before it's live.

can you give me permissions to manage the extension on addons.mozilla.org ?

@step21

This comment has been minimized.

Copy link

step21 commented Dec 22, 2017

Is the bounty still live for 'providing a script' or similar instead of just porting? Tried claiming this or one of the other extension bounties but somehow always ran out of gas or something else... for one of the bounties metamask suggested 200 $ equiv in gas ... if anything I would want to earn ether, not spend the little that I have in metamask atm... I wouldn't mind waiting a bit longer until it is included, but is it somehow still possible to pay at most 1$/1€ equivalent or not?

@gasolin

This comment has been minimized.

Copy link
Collaborator

gasolin commented Dec 22, 2017

@owocki 0.10 is drawn from reviewer because of the origin code inject JS via innerHTML. I made an extra patch to use browser.tabs.executeScript API instead of innerHTML and update 0.11. Now all lint warning are gone and is listed https://addons.mozilla.org/en-US/firefox/addon/gitcoin/

I also saw some issues but may not block this work:

  • match pattern in manifest.json matched all web site, but maybe this extension just need to deal with github?
  • chrome debugger report XMLHTTPReqest is synchronous, could be write in async way

Could fix them when the main file arch is settled

@owocki

This comment has been minimized.

Copy link
Member

owocki commented Dec 22, 2017

match pattern in manifest.json matched all web site, but maybe this extension just need to deal with github?

probably good to fix this

chrome debugger report XMLHTTPReqest is synchronous, could be write in async way

i could go either way on this. what do you think?

@owocki

This comment has been minimized.

Copy link
Member

owocki commented Dec 22, 2017

Is the bounty still live for 'providing a script' or similar instead of just porting?
@gasolin do you feel like your PR covvers this? or should cover it?

@owocki

This comment has been minimized.

Copy link
Member

owocki commented Dec 22, 2017

strangely... when i install this extension, it doesnt show up in the menu bar... so im not sure how to test it http://bits.owocki.com/3x0v3k443N2U/Screen%20Recording%202017-12-22%20at%2011.29%20AM.mov

@step21

This comment has been minimized.

Copy link

step21 commented Dec 23, 2017

Mmmh, seems to work fine for me. Maybe just some loading error or not added automatically? If you go to 'customize' is it there?

@gasolin

This comment has been minimized.

Copy link
Collaborator

gasolin commented Dec 23, 2017

It should shows in the menu bar...
The PR provided npm script for build and lint, others can use it to build different browser extensions.

@gasolin

This comment has been minimized.

Copy link
Collaborator

gasolin commented Dec 25, 2017

match pattern in manifest.json matched all web site, but maybe this extension just need to deal with github?

probably good to fix this

chrome debugger report XMLHTTPReqest is synchronous, could be write in async way

i could go either way on this. what do you think?

I think these 2 issues are not directly related to this issue and does not block publish the addon. So its better to file separate issues to solve them.

@owocki

This comment has been minimized.

Copy link
Member

owocki commented Dec 27, 2017

hmm now i can't install it from the add on store because of this error .

http://bits.owocki.com/2v3c2n0j101R/Screen%20Shot%202017-12-26%20at%205.23.25%20PM.png

@owocki

This comment has been minimized.

Copy link
Member

owocki commented Dec 27, 2017

If you go to 'customize' is it there?

ahh yes there it is! disregard the above

@owocki

This comment has been minimized.

Copy link
Member

owocki commented Dec 27, 2017

@rafaelfragosom

This comment has been minimized.

Copy link

rafaelfragosom commented Dec 27, 2017

@gasolin @owocki I'm currently working on a cross-browser solution for WebExtensions API and it will work with any browser supporting the API, including Opera, Brave, Firefox and Edge.

Not sure how to proceed since you've made the PR to add Firefox to the list.

@gasolin

This comment has been minimized.

Copy link
Collaborator

gasolin commented Dec 27, 2017

@rafaelfragosom I guess the major porting work will be related to Firefox https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Chrome_incompatibilities , since the patch applied webextension-polyfill, you can help extend package.json to add build scripts for Opera, Edge..etc and check if it still works well on Chrome and other browsers who share the same chrome API

@rafaelfragosom

This comment has been minimized.

Copy link

rafaelfragosom commented Dec 28, 2017

@gasolin Sure thing.

@owocki Can you accept the PR? I'm gonna start working on the other browsers.

@owocki

This comment has been minimized.

Copy link
Member

owocki commented Dec 28, 2017

@rafaelfragosom are you talking about this PR? #19

@gasolin it has a bunch of merge conflicts. can you tackle those? also its still labeled as "WIP" fwiw.

@gasolin

This comment has been minimized.

Copy link
Collaborator

gasolin commented Dec 29, 2017

@rafaelfragosom PR 25 landed the basis structure which support npm script
#25

You can base on current master branch and add new browser build script without wait for the firefox addon support

@owocki

This comment has been minimized.

Copy link
Member

owocki commented Dec 29, 2017

from mozilla


Your add-on Gitcoin has been reviewed and one or more of of its versions have been disabled due to critical issues discovered.

Details:
This add-on is creating DOM nodes from HTML strings containing potentially unsanitized data, by assigning to innerHTML, jQuery.html, or through similar means. Aside from being inefficient, this is a major security risk. For more information, see https://developer.mozilla.org/en/XUL_School/DOM_Building_and_HTML_Insertion . Here are some examples that were discovered:
popup/index.js#192 and others

Version(s) affected and disabled:
0.12
@gasolin

This comment has been minimized.

Copy link
Collaborator

gasolin commented Dec 29, 2017

That is caused by Firefox addon has a more strict policy for using innerHTML.

We should remove jquery.append and construct the DOM with createElement instead
https://github.com/gitcoinco/chrome_ext/blob/master/src/script/popup/index.js#L192

I'll file separate bugs for those improvements

@vs77bb

This comment has been minimized.

Copy link

vs77bb commented Jan 17, 2018

@gasolin Bumping this... any update / does it make sense to claim this one? cc @owocki

@gasolin

This comment has been minimized.

Copy link
Collaborator

gasolin commented Jan 18, 2018

Ooh I should link related bugs to this issue

  • merged
    #25 move extension into src/ folder
    #26 simplify body flow
    #32 scope content scripts to github domain only
    #30 construct the DOM with createElement instead of jquery.append
@KennethAshley

This comment has been minimized.

Copy link

KennethAshley commented Feb 17, 2018

@owocki I think I fixed this issue. checkout my PR.

@KennethAshley KennethAshley referenced this issue Feb 17, 2018

Open

Firefox #45

@KennethAshley

This comment has been minimized.

Copy link

KennethAshley commented Feb 17, 2018

Here is the extension working on Firefox. https://addons.mozilla.org/en-US/firefox/addon/kenneth-nicholson/

@KennethAshley

This comment has been minimized.

Copy link

KennethAshley commented Feb 17, 2018

Addon tests pass
screencapture-addons-mozilla-org-en-us-developers-upload-ba235c44a9c84d978675cd23adfe4635-1518832188703

@vs77bb

This comment has been minimized.

Copy link

vs77bb commented Feb 21, 2018

cc @owocki. Great work @KennethAshley you're an animal 🦁!!!

@KennethAshley

This comment has been minimized.

Copy link

KennethAshley commented Feb 24, 2018

@vs77bb thanks dude. Let me know if you see anything that needs fixing.

@gasolin

This comment has been minimized.

Copy link
Collaborator

gasolin commented Feb 27, 2018

@KennethAshley nice work, the tag in page shows correctly, though due to api incompatibility, the gitcoin badge in toolbar is not linked.
(
compare to chrome versin:

  1. should show the funding issues number as notification over the icon
  2. the project filter should apply onto the list after click the icon
    )

I'm occupied in other works, feel free to continue the good progress. I'll be happy to help review the PR.

@owocki

This comment has been minimized.

Copy link
Member

owocki commented Mar 2, 2018

just installed via the mozilla adds on link that @KennethAshley posted above...

the only issue i see:

  • Metamask not detected. Please install metamask to proceed displays even if metamask is installed
@KennethAshley

This comment has been minimized.

Copy link

KennethAshley commented Mar 3, 2018

@owocki yea i gotta look into this issue. Meta mask works after awhile but not initially.

@maqqju

This comment has been minimized.

Copy link

maqqju commented Mar 3, 2018

Hi all,

I downloaded the extension and tried to install it to FF. It seems like it's working. Is the problem specifically on not finding metamask now? @owocki @KennethAshley

@owocki

This comment has been minimized.

Copy link
Member

owocki commented Mar 5, 2018

that was the problem i experienced.. yes...

perhaps it doesnt matter too much though since metamask / web3 is not really used in the extension too much (although down the line we'd love to do inline web3 tx's in the extension)

@KennethAshley

This comment has been minimized.

Copy link

KennethAshley commented Mar 5, 2018

@owocki @maqqju I'm gonna continue to look at the interaction between the extension and metamask.

@owocki

This comment has been minimized.

Copy link
Member

owocki commented Mar 5, 2018

IIRC; the interaction between the extension and metamask was pretty kludgey... there are different execution instances in the main chrome window and the browser window, and in order to get them to talk to each other i had to do some really hacky stuff..

the more i think about it.. the more i think we might want to just remove the metamask functionality for now.. all it does is give you your ETH balance and your current ETH address ( http://bits.owocki.com/1Y070D251Y2L/Screen%20Shot%202018-03-05%20at%203.33.59%20PM.png ) which i dont think is value add enough to hold up the rest of the ticket

@owocki

This comment has been minimized.

Copy link
Member

owocki commented Mar 8, 2018

@KennethAshley what do you think about just removing the metamask warning for firefox and 🚢 ing it?

@KennethAshley

This comment has been minimized.

Copy link

KennethAshley commented Mar 8, 2018

@owocki Yea ill get that done today!

@owocki

This comment has been minimized.

Copy link
Member

owocki commented Mar 13, 2018

lmk !

@nemaniarjun

This comment has been minimized.

Copy link

nemaniarjun commented Jul 5, 2018

Update on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment