[GTK] Difficult to read combo box text in dark theme

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

Moving this bug from downstream:

"""
When <select> item is styled, the dropdown does not respect that style for background and it can be hard to read selected value, since text color is respected. See the example code:

<style>
select {color:#F8F8F8;
        background:#000000;}
</style>
<select>
<option>Foo</option>
<option>Bar</option>
</select>

In epiphany it is very hard to see what exactly is selected since it shows light gray (F8F8F8) text on gray background (default for form controls).

Also when the drop down is opened, it is not styled at all.
"""

[Carlos Bentzen]

I just tested it here and it seems like now neither the text color nor background color are respected.. 

The combobox just sticks with the default theme style. So it's readable but not with the correct behaviour.

[Carlos Bentzen]

bug 118234 made WebKitGTK+ override CSS color with theme color.

[Carlos Bentzen]

Also from bug 118234:

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

[Michael Catanzaro]

Carlos Eduardo says the dark theme readability issue was fixed in bug #118234, closing.

*** This bug has been marked as a duplicate of bug 118234 ***

[Michael Catanzaro]

(In reply to Carlos Eduardo Ramalho from comment #3)
> Also from bug 118234:
> 
> > I think we need to implement RenderTheme::isControlStyled() properly to decide whether to apply native style or not.

Bug #185856

[Carlos Bentzen]

The example given works ok but if one sets size=2 in the select then it gets difficult to read again.

[Michael Catanzaro]

Reopening.

[Michael Catanzaro]

Carlos Eduardo, is this a bug you'd be interested in working on, now that https://bugs.webkit.org/show_bug.cgi?id=126907 is solved?

[Carlos Bentzen]

(In reply to Michael Catanzaro from comment #8)
> Carlos Eduardo, is this a bug you'd be interested in working on, now that
> https://bugs.webkit.org/show_bug.cgi?id=126907 is solved?

Yes :) I'll look into this one.

[Michael Catanzaro]

OK. Please pay attention to https://bugs.webkit.org/show_bug.cgi?id=186146 which is likely going to affect your work here.

[Carlos Bentzen]

Created attachment 341720 [details]
Patch

[Carlos Bentzen]

(In reply to Michael Catanzaro from comment #10)
> OK. Please pay attention to https://bugs.webkit.org/show_bug.cgi?id=186146
> which is likely going to affect your work here.

Actually my change was is another code path. As mentioned here

(In reply to Carlos Eduardo Ramalho from comment #6)
> The example given works ok but if one sets size=2 in the select then it gets
> difficult to read again.

The bug is still happening only when the "size" attribute is >1. In this case, adjustStyle in RenderTheme.cpp arrives with part = ListboxPart, while with size=1 it arrives with part = MenuListPart, which is the case bug 186146 is solving, if I understand correctly.

I believe s/combo box/option lists/g in the bug title would clarify this.

[Carlos Garcia Campos]

Comment on attachment 341720 [details]
Patch

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

> Source/WebCore/rendering/RenderTheme.cpp:219
> +#if PLATFORM(GTK)
> +    case ListboxPart:
> +        return adjustListboxStyle(styleResolver, style, element);
> +#endif

Do not use platform ifdefs here.

> Source/WebCore/rendering/RenderTheme.cpp:952
> +#if PLATFORM(GTK)
> +void RenderTheme::adjustListboxStyle(StyleResolver&, RenderStyle&, const Element*) const
> +{
> +}
> +#endif

We could have a default empty implementation in the header for ports not implementing this.

> Source/WebCore/rendering/RenderTheme.h:310
> +#if PLATFORM(GTK)
> +    virtual void adjustListboxStyle(StyleResolver&, RenderStyle&, const Element*) const;
> +#endif

Remove the platform ifdefs here and simply use an empty implementation here like other virtual methods do. I wouldn't make it const, even if it's true that the gtk implementation is const.

[Carlos Bentzen]

(In reply to Carlos Garcia Campos from comment #13)

Thanks for the review!

> Comment on attachment 341720 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341720&action=review
> 
> > Source/WebCore/rendering/RenderTheme.cpp:219
> > +#if PLATFORM(GTK)
> > +    case ListboxPart:
> > +        return adjustListboxStyle(styleResolver, style, element);
> > +#endif
> 
> Do not use platform ifdefs here.

OK

> 
> > Source/WebCore/rendering/RenderTheme.cpp:952
> > +#if PLATFORM(GTK)
> > +void RenderTheme::adjustListboxStyle(StyleResolver&, RenderStyle&, const Element*) const
> > +{
> > +}
> > +#endif
> 
> We could have a default empty implementation in the header for ports not
> implementing this.

OK

> 
> > Source/WebCore/rendering/RenderTheme.h:310
> > +#if PLATFORM(GTK)
> > +    virtual void adjustListboxStyle(StyleResolver&, RenderStyle&, const Element*) const;
> > +#endif
> 
> Remove the platform ifdefs here and simply use an empty implementation here
> like other virtual methods do. I wouldn't make it const, even if it's true
> that the gtk implementation is const.

All the others adjustFooStyle() methods are const, so I just did the same. Those adjustFooStyle methods are supposed to operate on the RenderStyle reference passed as parameter anyway, and do not change any member data. I think it's better to leave it as const for consistency.

[Carlos Bentzen]

Created attachment 341751 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 341751 [details]
Patch

Thanks!

[WebKit Commit Bot]

Comment on attachment 341751 [details]
Patch

Clearing flags on attachment: 341751

Committed r232392: <https://trac.webkit.org/changeset/232392>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

Reopening due to https://bugs.webkit.org/show_bug.cgi?id=186244

[Michael Catanzaro]

Obsoleted by bug #197947.