browserpass / browserpass-otp

OTP functionality for browserpass
ISC License
24 stars 2 forks source link

Add dark theme support #20

Closed apiraino closed 2 years ago

apiraino commented 4 years ago

fixes #15

In order to reproduce:

in about:config set:

browser.in-content.dark-mode    true
widget.content.allow-gtk-dark-theme    true
browser.display.use_system_colors    true

and start firefox with GTK_THEME=Adwaita:dark firefox

To test dark theme, add ui.systemUsesDarkTheme=1 in about:config

maximbaz commented 4 years ago

I wonder if this style should be applied globally on body instead? Haven't tested if it will work though.

I think it won't fix #15, it will make the text color readable (which is great!), but #15 is requesting dark theme support (i.e. dark background and white text color).

apiraino commented 4 years ago

@erayd @maximbaz pinging for reviewing the latest changes :-)

maximbaz commented 4 years ago

I am fine with this approach 👍

apiraino commented 4 years ago

@erayd hi, any comment here? :-)

apiraino commented 4 years ago

Ok, I did some research and in 637fb48 there is my proposal for the colors, here's how they look like (on FF).

OTP generated

otp

No token

no-token

OTP being clicked

clicked

Note: I didn't like how the copy.svg looked too dark on the dark theme so I've copypasted it into something matching the text color. Unfortunately I'm not great with any kind of graphics so the resulting SVG is 2.5x bigger than copy.svg :-) If anyone more skilled can improve on this, that'd be great.

This said, how does it look like?

apiraino commented 4 years ago

@erayd would this PR make it in time before the OTP extension merges into browserpass?

EDIT: if you're interested in what this PR brings but prefer to cherry pick from the commits, feel free to do so :+1:

erayd commented 4 years ago

@apiraino The change to Browserpass is imminent - so there's really not much point merging this now I think - but I do like the idea of having it automatically follow the light / dark theme setting of the browser. So let's keep this open, as we may want to bring pieces of it in over there.

If you are curious, the PR for Browserpass is browserpass/browserpass-extension#229.

apiraino commented 4 years ago

@apiraino The change to Browserpass is imminent - so there's really not much point merging this now I think - but I do like the idea of having it automatically follow the light / dark theme setting of the browser. So let's keep this open, as we may want to bring pieces of it in over there.

If you are curious, the PR for Browserpass is browserpass/browserpass-extension#229.

Ah thanks for the link! Good to see it progressing :+1:

Then I assume I can live with this a few days more :-)

2020-09-09_101211

(hint: strange formatting of the deprecation warning)

erayd commented 4 years ago

@apiraino Thursday 17th.

Wow that looks bad - seems Firefox is being a bit silly with it. I just stuck a 280px min-width on it and tested in Chrome, which was fine... but that is just ridiculous. I should have checked in Firefox as well apparently. I'll roll another release to tidy that up.

erayd commented 4 years ago

@apiraino v0.2.5-dev is now released on the Firefox webstore, with a fixed width.

apiraino commented 4 years ago

@apiraino v0.2.5-dev is now released on the Firefox webstore, with a fixed width.

Looks better now - thanks :-)

2020-09-09_122722

For completeness I should have mentioned that I am on Ubuntu 19.x and Firefox 81.0b8

erayd commented 4 years ago

I should have tested it properly first! Thinking "it's just a three-line text notice; testing in Chrome is enough" was clearly sheer hubris on my part.