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

New default coloring #32

Closed
Madis0 opened this issue Mar 25, 2020 · 11 comments · Fixed by #33
Closed

New default coloring #32

Madis0 opened this issue Mar 25, 2020 · 11 comments · Fixed by #33

Comments

@Madis0
Copy link
Contributor

Madis0 commented Mar 25, 2020

As TLS 1.0 and 1.1 are getting disabled (someday), it is time to consider new colors to use to better highlight the differences.

  • Ideally, perfect security must be transparent, therefore I suggest using the same gray color for 1.3 as Firefox itself uses for the padlock (black with opacity 0.6).

  • Mixed protocols could perhaps use an icon crossed with a red line, similar to the broken padlock).
    A simple line should obstruct the number less than the previous warning triangle.

  • Could 1.2 be yellow now? It's not specifically bad until proven that a site uses insecure ciphers and it is still a recommended default. Maybe use a cross for bad configurations and 1.3 coloring for the rest?

  • 1.0 and 1.1 should be bright red while they still exist, but is that enough?

  • Wait, why is SSL 3 still in this extension?

  • Unknown icon could be inverted, as it doesn't give much info anyway.

@jannispinter
Copy link
Owner

Ideally, perfect security must be transparent, therefore I suggest using the same gray color for 1.3 as Firefox itself uses for the padlock (black with opacity 0.6).

Agreed, I like your gray version.

Mixed protocols could perhaps use an icon crossed with a red line, similar to the broken padlock).
A simple line should obstruct the number less than the previous warning triangle.

Agreed, a red line feels more "Firefox-like".

Could 1.2 be yellow now? It's not specifically bad until proven that a site uses insecure ciphers and it is still a recommended default. Maybe use a cross for bad configurations and 1.3 coloring for the rest?

Agreed, TLSv1.2 should be yellow now to push TLSv1.3.

1.0 and 1.1 should be bright red while they still exist, but is that enough?

Yes, let's make them red, they should not be used any more.

Wait, why is SSL 3 still in this extension?

SSLv3 is disabled by default, I'm not sure if support for it was entirely removed in Firefox (e.g. what happens if you set security.tls.version.min to 0). We can remove it, if there is no way in Firefox to re-enable it.

Unknown icon could be inverted, as it doesn't give much info anyway.

Agreed.

@Madis0
Copy link
Contributor Author

Madis0 commented Mar 26, 2020

SSLv3 is disabled by default, I'm not sure if support for it was entirely removed in Firefox

Confirmed that it does not exist in latest Nightly by SSLLabs, so unless you want to support old Pale Moon or something, you can remove this 😁

Agreed, a red line feels more "Firefox-like".

Could you revise and revert this commit then?

@jannispinter
Copy link
Owner

jannispinter commented Mar 26, 2020

The TLSv1.3 icon looks odd when using the (official) dark theme. I think 60% transparency does not work so well here. The padlock actually has a different color in both themes, so removing the transparency and replacing it with a solid gray color does not work either.
grafik
grafik

I'm not sure yet what the best way would be to deal with it. Maybe we need to use different icons depending on the theme.

@Madis0
Copy link
Contributor Author

Madis0 commented Mar 26, 2020

Well, if we did a solid gray color, it could blend in too much depending on the theme (maybe even the official dark one). Perhaps a white border around would help?

@Madis0
Copy link
Contributor Author

Madis0 commented Mar 26, 2020

Actually, why does it matter? As long as the numbers themselves are readable, it doesn't matter if the background is. Because deemphasis was the point.

@jannispinter
Copy link
Owner

Just fixed a few minor issues within the code to make it work better. I like the new icons a lot!

I noticed that the padlock icon uses a red bar that is oriented from top right to bottom left. But the new icons use a red bar from top left to bottom right, should we swap the orientation to match with the padlock?
grafik
grafik

We should also re-add some kind of explanatory text to make it clear why the icon has a red crossed bar. Maybe even just as a tooltip when hovering over the icon.

@Madis0
Copy link
Contributor Author

Madis0 commented Mar 27, 2020

should we swap the orientation to match with the padlock?

Sure, that would make sense. You could even use the original cross by taking it from the SVG chrome://browser/skin/connection-mixed-active-loaded.svg.

We should also re-add some kind of explanatory text to make it clear why the icon has a red crossed bar. Maybe even just as a tooltip when hovering over the icon.

I thought #19 is sufficient, but yes, you can add a tooltip.

@jannispinter
Copy link
Owner

Sure, that would make sense. You could even use the original cross by taking it from the SVG chrome://browser/skin/connection-mixed-active-loaded.svg.

Just updated the icons.

I thought #19 is sufficient, but yes, you can add a tooltip.

For some reason the triangle is not displayed in the current version, will investigate.

@jannispinter
Copy link
Owner

For some reason the triangle is not displayed in the current version, will investigate.

Fixed it. Thank you @Madis0! 🚀

I will test it further and release it tonight.

@532910
Copy link

532910 commented May 2, 2020

grafik
grafik

While white theme looks fine, the black color on the dark one confuses.

I'm not sure yet what the best way would be to deal with it. Maybe we need to use different icons depending on the theme.

It seems so.

@ArchangeGabriel
Copy link

Indeed, I think matching other icons colors would be nicer. So lighter background and dark text.

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

Successfully merging a pull request may close this issue.

4 participants