Bug 186219

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

Created attachment 342732 [details]
Patch

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

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.

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

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.

Comment on attachment 342732 [details]
Patch

LGTM

Created attachment 342909 [details]
Patch

Comment on attachment 342909 [details]
Patch

Clearing flags on attachment: 342909

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

All reviewed patches have been landed.  Closing bug.

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

Obsoleted by bug #197947.