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

Firefox support #52

Closed
r-a-y opened this issue Mar 21, 2017 · 106 comments
Closed

Firefox support #52

r-a-y opened this issue Mar 21, 2017 · 106 comments

Comments

@r-a-y
Copy link

r-a-y commented Mar 21, 2017

It should be relatively easy to support Firefox now that Firefox supports WebExtensions, which is an API that is compatible with Chrome's model:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Porting_a_Google_Chrome_extension

I tested Stylus briefly (using Chrome Store Foxified to install it in Firefox) and it works pretty good.

One minor thing I've found is the popup that appears when clicking on the Stylus toolbar button has scrollbars.

@tophf
Copy link
Member

tophf commented Mar 21, 2017

I've tried to fix it but the devtools experience in Firefox is just unbelievably terrible for WebExtensions compared to Chrome's, so sorry, I just can't stand it for more than a couple of minutes, probably because I'm a total noob in Firefox devtools. Hopefully other developers don't have this problem (@schomery?).

Things I've noticed that need fixing:

  • popup: .style-name content isn't clipped despite it having seemingly proper text-ellipsis and related rules applied.
  • popup: both style.height = '10px' lines should be removed. Not sure they're needed in Chrome as it seems to be working correctly without them.
  • manage: body { min-height: 100vh; } is needed to fill the entire page in Firefox for drag'n'dropping the backup files onto the page

@tophf tophf added the planned label Mar 21, 2017
@tophf
Copy link
Member

tophf commented Apr 9, 2017

#50 seems to be running on Firefox: zip.
I haven't fixed the text-ellipsis issue yet, though.

@r-a-y
Copy link
Author

r-a-y commented Apr 9, 2017

@tophf - Thanks for the update.

Works fine when I install it as a temporary addon, however when I attempted to self-sign it at addons.mozilla.org for my own usage, clicking on the "Manage" or "Open styles manager" button in Stylus fails.

Here's the validation report:

Unsafe assignment to innerHTML

Warning: None
codemirror/addon/dialog/dialog.js line 24 column 7

'openDialog' called with a non-literal uri

Warning: Calling 'openDialog' with variable parameters can result in potential security vulnerabilities if the variable contains a remote URI. Consider using 'window.open' with the 'chrome=no' flag.
codemirror/addon/search/search.js line 61 column 5

'openDialog' called with a non-literal uri

Warning: Calling 'openDialog' with variable parameters can result in potential security vulnerabilities if the variable contains a remote URI. Consider using 'window.open' with the 'chrome=no' flag.
codemirror/addon/search/search.js line 71 column 24

'openDialog' called with a non-literal uri

Warning: Calling 'openDialog' with variable parameters can result in potential security vulnerabilities if the variable contains a remote URI. Consider using 'window.open' with the 'chrome=no' flag.
codemirror/keymap/emacs.js line 238 column 7

Flagged file extensions found

Warning: Files were found that are either unnecessary or have been included unintentionally. They should be removed.
pull_locales.sh

Unsafe assignment to innerHTML

Warning: None
popup.js line 154 column 5

Unsafe assignment to innerHTML

Warning: None
localization.js line 22 column 5

Unsafe assignment to innerHTML

Warning: None
localization.js line 29 column 3

Unsafe call to insertAdjacentHTML

Warning: None
localization.js line 60 column 11

"storage.sync" can cause issues when loaded temporarily

Warning: This API can cause issues when loaded temporarily using about:debugging in Firefox unless you specify applications > gecko > id in the manifest. Please see: https://mzl.la/2hizK4a for more.
storage.js line 889 column 12

Unsafe assignment to innerHTML

Warning: None
manage.js line 340 column 7

Unsafe assignment to innerHTML

Warning: None
manage.js line 442 column 3

Unsafe assignment to innerHTML

Warning: None
manage.js line 517 column 7

'openDialog' called with a non-literal uri

Warning: Calling 'openDialog' with variable parameters can result in potential security vulnerabilities if the variable contains a remote URI. Consider using 'window.open' with the 'chrome=no' flag.
codemirror/keymap/vim.js line 587 column 36

'openDialog' called with a non-literal uri

Warning: Calling 'openDialog' with variable parameters can result in potential security vulnerabilities if the variable contains a remote URI. Consider using 'window.open' with the 'chrome=no' flag.
codemirror/keymap/vim.js line 3621 column 9

Out of those issues, the main one I believe is the "storage.sync" can cause issues when loaded temporarily one.

Apparently, Stylus will need to use an Add-on ID when using storage.sync:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/WebExtensions_and_the_Add-on_ID#When_do_you_need_an_Add-on_ID

Then, that ID needs to be unique and added to the applications property in the manifest.json file:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/WebExtensions_and_the_Add-on_ID#Developing_and_debugging

Here's some more info about the applications property:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/applications


Update: I attempted to add in the applications property myself and went through the self-signing process again, but the "Manage" button still doesn't work. Now, I'm guessing the problem could be the openDialog issues listed above.

@tophf
Copy link
Member

tophf commented Apr 9, 2017

  • All the warnings above can be ignored because on extension pages innerHTML/insertAdjacentHTML is safe with the default CSP which forbids inline code.
  • openDialog is a CodeMirror thing in the editor, it's not used on manage page.
  • Stylus doesn't need the addon id in general. You, personally, may want to add it in case you plan to use a temporarily loaded addon.

I'll try self-signing at some point in the future, when we're closer to the next version release.

@r-a-y
Copy link
Author

r-a-y commented May 8, 2017

Just updated to v1.0.7 and this fixed the issue with the popup styling in Firefox. Tested with Firefox 53.

@schomery
Copy link
Contributor

haven't fixed the text-ellipsis issue yet, though.

24b82f8#diff-b5dfc3ff7c7bccd1bec16cd3bc5f663eL122 fixes this.

@schomery
Copy link
Contributor

The current master branch works fine in Firefox browser. Let's submit it to AMO and get some feedback.

@schomery
Copy link
Contributor

@tophf and @narcolepticinsomniac if you have Firefox id, please share so that I can add you guys to AMO.

@schomery
Copy link
Contributor

@tophf
Copy link
Member

tophf commented May 14, 2017

I can't install it from AMO, Firefox says it's corrupt apparently because the addon is not signed. Anyway, I don't have an AMO id, nor want to have one as I don't use FF.

@schomery
Copy link
Contributor

I can't install it from AMO, Firefox says it's corrupt

Yes, it is not yet reviewed. But we should add this block to the manifest anyway as Linux version apparently needs it.

  "applications": {
    "gecko": {
      "id": ""
    }
  }

@tophf
Copy link
Member

tophf commented May 14, 2017

"applications" key spams errors in Chromium browsers AFAIK so it's better to have a separate manifest.json.

@schomery
Copy link
Contributor

I have no idea. So it's better to just add it before submission for now.

@tophf
Copy link
Member

tophf commented May 14, 2017

Okay, I've tried adding and it indeed shows as an error on chrome://extensions page. I really don't give a damn about FF so please don't add it in the main manifest.json.

@schomery
Copy link
Contributor

sure

@narcolepticinsomniac
Copy link
Member

It installs in WaterFox. I get a "Cannot communicate with the page" warning in the popup after installing. Browser restart was necessary to get rid of it.

I'm not a huge Firefox fan either, but it's pretty great to have an interchangeable DB backup. Every once in a while, I like to play around with FF to see if it has improved, and importing styles manually was a huge drawback.

I'm checking out my Stylus style in FF, and it just needs a few tweaks. One thing that's not immediately obvious to me is why ::selection doesn't seem to work. Anyone have any clue?

@TayliaM
Copy link

TayliaM commented May 14, 2017

I've installed in portable Dev FF. Same "Cannot Communicate" warning as Narco above when installing. So far:
Disabled Stylish for testing.
the Install button on US.o isn't working.
Copied and pasted 2 styles. Then imported my backup from Stylus Chrome. Very quick. Seem to have lost the 2 I had copied and pasted. Styles are applying fine so far.

@narcolepticinsomniac
Copy link
Member

@TayliaM

Seem to have lost the 2 I had copied and pasted.

Were they lost due to the backup import, you think?

Yeah, I hadn't checked, but installing styles from USO doesn't work. I think we might have another FF specific dilemma there as well. For FF, I think as far as regular styles for websites, users might really prefer Stylus since it has a much nicer UI, it has quite a few extra features, and it applies styles just as well, but it can't affect the browser UI and internal pages like the legacy Stylish. I'd bet that a lot of users who might want to use Stylus for the majority of userstyles, are still gonna want to have the old version of Stylish for the specialized stuff. In which case, there'll be a competition for the USO install button if we don't implement a workaround.

@TayliaM
Copy link

TayliaM commented May 15, 2017

Were they lost due to the backup import, you think?

I am assuming so. They were simply gone as soon as the backup was installed. Same thing when I tried again in nightly.

I'm genuinely loving the idea of one extension for both FF and Chrome type browsers, and agree with your assessment. I've been pretty impressed by Stylus-Chrome so far, so any love given to us diehard FF users is greatly appreciated!

@narcolepticinsomniac
Copy link
Member

narcolepticinsomniac commented May 15, 2017

Copied and pasted 2 styles. Then imported my backup from Stylus Chrome. Very quick. Seem to have lost the 2 I had copied and pasted.

@tophf

I got around to testing this and can confirm that not only does this occur in FF, but in Chrome as well. Fresh install, install a couple styles, and then import a backup. The couple installed beforehand are gone. I'm surprised with all the testing we did, something like that slipped by. I wouldn't think it's all that uncommon a scenario, it's just not a sequence I ever tested personally. I've seen the import successfully ignore preexisting styles so often, I kinda took it for granted that it was always working.

@tophf
Copy link
Member

tophf commented May 15, 2017

That's the original behavior of the backup process: it overwrites an old style with the same id. Not sure what a universal approach might be for such cases. I guess I can try to detect whether the old style and the new one have nothing in common by checking more fields, and assign a new id accordingly (that might introduce some unwanted duplicates but it's easier to delete a duplicate than to restore an overwritten style).

@narcolepticinsomniac
Copy link
Member

Well, that sounds much better than how it's currently handling the scenario. Not sure what fields you're referring to, but if it cross-references the style name and its applies-to rules, duplicates should be minimal. Depends how lenient the check for similarities can be made, which could be more-so with the more fields checked, I suppose.

@narcolepticinsomniac
Copy link
Member

A couple other observations in my brief testing of FF. The tab has no icon, which seems odd:

tabs

Also, when the manage page is loaded, the population of the list of installed styles (for a large DB) is much slower and clumsier in FF. Maybe that's just one of the many charms of FF that fans have learned to settle for. I always want to like FF, but slow and clumsy, and horrible with CPU (especially regarding simple javascript) has always been a deal-breaker. I wonder how v57 will differ in those regards.

@tophf
Copy link
Member

tophf commented May 15, 2017

The couple installed beforehand are gone

fixed via 4df35b8

the population of the list of installed styles (for a large DB) is much slower and clumsier in FF

Yeah. I checked performance timeline in devtools once and it showed such a ridiculous amount of FF overhead code that I immediately gave up as I have no idea how to tackle this.

@narcolepticinsomniac
Copy link
Member

I'm not seeing any difference. Tested a couple times with a style installed from USO (not present in the backup) and a random test style created (nothing even remotely similar in the backup). Both were lost upon importing the backup.

@tophf
Copy link
Member

tophf commented May 15, 2017

Indeed, it was wrong. Hopefully, fixed in 449f86e

@schomery
Copy link
Contributor

schomery commented May 15, 2017

the Install button on US.o isn't working.

@TayliaM can you recheck? This should be fixed with 6119a40

@narcolepticinsomniac
Copy link
Member

Hopefully, fixed

Ran the same couple of tests, and it seems fixed. Definitely a huge improvement anyway.

@narcolepticinsomniac
Copy link
Member

can you recheck?

How the hell do you even sideload an extension in FF?

@narcolepticinsomniac
Copy link
Member

How bout:

Each browser also restricts access to its own extensions gallery (like Chrome Web Store or AMO).

I can switch it up and and remove completed translations if that's good.

@schomery
Copy link
Contributor

Thanks! do we need to remove the completed translations?

@narcolepticinsomniac
Copy link
Member

do we need to remove the completed translations?

For the popup message, I would think so. It's a fairly significant addition to the message. NBD to me personally, just figured they'd need an update. If you think they're close enough, that's fine by me.

@schomery
Copy link
Contributor

That's right. Please remove the completed translations then.

@narcolepticinsomniac
Copy link
Member

Message is edited, and completed translation entries should all be removed.

@narcolepticinsomniac
Copy link
Member

narcolepticinsomniac commented May 25, 2017

@tophf I just tested in Nightly, and the issue with the style list speed on the manage page seems resolved. Pretty damn close to being on par with Chrome anyway.

@schomery I figured 1.0.8.1 contained those bug fixes, but it doesn't. Looks like Stylus has finally been approved by Mozilla, so we might wanna push those.

@BangDroid
Copy link

I see that Stylus is finally available for FF which is awesome. Not able to write and save a new style yet. Save button doesn't seem to work. Also tried importing styles from Chrome, but FF Stylus tab just hangs, Not sure if you guys know about this, or if it's a conflict with my install; vanilla FF up-to-date with NoScript & uBlock Origin.

@narcolepticinsomniac
Copy link
Member

I saw another report like this, but I haven't been able to replicate. Everything works fine. Have you tried without NoScript enabled? I just tried to install NoScript in WaterFox to test, and after installing, it's now crashing the whole browser every time I launch it.

@narcolepticinsomniac
Copy link
Member

Finally got Waterfox working with NoScript. and I already had UBo. I have no issues importing or saving. Double-checked in FF stable portable. Installed NoScript, then UBo, then Stylus. No issues there either.

You're not the only one reporting this though. I saw another comment here. I know NoScript is pretty powerful, but moz-extension is whitelisted anyway. Somebody who's experiencing this will need to figure out what's causing it, if it's ever gonna get fixed.

@narcolepticinsomniac
Copy link
Member

narcolepticinsomniac commented May 29, 2017

@BangDroid You have FF set to "never remember history"? Another user with the same issues figured out that was what was causing it here. Don't know if there's much that we could do about this. Seems like a FF bug to me.

@schomery
Copy link
Contributor

so we might wanna push those.

@narcolepticinsomniac I am preparing the FF version right now.

@BangDroid
Copy link

@narcolepticinsomniac I use FF with the same settings as private mode - but not an actual 'private window'. Changing FF to remember history fixed the issue with Stylus; I can write and import.
I've imported all my styles, and reverted back to my previous settings and Stylus was broken again. So it seems Stylus with anything like private mode is an issue.
ss

Other observations, I opened a Private Window to see if anything differed. The popup is non-functional - none of the buttons work and where it would normally say the URL was blank.

@schomery
Copy link
Contributor

@BangDroid Firefox's storage is pretty messed up at the moment. Chrome provides isolated storage for extensions so even in the private mode, extensions still can read/write to the window.localStorage. In Firefox though they simply can't! and hence Stylus is not able to store its internal preferences. I don't see any workaround to this issue.

@BangDroid
Copy link

@schomery Ok. I have Stylus working now, I had to de-select "never remember history" and use custom setting for history.
I de-selected "Remember my browsing and download history" and Stylus is still working fine even after restart.

@strel
Copy link

strel commented Jun 4, 2017

I found this happening for accented characters on Stylish for Firefox only on that panel message "Stylus doesn't work on pages like this":
stylusbug
@schomery When you update spanish locale, please update all strings to get latests adjusments (unrelated to this).

@schomery
Copy link
Contributor

schomery commented Jun 5, 2017

@strel es locale is updated to the latest; 19754a5

@stonecrusher
Copy link
Contributor

Same goes with german locale. Umlauts "ä ö ü Ä Ö Ü" and sharp s "ß".
And it would be cool if the button resizes according to the label length (I think it is "Verwalten").
screenshot 60

@schomery
Copy link
Contributor

it seems that the localized strings written to moz-extension://812e069f-0f48-1c40-989e-52f1c0055a5f/popup.css have encoding issue. Can somebody opens a bug in bugzilla?

@tophf
Copy link
Member

tophf commented Jun 10, 2017

Added a workaround for the FF bug with CSS messages in 2687d1e

@tophf
Copy link
Member

tophf commented Jun 14, 2017

@schomery, now that FF is supported, can we close this issue? I think new problems should be submitted as new issues. Also, it is possible to add a new version on AMO before it gets their review under the "Versions" at the bottom of the addon page?

@tophf tophf removed the planned label Jun 14, 2017
@r-a-y
Copy link
Author

r-a-y commented Jun 16, 2017

I would say close it. Stylus works well on Firefox.

@PowerWeb5
Copy link

The Stylus dialog for Stylus v1.1.0 shows up with no text for v55.0b2 (x64) (Developer Edition), as seen in this screenshot:
image
I tried changing Themes just in case its white on white text, etc. but to no avail.

@tophf
Copy link
Member

tophf commented Jun 19, 2017

@XeonX1, I can't reproduce in FF55 dev/standard edition nor 56 nightly. Make sure you don't block WebExtensions storage by using incorrect settings in addons like Cookie Monster etc. Or maybe you use the private mode in FF. I have no idea how to disable it so just read the posts above this one. If it won't help, check devtools console for errors, submit a new issue, include those errors if any. It's also weird your screenshot has a window title and frame because in FF the options are shown as a part of the addons manager page.

I'm closing this issue because FF support is a done deal.
New problems should be submitted as new issues.

@tophf tophf closed this as completed Jun 19, 2017
@openstyles openstyles locked and limited conversation to collaborators Jun 19, 2017
@tophf
Copy link
Member

tophf commented Jun 19, 2017

@schomery, please edit the AMO description (also mirrored on about:addons page for Stylus) that links to this thread, and relink to /issues/ URL. This thread has outlived its usefulness.

@schomery
Copy link
Contributor

please edit the AMO description

@tophf done.

@schomery
Copy link
Contributor

it is possible to add a new version on AMO before it gets their review under the "Versions" at the bottom of the addon page?

I think we can submit beta versions too. Not sure if they get signed or not.

@schomery
Copy link
Contributor

The Stylus dialog for Stylus v1.1.0 shows up with no text for v55.0b2 (x64) (Developer Edition), as seen in this screenshot:

@XeonX1 this is not a Stylus issue. You will get this blank UI on all WebExtension option pages. For instance try my Popup Blocker add-on with this "Extensions Options Menu" add-on. You need to report this to "Extensions Options Menu" forum or bug reporter.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests