Bug 51152 – <select> elements don't honor border:0px in chromium-linux

• Component: Forms
• Status: RESOLVED FIXED

A <select> menu with height:20px;border:0px is rendered a bit different in Linux than it is in Windows. In Linux 18px are used for the actual menu with a 1px grey border. In Windows 20px are used for the menu with no border.

Chromium report: at http://code.google.com/p/chromium/issues/detail?id=41729

Proposed patch forthcoming.

Created attachment 76722 [details]
Patch

This change makes selects larger to match their size in windows.  An example test update is included in this patch.  Approximately 50 layout tests are affected in the same manner as this example and will have to be rebaselined for chromium-linux.

It looks like themeChromiumLinux.css was only added in order to avoid rebaselining.  If so, and I'm about to take the rebaselining hit anyway, should I delete it?  That would allow a lot of the affected tests' expected render tree dumps (but not the pngs) in chromium-linux to be deleted because the rebaselined dumps would match chromium-win.  I'm not sure if that's desired, but it's an option.

Created attachment 76811 [details]
Patch

Fixed paths in patch

[Adam Langley]

(Note: I am not a WebKit reviewer. You need an r+ from a real reviewer as well.)

Code change LGTM. I'm not sure about the CSS.

The background colour in the CSS replaces the default system colour of selects and allows us to assume that we always have a CSS colour in the code.

Adding border: 0px seems wrong. Is the Windows default to have no border?

(In reply to comment #3)
> (Note: I am not a WebKit reviewer. You need an r+ from a real reviewer as well.)
> 
> Code change LGTM. I'm not sure about the CSS.
> 
> The background colour in the CSS replaces the default system colour of selects and allows us to assume that we always have a CSS colour in the code.

Wouldn't the select entry in html.css that sets background-color: white also allow the code to make that assumption?
https://trac.webkit.org/browser/trunk/WebCore/css/html.css?rev=73613#L507
This is probably more complicated than I initially thought; I'm ok with keeping it as-is.

> Adding border: 0px seems wrong. Is the Windows default to have no border?

I agree that border:0px seems wrong.  This change removes it.  (I think you misread the patch:)  Windows defaults to a border of 1px.

[Adam Langley]

(In reply to comment #4)
> Wouldn't the select entry in html.css that sets background-color: white also allow the code to make that assumption?

Yes. But with the wrong colour :)

> > Adding border: 0px seems wrong. Is the Windows default to have no border?
 
> I agree that border:0px seems wrong.  This change removes it.  (I think you misread the patch:)  Windows defaults to a border of 1px.

Ah. *I* added the border:0. I wonder why.

LGTM

[Tony Chang]

Comment on attachment 76811 [details]
Patch

You're missing a ChangeLog.  Please run prepare-ChangeLog or use webkit-patch (which should run prepare-ChangeLog for you).

Created attachment 78151 [details]
Patch with changelog

[Tony Chang]

Comment on attachment 78151 [details]
Patch with changelog

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

> third_party/WebKit/LayoutTests/platform/chromium-linux/editing/pasteboard/4641033-expected.txt:19
> -      RenderBlock {DIV} at (0,56) size 784x109
> +      RenderBlock {DIV} at (0,56) size 784x110

Why does the div change height here?  If I understand you correctly, we should just be moving two pixel from the border into the <select> rendering.  That said, if this makes us match Windows (i.e., windows renders this div at 110px), this seems like a fine change to make.

Comment on attachment 78151 [details]
Patch with changelog

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

>> third_party/WebKit/LayoutTests/platform/chromium-linux/editing/pasteboard/4641033-expected.txt:19
>> +      RenderBlock {DIV} at (0,56) size 784x110
> 
> Why does the div change height here?  If I understand you correctly, we should just be moving two pixel from the border into the <select> rendering.  That said, if this makes us match Windows (i.e., windows renders this div at 110px), this seems like a fine change to make.

I'm not sure why the size used to be smaller. The new size does match windows though: http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium-win/editing/pasteboard/4641033-expected.txt#L19

[Tony Chang]

Comment on attachment 78151 [details]
Patch with changelog

Ok, I think the code change is good.  Can you upload the patch with all the updated layout test results?

[Tony Chang]

Comment on attachment 78151 [details]
Patch with changelog

I'm going to try to resurrect this patch.  It's not easy since the theme code moved into chromium.

[David Grogan]

(In reply to comment #11)
> (From update of attachment 78151 [details])
> I'm going to try to resurrect this patch.  It's not easy since the theme code moved into chromium.

Yeah, sorry about my neglect.  I have a branch with a possible first round of chromium-side changes but haven't uploaded for review.  That was a couple of weeks ago though so things might have changed again since then.

Feel free to resurrect on your own or let me know if you'd like me to get the chrome-side stuff cleaned up and uploaded in the next couple of days.

[Tony Chang]

Created attachment 82072 [details]
Patch

[Tony Chang]

xiyuan, can you review this?  The chromium side patch is http://codereview.chromium.org/6490014 .

I think the right way to land this two sided patch is:
1) land the webkit side first with the tests that need new baselines marked as failing.
2) wait for the change to be rolled into chromium
3) land the chromium side change
4) update webkit DEPS to pull in the chromium change, land the new baselines, and remove the test_expectations.txt lines

[xiyuan]

(In reply to comment #14)
> xiyuan, can you review this?  The chromium side patch is http://codereview.chromium.org/6490014 .
> 
> I think the right way to land this two sided patch is:
> 1) land the webkit side first with the tests that need new baselines marked as failing.
> 2) wait for the change to be rolled into chromium
> 3) land the chromium side change
> 4) update webkit DEPS to pull in the chromium change, land the new baselines, and remove the test_expectations.txt lines

LGTM

Sorry that I made things are so complicated now. We might want to follow what win layout tests do, i.e. have a WebThemeEngine for layout test, to make things simple. 

Still not a committer to webkit so could not give you a review+. :p

[David Grogan]

Created attachment 82149 [details]
fast/forms/basic-selects-actual.png

example of weird rounded borders on styled and unstyled selects

[Tony Chang]

Created attachment 82191 [details]
Patch

[Tony Chang]

Created attachment 82195 [details]
basic-selects-actual.png with rounded corners

Here's a new patch where I pass through whether there are rounded corners.  If there are rounded corners, we only draw the drop down arrow and let WebCore draw the background and border.  This has the side effect of the button not getting the gradient if there's a border radius, but that seems OK.

Here are the platform results on win, mac, and for reference, on chromium linux.  GTK+ and QT don't seem to handle the border radius.

http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium-win/fast/forms/basic-selects-expected.png
http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/fast/forms/basic-selects-expected.png
http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium-linux/fast/forms/basic-selects-expected.png

[David Grogan]

Mine doesn't count, but LGTM.

[xiyuan]

(In reply to comment #17)
> Created an attachment (id=82191) [details]
> Patch

LGTM

Still not a committer and need help to give a review+. :p

[Dimitri Glazkov (Google)]

Comment on attachment 82191 [details]
Patch

ok.

[Tony Chang]

Comment on attachment 82191 [details]
Patch

http://trac.webkit.org/changeset/78493

[Tony Chang]

This should now be fixed.  New baselines were committed in http://trac.webkit.org/changeset/78610 .