Bug 56023 – [Chromium] Fix default single selection select's popup background on chromium/linux

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[xiyuan]

http://trac.webkit.org/changeset/78137 is supposed to change select (menuListButton apperance) popup background to #f7f7f7 on chromium/linux. However the css selector does not apply to default single selection select so it remains #dddddd.

[xiyuan]

Created attachment 85186 [details]
Proposed patch.

[Tony Chang]

Comment on attachment 85186 [details]
Proposed patch.

A pixel test can't capture this change, right?  Is this covered by a manual test?  If so, can you mention the test in the ChangeLog.  If not, can you add a manual test for this?

[xiyuan]

(In reply to comment #2)
> (From update of attachment 85186 [details])
> A pixel test can't capture this change, right?  Is this covered by a manual test?  If so, can you mention the test in the ChangeLog.  If not, can you add a manual test for this?

Select popup (aka dropdown) is not covered in pixel tests because popup is in another render widget and this is not supported by the test infrastructure currently.

I am adding a rule so that the default single selection select use #f7f7f7 background color. What do we want to manual test to test? To test that default select and select with size='0'|'1' attribute use the same popup background color?

Please clarify a bit. Thanks.

[Tony Chang]

(In reply to comment #3)
> I am adding a rule so that the default single selection select use #f7f7f7 background color. What do we want to manual test to test? To test that default select and select with size='0'|'1' attribute use the same popup background color?

Yes, exactly, that sounds like a good manual test to have.

I also wonder if it's possible to use getComputedStyle() to check this in an automated fashion.  Maybe not.

[xiyuan]

Created attachment 85365 [details]
Added a layout test.

Added a layout test using getComputedStyle as suggested.

[Tony Chang]

Comment on attachment 85365 [details]
Added a layout test.

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

> LayoutTests/fast/html/select-dropdown-consistent-background-color.html:20
> +  $('result').textContent=

Nit: add a space before the =

> LayoutTests/fast/html/select-dropdown-consistent-background-color.html:21
> +    (default_color == size0_color && default_color == size1_color) ? "PASS"

Nit: 4 space indent

> LayoutTests/fast/html/select-dropdown-consistent-background-color.html:24
> +    layoutTestController.dumpAsText();

Nit: 4 space indent

> LayoutTests/fast/html/select-dropdown-consistent-background-color.html:34
> +  <option>Item 1</option>
> +  <option>Item 2</option>

Nit: html should probably also use a 4 space indent

[xiyuan]

Comment on attachment 85365 [details]
Added a layout test.

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

>> LayoutTests/fast/html/select-dropdown-consistent-background-color.html:20
>> +  $('result').textContent=
> 
> Nit: add a space before the =

Done.

>> LayoutTests/fast/html/select-dropdown-consistent-background-color.html:21
>> +    (default_color == size0_color && default_color == size1_color) ? "PASS"
> 
> Nit: 4 space indent

Done.

>> LayoutTests/fast/html/select-dropdown-consistent-background-color.html:24
>> +    layoutTestController.dumpAsText();
> 
> Nit: 4 space indent

Done.

>> LayoutTests/fast/html/select-dropdown-consistent-background-color.html:34
>> +  <option>Item 2</option>
> 
> Nit: html should probably also use a 4 space indent

Done.

[xiyuan]

Created attachment 85372 [details]
Fix nits.

[xiyuan]

Created attachment 85374 [details]
Fix indentation in js functions.

[Tony Chang]

Committed r80777: <http://trac.webkit.org/changeset/80777>

[WebKit Review Bot]

http://trac.webkit.org/changeset/80777 might have broken GTK Linux 32-bit Release