Bug 28631 - Chromium Linux: don't stretch checkboxes
Summary: Chromium Linux: don't stretch checkboxes
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2009-08-21 15:12 PDT by Adam Langley
Modified: 2010-01-14 13:24 PST (History)
4 users (show)

See Also:

patch (3.31 KB, patch)
2009-08-21 15:16 PDT, Adam Langley
no flags Details | Formatted Diff | Diff
updated (3.14 KB, patch)
2009-11-23 16:42 PST, Evan Stade
levin: review-
Details | Formatted Diff | Diff
plus test (26.37 KB, patch)
2009-11-24 17:56 PST, Evan Stade
no flags Details | Formatted Diff | Diff
html cleanup (26.36 KB, patch)
2009-11-24 17:58 PST, Evan Stade
eric: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Langley 2009-08-21 15:12:55 PDT
Currently, we'll stretch checkboxes and radio buttons to fill the given rectangle. Thus, if a site specifies width: 200px, the result looks very odd.

With this patch, we match IE in positioning a 13x13 checkbox at the bottom right of the rectangle.
Comment 1 Adam Langley 2009-08-21 15:16:07 PDT
Created attachment 38404 [details]
Comment 2 Evan Stade 2009-11-23 16:42:03 PST
Created attachment 43741 [details]

Adam seems to have forgotten this patch, so I have merged it to ToT and changed the logic to center rather than flushing bottom right (this matches what firefox appears to do). I am sending a run to the chromium layout test try bots to see if any layout test expectations need updating. I also tested to make sure that checkboxes are unchanged in the default case (e.g., gmail login screen).
Comment 3 Eric Seidel (no email) 2009-11-23 20:38:15 PST
Comment on attachment 43741 [details]

 226 IntRect center(const IntRect& original, int width, int height) {

Does this change match other webkit ports behaviors?  I would assume it does if it matches IE.
Comment 4 Evan Stade 2009-11-24 12:02:22 PST
it matches win safari, win chrome, and win ie (which are all the same presumably because they all draw checkboxes via windows system calls)
Comment 5 David Levin 2009-11-24 15:04:32 PST
Comment on attachment 43741 [details]

r- for
 If a WebKit layout test already covers this functionality, just list it in the ChangeLog. If not, please add a simple one.

> Index: WebCore/rendering/RenderThemeChromiumSkia.cpp
> +IntRect center(const IntRect& original, int width, int height) {

Brace needs to go on the next line for functions.
Comment 6 Evan Stade 2009-11-24 17:56:20 PST
Created attachment 43819 [details]
plus test
Comment 7 Evan Stade 2009-11-24 17:58:36 PST
Created attachment 43820 [details]
html cleanup
Comment 8 Eric Seidel (no email) 2009-11-26 21:17:31 PST
Comment on attachment 43820 [details]
html cleanup

I'm not a big fan of the name "center", but I'm not sure I have a better suggestion.  The patch looks fine.  We'll need results for other platforms.  I'm surprised that this isn't already covered by other tests.

Also, WebKit has no firm wrapping rule:
+        Make the default size for checkboxes/radio buttons also the maximum
+        size.

We generally just wrap when it's convenient. :)
Comment 9 Eric Seidel (no email) 2009-12-01 13:12:07 PST
Comment on attachment 43820 [details]
html cleanup

Evan asked me via email if this could be cq'd.  We could cq this, but then there are results that will be missing on the various bots and someone else will have to commit them in a separate run.  It might be easier to have a committer commit this manually and grab the results off the bot.  We really need some sort of try-server setup which could produce the various platform results to avoid this sort of pain. :(

You can of course always request a cq by setting commit-queue=?, but I think this particular patch should be landed by hand, unless someone wants to watch the bots after the commit-queue lands it, but at that point one might as well just land it by hand. :)
Comment 10 Eric Seidel (no email) 2009-12-29 09:47:01 PST
Comment on attachment 43820 [details]
html cleanup

I'll pull the results off of the windows bot once this lands.
Comment 11 WebKit Commit Bot 2009-12-29 09:52:40 PST
Comment on attachment 43820 [details]
html cleanup

Rejecting patch 43820 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueueSVN/LayoutTests
Testing 11853 test cases.
fast/css/non-standard-checkbox-size.html -> failed

Exiting early after 1 failures. 5218 tests run.
78.53s total testing time

5217 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/151590
Comment 12 Eric Seidel (no email) 2010-01-14 13:02:16 PST
Comment on attachment 43820 [details]
html cleanup

Test results are incorrect per the commit-queue.  Sorry you got a-run-around here, but the change you're attempting to make is not one which can be made drive-by.  Because it requires test results on many platforms it requires the engagement of an active committer.
Comment 13 Eric Seidel (no email) 2010-01-14 13:11:29 PST
I'll give landing this a whirl.
Comment 14 Eric Seidel (no email) 2010-01-14 13:12:18 PST
Comment on attachment 43820 [details]
html cleanup

Marking r+ again so the apply scripts work.
Comment 15 Eric Seidel (no email) 2010-01-14 13:21:26 PST
Committed r53289: <http://trac.webkit.org/changeset/53289>
Comment 16 Eric Seidel (no email) 2010-01-14 13:24:38 PST
I ended up just borrowing a trick from Chromium's book and skipping the test on platforms for which I don't have results.

I should have just encouraged you to do that in the first place.  I think WebKit is too scared of using the Skipped list.  Yes, this particular use of it isn't very good, but it's better than turning the bots red. :)  It's impractical to expect you, or anyone else, to be able to generate pixel results for all platforms with our current infrastructure.