Bug 37779 – [GTK] Button and Entry font color doesn't respect the system's style

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Olivier Tilloy]

Created attachment 53640 [details]
A screenshot of the issue

This bug is the GTK-specific counterpart of bug #35024.

This is particularly visible with a dark theme such as Nodoka-Midnight. The text color of buttons and text entries in forms doesn't respect the system's style. In Nodoka-Midnight, the text color for buttons is very light in contrast with the background color which is very dark. However in WebKitGTK the labels of buttons are rendered using the default color (defined by RenderTheme::systemColor), which is black (CSSValueButtontext), resulting in unreadable buttons. The same applies to text entries, although the code path may be different.

The attached screenshot should be quite self-explanatory.

[Olivier Tilloy]

In fact it also looks like the background color of buttons and text entries is incorrect vs the system's style.

[Olivier Tilloy]

Created attachment 53663 [details]
A preliminary patch

This is a preliminary patch that fixes the font color of buttons.
Unfortunately I could figure out how to extend the patch to text inputs and areas, it looks like the code path is quite different. Any input/feedback is welcome.

Please note that this patch breaks at least one layout test (fast/forms/button-style-color.html). This is because the expected layout hard codes the colors of the buttons, which is obviously going to break if such colors depend on the current GTK style.

[WebKit Review Bot]

Attachment 53663 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/gtk/RenderThemeGtk.cpp:590:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Olivier Tilloy]

Created attachment 53664 [details]
A preliminary patch

Updated patch to fix coding style issues.

[Olivier Tilloy]

Created attachment 54065 [details]
A complete patch

This is an updated patch that fixes the issue for all input widgets.
Please note that it predictably breaks the following layouts tests which will need to be adapted or skipped:

  editing/selection/select-box.html
  fast/css/text-input-with-webkit-border-radius.html
  fast/forms/button-style-color.html

Known limitation: when the theme is changed on the fly, the text of the input widgets is not re-rendered, so the font color is wrong. This is not a regression though, it was already the case. Either the functionality to trigger a re-rendering is missing in webkit itself, or I couldn't locate it. For comparison, firefox does re-render.

Comments welcome!

[Gustavo Noronha (kov)]

Comment on attachment 54065 [details]
A complete patch

In general the change looks good to me, but the change to html.css makes me nervous, since it is likely used for all platforms, and we don't want to change their behaviour. Let's try to get someone who knows about that to look at the change.

[Gustavo Noronha (kov)]

I think Hyatt could lend us a hand regarding the CSS change? =)

[Olivier Tilloy]

Indeed the patch likely impacts all the other ports, and depending on the expected outputs possibly breaks the same layout tests I mentioned for them as well.

However the changes to html.css are consistent with what was already there:
the color for buttons was set to "ButtonText" (which ensures platform-specific theming) in contrast with text input for which it was set to "initial". And the text color for dropdown lists (select and keygen) was hard-coded to "black", which doesn't seem right.

I suppose the layout tests could be adapted by styling the input elements to ensure the resulting color are independent from the platform.

[Olivier Tilloy]

Dave commented that the patch looks fine, we now need to check with the Chromium team that the change is ok there too:

  [dhyatt] change looks fine to me, but i'd check with chromium guys too
  [dhyatt] make sure CaptionText is right for their platform

[Shinichiro Hamaji]

Looks like windows chromium only uses RenderTheme::systemColor().  RenderThemeChromiumWin::systemColor() uses GetSystemColor(COLOR_CAPTIONTEXT) as the color value for CaptionText. I don't know if this color is black. I've sent a try job to check this, but it seems the trybot is unhealthy for now (http://build.chromium.org/buildbot/try-server/builders/layout_win/builds/1898). Windows port seems to be using the same color, so this patch would be OK if this patch doesn't change the results of win safari.

CCing pkasting (the guy who wrote RenderThemeChromiumWin::systemColor).

By the way, I want you to remove [GTK] from the ChangeLog description (or please mention that we are changing the behavior of GTK, Win, and Chromium win). Otherwise, someone will miss this change when she/he is investigating the history.

[Shinichiro Hamaji]

It seems many tests are failing

http://build.chromium.org/buildbot/try-server/builders/layout_win/builds/1900

I guess these tests will fail with Win Safari. I think there are three ways now:

1. ask chromium-win and win safari people if this UI change is OK
2. modify themeWin.css to cancel this change for chromium-win and win safari
3. add themeGtk.css and makes this change affect only gtk for now

Personally, I think this change is aiming a good direction and the choice 1 would be the best, but maybe making a consensus might be a bit more difficult than other two options.

[Shinichiro Hamaji]

FYI, all tests passed on linux chromium and mac chromium, unsurprisingly.

http://build.chromium.org/buildbot/try-server/builders/layout_linux/builds/1892
http://build.chromium.org/buildbot/try-server/builders/layout_mac/builds/1698

[Olivier Tilloy]

As discussed with Hamaji, I'm going to split this patch into two, one with the GTK specific changes, and another one with the changes to the default CSS, which I will attach to a new bug report for discussion and in-depth review by relevant people from the various ports.

[Olivier Tilloy]

Created attachment 57464 [details]
Split patch - GTK specific changes only

As commented above, this updated patch contains only the GTK specific changes, it should be easier to merge in a first step.
I verified that it doesn't break any existing layout test.
Note that because changes to the default CSS are needed, it only partially fixes the bug: the foreground color for buttons is now correct, but that of text entries isn't.

[Olivier Tilloy]

I filed bug #39956 to discuss and review the changes to the default CSS.

[Gustavo Noronha (kov)]

Comment on attachment 57464 [details]
Split patch - GTK specific changes only

Looks sane to me.

[WebKit Commit Bot]

Comment on attachment 57464 [details]
Split patch - GTK specific changes only

Clearing flags on attachment: 57464

Committed r60440: <http://trac.webkit.org/changeset/60440>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.