[webui] fix vertical alignment of buttons and text inputs

[webui] fix vertical alignment of buttons and text inputs

The crux of the problem is that buttons and text inputs are being aligned on their baseline, which I believe is correct. However they place their baseline differently relative to the top and bottom border, so the top and bottom borders wind up misaligned. To correct this, we have to add padding to the buttons and/or text inputs. The amount of padding is platform-specific.

Using vertical-align: (not-baseline) would work, but would make it look bad because the baselines would not align.

BUG=117662
TEST=visual (on win, mac, linux, chromeos)


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126796

[Evan Stade, 2012-03-13 01:35:31]

dbeam: please double check on windows (you don't have to compile -- just add the
new rule via the inspector)

csilv: same, for mac

I've checked linux and chromeos.

http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/shared...
File chrome/browser/resources/shared/css/widgets.css (right):

http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/css/widgets.css:38: <if
expr="pp_ifdef('chromeos')">
this seems better than html[os='chromeos'] because

1. you don't have to call cr.enablePlatformSpecificCss() in every page (people
definitely will forget to do this)
2. you don't have to reproduce as many long-winded selectors (more readable and
maintainable)
3. slightly better performance wise.

therefore I think we should generally deprecate the html[os=foo] approach and
always do it at compile time.

[Dan Beam, 2012-03-13 01:37:42]

why are you enabling platform specific CSS but not using [os='X'] in a selector
(and using grit instead)?

[Dan Beam, 2012-03-13 01:50:10]

http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/histor...
File chrome/browser/resources/history/history.js (right):

http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/histor...
chrome/browser/resources/history/history.js:872:
cr.enablePlatformSpecificCSSRules();
where is this necessary?

http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/shared...
File chrome/browser/resources/shared/css/widgets.css (right):

http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/css/widgets.css:38: <if
expr="pp_ifdef('chromeos')">
> 1. you don't have to call cr.enablePlatformSpecificCss() in every page (people
> definitely will forget to do this)

+1

> 2. you don't have to reproduce as many long-winded selectors (more readable
and
> maintainable)

I think XML in the middle of CSS is about as readable as [os='blah'] if not less
so (but overall, a wash).

> 3. slightly better performance wise.

I agree with this for sure.

> therefore I think we should generally deprecate the html[os=foo] approach and
> always do it at compile time.

+1

http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/shared...
File chrome/browser/resources/shared_resources.grd (right):

http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared_resources.grd:44: file="shared/css/widgets.css"
type="BINDATA" flattenhtml="true"/>
nit: I guess a \s before /> to follow convention in this file

[Dan Beam, 2012-03-13 01:50:26]

On 2012/03/13 01:37:42, Dan Beam wrote:
> why are you enabling platform specific CSS but not using [os='X'] in a
selector
> (and using grit instead)?

sorry, I didn't read the rest of your comment

[csilv, 2012-03-13 01:55:51]

Tested on Mac, looks good.

LGTM

http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/shared...
File chrome/browser/resources/shared/css/widgets.css (right):

http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/css/widgets.css:38: <if
expr="pp_ifdef('chromeos')">
Agreed 100%.

[Evan Stade, 2012-03-13 01:58:09]

http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/histor...
File chrome/browser/resources/history/history.js (right):

http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/histor...
chrome/browser/resources/history/history.js:872:
cr.enablePlatformSpecificCSSRules();
On 2012/03/13 01:50:10, Dan Beam wrote:
> where is this necessary?

I put this here as a "it might be necessary" sort of thing. Like if someone
added an html[os=foo] rule before we have a chance to remove them all, they
wouldn't realize it wasn't going to apply to history.

http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/shared...
File chrome/browser/resources/shared/css/widgets.css (right):

http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/css/widgets.css:38: <if
expr="pp_ifdef('chromeos')">
On 2012/03/13 01:50:10, Dan Beam wrote:
> > 1. you don't have to call cr.enablePlatformSpecificCss() in every page
(people
> > definitely will forget to do this)
> 
> +1
> 
> > 2. you don't have to reproduce as many long-winded selectors (more readable
> and
> > maintainable)
> 
> I think XML in the middle of CSS is about as readable as [os='blah'] if not
less
> so (but overall, a wash).

I meant that if we were to do this in css, we would have to copy the entire
selector block and put html[os=] in front of each line; it's less readable
because you have to realize that the two blocks are exactly the same.

> 
> > 3. slightly better performance wise.
> 
> I agree with this for sure.
> 
> > therefore I think we should generally deprecate the html[os=foo] approach
and
> > always do it at compile time.
> 
> +1

http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/shared...
File chrome/browser/resources/shared_resources.grd (right):

http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared_resources.grd:44: file="shared/css/widgets.css"
type="BINDATA" flattenhtml="true"/>
On 2012/03/13 01:50:10, Dan Beam wrote:
> nit: I guess a \s before /> to follow convention in this file

done.

by the way, NB: this is necessary if you are going to use <if>, but only for
pp_ifdef it seems (i.e. it works if you forget this for is_win or is_linux).

[Patrick Dubroy, 2012-03-13 19:11:46]

Thanks for doing this -- it's been bugging me for a while. One thing though: I
don't think this is actually a platform-specific issue, it's font-specific. You
can see the difference on a single platform: on Mac, Times New Roman and
Helvetica have different baselines.

Maybe per-platform CSS is still the best way to fix it, but there will still be
issues with different fonts.

On 2012/03/13 01:58:09, Evan Stade wrote:
>
http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/histor...
> File chrome/browser/resources/history/history.js (right):
> 
>
http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/histor...
> chrome/browser/resources/history/history.js:872:
> cr.enablePlatformSpecificCSSRules();
> On 2012/03/13 01:50:10, Dan Beam wrote:
> > where is this necessary?
> 
> I put this here as a "it might be necessary" sort of thing. Like if someone
> added an html[os=foo] rule before we have a chance to remove them all, they
> wouldn't realize it wasn't going to apply to history.
> 
>
http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/shared...
> File chrome/browser/resources/shared/css/widgets.css (right):
> 
>
http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/shared...
> chrome/browser/resources/shared/css/widgets.css:38: <if
> expr="pp_ifdef('chromeos')">
> On 2012/03/13 01:50:10, Dan Beam wrote:
> > > 1. you don't have to call cr.enablePlatformSpecificCss() in every page
> (people
> > > definitely will forget to do this)
> > 
> > +1
> > 
> > > 2. you don't have to reproduce as many long-winded selectors (more
readable
> > and
> > > maintainable)
> > 
> > I think XML in the middle of CSS is about as readable as [os='blah'] if not
> less
> > so (but overall, a wash).
> 
> I meant that if we were to do this in css, we would have to copy the entire
> selector block and put html[os=] in front of each line; it's less readable
> because you have to realize that the two blocks are exactly the same.
> 
> > 
> > > 3. slightly better performance wise.
> > 
> > I agree with this for sure.
> > 
> > > therefore I think we should generally deprecate the html[os=foo] approach
> and
> > > always do it at compile time.
> > 
> > +1
> 
>
http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/shared...
> File chrome/browser/resources/shared_resources.grd (right):
> 
>
http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/shared...
> chrome/browser/resources/shared_resources.grd:44:
file="shared/css/widgets.css"
> type="BINDATA" flattenhtml="true"/>
> On 2012/03/13 01:50:10, Dan Beam wrote:
> > nit: I guess a \s before /> to follow convention in this file
> 
> done.
> 
> by the way, NB: this is necessary if you are going to use <if>, but only for
> pp_ifdef it seems (i.e. it works if you forget this for is_win or is_linux).

[Evan Stade, 2012-03-13 20:09:55]

On 2012/03/13 19:11:46, dubroy wrote:
> Thanks for doing this -- it's been bugging me for a while. One thing though: I
> don't think this is actually a platform-specific issue, it's font-specific.
You
> can see the difference on a single platform: on Mac, Times New Roman and
> Helvetica have different baselines.

yea

> 
> Maybe per-platform CSS is still the best way to fix it, but there will still
be
> issues with different fonts.

well, the other option is to force alignment of the controls' borders, but give
up baseline alignment. I think this patch represents the best option because it
will look good for the largest number of users (everyone who doesn't change the
default).

> 
> On 2012/03/13 01:58:09, Evan Stade wrote:
> >
>
http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/histor...
> > File chrome/browser/resources/history/history.js (right):
> > 
> >
>
http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/histor...
> > chrome/browser/resources/history/history.js:872:
> > cr.enablePlatformSpecificCSSRules();
> > On 2012/03/13 01:50:10, Dan Beam wrote:
> > > where is this necessary?
> > 
> > I put this here as a "it might be necessary" sort of thing. Like if someone
> > added an html[os=foo] rule before we have a chance to remove them all, they
> > wouldn't realize it wasn't going to apply to history.
> > 
> >
>
http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/shared...
> > File chrome/browser/resources/shared/css/widgets.css (right):
> > 
> >
>
http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/shared...
> > chrome/browser/resources/shared/css/widgets.css:38: <if
> > expr="pp_ifdef('chromeos')">
> > On 2012/03/13 01:50:10, Dan Beam wrote:
> > > > 1. you don't have to call cr.enablePlatformSpecificCss() in every page
> > (people
> > > > definitely will forget to do this)
> > > 
> > > +1
> > > 
> > > > 2. you don't have to reproduce as many long-winded selectors (more
> readable
> > > and
> > > > maintainable)
> > > 
> > > I think XML in the middle of CSS is about as readable as [os='blah'] if
not
> > less
> > > so (but overall, a wash).
> > 
> > I meant that if we were to do this in css, we would have to copy the entire
> > selector block and put html[os=] in front of each line; it's less readable
> > because you have to realize that the two blocks are exactly the same.
> > 
> > > 
> > > > 3. slightly better performance wise.
> > > 
> > > I agree with this for sure.
> > > 
> > > > therefore I think we should generally deprecate the html[os=foo]
approach
> > and
> > > > always do it at compile time.
> > > 
> > > +1
> > 
> >
>
http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/shared...
> > File chrome/browser/resources/shared_resources.grd (right):
> > 
> >
>
http://codereview.chromium.org/9689048/diff/1/chrome/browser/resources/shared...
> > chrome/browser/resources/shared_resources.grd:44:
> file="shared/css/widgets.css"
> > type="BINDATA" flattenhtml="true"/>
> > On 2012/03/13 01:50:10, Dan Beam wrote:
> > > nit: I guess a \s before /> to follow convention in this file
> > 
> > done.
> > 
> > by the way, NB: this is necessary if you are going to use <if>, but only for
> > pp_ifdef it seems (i.e. it works if you forget this for is_win or is_linux).

[Dan Beam, 2012-03-14 01:26:52]

lgtm (may want to write TODO above cr.enablePlatformSpecificCSSRules();)

[commit-bot: I haz the power, 2012-03-14 21:00:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/9689048/1006

[commit-bot: I haz the power, 2012-03-15 00:13:03]

Change committed as 126796