Bug 165072 - [GTK] Difficult to read combo box text in dark theme
Summary: [GTK] Difficult to read combo box text in dark theme
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-11-25 09:28 PST by Michael Catanzaro
Modified: 2019-05-16 06:56 PDT (History)
6 users (show)

See Also:


Attachments
Patch (5.17 KB, patch)
2018-05-31 18:04 PDT, Carlos Eduardo Ramalho
no flags Details | Formatted Diff | Diff
Patch (4.60 KB, patch)
2018-06-01 05:00 PDT, Carlos Eduardo Ramalho
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2016-11-25 09:28:48 PST
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.
"""
Comment 1 Carlos Eduardo Ramalho 2018-05-15 20:10:19 PDT
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.
Comment 2 Carlos Eduardo Ramalho 2018-05-15 20:39:45 PDT
bug 118234 made WebKitGTK+ override CSS color with theme color.
Comment 3 Carlos Eduardo Ramalho 2018-05-15 20:43:53 PDT
Also from bug 118234:

> I think we need to implement RenderTheme::isControlStyled() properly to decide whether to apply native style or not.
Comment 4 Michael Catanzaro 2018-05-21 18:39:27 PDT
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 ***
Comment 5 Michael Catanzaro 2018-05-21 19:03:49 PDT
(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
Comment 6 Carlos Eduardo Ramalho 2018-05-22 08:49:16 PDT
The example given works ok but if one sets size=2 in the select then it gets difficult to read again.
Comment 7 Michael Catanzaro 2018-05-22 08:52:16 PDT
Reopening.
Comment 8 Michael Catanzaro 2018-05-31 08:58:18 PDT
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?
Comment 9 Carlos Eduardo Ramalho 2018-05-31 15:23:50 PDT
(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.
Comment 10 Michael Catanzaro 2018-05-31 17:04:38 PDT
OK. Please pay attention to https://bugs.webkit.org/show_bug.cgi?id=186146 which is likely going to affect your work here.
Comment 11 Carlos Eduardo Ramalho 2018-05-31 18:04:51 PDT
Created attachment 341720 [details]
Patch
Comment 12 Carlos Eduardo Ramalho 2018-05-31 18:09:42 PDT
(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.
Comment 13 Carlos Garcia Campos 2018-05-31 23:56:46 PDT
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.
Comment 14 Carlos Eduardo Ramalho 2018-06-01 04:58:19 PDT
(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.
Comment 15 Carlos Eduardo Ramalho 2018-06-01 05:00:04 PDT
Created attachment 341751 [details]
Patch
Comment 16 Carlos Garcia Campos 2018-06-01 05:35:48 PDT
Comment on attachment 341751 [details]
Patch

Thanks!
Comment 17 WebKit Commit Bot 2018-06-01 06:02:44 PDT
Comment on attachment 341751 [details]
Patch

Clearing flags on attachment: 341751

Committed r232392: <https://trac.webkit.org/changeset/232392>
Comment 18 WebKit Commit Bot 2018-06-01 06:02:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Michael Catanzaro 2018-06-03 15:33:53 PDT
Reopening due to https://bugs.webkit.org/show_bug.cgi?id=186244