openstyles / stylus

Stylus - Userstyles Manager
https://add0n.com/stylus.html
GNU General Public License v3.0
5.29k stars 299 forks source link

Firefox support #52

Closed r-a-y closed 7 years ago

r-a-y commented 7 years ago

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 commented 7 years ago

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:

tophf commented 7 years ago

50 seems to be running on Firefox: zip.

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

r-a-y commented 7 years ago

@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 commented 7 years ago

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

r-a-y commented 7 years ago

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

schomery commented 7 years ago

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

https://github.com/schomery/stylish-chrome/commit/24b82f8bfe2f75865597f0d2b69f112714c15514#diff-b5dfc3ff7c7bccd1bec16cd3bc5f663eL122 fixes this.

schomery commented 7 years ago

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

schomery commented 7 years ago

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

schomery commented 7 years ago

https://addons.mozilla.org/firefox/addon/styl-us/

tophf commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

schomery commented 7 years ago

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

tophf commented 7 years ago

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 commented 7 years ago

sure

narcolepticinsomniac commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

The couple installed beforehand are gone

fixed via 4df35b829b88bfc94b79fddfb5a635ce33de8f6f

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 commented 7 years ago

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 commented 7 years ago

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

schomery commented 7 years ago

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

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

narcolepticinsomniac commented 7 years ago

Hopefully, fixed

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

narcolepticinsomniac commented 7 years ago

can you recheck?

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

schomery commented 7 years ago

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

open "about:debugging" in a browser tab. Point "Load Temporary Add-on" to the manifest.json

schomery commented 7 years ago

btw, we can fix Vivaldi's installation issue as well;

Replacing https://github.com/schomery/stylish-chrome/blob/master/install.js#L5-L6 with

document.addEventListener("stylishInstall", onInstallClicked); // for Firefox
document.addEventListener("stylishInstallOpera", onInstallClicked); // for Opera and Vivaldi
document.addEventListener("stylishInstallChrome", onInstallClicked); // for Chrome and Chromium

document.addEventListener("stylishUpdate", onUpdateClicked);
document.addEventListener("stylishUpdateOpera", onUpdateClicked);
document.addEventListener("stylishUpdateChrome", onUpdateClicked);

fixes the issue. Currently Vivaldi is mapped to the Opera browser instead of Chrome

narcolepticinsomniac commented 7 years ago

OK, I was pretty close, but I didn't know to point it at the manifest. Sideloads disappear once you close the browser? Damn, I thought Chrome was bad with their warnings.

USO installs are good. With Stylish also installed, I get an install confirmation for both, which is better than one or the other like I figured. Ours was underneath the one for Stylish, which isn't great. It'd be better if they were side by side, but I'm not sure how much control we'd have over that. Still, better than I imagined, as it is.

Any idea why there's no tab icon?

tophf commented 7 years ago

Any idea why there's no tab icon?

FF doesn't show favicons on extension pages, apparently. There's only a title with the default UI theme:

pic

tophf commented 7 years ago

Huh, adding <link rel="icon" href="/images/icon/16.png"> reveals the icon.

schomery commented 7 years ago

Okay then let's force to have icon for the manage.html. I am going to fix Vivaldi's installation issue as well.

tophf commented 7 years ago

let's force to have icon for the manage.html

1706f380f221c8ca3b9856035f4bf58e6e21bc3d

schomery commented 7 years ago

Should we release 1.0.8?

narcolepticinsomniac commented 7 years ago

Might as well throw the touched up icons in first. https://github.com/schomery/stylish-chrome/issues/7#issuecomment-295922123

narcolepticinsomniac commented 7 years ago

The odd sizes are missing, but I'm not even sure they're required anymore with the min version.

tophf commented 7 years ago

The odd sizes are missing

The odd sizes are used in Chromium forks (Vivaldi, for example) that still use the classic icon size so they should be kept.

narcolepticinsomniac commented 7 years ago

That's fine. I just meant that them not being touched-up yet isn't that important. I imagine he'll get around to it soon. NBD either way.

narcolepticinsomniac commented 7 years ago

Alright, I replaced the relevant icons. I didn't realize a lot of the colors/sizes he did weren't necessary. https://github.com/schomery/stylish-chrome/commit/f53cd9fe295730f238f2639d450f5265e8d86398

narcolepticinsomniac commented 7 years ago

FF still has the "cannot communicate" issue upon installing. It remains till browser restart. Not the best first impression.

popup

Has it been submitted for review by Mozilla? Maybe it's automatic, IDK.

That trick to force tab icons in FF is negatively affecting Chrome. For whatever reason, it seems to be making Chrome show a duller, blurrier icon. Side by side:

icons

tophf commented 7 years ago

The blurry icon issue should have been fixed yesterday. Are you using the latest master version?

narcolepticinsomniac commented 7 years ago

Nah, the WebStore version. I thought it was current.

Hey we got a write-up on Ghacks: https://www.ghacks.net/2017/05/16/stylus-is-a-stylish-fork-without-analytics/

Not sure FF was ready for the attention. It can't even be installed in regular FF yet because it's unsigned.

narcolepticinsomniac commented 7 years ago

Latest master, same blurry icon. Now I can see the sharper one start to load for a few milliseconds, then the other is loaded. Not sure that was happening before.

tophf commented 7 years ago

Huh, apparently it behaves differently in Chrome depending on UI theme. Okay, I'll make the favicon FF-only.

narcolepticinsomniac commented 7 years ago

Can anyone verify if "advanced settings" for styles are working in FF? I made some tweaks to my editor style for FF compatibility and updated USO. Advanced settings for compact layout still work in Chrome, but not FF. Someone else implemented the settings for me, so I guess I might've updated wrong. Weird that they'd still work in Chrome though, if that's the case.

https://userstyles.org/styles/142097/stylus-gray-matter-w-optional-compact-layout

I'll be offline for a little while, but if anyone could double-check in the meantime, that'd be helpful.

TayliaM commented 7 years ago

Can anyone verify if "advanced settings" for styles are working in FF?

Not working correctly, as far as I have tested. The advanced settings for FF will only download the default settings selected by the author from the Style Settings menu, regardless of the selections the user makes.

narcolepticinsomniac commented 7 years ago

@TayliaM Thanks for confirming.

@schomery Check your email.