Bug 56023

Summary: [Chromium] Fix default single selection select's popup background on chromium/linux
Product: WebKit Reporter: xiyuan <xiyuan>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, eric, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Proposed patch.
tony: review-
Added a layout test.
tony: review+
Fix nits.
none
Fix indentation in js functions. tony: review+

Description xiyuan 2011-03-09 09:42:00 PST
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.
Comment 1 xiyuan 2011-03-09 09:45:29 PST
Created attachment 85186 [details]
Proposed patch.
Comment 2 Tony Chang 2011-03-09 12:11:08 PST
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?
Comment 3 xiyuan 2011-03-09 13:00:32 PST
(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.
Comment 4 Tony Chang 2011-03-09 13:26:16 PST
(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.
Comment 5 xiyuan 2011-03-10 12:07:57 PST
Created attachment 85365 [details]
Added a layout test.

Added a layout test using getComputedStyle as suggested.
Comment 6 Tony Chang 2011-03-10 12:31:03 PST
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
Comment 7 xiyuan 2011-03-10 12:35:27 PST
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.
Comment 8 xiyuan 2011-03-10 12:35:57 PST
Created attachment 85372 [details]
Fix nits.
Comment 9 xiyuan 2011-03-10 12:38:14 PST
Created attachment 85374 [details]
Fix indentation in js functions.
Comment 10 Tony Chang 2011-03-10 15:46:37 PST
Committed r80777: <http://trac.webkit.org/changeset/80777>
Comment 11 WebKit Review Bot 2011-03-10 17:33:21 PST
http://trac.webkit.org/changeset/80777 might have broken GTK Linux 32-bit Release