Bug 186219

Summary: [GTK] Correct behavior for dark themes
Product: WebKit Reporter: Carlos Bentzen <cadubentzen>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, calvaris, cgarcia, commit-queue, mcatanzaro
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=126907
https://bugs.webkit.org/show_bug.cgi?id=165072
https://bugs.webkit.org/show_bug.cgi?id=197947
Bug Depends on:    
Bug Blocks: 126907    
Attachments:
Description Flags
Patch
none
Patch none

Carlos Bentzen
Reported 2018-06-01 19:22:42 PDT
Now that dark themes are working properly on text inputs after bug 126907, some issues that happen on Firefox can happen on WebKitGTK+ as well. E.g. Say some website sets input[type=text] { background-color: "white" } and leave the color property alone. In dark themes it'll render a light text on a white background. This issue can actually happen with any browser, just set input[type=text] { color: "white" } and you get white text on light background. However, as many developers won't test their design in WebKitGTK+ with dark themes, it can be frequent to have pages with partial styling and then trouble in dark themes. A few thoughts on this: - As discussed on matrix with mcatanzaro (quote): Ideally we would have some way to say: "do not use GTK theme colors at all if any parent element has been styled". That might be quite hard to implement, though. - Easy solution: stop using GTK for theming the web content or just stick to Adwaita light. Not so good for some distributions like Elementary that want to have their look and feel. - Another possibility: adding a property in public API (UIProcess) to select the theme. So users could pick their theme of choice for the webview elements. - Another: always switch to the light variant of the current theme. Not sure if this can be done reliably only with GTK calls or something would need to be hard-coded. It seems like in Firefox they use “gtk-application-prefer-dark-theme” but I get rendering issues with dark themes anyway. Opinions on this would be highly appreciated to decide which path to pursue.
Attachments
Patch (4.47 KB, patch)
2018-06-14 05:06 PDT, Carlos Bentzen 		no flags
DetailsFormatted DiffDiff
Patch (4.48 KB, patch)
2018-06-17 00:43 PDT, Carlos Bentzen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Michael Catanzaro
Comment 1 2018-06-02 08:21:20 PDT
(In reply to Carlos Eduardo Ramalho from comment #0) > - As discussed on matrix with mcatanzaro (quote): Ideally we would have some > way to say: "do not use GTK theme colors at all if any parent element has > been styled". That might be quite hard to implement, though. This is the way to go. Carlos Garcia's comment in bug #118234 might be relevant: (In reply to Carlos Garcia Campos from comment #12) > I think we need to implement RenderTheme::isControlStyled() properly to > decide whether to apply native style or not.
Carlos Bentzen
Comment 2 2018-06-14 05:06:06 PDT
Created attachment 342732 [details] Patch
Carlos Bentzen
Comment 3 2018-06-14 05:14:00 PDT
(In reply to Michael Catanzaro from comment #1) > Carlos Garcia's comment in bug #118234 might be relevant: > > (In reply to Carlos Garcia Campos from comment #12) > > I think we need to implement RenderTheme::isControlStyled() properly to > > decide whether to apply native style or not. I've been doing a few tests on this and here some notes: The return of isControlStyled() impacts whether the adjustXyzStyle() methods are called AND paintXyz() (though it's not coded in RenderTheme, but in RenderBox checking for style.appearance()). The adjustXyzStyle() basically set theme styles for the foreground (which is always drawn by the engine) and the paintXyz() methods draw the background directly to the cairo context via GTK foreign drawing (meaning basically background-color and rounded borders from GTK) Checking if the color was modified in isControlStyled results in the background also not being drawn. This already happens if the background is changed via CSS as we don't the rounded border in some tests. The difference is that if one sets the color now, we don't customize the background either. This is expected, expect I don't like having the inputs not rounded when the color is set :(
Carlos Bentzen
Comment 4 2018-06-14 05:22:06 PDT
Now I didn't include buttons in the patch because buttons already have some other logic coming from other classes. Basically the default color for the buttons is not RenderStyle::initialColor(), but RenderTheme::systemColor(): Color RenderThemeGtk::systemColor(CSSValueID cssValueId, OptionSet<StyleColor::Options> options) const { switch (cssValueId) { case CSSValueButtontext: return styleColor(Button, GTK_STATE_FLAG_ACTIVE, StyleColorForeground); case CSSValueCaptiontext: return styleColor(Entry, GTK_STATE_FLAG_ACTIVE, StyleColorForeground); default: return RenderTheme::systemColor(cssValueId, options); } } And checking for it in isControlStyled() would need to check the disabled color as well (which is also set before reaching isControlStyled() and hardcoded in RenderTheme::systemColor in the CSSGrayText clause)... anyway, a lot of logic outside the RenderTheme code and I was able to fix the regressions related to bug 126907 without having to change the button behavior here.
Carlos Bentzen
Comment 5 2018-06-14 05:26:45 PDT
Lading this would open room for the patch in bug 126907 to be landed back with just some minor modifications.
Michael Catanzaro
Comment 6 2018-06-14 09:51:44 PDT
Comment on attachment 342732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342732&action=review Makes sense to me. I wonder if Carlos Garcia will want to review this, as well. > Source/WebCore/rendering/RenderThemeGtk.cpp:930 > + if ((style.appearance() == TextFieldPart || style.appearance() == TextAreaPart || style.appearance() == SearchFieldPart) && style.color() != RenderStyle::initialColor()) > + return true; This is confusing enough that it merits a comment in the code to explain what is going on, I think. > LayoutTests/platform/gtk/TestExpectations:3399 > +# When text input colors are set we do not style with GTK, so these reftests will fail > +webkit.org/b/186219 fast/forms/input-placeholder-text-indent.html [ ImageOnlyFailure ] > +webkit.org/b/186219 fast/forms/password-placeholder-text-security.html [ ImageOnlyFailure ] > +webkit.org/b/186219 fast/forms/placeholder-with-positioned-element.html [ ImageOnlyFailure ] > +webkit.org/b/186219 fast/forms/textarea-placeholder-wrapping.html [ ImageOnlyFailure ] Since these are desired failures, they should go at the top of the file under section two.
Carlos Garcia Campos
Comment 7 2018-06-16 23:16:28 PDT
Comment on attachment 342732 [details] Patch LGTM
Carlos Bentzen
Comment 8 2018-06-17 00:43:14 PDT
Created attachment 342909 [details] Patch
WebKit Commit Bot
Comment 9 2018-06-17 08:47:45 PDT
Comment on attachment 342909 [details] Patch Clearing flags on attachment: 342909 Committed r232913: <https://trac.webkit.org/changeset/232913>
WebKit Commit Bot
Comment 10 2018-06-17 08:47:47 PDT
All reviewed patches have been landed. Closing bug.
Carlos Garcia Campos
Comment 11 2019-04-24 22:52:14 PDT
Reopening, this was reverted in r244635. We need to find a better solution for this.
Michael Catanzaro
Comment 12 2020-01-29 11:15:04 PST
Obsoleted by bug #197947.
Note You need to log in before you can comment on or make changes to this bug.