-
-
Notifications
You must be signed in to change notification settings - Fork 47
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
feat: WIP support for user style overrides. #239
base: master
Are you sure you want to change the base?
Conversation
Status
Anything else that should be added here? Safety Verification
Any other security-related tests to add? |
I think that covers it! On the UI front, what do you think of putting it in a dialog or maybe even in a separate page (double secret advanced settings?) and linking/adding a button on the main settings page? Also, what do you think of syncing the CSS but limiting the size to something that would fit in synced storage (like 10KB)? Would avoid the need to manually sync between browsers. Just as a point of comparison, the entire set of Less files under |
Great! I've looked around and it doesn't seem like there's a clear way to communicate with Mozilla and ask them if doing something is okay. I haven't had a chance yet, but I plan to look into the forums/Matrix channel they link to. I'm not sure if that's an avenue that will get an official response, but I will at least try. If I can't get an official response would showing that other popular tab-handling addons already have this feature without it (apparently) causing issues work? For example: Tree Style Tab has a section on CSS customizations: https://github.com/piroor/treestyletab/wiki/Code-snippets-for-custom-style-rules#for-version-20-and-later (you can see from the screenshot at the start of that section there aren't even really warnings/disclaimers and it's visible without "expert options" being unlocked too.) Sidebery has this feature as well: https://github.com/mbnuqw/sidebery/wiki/Sidebery-Styles-Snippets — it's just a text box you can dump CSS into, there are no warnings or anything at all just a prompt to "write custom CSS here". Those are the two I can recall off the top of my head, but I know I've seen other addons with that feature as well.
Sure, whichever you'd prefer! The only thing I'd probably suggest against is a dialog since part of iterating on making changes is observing how they look under different conditions (like expanding/closing folders, triggering a tab to load, etc). A dialog could make interacting with the rest of the browser difficult, at least if it was handled like a modal.
I think it's reasonable to disable syncing at a a certain point, but I'd prefer not to have a hard limit that would affect people who aren't even using sync. I personally don't use it at all. Just for reference, the changes I posted over in #232 come to over 3k and they're generally pretty simple. I think running into a 10k limit would be pretty easy if someone trying to make a real theme, especially if they were doing stuff like including icons in Maybe have it as a non-synced preference but include something like a |
Since there seems to be precedent elsewhere (even amongst Recommended extensions), we can probably ask forgiveness instead of permission. ;) I can make a point of highlighting it in the reviewer notes when I submit. I think as long as we take care to follow the "no surprises" rule and people are warned about the risks, it would likely be accepted.
The modality of the dialog would have to be limited to the options page (you could still interact with other windows/tabs/the sidebar), but I think I would lean towards a separate page anyway for a few reasons:
Seems like a good approach to me. Maybe it could even be automatic below the size cutoff (with an appropriate warning displayed above the cutoff). Then you get the best of both worlds--automatic syncing where possible, and a clear warning when not. |
I agree with that. The rest also sounds reasonable to me. I'll look into actually trying to implement it when I get a chance, but I'm not really sure when that will be. Hopefully some time this month. |
Proof of concept for allowing user style overrides.
Adds more CSP policies to further restrict the stash-list page privileges. Unfortunately I don't see a way to get around
style-src 'unsafe-inline'
part. Ideally a nonce or something would be used instead, but thestash-list.html
page is static. The other policies should prevent anything like loading/importing remote resources.The preferences part is really rough, although it is usable. Obviously it would need actual warnings for the dangers rather than those placeholders.
These styles only affect the stash-list page, so it's not possible to do something like make the options page unusable. Even something like
html { display: none !important; }
removes all content from the stash-list but can easily be reverted from the options.