pop-os / gtk-theme

System76 Pop GTK+ Theme
GNU General Public License v3.0
788 stars 83 forks source link

fix(apps): make calendar labels more legible #488

Closed isantop closed 3 years ago

isantop commented 4 years ago

Makes the calendar event labels white in the dark theme and the FG color in the light theme. This greatly improves the visibility of the labels in the dark theme without making them illegible in the light theme.

Fixes #425

jacobgkau commented 4 years ago

This does slightly improve visibility in the dark theme.

Before:

Screenshot from 2020-11-19 14-00-23 Screenshot from 2020-11-19 14-00-30

After:

Screenshot from 2020-11-19 14-01-01 Screenshot from 2020-11-19 14-01-10

The default calendar (used for the events on the 19th and 20th) with the dark theme gave me a 1:1 contrast ratio before, and a 1.6:1 contrast ratio after. TESTING.md says we're aiming for a 7.5:1 contrast ratio. Since the background colors of the event blocks barely change between the light and dark theme, are we sure it wouldn't make more sense to use black for the labels in both themes? That would improve the contrast ratio of the dark theme to 13.1:1. (It would break the black background color in the dark theme, but this PR as-is breaks the black background color in the light theme. It seems like we've got to pick dark or light unless the label color can be different depending on the background color-- which it looks like the light theme did before this change?)

isantop commented 4 years ago

@jacobgkau I can't make only colored labels black. I can make the event labels black, but it affects all of the labels instead of just the ones with low contrast.

jacobgkau commented 3 years ago

@jacobgkau I can't make only colored labels black. I can make the event labels black, but it affects all of the labels instead of just the ones with low contrast.

I was able to make the dark-colored labels have white text and the light-colored labels have black text:

Screenshot from 2021-01-19 17-13-20

Screenshot from 2021-01-19 17-13-24

There is already logic in GNOME Calendar to style the text differently based on the darkness (or "intensity") of the background color: https://gitlab.gnome.org/GNOME/gnome-calendar/-/blob/442df955c18e9709e624e43dbb8b56b8ba97ffd4/src/gui/gcal-event-widget.c#L287

You were already using the two resulting classes (.color-light and .color-dark), so all I had to do was separate those out. (The reason this wasn't working by default is because .color-light defaults to @theme_fg_color; .color-dark already defaulted to white, which is why the black labels were working before this PR and were broken earlier in the PR.)

Let me know if I'm missing anything.

isantop commented 3 years ago

@jacobgkau With the current changes, non-highlighted calendar labels are black against the dark background:

image

I believe these events are also .color-light which is causing the issue. The problem lies with in inadequate number of selectors in the upstream codebase, which can't be fixed with the theme.

This is why I had had the selectors combined together, because they both need to be visible against both light and dark backgrounds.

jacobgkau commented 3 years ago

Thanks, I hadn't tried an event that's less than a full day yet. I found a solution for that too (see the last commit.)

This is the current state: Screenshot from 2021-01-20 14-39-28 Screenshot from 2021-01-20 14-39-30 Screenshot from 2021-01-20 14-39-33

The only remaining issue I see is the event labels in week view (for non-full-day events), but those were already broken before the PR.

jacobgkau commented 3 years ago

Fixed week view. Current state of dark theme: Screenshot from 2021-01-20 15-02-55 Screenshot from 2021-01-20 15-02-58 Screenshot from 2021-01-20 15-03-00

Light theme: Screenshot from 2021-01-20 15-03-05 Screenshot from 2021-01-20 15-03-07 Screenshot from 2021-01-20 15-03-09

isantop commented 3 years ago

The only remaining thing I see is that this needs to get if'd based on whether it's the light or dark theme. With this commit installed, many labels are now too light to have good contrast against their backgrounds when the light theme is in use.

isantop commented 3 years ago

image

Note the small green events.

jacobgkau commented 3 years ago

Note the small green events.

Are you referring to the New Year's Day and Martin Luther King J... labels? That doesn't appear to be a preset background color. A color picker says it's #73c9a2, and when I plug that into a calendar's color, I get black labels: image How did you get those holidays to appear on your calendar, are they different from regular events? (I tried importing the Google holiday calendar .ical, but the labels showed up normally.)

The only remaining thing I see is that this needs to get if'd based on whether it's the light or dark theme.

It seems like we're trying to avoid doing that. Text with a background color needs to ignore the theme and match its background color. Only labels that don't have a background color should be getting if'd (and from what I've seen, those can use @theme_fg_color since that already changes based on light/dark theme.)

isantop commented 3 years ago

They are coming via Google, specifically the "Holidays in the United States" calendar.

I just checked again, and it appears it's happening on the current version in master too, so I don't consider that a regression. Sorry about that.

I (reasonably) cannot review my own PR, but if no one else on @pop-os/engineering has any objections, we can likely consider this approved on that end.

angelod1as commented 3 years ago

(sorry for being a complete noob, but how can I implement this on my system? @isantop )

isantop commented 3 years ago

It's been merged into master, so it should be available in your regular software updates within a few days.