| Summary: | [GTK] Hardcoded text color in input fields | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Claudio Saavedra <csaavedra> | ||||||||||||||||||||||||||||||||||||
| Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | agomez, aperez, bugs-noreply, cadubentzen, cgarcia, commit-queue, ews-watchlist, frederic.romagne, john.frankish, juanj.marin, mcatanzaro, mitya57, mrobinson, nekohayo, otte, rniwa, torben.andresen, webkit, wk | ||||||||||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||
| See Also: |
https://bugzilla.gnome.org/show_bug.cgi?id=761986 https://bugzilla.gnome.org/show_bug.cgi?id=602217 https://bugzilla.gnome.org/show_bug.cgi?id=771363 https://bugs.webkit.org/show_bug.cgi?id=165072 https://bugs.webkit.org/show_bug.cgi?id=186244 https://bugs.webkit.org/show_bug.cgi?id=197276 https://bugs.webkit.org/show_bug.cgi?id=197947 |
||||||||||||||||||||||||||||||||||||||
| Bug Depends on: | 186219 | ||||||||||||||||||||||||||||||||||||||
| Bug Blocks: | 196685 | ||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||
|
Description
Claudio Saavedra
2014-01-13 08:35:24 PST
Isn't this a duplicate of https://bugs.webkit.org/show_bug.cgi?id=118234 ? Not sure whether that's the same issue? That's the button text. Although the fix is probably similar and they're closely related, yes. I dareasking the status as I am installing webkit-gtk-2.4.4 and the changelog doesn't mention any change for this bug. (I am currently filling the comment with black text over a dark textarea...) I tried applying the patch from #40024 but it didn't change the issue. That's two years this bug is open and six years since the corresponding bug was submitted for epiphany. Is there any hope of a fix? Just as a note: in https://bugzilla.gnome.org/show_bug.cgi?id=602217#c11 a proof-of-concept workaround identified that a user stylesheet override could affect the behavior of this issue. I then realized that one can simply use the CSS below to force a new hardcoded color that "sort of works" in both modes (but it's still hardcoded, so it's crap) input,select,textarea,option { color:gray; } Maybe it helps pointing out the rough location of the issue, at least. (In reply to comment #5) > Maybe it helps pointing out the rough location of the issue, at least. The issue is that RenderThemeGtk is inconsistent in its use of GTK+ theme colors (using them for the background in the combo but not for the text), or perhaps it's not smart enough to decide not to use theme colors in the case of a conflict with the web site (e.g. if the web site forces the text in the combo to black, we really need to not use the theme colors). *** Bug 135232 has been marked as a duplicate of this bug. *** Created attachment 302499 [details]
Example of <input> and <textarea> rendering
A simple input or textarea without more styles is just unusable on "dark mode".
I believe it'd be better to completely ignore the "dark mode" than the current situation.
Still happening with 2.19.6 (In reply to Manuel Rego Casasnovas from comment #8) > Created attachment 302499 [details] > Example of <input> and <textarea> rendering > > A simple input or textarea without more styles is just unusable on "dark > mode". > I believe it'd be better to completely ignore the "dark mode" than the > current situation. +1 *** Bug 184264 has been marked as a duplicate of this bug. *** (In reply to Manuel Rego Casasnovas from comment #8) > Created attachment 302499 [details] > Example of <input> and <textarea> rendering > > A simple input or textarea without more styles is just unusable on "dark > mode". > I believe it'd be better to completely ignore the "dark mode" than the > current situation. +1 This bug and bug 165072 have been around for a while and I think it affects many users (basically everyone that uses a GTK+ dark theme)... In https://bugzilla.mozilla.org/show_bug.cgi?id=1158076 the way they solved it was using a light theme only for InHTML elements like buttons, textareas and comboboxes, which are the troubling elemens in dark themes for which bugs are open. Do you think this is a reasonable way to go? If so I would like to give it a try. (In reply to Carlos Eduardo Ramalho from comment #14) > This bug and bug 165072 have been around for a while and I think it affects > many users (basically everyone that uses a GTK+ dark theme)... > > In https://bugzilla.mozilla.org/show_bug.cgi?id=1158076 the way they solved > it was using a light theme only for InHTML elements like buttons, textareas > and comboboxes, which are the troubling elemens in dark themes for which > bugs are open. > > Do you think this is a reasonable way to go? If so I would like to give it a > try. InHTML only so it doesn't affect widgets outside the page like the browser UI. Yeah that sounds good. Question is, how will you determine if it is a dark theme or not? I don't think there's an easy way to test which colors are going to be drawn. Hint: start at RenderThemeGTK. (In reply to Michael Catanzaro from comment #16) > Yeah that sounds good. Question is, how will you determine if it is a dark > theme or not? I'll just set the widgets to a light theme, no check whether it's in a dark theme or not. > I don't think there's an easy way to test which colors are going to be drawn. > Hint: start at RenderThemeGTK. I don't know if info about page styling override is available at RenderTheme calls to detect e.g. black text in textarea while using dark theme. If the info were there we could test it to prevent the non-readable combination, but there are many colors combination to avoid and doing so would still be cumbersome. Do you agree to just set light colors to the InHTML widgets unconditionally? (In reply to Carlos Eduardo Ramalho from comment #17) > I don't know if info about page styling override is available at RenderTheme > calls to detect e.g. black text in textarea while using dark theme. Self correcting: the page style comes as RenderStyle parameter but the difficulty indeed is easy and reliably checking for dark theme and determine what colors will be drawn, as you said. I'm not sure what you're planning. Why don't you go ahead and submit a patch. Carlos Garcia will be back tomorrow and will want to review it. I think the easiest way would be making the content process force using the light theme for any widget. This is what Firefox does or did some time ago. Created attachment 340464 [details] Easiest solution. WIP Well, the easiest solution I found here was simply enforcing Adwaita light theme in RenderThemeGadget. To test in MiniBrowser: $ export GTK_THEME=Adwaita:dark $ run-minibrowser --gtk https://bugs.webkit.org/query.cgi?format=advanced You'll see that the browser UI is dark but the inputs inside the page aren't However, it's far from ideal as others GTK+ light themes could not be used and you guys probably won't like it. Maybe it's not in the best place either. In my opinion WebKitGTK+ should just stick to Adwaita light for styling webpage widgets as it's really up to the web developer to style its elements and we should just not interfere with pages that don't. For example, currently a <input type="text" /> with no style has its text unreadable if using a GTK dark theme. Also, text inputs, checkboxes and radio buttons seem very strange in pages with light theming e.g bugs.webkit.org However it's the owners decision. So what do you think? (In reply to Carlos Eduardo Ramalho from comment #21) > > In my opinion WebKitGTK+ should just stick to Adwaita light for styling > webpage widgets as it's really up to the web developer to style its elements > and we should just not interfere with pages that don't. I agree. (In reply to Andres Gomez Garcia from comment #22) > (In reply to Carlos Eduardo Ramalho from comment #21) > > > > In my opinion WebKitGTK+ should just stick to Adwaita light for styling > > webpage widgets as it's really up to the web developer to style its elements > > and we should just not interfere with pages that don't. > > I agree. While I am going to miss the themed widgets for those themes that render nicely (e.g. the light variant of Arc), I do agree that forcing usage of Adwaita is the path of least effort which will ensure widgets are rendered properly — so let's do that for now. Another option could be providing custom styling for widgets in web content, having some theme-agnostic look (like Chromium does) when the GTK+ theme is other than Adwaita. Dunno. (Side note: Firefox does render widgets from dark themes, you can check that here in Bugzilla while having Adwaita-dark enabled.) Created attachment 340489 [details] Set color in textfield > (Side note: Firefox does render widgets from dark themes, you can check that here in Bugzilla while having Adwaita-dark enabled.) You are right indeed. They do. Another solution I found: setting the text color equals the theme foreground color. This way we can keep dark themes and it avoids rendering dark text on dark background. With light themes it works nicely as well. Tell me which solution you prefer and I can polish it add ChangeLog entries. (Or even if neither solution is enough) (In reply to Carlos Eduardo Ramalho from comment #24) > Another solution I found: setting the text color equals the theme foreground > color. This way we can keep dark themes and it avoids rendering dark text on > dark background. With light themes it works nicely as well. This sounds like a better approach to me. (In reply to Michael Catanzaro from comment #25) > (In reply to Carlos Eduardo Ramalho from comment #24) > > Another solution I found: setting the text color equals the theme foreground > > color. This way we can keep dark themes and it avoids rendering dark text on > > dark background. With light themes it works nicely as well. > > This sounds like a better approach to me. Same opinion here, good job! (In reply to Andres Gomez Garcia from comment #22) > (In reply to Carlos Eduardo Ramalho from comment #21) > > > > In my opinion WebKitGTK+ should just stick to Adwaita light for styling > > webpage widgets as it's really up to the web developer to style its elements > > and we should just not interfere with pages that don't. > > I agree. As a positive side-effect, this would reduce the attack surface for browser fingerprinting as any default widget has (mostly) the same style. (In reply to Carlos Eduardo Ramalho from comment #24) > Another solution I found: setting the text color equals the theme foreground > color. This way we can keep dark themes and it avoids rendering dark text on > dark background. With light themes it works nicely as well. This would also be needed for foreground and background color of all the other widgets such as buttons, check boxes, …, right? This adds more complexity compared to just forcing use of Adwaita:light. I don't think we want to force use of Adwaita light. That's not polite to other themes and distributors, especially elementary which is going to want proper integration for its own theme. (In reply to Christian Stadelmann from comment #28) > This would also be needed for foreground and background color of all the > other widgets such as buttons, check boxes, …, right? This adds more > complexity compared to just forcing use of Adwaita:light. Only widgets with text I guess (entries, comboboxes and button). I think checkboxes are radio buttons are ok. (In reply to Michael Catanzaro from comment #29) > I don't think we want to force use of Adwaita light. That's not polite to > other themes and distributors, especially elementary which is going to want > proper integration for its own theme. It's a valid point. I'll just test my second solution here better on some themes and websites and then I send the patch for landing. I ran intro trouble with HighContrast theme which is now drawing light text with light background. (In reply to Carlos Eduardo Ramalho from comment #30) > I ran intro trouble with HighContrast theme which is now drawing light text with light > background. The GTK+ developers are telling me that this should work, because the foreground color is black for HighContrast. Exactly which color are you using? I'm looking at https://gitlab.gnome.org/GNOME/gtk/blob/gtk-3-22/gtk/theme/HighContrast/_colors.scss. Company the problem is this:
GTK uses white as the default foreground color
and we use proper css inheritance
entry is set to inherit its foreground color in HC
so in Webkit, it has no parent, so it inherits the default white
and in a regular GTK app
it's ultimately contained in something toplevel
and all GTK toplevels set the .background class
so the entry's color ends up being set by https://gitlab.gnome.org/GNOME/gtk/blob/gtk-3-22/gtk/theme/HighContrast/gtk-contained.css#L38
...
so my suggested fix is creating a "window.background" base gadget that all other gadgets are made children of
...
that constructor should create some "Default gadget" that is a window + webview
...
window.background + webview
the window.background is the important part, the webview one would be nice for themers
So the place to do that would be in the RenderThemeGadget constructor. There are two places we currently use gtk_widget_path_new(). Those would need to instead use a default base widget path. "window.background" is what Company thinks would fix the text color. And then, optionally, "webview" to allow theme developers to provide custom styling for WebKit (a totally separate issue, but this has been requested before and now would be the perfect time to add it). If you're lucky, and that actually works, then we can say it was simple. :P
Created attachment 340924 [details] Example (In reply to Michael Catanzaro from comment #31) > (In reply to Carlos Eduardo Ramalho from comment #30) > > I ran intro trouble with HighContrast theme which is now drawing light text with light > > background. > > The GTK+ developers are telling me that this should work, because the > foreground color is black for HighContrast. Exactly which color are you > using? I'm looking at > https://gitlab.gnome.org/GNOME/gtk/blob/gtk-3-22/gtk/theme/HighContrast/ > _colors.scss. Attached is an (ugly) example of getting color from after using gtk_widget_path_new (). Try running it with GTK_THEME=Adwaita or HighConstrastInverse and then with GTK_THEME=HighContrast: every theme gives the expected color except HighContrast. One interesting thing is it happens only for Entries (try changing entry to button in gtk_widget_path_iter_set_object_name and check). So I dig in the scss files and found that comparing https://gitlab.gnome.org/GNOME/gtk/blob/gtk-3-22/gtk/theme/Adwaita/_drawing.scss#L49 and https://gitlab.gnome.org/GNOME/gtk/blob/gtk-3-22/gtk/theme/HighContrast/_drawing.scss#L58 shows that in HighContrast it lacks "color:" in entries, so somehow it gets white. Simply adding "color: $fg_color" solved it for me. Can the GTK developers confirm this is the case? (In reply to Carlos Eduardo Ramalho from comment #33) > One interesting thing is it happens only for Entries (try changing entry to > button in gtk_widget_path_iter_set_object_name and check). So I dig in the > scss files and found that comparing I ended up sending the attachment with "button". Set it to "entry" first. (sorry for the mistake). The steps I did was trying to simuate what WebKitGTK does currently. (In reply to Michael Catanzaro from comment #32) > Company the problem is this: > GTK uses white as the default foreground color > and we use proper css inheritance > entry is set to inherit its foreground color in HC > so in Webkit, it has no parent, so it inherits the default white > and in a regular GTK app > it's ultimately contained in something toplevel > and all GTK toplevels set the .background class > so the entry's color ends up being set by > https://gitlab.gnome.org/GNOME/gtk/blob/gtk-3-22/gtk/theme/HighContrast/gtk- > contained.css#L38 In this line foreground color is black no? > ... > so my suggested fix is creating a "window.background" base gadget > that all other gadgets are made children of > ... > that constructor should create some "Default gadget" that is a > window + webview > ... > window.background + webview > the window.background is the important part, the webview one would > be nice for themers > > > So the place to do that would be in the RenderThemeGadget constructor. There > are two places we currently use gtk_widget_path_new(). Those would need to > instead use a default base widget path. "window.background" is what Company > thinks would fix the text color. And then, optionally, "webview" to allow > theme developers to provide custom styling for WebKit (a totally separate > issue, but this has been requested before and now would be the perfect time > to add it). If you're lucky, and that actually works, then we can say it was > simple. :P I'm not sure window.background is really the way to go because of my findings in the above comment which I did before seeing this. However the "webview" gadget is a good idea for themers indeed. Maybe place this in another bug? (In reply to Carlos Eduardo Ramalho from comment #33) > Simply adding "color: $fg_color" solved it for me. Can the GTK developers > confirm this is the case? I guess the problem here is that it proves that color is not actually what GTK+ is looking at when it draws the text entry here. Otherwise, text entries would be broken in all other GTK+ apps, right? So we can certainly add a line of CSS to the theme, that's no problem, but I suspect it's not the right fix as we'd ideally want to draw the same thing as GTK+. Eventually we're going to have to rewrite everything to not use gtk_css_context_get_color() at all (it's at risk of removal from GTK+), nor gtk_css_context_get_background_color() (a more immediate problem as that one is gone in GTK+ 4), but that's definitely beyond the scope of this bug. (In reply to Carlos Eduardo Ramalho from comment #35) > I'm not sure window.background is really the way to go because of my > findings in the above comment which I did before seeing this. However the > "webview" gadget is a good idea for themers indeed. Maybe place this in > another bug? We should do both, perhaps in another bug if it turns out to not be necessary to solve this one, yes. (In reply to Michael Catanzaro from comment #36) > Eventually we're going to have to rewrite everything to not use > gtk_css_context_get_color() at all (it's at risk of removal from GTK+), nor > gtk_css_context_get_background_color() (a more immediate problem as that one > is gone in GTK+ 4), but that's definitely beyond the scope of this bug. I lied, Company says: "also, gtk_style_context_get_color() is gonna stay only get_background_color() is going away" (In reply to Carlos Eduardo Ramalho from comment #35) > In this line foreground color is black no? As we discussed on Matrix: yes, the problem is that line won't ever be reached unless we add window.background to the widget path. Company showed us that the we're getting the default color from https://gitlab.gnome.org/GNOME/gtk/blob/gtk-3-22/gtk/gtkcssstylepropertyimpl.c#L1065 instead. (In reply to Carlos Eduardo Ramalho from comment #33) > Simply adding "color: $fg_color" solved it for me. Can the GTK developers > confirm this is the case? So this should not be needed once we fix the widget path, since the color should then become inherited from the background. Hopefully. ;) Created attachment 341088 [details]
Patch
Comment on attachment 341088 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341088&action=review I think Carlos Garcia should review the RenderThemeGtk changes, but I can review the RenderThemeGadget bits. You're on the right track, but this isn't quite what I had in mind. > Source/WebCore/platform/gtk/RenderThemeGadget.cpp:64 > + // Class to be used by GTK themers if they want custom styling inside webviews. > + gtk_style_context_add_class(context.get(), "webview"); Hm, style classes have fallen out of favor, so let's not add a new one now. It would be better to modify the widget path instead. > Source/WebCore/platform/gtk/RenderThemeGadget.cpp:245 > + // This workaround is for some themes like HighConstrast lacking "color" gtk-css attribute in entries. > + // When the widgets are drawn by GTK they go to this fallback theme. However here they were getting the default color. > + // As a result, for example, HighConstrast theme was drawing white text in white background in text inputs. > + if (m_context) > + gtk_style_context_add_class(m_context.get(), "background"); It's not a workaround at all! It's just the right thing to do. We're trying to put together a widget path that mimics a real GTK+ app. This isn't the right place, though, because the new widget path should be used for all gadgets, not just text fields. It could fix more than just text color in input fields. Also, the goal is to adjust the widget path, not add a style class. I'm kinda surprised this worked, actually. Look at my comment #32. """So the place to do that would be in the RenderThemeGadget constructor. There are two places we currently use gtk_widget_path_new(). Those would need to instead use a default base widget path. "window.background" is what Company thinks would fix the text color. And then, optionally, "webview" to allow theme developers to provide custom styling for WebKit....""" See here: GRefPtr<GtkWidgetPath> path = parent ? adoptGRef(gtk_widget_path_copy(gtk_style_context_get_path(parent->context()))) : adoptGRef(gtk_widget_path_new()); if (!siblings.isEmpty()) { GRefPtr<GtkWidgetPath> siblingsPath = adoptGRef(gtk_widget_path_new()); Instead of passing gtk_widget_path_new(), we want to create a custom widget path to pass there instead. So let's add a createWidgetPath() function to use there instead. Should look something like: static GRefPtr<GtkWidgetPath> createWidgetPath() { GRefPtr<GtkWidgetPath> path = adoptGRef(gtk_widget_path_new()); gtk_widget_path_append_type(path, GTK_TYPE_WINDOW); gtk_widget_path_iter_add_class (path, -1, GTK_STYLE_CLASS_BACKGROUND); gtk_widget_path_append_type(path, WEBKIT_TYPE_WEB_VIEW); gtk_widget_path_iter_set_object_name(path, -1, "webview"); return path; } Then we should have a sane widget path for everything that we draw. I copied the first part of that from https://gitlab.gnome.org/GNOME/gtk/blob/gtk-3-24/testsuite/gtk/firefox-stylecontext.c, which seems sufficiently-authoritative to me. Now, will it work? That is the question.... > Source/WebCore/rendering/RenderThemeGtk.cpp:502 > + switch (part) { > + default: > + ASSERT_NOT_REACHED(); default at the top is really weird, is that copied from elsewhere in this file? It's idiomatic to put the default case at the bottom of the switch. Related, I found this code in Firefox: https://hg.mozilla.org/mozilla-central/file/50355ef2f138/widget/gtk/nsLookAndFeel.cpp#l823 // Dark themes interacts poorly with widget styling (see bug 1216658). + −// We disable dark themes by default for all processes (chrome, web content) // but allow user to overide it by prefs. const gchar* dark_setting = "gtk-application-prefer-dark-theme"; gboolean darkThemeDefault; g_object_get(settings, dark_setting, &darkThemeDefault, nullptr); // To avoid triggering reload of theme settings unnecessarily, only set the // setting when necessary. if (darkThemeDefault) { bool allowDarkTheme; if (XRE_IsContentProcess()) { allowDarkTheme = mozilla::Preferences::GetBool("widget.content.allow-gtk-dark-theme", false); } else { allowDarkTheme = (PR_GetEnv("MOZ_ALLOW_GTK_DARK_THEME") != nullptr) || mozilla::Preferences::GetBool("widget.chrome.allow-gtk-dark-theme", false); } if (!allowDarkTheme) { g_object_set(settings, dark_setting, FALSE, nullptr); } } It's too complicated for us, and we would definitely not want to override the theme in the UI process. But maybe we should override it in the web process? That would give us a better chance of avoiding theme problems on websites, probably. It's really a totally separate change, though, and I don't know if we should: if you're able to get dark themes working well enough without it, maybe we don't need to. > gtk_widget_path_append_type(path, GTK_TYPE_WINDOW); > gtk_widget_path_iter_add_class (path, -1, GTK_STYLE_CLASS_BACKGROUND); > > gtk_widget_path_append_type(path, WEBKIT_TYPE_WEB_VIEW); > gtk_widget_path_iter_set_object_name(path, -1, "webview"); Since GTK 3.20, you need to gtk_widget_path_iter_set_object_name() after every appent_type() call, otherwise things won't work. So this needs to look like gtk_widget_path_append_type(path, GTK_TYPE_WINDOW); gtk_widget_path_iter_set_object_name(path, -1, "window"); gtk_widget_path_iter_add_class (path, -1, GTK_STYLE_CLASS_BACKGROUND); Otherwise this looks right for the widget path. But you absolutely want to create real RenderThemeGadgets for those two elements. Otherwise the color inheritance is not going to work because you have no parent GtkStyleContext to inherit from. (In reply to Benjamin Otte from comment #42) > Since GTK 3.20, you need to gtk_widget_path_iter_set_object_name() after > every appent_type() call, otherwise things won't work. So this needs to look > like > > gtk_widget_path_append_type(path, GTK_TYPE_WINDOW); > gtk_widget_path_iter_set_object_name(path, -1, "window"); > gtk_widget_path_iter_add_class (path, -1, GTK_STYLE_CLASS_BACKGROUND); Thanks for the correction! (In reply to Benjamin Otte from comment #42) > But you absolutely want to create real RenderThemeGadgets for those two > elements. Otherwise the color inheritance is not going to work because you > have no parent GtkStyleContext to inherit from. OK, that sounds more complicated... Carlos Eduardo, do you want to investigate that? I don't know how hard it would be. (In reply to Michael Catanzaro from comment #40) Thanks for the review! > Hm, style classes have fallen out of favor, so let's not add a new one now. > It would be better to modify the widget path instead. Hmm really? Ok. I started out trying that and things didn't work so I went the other way. I must've done something wrong and will check it then. > > > Source/WebCore/platform/gtk/RenderThemeGadget.cpp:245 > > + // This workaround is for some themes like HighConstrast lacking "color" gtk-css attribute in entries. > > + // When the widgets are drawn by GTK they go to this fallback theme. However here they were getting the default color. > > + // As a result, for example, HighConstrast theme was drawing white text in white background in text inputs. > > + if (m_context) > > + gtk_style_context_add_class(m_context.get(), "background"); > > It's not a workaround at all! It's just the right thing to do. We're trying > to put together a widget path that mimics a real GTK+ app. > > This isn't the right place, though, because the new widget path should be > used for all gadgets, not just text fields. It could fix more than just text > color in input fields. I was doing it for all gadgets but some weird background colors were showing up in other gadgets like buttons so I opted to place it only for input fields. As I put it specifically for input fields, that's why I called it a workaround. However, as the whole approach by setting additional classes might be wrong, then maybe if I use the widget path approach correctly it will work when setting it for all gadgets. > > Also, the goal is to adjust the widget path, not add a style class. I'm > kinda surprised this worked, actually. > > Look at my comment #32. """So the place to do that would be in the > RenderThemeGadget constructor. There are two places we currently use > gtk_widget_path_new(). Those would need to instead use a default base widget > path. "window.background" is what Company thinks would fix the text color. > And then, optionally, "webview" to allow theme developers to provide custom > styling for WebKit....""" > > See here: > > GRefPtr<GtkWidgetPath> path = parent ? > adoptGRef(gtk_widget_path_copy(gtk_style_context_get_path(parent- > >context()))) : adoptGRef(gtk_widget_path_new()); > if (!siblings.isEmpty()) { > GRefPtr<GtkWidgetPath> siblingsPath = > adoptGRef(gtk_widget_path_new()); > > Instead of passing gtk_widget_path_new(), we want to create a custom widget > path to pass there instead. So let's add a createWidgetPath() function to > use there instead. Should look something like: > > static GRefPtr<GtkWidgetPath> createWidgetPath() > { > GRefPtr<GtkWidgetPath> path = adoptGRef(gtk_widget_path_new()); > > gtk_widget_path_append_type(path, GTK_TYPE_WINDOW); > gtk_widget_path_iter_add_class (path, -1, GTK_STYLE_CLASS_BACKGROUND); > > gtk_widget_path_append_type(path, WEBKIT_TYPE_WEB_VIEW); > gtk_widget_path_iter_set_object_name(path, -1, "webview"); > > return path; > } It seems good :) I'll do so following Benjamin Otte correction and see what I get! > > Then we should have a sane widget path for everything that we draw. I copied > the first part of that from > https://gitlab.gnome.org/GNOME/gtk/blob/gtk-3-24/testsuite/gtk/firefox- > stylecontext.c, which seems sufficiently-authoritative to me. Now, will it > work? That is the question.... > > > Source/WebCore/rendering/RenderThemeGtk.cpp:502 > > + switch (part) { > > + default: > > + ASSERT_NOT_REACHED(); > > default at the top is really weird, is that copied from elsewhere in this > file? It's idiomatic to put the default case at the bottom of the switch. Indeed it is copied from another function in the file! I thought if it was like that in there then it was the convention followed. I'll fix that. (In reply to Michael Catanzaro from comment #43) > (In reply to Benjamin Otte from comment #42) > > But you absolutely want to create real RenderThemeGadgets for those two > > elements. Otherwise the color inheritance is not going to work because you > > have no parent GtkStyleContext to inherit from. > > OK, that sounds more complicated... Carlos Eduardo, do you want to > investigate that? I don't know how hard it would be. I'll look into that. I have tried the suggestions on setting the GtkWidgetPath properly, but no luck. In GTK apps that's the widget path we get for a simple entry inside a window (with gtk_widget_path_to_string()): `window:dir-ltr.background entry:dir-ltr` After following the suggestion we do get `window.background entry` but HighConstrast theme still renders using white text color from https://gitlab.gnome.org/GNOME/gtk/blob/gtk-3-22/gtk/gtkcssstylepropertyimpl.c#L1065 anyway I don't know the Gtk styling code too much to solve this properly, it seems. If someone else who understands it better want to take this over, feel free. It seems simple but I've struggled to do things right as a GTK newbie. Setting the widget path is not gonna help, the widget path just sets the color to "inherit" at which point the CSS engine looks up the color in the parent style context. Which is why you need a parent RenderThemeGadget set to "window.background" that has the correct color, so GTK finds the right StyleContext to inherit from. Created attachment 341463 [details]
Patch
(In reply to Michael Catanzaro from comment #40) > static GRefPtr<GtkWidgetPath> createWidgetPath() > { > GRefPtr<GtkWidgetPath> path = adoptGRef(gtk_widget_path_new()); > > gtk_widget_path_append_type(path, GTK_TYPE_WINDOW); > gtk_widget_path_iter_add_class (path, -1, GTK_STYLE_CLASS_BACKGROUND); > > gtk_widget_path_append_type(path, WEBKIT_TYPE_WEB_VIEW); > gtk_widget_path_iter_set_object_name(path, -1, "webview"); > > return path; > } I did not add the WEBKIT_TYPE_WEB_VIEW part because then I would have to include a header from Source/WebKit.. Would that be correct? I thought only WebKit could include files from WebCore and not the other way around... Or is there other way to call the append_type without doing that? (In reply to Benjamin Otte from comment #46) > Setting the widget path is not gonna help, the widget path just sets the > color to "inherit" at which point the CSS engine looks up the color in the > parent style context. > > Which is why you need a parent RenderThemeGadget set to "window.background" > that has the correct color, so GTK finds the right StyleContext to inherit > from. There is already a parent RenderThemeGadget that all other gadgets inherit from.. The thing is when creating the GtkStyleContext in the RenderThemeGadget constructor, the parent StyleContext was null when not coming from a another RenderThemeGadget already. So I set it to a window.background StyleContext here: - m_context = createStyleContext(path.get(), parent ? parent->context() : nullptr); + m_context = createStyleContext(path.get(), parent ? parent->context() : baseContext.get()); It fixes the problem here. What do you think? (In reply to Carlos Eduardo Ramalho from comment #48) > I did not add the WEBKIT_TYPE_WEB_VIEW part because then I would have to > include a header from Source/WebKit.. Would that be correct? I thought only > WebKit could include files from WebCore and not the other way around... Or > is there other way to call the append_type without doing that? You are right, Source/WebCore cannot include anything from Source/WebKit, it would be a layering violation. Let's leave it be for now, since that's not required for fixing this bug. There is a workaround: we can add a delegate interface to WebCore which the WebKit layer could call to pass in the GType of WebKitWebView. It's a bit messy, so let's not do that now. > I did not add the WEBKIT_TYPE_WEB_VIEW part because then I would have to include a header from Source/WebKit
I think you can just use G_TYPE_NONE there because the CSS matcher ignores types when an object name is set.
If you wanted to be tricky, you could probably also use g_type_from_name("WebkitWebView"), but that shouldn't be necessary.
But all of that's not necessary to fix this issue and was merely thought as an additional feature to help theme authors, so leaving it out (or splitting it into a different bug) doesn't hurt.
Comment on attachment 341463 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341463&action=review Good job! I know this was tricky. The patch looks good to me, I just have one comment. It'd be great if Benjamin could review this version as a sanity-check. Also, Carlos Garcia should review it as well, at least the bits in RenderThemeGtk. > Source/WebCore/platform/gtk/RenderThemeGadget.cpp:91 > + static GRefPtr<GtkStyleContext> baseContext = createBaseStyleContext(); It looks good, but there is a nasty trick: this is an exit-time destructor. Even if it works properly right now, it's risky since we could easily wind up destroying the GtkStyleContext after it's no longer safe to do so in the future. It's better to just leak it, since we only ever create one. The way to document this is to use the NeverDestroyed template, like this: static NeverDestroyed<GRefPtr<GtkStyleContext>> baseContext = createBaseStyleContext(); I know it might look pretty weird to put a GRefPtr into a NeverDestroyed, but that's the idiom we use, just roll with it. Also, as a matter of style, I would remove the blank line below it. (In reply to Michael Catanzaro from comment #51) > It'd be great if Benjamin could review this version as a sanity-check. OK, looks like we had a little five-second race there. Since adding WebKitWebView to the widget path is really easy, I'd go ahead and do it now. > Benjamin could review this version as a sanity-check.
I'm out of my league for 90% of what's going on, but the 10% I do understand look exactly like I would expect them to look.
One comment:
There's some GTK_CHECK_VERSION(3, 20, 0) parts but they don't cover gtk_widget_path_iter_set_object_name() - Those and using names for CSS was added with GTK 3.20, so either those parts also need #ifdef's or (my suggestion) the dependency should be upped to at least 3.20.
Oh good catch, we actually can't increase the dependency yet, but createBaseStyleContext() could simply return gtk_style_context_new() in the fallback case. Thanks for the reviews Michael Catanzaro and Benjamin Otte! (In reply to Michael Catanzaro from comment #51) > static NeverDestroyed<GRefPtr<GtkStyleContext>> baseContext = > createBaseStyleContext(); > > I know it might look pretty weird to put a GRefPtr into a NeverDestroyed, > but that's the idiom we use, just roll with it. Even weirder having baseContext.get().get() to get the raw pointer... But nice catch indeed! (In reply to Benjamin Otte from comment #53) > One comment: > There's some GTK_CHECK_VERSION(3, 20, 0) parts but they don't cover > gtk_widget_path_iter_set_object_name() - Those and using names for CSS was > added with GTK 3.20, so either those parts also need #ifdef's or (my > suggestion) the dependency should be upped to at least 3.20. The whole RenderThemeGadget file is under `#if GTK_CHECK_VERSION(3, 20, 0)`, which is hidden out in the patch view :) (In reply to Michael Catanzaro from comment #52) > Since adding WebKitWebView to the widget path is really easy, I'd go ahead > and do it now. I tried adding it with G_TYPE_NONE but then the problem with HighContrast comes back. static GRefPtr<GtkStyleContext> createBaseStyleContext() { GRefPtr<GtkWidgetPath> path = adoptGRef(gtk_widget_path_new()); gtk_widget_path_append_type(path.get(), GTK_TYPE_WINDOW); gtk_widget_path_iter_set_object_name(path.get(), -1, "window"); gtk_widget_path_iter_add_class(path.get(), -1, GTK_STYLE_CLASS_BACKGROUND); gtk_widget_path_append_type(path.get(), G_TYPE_NONE); gtk_widget_path_iter_set_object_name(path.get(), -1, "webview"); GRefPtr<GtkStyleContext> baseContext = adoptGRef(gtk_style_context_new()); gtk_style_context_set_path(baseContext.get(), path.get()); gtk_style_context_set_parent(baseContext.get(), nullptr); return baseContext; } Getting the return `pos = gtk_widget_path_append_type(path, G_TYPE_NONE)` and using it as position for gtk_widget_path_iter_set_object_name(path.get(), pos, "webview") also didn't help. It seems like it requires more investigation, so I think should move it to another bug. Created attachment 341468 [details]
Patch
Comment on attachment 341468 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341468&action=review Patch looks good to me, but I think it can be improved a bit. Thanks! > Source/WebCore/platform/gtk/RenderThemeGadget.cpp:78 > + I would remove this empty line. > Source/WebCore/platform/gtk/RenderThemeGadget.cpp:92 > + static NeverDestroyed<GRefPtr<GtkStyleContext>> baseContext = createBaseStyleContext(); This is weird, we have a create function that is always going to be used with a NeverDestroyed. I would rename createBaseStyleContext() as baseStyleContext() and make it return a plain pointer GtkStyleContext*. Then make the path created inside the function static and return .get(). > Source/WebCore/platform/gtk/RenderThemeGadget.cpp:101 > + m_context = createStyleContext(path.get(), parent ? parent->context() : baseContext.get().get()); And here you could simply use baseStyleContext() > Source/WebCore/rendering/RenderThemeGtk.cpp:497 > +static Color inputColor(const Element* element, RenderThemePart part) This should receive a reference instead of a pointer. > Source/WebCore/rendering/RenderThemeGtk.cpp:512 > + GtkStateFlags state = element->isDisabledFormControl() ? GTK_STATE_FLAG_INSENSITIVE : GTK_STATE_FLAG_NORMAL; > + gadget->setState(state); We can probably make this a single line: gadget->setState(element.isDisabledFormControl() ? GTK_STATE_FLAG_INSENSITIVE : GTK_STATE_FLAG_NORMAL); > Source/WebCore/rendering/RenderThemeGtk.cpp:530 > + if (element) > + style.setColor(inputColor(element, Button)); I wonder why we need inputColor, couldn't this be: if (element) style.setColor(styleColor(Button, element->isDisabledFormControl() ? GTK_STATE_FLAG_INSENSITIVE : GTK_STATE_FLAG_NORMAL, StyleColorForeground)); > Source/WebCore/rendering/RenderThemeGtk.cpp:982 > + if (element) > + style.setColor(inputColor(element, Entry)); I think we should move this after the next if. Again, I think this could be done with current styleColor(). > Source/WebCore/rendering/RenderThemeGtk.cpp:1123 > + if (element) > + style.setColor(inputColor(element, Entry)); Same here. > Source/WebCore/rendering/RenderThemeGtk.cpp:1248 > + if (element) > + style.setColor(inputColor(element, Entry)); And here. (In reply to Carlos Eduardo Ramalho from comment #55) > I tried adding it with G_TYPE_NONE but then the problem with HighContrast > comes back. > You'd need something like this I think: static GRefPtr<GtkStyleContext> createBaseStyleContext() { GRefPtr<GtkWidgetPath> path = adoptGRef(gtk_widget_path_new()); gtk_widget_path_append_type(path.get(), GTK_TYPE_WINDOW); gtk_widget_path_iter_set_object_name(path.get(), -1, "window"); gtk_widget_path_iter_add_class(path.get(), -1, GTK_STYLE_CLASS_BACKGROUND); GRefPtr<GtkStyleContext> windowContext = adoptGRef(gtk_style_context_new()); gtk_style_context_set_path(windowContext.get(), path.get()); gtk_style_context_set_parent(windowContext.get(), nullptr); path = adoptGRef(gtk_widget_path_new()); gtk_widget_path_append_type(path.get(), GTK_TYPE_WINDOW); gtk_widget_path_iter_set_object_name(path.get(), -1, "window"); gtk_widget_path_iter_add_class(path.get(), -1, GTK_STYLE_CLASS_BACKGROUND); gtk_widget_path_append_type(path.get(), G_TYPE_NONE); gtk_widget_path_iter_set_object_name(path.get(), -1, "webview"); GRefPtr<GtkStyleContext> baseContext = adoptGRef(gtk_style_context_new()); gtk_style_context_set_path(baseContext.get(), path.get()); gtk_style_context_set_parent(baseContext.get(), windowContext.get()); return baseContext; } Created attachment 341565 [details]
Patch
(In reply to Benjamin Otte from comment #58) > (In reply to Carlos Eduardo Ramalho from comment #55) > > I tried adding it with G_TYPE_NONE but then the problem with HighContrast > > comes back. > > > > You'd need something like this I think: > > static GRefPtr<GtkStyleContext> createBaseStyleContext() > { > GRefPtr<GtkWidgetPath> path = adoptGRef(gtk_widget_path_new()); > gtk_widget_path_append_type(path.get(), GTK_TYPE_WINDOW); > gtk_widget_path_iter_set_object_name(path.get(), -1, "window"); > gtk_widget_path_iter_add_class(path.get(), -1, > GTK_STYLE_CLASS_BACKGROUND); > > GRefPtr<GtkStyleContext> windowContext = > adoptGRef(gtk_style_context_new()); > gtk_style_context_set_path(windowContext.get(), path.get()); > gtk_style_context_set_parent(windowContext.get(), nullptr); > > path = adoptGRef(gtk_widget_path_new()); > gtk_widget_path_append_type(path.get(), GTK_TYPE_WINDOW); > gtk_widget_path_iter_set_object_name(path.get(), -1, "window"); > gtk_widget_path_iter_add_class(path.get(), -1, > GTK_STYLE_CLASS_BACKGROUND); > > gtk_widget_path_append_type(path.get(), G_TYPE_NONE); > gtk_widget_path_iter_set_object_name(path.get(), -1, "webview"); > > GRefPtr<GtkStyleContext> baseContext = > adoptGRef(gtk_style_context_new()); > gtk_style_context_set_path(baseContext.get(), path.get()); > gtk_style_context_set_parent(baseContext.get(), windowContext.get()); > > return baseContext; > } Your suggestion solved the problems I was having. Thanks! However I got some weird results when testing theme customizations, so I'll leave it to another patch for now as it was not the original topic of this bug. LGTM. Carlos Garcia, do you want to give the final review? (In reply to Carlos Garcia Campos from comment #57) Thanks for the review! Forgot to respond to the suggestions and there was just one point I didn't follow: > > Source/WebCore/rendering/RenderThemeGtk.cpp:982 > > + if (element) > > + style.setColor(inputColor(element, Entry)); > > I think we should move this after the next if. Again, I think this could be > done with current styleColor(). Moving it after the if would result in only calling setColor in textInputs with spinButtons. (In reply to Carlos Eduardo Ramalho from comment #62) > (In reply to Carlos Garcia Campos from comment #57) > > Thanks for the review! Forgot to respond to the suggestions and there was > just one point I didn't follow: > > > > Source/WebCore/rendering/RenderThemeGtk.cpp:982 > > > + if (element) > > > + style.setColor(inputColor(element, Entry)); > > > > I think we should move this after the next if. Again, I think this could be > > done with current styleColor(). > > Moving it after the if would result in only calling setColor in textInputs > with spinButtons. Oh, you are right. Comment on attachment 341565 [details]
Patch
Excellent!
Comment on attachment 341565 [details] Patch Clearing flags on attachment: 341565 Committed r232338: <https://trac.webkit.org/changeset/232338> All reviewed patches have been landed. Closing bug. Tracking a couple regressions in bug #186244. I also notice that disabled text areas are harder to read now, compare: Before: https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/r232449%20(6864)/retries/fast/forms/basic-textareas-quirks-expected.png After: https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/r232449%20(6864)/retries/fast/forms/basic-textareas-quirks-actual.png I guess that's fine, since the text color is now coming from the theme, and the background color should be coming from the theme too. Just wanted to mention it. Reopening due to https://bugs.webkit.org/show_bug.cgi?id=186244. Created attachment 343018 [details]
Patch
(In reply to Carlos Eduardo Ramalho from comment #69) > Created attachment 343018 [details] > Patch Adressing regressions after bug 186219 landed. Updated test expectations and fixed some details that broke tests. Comment on attachment 343018 [details] Patch Attachment 343018 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/8243991 New failing tests: accessibility/mac/selection-notification-focus-change.html Created attachment 343026 [details]
Archive of layout-test-results from ews105 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 343018 [details] Patch Attachment 343018 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/8244586 New failing tests: http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html Created attachment 343033 [details]
Archive of layout-test-results from ews205 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 343018 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343018&action=review > Source/WebCore/platform/gtk/RenderThemeGadget.cpp:28 > #include "RenderThemeGadget.h" > +#include <mutex> There should be a blank line before including <mutex>. I'm surprised the style bot didn't complain. > Source/WebCore/rendering/RenderTheme.cpp:99 > case TextFieldPart: > - adjustTextFieldStyle(styleResolver, style, element); > + if (is<HTMLInputElement>(element) && shouldHaveSpinButton(downcast<HTMLInputElement>(*element))) > + adjustTextFieldStyle(styleResolver, style, element); > FALLTHROUGH; Hmm, what does this change do? Why is it appropriate to adjust text field style only if shouldHaveSpinButton is true? This seems unlikely to be correct. I see the GTK-specific code was already performing this check, but I'm not sure why, and I'm not sure why it should be moved into cross-platform code. Other than that, the patch looks fine to me, but Carlos Garcia should review it because I don't understand most of it. (In reply to Michael Catanzaro from comment #75) > Hmm, what does this change do? Why is it appropriate to adjust text field > style only if shouldHaveSpinButton is true? This seems unlikely to be > correct. I see the GTK-specific code was already performing this check, but > I'm not sure why, and I'm not sure why it should be moved into > cross-platform code. Because it was added there in the first place to enforce drawing spin buttons even if the page has styled the text input. It was landed in r219332 to fix problems with the spinbuttons appearing over the input value. Calling it unconditionally would force GTK styling (that now also sets the input colors) even when the input is styled. I agree that it seems odd. Maybe it should be refactored to its own new function (adjustNumberInputStyle ?). It behaves correctly the same way it was before (at least does not break layout tests any more) but I'll probably have to change this again when solving bug 175067. If you think it looks ugly I can refactor it here already. Please try refactoring it, yes. (In reply to Carlos Garcia Campos from comment #57) > > Source/WebCore/platform/gtk/RenderThemeGadget.cpp:92 > > + static NeverDestroyed<GRefPtr<GtkStyleContext>> baseContext = createBaseStyleContext(); > > This is weird, we have a create function that is always going to be used > with a NeverDestroyed. I would rename createBaseStyleContext() as > baseStyleContext() and make it return a plain pointer GtkStyleContext*. Then > make the path created inside the function static and return .get(). Please also still use NeverDestroyed, though. It should be NeverDestroyed<GtkStyleContext> with no GRefPtr involved. (It was dumb of me to suggest that.) See bug #186969 for a bunch of examples of how to do this. This will ensure we avoid leak warnings from asan. Created attachment 343657 [details]
Patch
Attachment 343657 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/gtk/RenderThemeGadget.cpp:80: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 1 in 14 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 343659 [details]
Patch
(In reply to Michael Catanzaro from comment #78) > Please also still use NeverDestroyed, though. It should be > NeverDestroyed<GtkStyleContext> with no GRefPtr involved. (It was dumb of me > to suggest that.) See bug #186969 for a bunch of examples of how to do this. > This will ensure we avoid leak warnings from asan. OK. done (In reply to Michael Catanzaro from comment #75) > Comment on attachment 343018 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=343018&action=review > > > Source/WebCore/platform/gtk/RenderThemeGadget.cpp:28 > > #include "RenderThemeGadget.h" > > +#include <mutex> > > There should be a blank line before including <mutex>. I'm surprised the > style bot didn't complain. OK. done (In reply to Michael Catanzaro from comment #77) > Please try refactoring it, yes. I'll try to land a solution for bug 175067 first before landing here to avoid having to refactor code that will be removed anyway. Comment on attachment 343659 [details] Patch Attachment 343659 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8356390 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html Created attachment 343695 [details]
Archive of layout-test-results from ews201 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 368142 [details]
New patch
This patch makes the dark mode look much better.
Created attachment 368149 [details]
Patch
I was using the wrong color for the frame view background. It should be fixed now, I'll attach a screnshot.
Created attachment 368150 [details]
Screenshot
Committed r244635: <https://trac.webkit.org/changeset/244635> |
/r232449%20(6864)/retries/fast/forms/basic-textareas-quirks-expected.png){kind=link}
/r232449%20(6864)/retries/fast/forms/basic-textareas-quirks-actual.png){kind=link}