Bug 15597 – [GTK] Text not discernable when using a dark Gtk+ style

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: New Bugs

[Alp Toker]

The text in buttons and other UI elements in the browser remains black, even when the background is also black, causing thr text to be illegible.

The fix in RenderThemeGtk will be to extract as much information as possible out of the GTK+ style and modify WebKit's formatting to suit and match what GTK+ would have done.

There is a delicate balance, be we can fix the vast majority of issues quite easily and then work on the subtleties.

[Alp Toker]

Created attachment 23917 [details]
gtkrc demonstrating the issues

Got this from a Debian bug report. Demonstrates the issue, confirmed present in trunk (and has been this way since at least when we switched to gtkmoz RenderTheme, maybe before).

Put this in ~/.gtkrc-2.0 to see the issues, eg:

Widget (button, entry etc.) backgrounds are dark, yet the foreground (text) color is also dark resulting in illegible text.

Text selection color in entries also has no contrast which is no good.

[Alp Toker]

The fix for this is straightforward, but needs a little testing to make sure it works consistently with dark and light themes and matches GTK+ appearances as closely as possible.

What needs to be done to make this work:

In RenderThemeGtk.cpp, we need to adjust the CSS style's colors to match what GTK+ would use.

 void RenderThemeGtk::adjustTextFieldStyle(CSSStyleSelector* selector, RenderStyle* style, Element* e) const
{
...
+        style->setColor(gtkEntry()->style->text[GTK_STATE_NORMAL]);
+        style->setBackgroundColor(gtkEntry()->style->base[GTK_STATE_NORMAL]);
...
}

 void RenderThemeGtk::adjustButtonStyle(CSSStyleSelector* selector, RenderStyle* style, WebCore::Element* e) const
{
...
+    if (gButtonWidget) {
+        printf ("gButtonWidget()\n");
+        style->setColor(gButtonWidget->style->fg[GTK_STATE_NORMAL]);
+        style->setBackgroundColor(gButtonWidget->style->bg[GTK_STATE_NORMAL]);
+    }
...
}

Don't have time to make a patch right now. If someone can pick this up, it'd be great and I'll be happy to review.

[Christian Dywan]

*** Bug 19486 has been marked as a duplicate of this bug. ***

[Yo'av Moshe]

Created attachment 30177 [details]
A fix for gtkEntry

Hey,
I did as you wrote and it indeed fixed the gtkEntry problem.
I had no luck with the buttons though.

Please include at least this fix, and if you can furthur help me with the buttons I'll be glad to help. I have no knowledge of C/C++, but I can learn and this bug annoys me quite a bit...

[Yo'av Moshe]

Created attachment 30178 [details]
This fixes the whole problem

This fixes the whole problem, but I'm not sure it's the Right Way(tm) to do it. I'm using TreeView to get the colors from, which seems to work great with all the themes I tested (dark and light themes).

Please review...

[Christian Dywan]

Comment on attachment 30178 [details]
This fixes the whole problem

Please mark it as '?' if you want it to be reviewed.

[Holger Freyther]

(In reply to comment #5)

> Please review...
>

Please see http://webkit.org/coding/contributing.html and provide a changelog entry.

[Jan Alonzo]

(In reply to comment #5)
> Created an attachment (id=30178) [review]
> This fixes the whole problem
> 
> This fixes the whole problem, but I'm not sure it's the Right Way(tm) to do it.
> I'm using TreeView to get the colors from, which seems to work great with all
> the themes I tested (dark and light themes).
> 
> Please review...

This is not gonna work if the theme suddenly changed from a light to a dark theme or vice-versa. The reason being is that adjust*Styles will not be called when the theme changes hence, the text colors will retain the previous themes color (e.g. changing from Darklooks to Clearlooks will leave the text color using Darklooks's fg color and not Clearlook's). 

[Jan Alonzo]

Comment on attachment 30178 [details]
This fixes the whole problem

This patch requires a ChangeLog. Please see http://webkit.org/coding/contributing.html for more info.

> --- RenderThemeGtk.cppp	2009-05-11 12:27:40.523120385 +0300
> +++ RenderThemeGtk.cpp	2009-05-11 12:27:27.969801589 +0300

Please do a diff in Webkit/ instead of WebCore/platform/gtk next time.

> @@ -272,6 +272,7 @@ void RenderThemeGtk::adjustButtonStyle(C
>          style->resetBorder();
>          style->setHeight(Length(Auto));
>          style->setWhiteSpace(PRE);
> +        style->setColor(gtkTreeView()->style->text[GTK_STATE_NORMAL]);
>          setButtonPadding(style);
>      } else {
>          // FIXME: This should not be hard-coded.
> @@ -306,6 +307,8 @@ void RenderThemeGtk::adjustTextFieldStyl
>      style->resetPadding();
>      style->setHeight(Length(Auto));
>      style->setWhiteSpace(PRE);
> +    style->setColor(gtkEntry()->style->text[GTK_STATE_NORMAL]);
> +    style->setBackgroundColor(gtkEntry()->style->base[GTK_STATE_NORMAL]);
>      adjustMozStyle(style, MOZ_GTK_ENTRY);
>  }

As I said in comment #8, we also need to accomodate for on-the-fly desktop theme changes. 

I'm going to say r- until the issues I raised above gets resolved.

[Yo'av Moshe]

Hey, sorry about the incorrect way of posting the patch. I'm having difficulties doing 'svn checkout' for the whole repo, so I can't quite use the tools making the changelog or the diff. If someone could do that I'll be thankful.

As for the colors not changing when the user changes theme - I think it's a pretty small problem. With this patch, A might see the text color wrong when switching from Darklooks to Clearlooks while using WebKit/GTK, but he'd see it *all the time* when using Darklooks without this patch. Compare the number of people using a dark theme to the number of people switching from a dark theme to a light one while running WebKit/GTK - surely the latter used a dark theme before switching, but most of users a using dark theme don't change it all the time...

I simply have no idea how to check if the user changed the theme, but if this is all we have right now I think it's much better than nothing. Sure it's not complete, but a lot of things are and we should progress one step after another.

Also, sorry if my english is incorrect, I'm breaking my teeth here :) 
Thanks.

[Sergey Alirzaev]

(In reply to comment #5)
> Created an attachment (id=30178) [details]
> This fixes the whole problem
> 
> This fixes the whole problem, but I'm not sure it's the Right Way(tm) to do it. I'm using TreeView to get the colors from, which seems to work great with all the themes I tested (dark and light themes).
> 
> Please review...

The patch doesn't fix the <textarea> font color: i am ending up with black text on black background. Tested on 1.2.5.

[Martin Robinson]

There's another bug tracking the same (or very similar) issue. I'm closing this as a duplicate as that bug has a more in-depth patch and the approach in this patch looks incorrect. The color should not unconditionally override the CSS style.

*** This bug has been marked as a duplicate of bug 40024 ***