Bug 118234 - [GTK] Menu list button doesn't use the text color from the theme
Summary: [GTK] Menu list button doesn't use the text color from the theme
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2013-07-01 03:33 PDT by Carlos Garcia Campos
Modified: 2019-05-16 06:56 PDT (History)
9 users (show)

See Also:


Attachments
Patch (4.50 KB, patch)
2013-07-01 03:40 PDT, Carlos Garcia Campos
mcatanzaro: review-
Details | Formatted Diff | Diff
Patch (4.12 KB, patch)
2016-04-15 04:26 PDT, Carlos Garcia Campos
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2013-07-01 03:33:37 PDT
GtkComboBox uses a cell area, so the text color is defined in the cell css class.
Comment 1 Carlos Garcia Campos 2013-07-01 03:40:31 PDT
Created attachment 205793 [details]
Patch
Comment 2 Martin Robinson 2013-07-01 06:49:18 PDT
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?
Comment 3 Carlos Garcia Campos 2013-07-01 07:03:19 PDT
(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.
Comment 4 Martin Robinson 2013-07-01 08:05:15 PDT
(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.
Comment 5 Xavier Claessens 2013-07-04 02:00:47 PDT
I confirm it fixes the bug in my case. Thanks!
Comment 6 Claudio Saavedra 2013-09-01 07:56:17 PDT
I think the same bug is happening with input buttons.
Comment 7 Claudio Saavedra 2013-09-01 08:05:19 PDT
(In reply to comment #6)
> I think the same bug is happening with input buttons.

Actually no; this seems to be a different bug.
Comment 8 Claudio Saavedra 2013-12-08 07:31:10 PST
Martin, do you object to this patch? It would be good to get it in.
Comment 9 Martin Robinson 2013-12-09 00:23:09 PST
(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.
Comment 10 Michael Catanzaro 2015-12-31 16:12:24 PST
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....
Comment 11 Carlos Garcia Campos 2016-04-15 04:26:33 PDT
Created attachment 276473 [details]
Patch

People using dark themes have suffered this for too long already.
Comment 12 Carlos Garcia Campos 2016-04-15 06:14:48 PDT
(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.
Comment 13 Darin Adler 2016-04-15 11:08:13 PDT
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();
Comment 14 Carlos Garcia Campos 2016-04-18 01:44:54 PDT
Committed r199659: <http://trac.webkit.org/changeset/199659>
Comment 15 Michael Catanzaro 2018-05-21 18:39:27 PDT
*** Bug 165072 has been marked as a duplicate of this bug. ***