[Chromium] chromium/linux breaks expectation of select popup background due to bad UA css rules

• Component: New Bugs
• Status: RESOLVED FIXED

[xiyuan]

Related chromium bug http://crbug.com/165031

The following UA css rule in themeChromiumLinux.css breaks some website's expectation that select background-color would be used as the popup's background-color:

select:not([size]):not([multiple]) option,
select[size="0"] option,
select[size="1"] option {
    background-color: #f7f7f7;
}

Example of such web page http://jsfiddle.net/rbyers/VEvCr/ (from [e-mail])

[xiyuan]

Created attachment 192254 [details]
Patch

[Tony Chang]

Comment on attachment 192254 [details]
Patch

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.

[Tony Chang]

I wonder if we can write a test for this using getComputedStyle on an <option> element.

[xiyuan]

Comment on attachment 192254 [details]
Patch

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.

Done.

>> 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.

Done. Thanks for the sample. It's well put.

>> Source/WebCore/platform/chromium/PopupListBox.cpp:409
>> +            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.

Right. I missed that.

Added a m_hasCustomBackgroundColor to PopupMenuStyle as suggested and set its value in RenderMenuList. However, isControlStyled depends on knowing an element's border/backgroundLayer/backgroundColor etc and hard to use out side StyleResolver context. I found a way to get UA style rules and set the custom background flag for <select> by comparing the UA rule's background with the current style's background.

[xiyuan]

(In reply to comment #3)
> I wonder if we can write a test for this using getComputedStyle on an <option> element.

getComputedStyle seems not able to get the actual background color of the popup list. :(

[xiyuan]

Created attachment 192336 [details]
Patch

[WebKit Review Bot]

Attachment 192336 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/themeChromiumLinux.css', u'Source/WebCore/platform/PopupMenuStyle.h', u'Source/WebCore/platform/chromium/PopupListBox.cpp', u'Source/WebCore/rendering/RenderMenuList.cpp', u'Source/WebCore/rendering/RenderMenuList.h', u'Source/WebCore/rendering/RenderSearchField.cpp', u'Source/WebCore/rendering/RenderThemeChromiumDefault.cpp', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/src/AutofillPopupMenuClient.cpp', u'Source/WebKit/chromium/tests/PopupMenuTest.cpp']" exit_code: 1
Source/WebKit/chromium/src/AutofillPopupMenuClient.cpp:294:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/src/AutofillPopupMenuClient.cpp:304:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[xiyuan]

Created attachment 192351 [details]
Patch

[Tony Chang]

Comment on attachment 192351 [details]
Patch

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

> Source/WebCore/platform/PopupMenuStyle.h:40
> +    PopupMenuStyle(const Color& foreground, const Color& background, const Font& font, bool visible, bool isDisplayNone, Length textIndent, TextDirection textDirection, bool hasTextDirectionOverride, BackgroundColorType backgroundColorType, PopupMenuType menuType = SelectPopup)

I would probably make BackgroundColorType an optional argument and make the default DefaultBackgroundColor since most of the callers don't care about this parameter.

> Source/WebCore/rendering/RenderMenuList.cpp:162
> +    bool backgroundColorChanged = !oldStyle || oldStyle->visitedDependentColor(CSSPropertyBackgroundColor) != style()->visitedDependentColor(CSSPropertyBackgroundColor);
> +    if (backgroundColorChanged)
> +        updateHasCustomBackgroundColor();

I think this is unnecessary and doesn't work.  This code only runs if the style is changed dynamically.

I would remove m_hasCustomBackgroundColor and instead pass in the <select> background color to itemStyle as a param.  Then you can use CustomBackgroundColor if the <select> background color doesn't match the passed in value and the option is transparent.

[xiyuan]

Comment on attachment 192351 [details]
Patch

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

>> Source/WebCore/platform/PopupMenuStyle.h:40
>> +    PopupMenuStyle(const Color& foreground, const Color& background, const Font& font, bool visible, bool isDisplayNone, Length textIndent, TextDirection textDirection, bool hasTextDirectionOverride, BackgroundColorType backgroundColorType, PopupMenuType menuType = SelectPopup)
> 
> I would probably make BackgroundColorType an optional argument and make the default DefaultBackgroundColor since most of the callers don't care about this parameter.

Done.

>> Source/WebCore/rendering/RenderMenuList.cpp:162
>> +        updateHasCustomBackgroundColor();
> 
> I think this is unnecessary and doesn't work.  This code only runs if the style is changed dynamically.
> 
> I would remove m_hasCustomBackgroundColor and instead pass in the <select> background color to itemStyle as a param.  Then you can use CustomBackgroundColor if the <select> background color doesn't match the passed in value and the option is transparent.

Removed the unnecessary flag and logic and set CustomBackgroundColor if the background color is tinted from <option>.

[xiyuan]

Created attachment 192518 [details]
Patch

[Tony Chang]

Comment on attachment 192518 [details]
Patch

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

Looking good.  Just some final style nits.

> Source/WebCore/ChangeLog:19
> +        No new tests (no change in behaviour).

I would write: No new tests, this tests <select> popups and can be verified by ManualTests/select-scroll.html.

> Source/WebCore/css/themeChromiumLinux.css:34
> +    /* Selects look like buttons with a drop down triangle on Linux */

Nit: End the sentence with a period.

> Source/WebCore/rendering/RenderMenuList.cpp:469
> +    getItemBackgroundColorAndFlag(listIndex, itemBackgroundColor, itemHasCustomBackgroundColor);

AndFlag doesn't seem to add much to this function.  I would just call it getItemBackgroundColor.

> Source/WebCore/rendering/RenderMenuList.cpp:477
> +void RenderMenuList::getItemBackgroundColorAndFlag(unsigned listIndex, Color& outItemBackgroundColor, bool& outItemHasCustomBackgroundColor) const

I don't think it's WebKit style to prefix parameters with 'out'.  I would drop that prefix.

> Source/WebCore/rendering/RenderMenuList.h:118
> +    void getItemBackgroundColorAndFlag(unsigned listIndex, Color& outItemBackgroundColor, bool& outItemHasCustomBackgroundColor) const;

Nit: Color does not need to be named.

> Source/WebKit/chromium/src/AutofillPopupMenuClient.cpp:295
>  

Should this be Custom or Default?  I guess I would use Default since it's the original value.

[xiyuan]

Created attachment 192527 [details]
Patch

[xiyuan]

Comment on attachment 192518 [details]
Patch

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

>> Source/WebCore/ChangeLog:19
>> +        No new tests (no change in behaviour).
> 
> I would write: No new tests, this tests <select> popups and can be verified by ManualTests/select-scroll.html.

Done

>> Source/WebCore/css/themeChromiumLinux.css:34
>> +    /* Selects look like buttons with a drop down triangle on Linux */
> 
> Nit: End the sentence with a period.

Done.

>> Source/WebCore/rendering/RenderMenuList.cpp:469
>> +    getItemBackgroundColorAndFlag(listIndex, itemBackgroundColor, itemHasCustomBackgroundColor);
> 
> AndFlag doesn't seem to add much to this function.  I would just call it getItemBackgroundColor.

Done.

>> Source/WebCore/rendering/RenderMenuList.cpp:477
>> +void RenderMenuList::getItemBackgroundColorAndFlag(unsigned listIndex, Color& outItemBackgroundColor, bool& outItemHasCustomBackgroundColor) const
> 
> I don't think it's WebKit style to prefix parameters with 'out'.  I would drop that prefix.

Done.

>> Source/WebCore/rendering/RenderMenuList.h:118
>> +    void getItemBackgroundColorAndFlag(unsigned listIndex, Color& outItemBackgroundColor, bool& outItemHasCustomBackgroundColor) const;
> 
> Nit: Color does not need to be named.

Done.

>> Source/WebKit/chromium/src/AutofillPopupMenuClient.cpp:295
>>  
> 
> Should this be Custom or Default?  I guess I would use Default since it's the original value.

Default is subject to the color mapping and is more correct because we don't want that to happen for autofill popup.

[WebKit Review Bot]

Comment on attachment 192527 [details]
Patch

Clearing flags on attachment: 192527

Committed r145406: <http://trac.webkit.org/changeset/145406>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Hin-Chung Lam]

Reopening to attach new patch.

[Hin-Chung Lam]

Created attachment 192664 [details]
Patch

[Hin-Chung Lam]

Whoops.. webkit-patch failed me..

[xiyuan]

*** Bug 69619 has been marked as a duplicate of this bug. ***