[webkit-reviews] review denied: [Bug 111873] [Chromium] chromium/linux breaks expectation of select popup background due to bad UA css rules : [Attachment 192254] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied xiyuan <xiyuan at chromium.org>'s
request for review:
Bug 111873: [Chromium] chromium/linux breaks expectation of select popup
background due to bad UA css rules
https://bugs.webkit.org/show_bug.cgi?id=111873

Attachment 192254: Patch
https://bugs.webkit.org/attachment.cgi?id=192254&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=192254&action=review


> Source/WebCore/css/themeChromiumLinux.css:33
>  select {

Nit: I would probably add a comment here saying that on Linux, selects look
like buttons with a drop down triangle.

> Source/WebCore/platform/chromium/PopupListBox.cpp:407
> +	   // Map background color from default button face to menu color.

This should be a 'why' comment, not a 'what' comment.  For example:
On other platforms, the <option> background color is the same as the <select>
background color. On Linux, that makes the <option> background color very dark,
so by default, try to use a lighter background color for <option>s.

> Source/WebCore/platform/chromium/PopupListBox.cpp:409
> +	   if (RenderTheme::defaultTheme()->systemColor(CSSValueButtonface) ==
backColor)
> +	       backColor =
RenderTheme::defaultTheme()->systemColor(CSSValueMenu);

I don't think this is correct since this is only looking at the <option>.  For
example, if someone explicitly had "option { background-color: #ddd; }", we
would override the color.

I think you want to add a bool to PopupMenuStyle like
m_hasCustomBackgroundColor and in itemStyle(), set m_hasCustomBackgroundColor
if the <select> is styled (maybe isControlStyled?) and the option is
transparent.