Bug 186219 – [GTK] Correct behavior for dark themes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Bentzen]

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.

[Michael Catanzaro]

(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]

Created attachment 342732 [details]
Patch

[Carlos Bentzen]

(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]

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]

Lading this would open room for the patch in bug 126907 to be landed back with just some minor modifications.

[Michael Catanzaro]

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 on attachment 342732 [details]
Patch

LGTM

[Carlos Bentzen]

Created attachment 342909 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 342909 [details]
Patch

Clearing flags on attachment: 342909

Committed r232913: <https://trac.webkit.org/changeset/232913>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

Reopening, this was reverted in r244635. We need to find a better solution for this.

[Michael Catanzaro]

Obsoleted by bug #197947.