Bug 165072

Summary: [GTK] Difficult to read combo box text in dark theme
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: agomez, bugs-noreply, cadubentzen, cgarcia, commit-queue, mcatanzaro
Priority: P2    
Version: WebKit Nightly Build   
Hardware: PC   
OS: Linux   
See Also: https://bugzilla.redhat.com/show_bug.cgi?id=1317542
https://bugs.webkit.org/show_bug.cgi?id=126907
https://bugs.webkit.org/show_bug.cgi?id=118234
https://bugs.webkit.org/show_bug.cgi?id=185856
https://bugs.webkit.org/show_bug.cgi?id=186146
https://bugs.webkit.org/show_bug.cgi?id=197947
Attachments:
Description Flags
Patch
none
Patch none

Michael Catanzaro
Reported 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. """
Attachments
Patch (5.17 KB, patch)
2018-05-31 18:04 PDT, Carlos Bentzen 		no flags
DetailsFormatted DiffDiff
Patch (4.60 KB, patch)
2018-06-01 05:00 PDT, Carlos Bentzen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Carlos Bentzen
Comment 1 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.
Carlos Bentzen
Comment 2 2018-05-15 20:39:45 PDT
bug 118234 made WebKitGTK+ override CSS color with theme color.
Carlos Bentzen
Comment 3 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.
Michael Catanzaro
Comment 4 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 ***
Michael Catanzaro
Comment 5 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
Carlos Bentzen
Comment 6 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.
Michael Catanzaro
Comment 7 2018-05-22 08:52:16 PDT
Reopening.
Michael Catanzaro
Comment 8 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?
Carlos Bentzen
Comment 9 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.
Michael Catanzaro
Comment 10 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.
Carlos Bentzen
Comment 11 2018-05-31 18:04:51 PDT
Created attachment 341720 [details] Patch
Carlos Bentzen
Comment 12 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.
Carlos Garcia Campos
Comment 13 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.
Carlos Bentzen
Comment 14 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.
Carlos Bentzen
Comment 15 2018-06-01 05:00:04 PDT
Created attachment 341751 [details] Patch
Carlos Garcia Campos
Comment 16 2018-06-01 05:35:48 PDT
Comment on attachment 341751 [details] Patch Thanks!
WebKit Commit Bot
Comment 17 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>
WebKit Commit Bot
Comment 18 2018-06-01 06:02:46 PDT
All reviewed patches have been landed. Closing bug.
Michael Catanzaro
Comment 19 2018-06-03 15:33:53 PDT
Reopening due to https://bugs.webkit.org/show_bug.cgi?id=186244
Michael Catanzaro
Comment 20 2020-01-29 11:14:51 PST
Obsoleted by bug #197947.
Note You need to log in before you can comment on or make changes to this bug.