m-khvoinitsky / dark-background-light-text-extension

Firefox addon that turns every page colors into "light text on dark background"
Mozilla Public License 2.0
361 stars 27 forks source link

Simple attempt to fix #304 #339

Closed hamburger1984 closed 1 year ago

hamburger1984 commented 1 year ago

Update 2: See comment below. For usability we're down to on, off, system preference. (Most OSes include a way to toggle between dark and light mode based on time - so why replicate this in this extension?)

Update: See comment below. This got extended to support off, on, time (night only dark mode) and system-theme.


This change adds an option "Night time only". By default it is false and the current behavior remains untouched.

Setting "Night time only" to true will enable this extension just from 6PM in the evening until 6AM in the morning. During day time (6AM until 6PM) this extension is disabled. On and off times are hardcoded for now, but could definitely become a setting at one point.

Any feedback is welcome, hope this is a first good step towards solving #304.

grafik

20NE commented 1 year ago

Please not based on time but browser's theme, like firefox's default theme is dark/light auto theme.

hamburger1984 commented 1 year ago

Thanks for your suggestion, @20NE . For now I'm trying to fix that issue (not having any automation at all). If you want this done differently, please go ahead and try yourself. Or at least give some hints on how to detect the browser theme from the extension (I had a quick look, but could not find it yesterday). 😉

hamburger1984 commented 1 year ago

I may have found a way to query the system dark mode (which is ≠ the firefox dark theme). So using this as a hint to disable dark mode could work. However firefox does not seem to expose information on whether it considers its theme dark or light.

hamburger1984 commented 1 year ago

Extended the change to offer one more option to select.

grafik

  1. off - globally disabled
  2. on - enabled
  3. time - as described above: start dark mode at 6PM, stop it at 6AM
  4. system - switch dark mode on or off according to system preferences.
m-khvoinitsky commented 1 year ago

Thank you for your contribution!

In order to keep it simple I would keep only "follow prefers-color-scheme" variant in addition to "enabled" and "disabled". I suppose that there are either OS settings to automatically switch dark/light modes or other add-ons which force certain value of prefers-color-scheme based on time, however, I did not researched the subject, so if anyone can confirm this — please, comment.

I will take a more careful look at the code later today but we probably need to think more about UI of the new settings. May be tri-state toggle switch? Plus, current code for browser action rendering is shit (sorry that you had to dig into it), it needs to be rewritten to Svelte (same as preferences) but this is a task for me.

hamburger1984 commented 1 year ago

Hey @m-khvoinitsky, Sure - reducing this to three options is a good idea to keep this usable.

grafik

The action rendering wasn't too bad - I just did a small refactoring to have less indentation. To me creating a list of options was painful (enums, const assertions, string literal union types .. they all seemed to allow some but not all I needed). But that feels more like a TypeScript/JavaScript limitation (or too little experience with those languages on my side).

I'm looking forward to seeing some kind of an automation hitting the "real" extension. If I can support in that process - tell me. I tried to keep my changes readable.. ;-)

siffegh commented 1 year ago

@hamburger1984 - would you be able to help with https://github.com/m-khvoinitsky/dark-background-light-text-extension/issues/230?

hamburger1984 commented 1 year ago

@siffegh we will see.. Right now I'm basically scratching my own itch in my free time here. Export and import of settings is no issue for me at the moment. Maybe you could have a look yourself? It might not be too hard 🤗

siffegh commented 1 year ago

@siffegh we will see.. Right now I'm basically scratching my own itch in my free time here. Export and import of settings is no issue for me at the moment. Maybe you could have a look yourself? It might not be too hard hugs

OK, thanks. I've never done this sort of coding. Likely am capable, but not willing to learn - too many other things to get done in my old age :-)

m-khvoinitsky commented 1 year ago

I just did a small refactoring to have less indentation

Yeah, that's better, thanks. And it's good that you've done it in separate commit. Some of work-in-progress kind of commits should be squashed before merge, IMO, but this should be separate. However, I'd like to ask you not to make pure style changes (like added whitespaces) even if they make a better code — I think I should bring prettier to the project so such kind of discussion would not be a thing at all. As for now, let's not pollute blame.

To me creating a list of options was painful (enums, const assertions, string literal union types .. they all seemed to allow some but not all I needed). But that feels more like a TypeScript/JavaScript limitation (or too little experience with those languages on my side).

My first thought was to make it const enum (may be even with explicit number values to avoid accidental breakage of stored preferences in future versions). But you'd have to have a separate map of human-readable options in preferences which is not a problem. Have you considered this?

Before the general public release I'd like to port browser action code to Svelte which would make it much simpler to implement more complex logic which is needed. Like, for example, replacing dropdown with tri-state slider with hints, explaining to the user that their system has light theme hence the add-on seems not to work, etc. I think I'll be able to do this somewhere in mid-December.

Also, should all open tabs be automatically re-processed upon system theme change or only after page reload? I'd say that it should re-process everything automatically but it may cause some issues like broken pages or CPU usage spikes.

Last but not least, old value of old "enabled" option should be migrated to the new one.

hamburger1984 commented 1 year ago

Hey @m-khvoinitsky, thanks for that in-depth response.

I'm going to have a look at that "enabled" migration. If I find time and come up with a solution, I'll add it to this PR.

Regarding svelte porting: Do you already have a branch where this is on-going (so I could maybe already take that into my branch?). Else, I'll wait for mid-December and continue then :)

hamburger1984 commented 1 year ago

Not sure I'll continue with this. So if somebody wants to pick it up, please do - I'll close it for now.