Fix text color in non-selected tabs in Firefox
AbandonedPublic

Authored by mthw on Jul 14 2019, 8:54 AM.

Details

Reviewers
ndavis
ngraham
Group Reviewers
VDG
Breeze
Summary

Currently labels on tabs in firefox that are not selected are hard to read (black text on dark background). This fixes it.

BUG: 407639

Test Plan

Labels in Firefox are easy to read on all tabs. No other visual changes.

Diff Detail

Repository
R98 Breeze for Gtk
Branch
fix-firefox-tab-text-color (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13932
Build 13950: arc lint + arc unit
mthw created this revision.Jul 14 2019, 8:54 AM
Restricted Application added a project: Plasma. · View Herald TranscriptJul 14 2019, 8:54 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
mthw requested review of this revision.Jul 14 2019, 8:54 AM
mthw edited the summary of this revision. (Show Details)Jul 14 2019, 8:55 AM
mthw added reviewers: VDG, Breeze.
zzag added a subscriber: zzag.Jul 14 2019, 9:01 AM

I'm not a Breeze GTK maintainer or developer, but perhaps you need to create separate folder for application specific styles, e.g.

src/XXX/apps

and put Firefox styles in a corresponding file, e.g.

src/XXX/apps/_firefox.scss
src/gtk320/widgets/_menus.scss
258 ↗(On Diff #61728)

It'd be better not to hard code the color.

mthw updated this revision to Diff 61729.Jul 14 2019, 9:16 AM

Separate file and avoid hard-conding color.

mthw updated this revision to Diff 61730.Jul 14 2019, 9:18 AM

Fixed duplicate.

mthw updated this revision to Diff 61731.Jul 14 2019, 9:20 AM

Formatting

ndavis added a subscriber: ndavis.EditedJul 14 2019, 9:31 AM

Can you post a picture of what this change is supposed to do?

ndavis added inline comments.Jul 14 2019, 9:35 AM
src/gtk320/apps/_firefox.scss
5

I see that you are hardcoding a color here. Have you tested the change with Breeze Dark?

mthw added a comment.Jul 14 2019, 9:43 AM

Can you post a picture of what this change is supposed to do?

Before: https://imgur.com/a/MqqgUQm
After: https://imgur.com/a/gu5RV6M

mthw marked an inline comment as done.Jul 14 2019, 9:49 AM

Yes I have tried with Breeze Dark, it works correctly.

ndavis accepted this revision.Jul 14 2019, 9:53 AM

Looks good to me.

This revision is now accepted and ready to land.Jul 14 2019, 9:53 AM
mthw planned changes to this revision.Jul 14 2019, 9:54 AM

I seem to have some problems building this, I guess I need to change something in build files. Could you help me find it, I am new at this?

In D22452#495224, @zzag wrote:

I'm not a Breeze GTK maintainer or developer

There is currently no maintainer of breeze-gtk

In D22452#495265, @mthw wrote:

I seem to have some problems building this, I guess I need to change something in build files. Could you help me find it, I am new at this?

How did you get it working in the first place? It builds fine for me, but it seems to do nothing, even when I add @import "apps/firefox"; to src/gtk320/gtk.scss

ndavis requested changes to this revision.Jul 14 2019, 10:50 AM
ndavis accepted this revision as: VDG.
mthw updated this revision to Diff 61733.Jul 14 2019, 11:44 AM

Fixed wrong color name.

mthw added a comment.Jul 14 2019, 11:49 AM

... it seems to do nothing ...

The problem was that I tried to use a color name instead of a hex code and put a wrong name there (still don't know the correct one).

ndavis requested changes to this revision.Jul 14 2019, 12:02 PM

This causes a problem with Breeze and Breeze Dark when the titlebar is enabled.

With Patch

Breeze:


Breeze Dark:

Without Patch

Breeze:


Breeze Dark:

This revision now requires changes to proceed.Jul 14 2019, 12:02 PM
mthw added a comment.Jul 14 2019, 12:18 PM
This comment was removed by mthw.
mthw added a comment.Jul 14 2019, 12:56 PM

Yes, I see it is different, but isn't it better now (after the patch)? https://imgur.com/a/aauVgPp

Whether or not it's better is not relevant for this patch, which right now is only about fixing the text color. It's best not to mix multiple changes like this, unless you want to expand the patch in scope, in which case it should be re-titled accordingly.

Still, I would recommend doing one change per patch, because it makes each one more likely to get in compared to patches with multiple changes (especially when any one of those individual changes may be controversial or require discussion).

cblack added a subscriber: cblack.Jul 14 2019, 1:29 PM

I would probably replace
color: #eff0f1; -> color: t_color(text);
background-color: #475057; -> background-color: t_color(background);
in _firefox.scss to not hardcode the colors

mthw updated this revision to Diff 61740.Jul 14 2019, 1:42 PM

Replace hardcoding

zzag added a comment.Jul 14 2019, 7:21 PM

We should not style server-side decorated Firefox windows. They are fine.

The problem you're trying to solve only affects client-side decorated Firefox windows

:backdrop selector currently doesn't have any effect, see https://bugzilla.mozilla.org/show_bug.cgi?id=1562507.

mthw added a comment.Jul 15 2019, 7:22 AM

Whether or not it's better is not relevant for this patch, which right now is only about fixing the text color. It's best not to mix multiple changes like this, unless you want to expand the patch in scope, in which case it should be re-titled accordingly.

Still, I would recommend doing one change per patch, because it makes each one more likely to get in compared to patches with multiple changes (especially when any one of those individual changes may be controversial or require discussion).

If I did re-title and change the description, would this have a better chance of passing? The chage is so small (number of lines) that It probably wouldn't make sense to split it. If not, then I am stuck and I have clearly bitten more than I can chew.

In D22452#495666, @mthw wrote:

Whether or not it's better is not relevant for this patch, which right now is only about fixing the text color. It's best not to mix multiple changes like this, unless you want to expand the patch in scope, in which case it should be re-titled accordingly.

Still, I would recommend doing one change per patch, because it makes each one more likely to get in compared to patches with multiple changes (especially when any one of those individual changes may be controversial or require discussion).

If I did re-title and change the description, would this have a better chance of passing?

Yes, but then you would still have to convince people that the other included color change is good as well.

The chage is so small (number of lines) that It probably wouldn't make sense to split it.

It's not about number of lines; it's about how many distinct changes it introduces. Imagine a three-line patch that introduces three changes, where one is a bugfix and two of which are potentially controversial visual or behavioral changes. It's a small patch, but it would benefit from being split into three so the uncontroversial bugfix can be committed ASAP, without getting derailed by a discussion of the other issues.

Does that make sense? If the problem here is that you don't know how to separate your bugfix from the visual change, I would encourage you to big into that a bit!

ndavis added a comment.EditedJul 15 2019, 5:02 PM

The latest version of this patch is better because it works well for people using the Breeze GTK theme with the Breeze colorscheme and it doesn't make the Breeze Dark GTK theme worse when combined with the Breeze Dark colorscheme. However, it does cause Breeze GTK to look odd when used with the Breeze Dark colorscheme:

Normally, I wouldn't consider this too much of a problem since people usually want to use the same theme system wide, but sometimes apps that use GTK themes don't play well with dark GTK themes. It is realistic to assume some people who normally use Breeze Dark will use the Breeze GTK theme via the system settings rather than an environment variable for the few problematic apps (GTK_THEME=Breeze app-name-here). Examples of apps that don't play well with dark GTK themes are Eclipse, Intellij IDEA and Firefox (before version 68).

mthw abandoned this revision.Aug 11 2019, 7:05 AM

Someone else is working on this issue now, here: https://phabricator.kde.org/D23079