[GTK] Menu list button doesn't use the text color from the theme

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

GtkComboBox uses a cell area, so the text color is defined in the cell css class.

[Carlos Garcia Campos]

Created attachment 205793 [details]
Patch

[Martin Robinson]

Comment on attachment 205793 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205793&action=review

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:220
> +    style->setColor(menuListButtonTextColor(element));

Doesn't this prevent the CSS theme from adjusting the text color?

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 205793 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205793&action=review
> 
> > Source/WebCore/platform/gtk/RenderThemeGtk.cpp:220
> > +    style->setColor(menuListButtonTextColor(element));
> 
> Doesn't this prevent the CSS theme from adjusting the text color?

I'm not sure, maybe the css color is overriden after this. Check RenderThemeMac::adjustMenuListStyle, mac also sets the color there.

[Martin Robinson]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 205793 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=205793&action=review
> > 
> > > Source/WebCore/platform/gtk/RenderThemeGtk.cpp:220
> > > +    style->setColor(menuListButtonTextColor(element));
> > 
> > Doesn't this prevent the CSS theme from adjusting the text color?
> 
> I'm not sure, maybe the css color is overriden after this. Check RenderThemeMac::adjustMenuListStyle, mac also sets the color there.

It might be worth overriding it always, but it also should be possible to avoid it, I think, by only overriding it if it is set to the default color.

[Xavier Claessens]

I confirm it fixes the bug in my case. Thanks!

[Claudio Saavedra]

I think the same bug is happening with input buttons.

[Claudio Saavedra]

(In reply to comment #6)
> I think the same bug is happening with input buttons.

Actually no; this seems to be a different bug.

[Claudio Saavedra]

Martin, do you object to this patch? It would be good to get it in.

[Martin Robinson]

(In reply to comment #8)
> Martin, do you object to this patch? It would be good to get it in.

This patch overrides the color even if the CSS changes it. I think it should either have RenderThemeGtk::systemColor the proper color or if that is impossible to check if the style specifies the default color and in that case to override it conditionally.

[Michael Catanzaro]

Comment on attachment 205793 [details]
Patch

"This patch overrides the color even if the CSS changes it." <-- This is a problem. I guess it's probably a problem elsewhere in this file, though.

Also, this code won't work now that the button style class was removed from GTK+. I think the cell style class might be due for removal as well....

[Carlos Garcia Campos]

Created attachment 276473 [details]
Patch

People using dark themes have suffered this for too long already.

[Carlos Garcia Campos]

(In reply to comment #10)
> Comment on attachment 205793 [details]
> Patch
> 
> "This patch overrides the color even if the CSS changes it." <-- This is a
> problem. I guess it's probably a problem elsewhere in this file, though.
> 
> Also, this code won't work now that the button style class was removed from
> GTK+. I think the cell style class might be due for removal as well....

I think we need to implement RenderTheme::isControlStyled() properly to decide whether to apply native style or not.

[Darin Adler]

Comment on attachment 276473 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276473&action=review

> Source/WebCore/rendering/RenderThemeGtk.cpp:776
> +#if GTK_CHECK_VERSION(3, 20, 0)

Seems slightly nicer to put the #if inside the function since there is no difference in the arguments type or usage.

> Source/WebCore/rendering/RenderThemeGtk.cpp:779
> +    RenderThemeGadget::Info info = { RenderThemeGadget::Type::Generic, "combobox", element->isDisabledFormControl() ? GTK_STATE_FLAG_INSENSITIVE : GTK_STATE_FLAG_NORMAL, { } };

I believe there is no need for the "=" here.

> Source/WebCore/rendering/RenderThemeGtk.cpp:781
> +    Vector<RenderThemeGadget::Info> children = {

I believe there is no need for the "=" here.

> Source/WebCore/rendering/RenderThemeGtk.cpp:786
> +    return std::make_unique<RenderThemeBoxGadget>(info, children, comboGadget.get())->child(0)->color();

Why does this need to be constructed on the heap with make_unique instead of just constructing it on the stack?

    return RenderThemeBoxGadget(info, children, comboGadget.get()).child(0)->color();

[Carlos Garcia Campos]

Committed r199659: <http://trac.webkit.org/changeset/199659>

[Michael Catanzaro]

*** Bug 165072 has been marked as a duplicate of this bug. ***