Bug 51152 - <select> elements don't honor border:0px in chromium-linux
Summary: <select> elements don't honor border:0px in chromium-linux
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P4 Minor
Assignee: Tony Chang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-15 17:03 PST by dgrogan
Modified: 2011-02-15 13:35 PST (History)
7 users (show)

See Also:


Attachments
Patch (58.28 KB, patch)
2010-12-15 18:06 PST, dgrogan
no flags Details | Formatted Diff | Diff
Patch (56.05 KB, patch)
2010-12-16 13:46 PST, dgrogan
tony: review-
Details | Formatted Diff | Diff
Patch with changelog (58.10 KB, patch)
2011-01-06 12:38 PST, dgrogan
no flags Details | Formatted Diff | Diff
Patch (6.78 KB, patch)
2011-02-10 16:46 PST, Tony Chang
no flags Details | Formatted Diff | Diff
fast/forms/basic-selects-actual.png (36.24 KB, image/png)
2011-02-11 11:20 PST, David Grogan
no flags Details
Patch (7.04 KB, patch)
2011-02-11 15:21 PST, Tony Chang
no flags Details | Formatted Diff | Diff
basic-selects-actual.png with rounded corners (35.86 KB, image/png)
2011-02-11 15:31 PST, Tony Chang
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description dgrogan 2010-12-15 17:03:19 PST
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.
Comment 1 dgrogan 2010-12-15 18:06:37 PST
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.
Comment 2 dgrogan 2010-12-16 13:46:37 PST
Created attachment 76811 [details]
Patch

Fixed paths in patch
Comment 3 Adam Langley 2010-12-17 09:30:08 PST
(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?
Comment 4 dgrogan 2010-12-17 13:16:49 PST
(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.
Comment 5 Adam Langley 2010-12-17 13:25:17 PST
(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
Comment 6 Tony Chang 2010-12-26 00:12:02 PST
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).
Comment 7 dgrogan 2011-01-06 12:38:38 PST
Created attachment 78151 [details]
Patch with changelog
Comment 8 Tony Chang 2011-01-06 12:48:26 PST
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 9 dgrogan 2011-01-07 10:11:42 PST
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
Comment 10 Tony Chang 2011-01-07 10:14:52 PST
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?
Comment 11 Tony Chang 2011-02-10 12:08:36 PST
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.
Comment 12 David Grogan 2011-02-10 12:15:19 PST
(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.
Comment 13 Tony Chang 2011-02-10 16:46:23 PST
Created attachment 82072 [details]
Patch
Comment 14 Tony Chang 2011-02-10 16:54:00 PST
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
Comment 15 xiyuan 2011-02-10 17:04:29 PST
(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
Comment 16 David Grogan 2011-02-11 11:20:43 PST
Created attachment 82149 [details]
fast/forms/basic-selects-actual.png

example of weird rounded borders on styled and unstyled selects
Comment 17 Tony Chang 2011-02-11 15:21:05 PST
Created attachment 82191 [details]
Patch
Comment 18 Tony Chang 2011-02-11 15:31:24 PST
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
Comment 19 David Grogan 2011-02-11 18:39:05 PST
Mine doesn't count, but LGTM.
Comment 20 xiyuan 2011-02-14 09:40:31 PST
(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
Comment 21 Dimitri Glazkov (Google) 2011-02-14 11:28:45 PST
Comment on attachment 82191 [details]
Patch

ok.
Comment 22 Tony Chang 2011-02-14 11:42:44 PST
Comment on attachment 82191 [details]
Patch

http://trac.webkit.org/changeset/78493
Comment 23 Tony Chang 2011-02-15 13:35:08 PST
This should now be fixed.  New baselines were committed in http://trac.webkit.org/changeset/78610 .