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.
In fact it also looks like the background color of buttons and text entries is incorrect vs the system's style.
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.
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.
Created attachment 53664 [details] A preliminary patch Updated patch to fix coding style issues.
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!
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.
I think Hyatt could lend us a hand regarding the CSS change? =)
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.
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
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.
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.
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
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.
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.
I filed bug #39956 to discuss and review the changes to the default CSS.
Comment on attachment 57464 [details] Split patch - GTK specific changes only Looks sane to me.
Comment on attachment 57464 [details] Split patch - GTK specific changes only Clearing flags on attachment: 57464 Committed r60440: <http://trac.webkit.org/changeset/60440>
All reviewed patches have been landed. Closing bug.