Bug 38016 – Update padding on Windows?

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Ojan Vafai]

In bug 35629, Chromium removed the extra internal padding on buttons since it was causing compatibility problems. So far we've gotten no compatibility bugs filed, but we have had a few complaints that buttons look ugly. Through some discussion with people complaining, it looks like adding back in the 1px of top/bottom padding will meet their needs without adding a compatibility hit.

I propose that we get rid of the internal button padding on Windows (and thus the need for a concept of internal button padding) and instead add the following to themeWin.css, which increases padding-top/bottom by 1px from what's in html.css:
input[type="button"], input[type="submit"], input[type="reset"], input[type="file"]::-webkit-file-upload-button, button {
    padding-top: 3px;
    padding-bottom: 4px;
}

This will minimize compatibility problems without making the buttons look awful.

That sound OK to you all?

[Shinichiro Hamaji]

Looks like we've already had the selector in themeWin.css ?

themeWin.css:

input[type="button"], input[type="submit"], input[type="reset"], input[type="file"]::-webkit-file-upload-button, button {
    /* Matches Firefox */
    padding: 0 6px;
}

So, we may want to have "padding: 1px 6px;" instead?

A demo: http://tinyurl.com/2358nda

[Ojan Vafai]

(In reply to comment #1)
> themeWin.css:
> input[type="button"], input[type="submit"], input[type="reset"],
> input[type="file"]::-webkit-file-upload-button, button {
>     /* Matches Firefox */
>     padding: 0 6px;
> }
> 
> So, we may want to have "padding: 1px 6px;" instead?

Good point. :) We should also remove the erroneous "matches firefox" comment.

[Shinichiro Hamaji]

Created attachment 57427 [details]
Patch v1

[Shinichiro Hamaji]

Created attachment 57428 [details]
Patch v1 - a typo fix

[Shinichiro Hamaji]

Comment on attachment 57428 [details]
Patch v1 - a typo fix

This patch will change the layout test results on windows. I'm planning to land an expectation fix soon after this patch is landed. It seems my windows machine doesn't generate the same output as the buildbot.

[Ojan Vafai]

Comment on attachment 57428 [details]
Patch v1 - a typo fix

Adele, hyatt, mitz I'd appreciate if one of you could approve this. The code looks good to me. I'm hesitant to r+ since I'm the one who suggested the change in the first place. 

(In reply to comment #5)
> (From update of attachment 57428 [details])
> This patch will change the layout test results on windows. I'm planning to land an expectation fix soon after this patch is landed. It seems my windows machine doesn't generate the same output as the buildbot.

Unfortunately, the bots will stop running the tests after 20 tests have failed (we really should increase that number to 100 or something). So, we'll need to find another way to get new test results. I also have never succeeded in getting all the layout tests to pass on Windows. It's a bit gross, but we could add the tests to the Windows skipped list, then take 15 off at a time and grab those results off the bot. If you do that during off-hours for PST, I think it's probably OK.

r- for the tests since this can't be submitted as is.

[Shinichiro Hamaji]

Created attachment 60090 [details]
Patch v2

[Shinichiro Hamaji]

> Unfortunately, the bots will stop running the tests after 20 tests have failed (we really should increase that number to 100 or something). So, we'll need to find another way to get new test results. I also have never succeeded in getting all the layout tests to pass on Windows. It's a bit gross, but we could add the tests to the Windows skipped list, then take 15 off at a time and grab those results off the bot. If you do that during off-hours for PST, I think it's probably OK.

My updated patch skips all tests which are failing on Chromium. Soon after I land this patch, I'll grab results of them and remove the big skip list. Does this plan sound OK?

[Ojan Vafai]

Comment on attachment 60090 [details]
Patch v2

> +++ b/WebCore/css/html.css
> @@ -304,7 +304,7 @@ button {
>  input, textarea, keygen, select, button, isindex, datagrid {
>      margin: 0__qem;
>      font: -webkit-small-control;
> -    color: initial;
> +    color: CaptionText;
>      letter-spacing: normal;
>      word-spacing: normal;
>      line-height: normal;
> @@ -486,7 +486,6 @@ keygen, select {
>      -webkit-border-radius: 5px;
>      white-space: pre;
>      -webkit-rtl-ordering: logical;
> -    color: black;
>      background-color: white;

Did you mean to include this?

[Shinichiro Hamaji]

Created attachment 60837 [details]
Patch v3

[Shinichiro Hamaji]

> Did you mean to include this?

Oops! Removed, thanks.

[Ojan Vafai]

Comment on attachment 60837 [details]
Patch v3

> +++ b/LayoutTests/platform/win/Skipped

Ugh. I know I asked you to add these to the Skipped list, but if you can wait a day or two, then you won't need to do this. Once the bots are all idle (hopefully tonight), we'll restart the buildbot master and the bots will change to only abort early if there are >=20 crashes/timeouts. So you should be able to submit this and then get new expected results for all the tests at once without messing with the Skipped list.

Other than that, r=me.

[Shinichiro Hamaji]

> Ugh. I know I asked you to add these to the Skipped list, but if you can wait a day or two, then you won't need to do this. Once the bots are all idle (hopefully tonight), we'll restart the buildbot master and the bots will change to only abort early if there are >=20 crashes/timeouts. So you should be able to submit this and then get new expected results for all the tests at once without messing with the Skipped list.

I see. Thanks for the info! So, the cool feature will be available in Sunday midnight in PST ? If so, I'd land this patch in this time (it's Monday afternoon in JST and it would be the best time).

[Shinichiro Hamaji]

Committed r63071: <http://trac.webkit.org/changeset/63071>

[Shinichiro Hamaji]

(In reply to comment #14)
> Committed r63071: <http://trac.webkit.org/changeset/63071>

Hmm... I don't see any new failures in Win bot. Maybe win port didn't use this code? I'll investigate more tomorrow.

[Shinichiro Hamaji]

Committed r63196: <http://trac.webkit.org/changeset/63196>

[Shinichiro Hamaji]

Committed r63220: <http://trac.webkit.org/changeset/63220>