Standardize size of WebUI controls for touch and non-touch.

Standardize size of WebUI controls for touch and non-touch.

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

[kevers, 2012-08-09 19:41:19]

Hi Evan,

Can you please have a look at this CL.

[Evan Stade, 2012-08-10 02:44:16]

http://codereview.chromium.org/10837190/diff/1/chrome/browser/resources/share...
File chrome/browser/resources/shared/css/chrome_shared.css (right):

http://codereview.chromium.org/10837190/diff/1/chrome/browser/resources/share...
chrome/browser/resources/shared/css/chrome_shared.css:108: li >
button.link-button {
where is this pattern found? it seems dangerous to make a rule this generic when
it's quite easy to imagine new or existing code using this hierarchy but not
wanting the padding effects.

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

http://codereview.chromium.org/10837190/diff/1/chrome/browser/resources/share...
chrome/browser/resources/shared/css/widgets.css:73: margin-top: 6px;
how much verification did you do that this doesn't make anything look crazy?

[kevers, 2012-08-10 13:56:03]

http://codereview.chromium.org/10837190/diff/1/chrome/browser/resources/share...
File chrome/browser/resources/shared/css/chrome_shared.css (right):

http://codereview.chromium.org/10837190/diff/1/chrome/browser/resources/share...
chrome/browser/resources/shared/css/chrome_shared.css:108: li >
button.link-button {
On 2012/08/10 02:44:16, Evan Stade wrote:
> where is this pattern found? it seems dangerous to make a rule this generic
when
> it's quite easy to imagine new or existing code using this hierarchy but not
> wanting the padding effects.

To the best of my knowledge, this pattern appears in only one place in the UI,
namely, the chromeOS version of the "Add Language" dialog.  Without the padding,
the dialog is unusable with touch.  

This rule has been in effect for some time when the touch enabled flag is on. 
Could move the rule specifically to the language dialog, which would mean that
any new code written using this pattern would likely fail to be touch friendly.

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

http://codereview.chromium.org/10837190/diff/1/chrome/browser/resources/share...
chrome/browser/resources/shared/css/widgets.css:73: margin-top: 6px;
On 2012/08/10 02:44:16, Evan Stade wrote:
> how much verification did you do that this doesn't make anything look crazy?

This rule has been in effect for a while now with the touch optimized UI. 
Without it, a vertical stack of buttons are nearly touching, which creates some
tap errors.  The alternative is to use larger buttons; however, UX feedback
suggests keeping the button sizes smaller if possible.

One place where this pattern is used is on the keyboard shortcuts dialog. 
Attached a screenshot in the email.  Can do some experimentation to determine
how small the margin can be before tap error rates climb.

[kevers, 2012-08-10 16:10:07]

http://codereview.chromium.org/10837190/diff/6001/chrome/browser/resources/op...
File chrome/browser/resources/options2/language_options.css (right):

http://codereview.chromium.org/10837190/diff/6001/chrome/browser/resources/op...
chrome/browser/resources/options2/language_options.css:135: padding-top: 8px;
Moved from chrome-shared.

http://codereview.chromium.org/10837190/diff/6001/chrome/browser/resources/op...
File chrome/browser/resources/options2/search_engine_manager.css (right):

http://codereview.chromium.org/10837190/diff/6001/chrome/browser/resources/op...
chrome/browser/resources/options2/search_engine_manager.css:20: line-height:
16px;
Vertical alignment of the favicon was being thrown off due to line-height being
greater than height.

http://codereview.chromium.org/10837190/diff/6001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/css/widgets.css (right):

http://codereview.chromium.org/10837190/diff/6001/chrome/browser/resources/sh...
chrome/browser/resources/shared/css/widgets.css:73: margin-top: 2px;
Minimum margins for 28px vertical center to center distance on the settings
page.  Without this margin, controls in the "Appearance" and "Languages"
settings are packed too tightly for touch.

[Evan Stade, 2012-08-10 21:21:08]

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

http://codereview.chromium.org/10837190/diff/1/chrome/browser/resources/share...
chrome/browser/resources/shared/css/widgets.css:73: margin-top: 6px;
On 2012/08/10 13:56:03, kevers wrote:
> On 2012/08/10 02:44:16, Evan Stade wrote:
> > how much verification did you do that this doesn't make anything look crazy?
> 
> This rule has been in effect for a while now with the touch optimized UI. 
> Without it, a vertical stack of buttons are nearly touching, which creates
some
> tap errors.  The alternative is to use larger buttons; however, UX feedback
> suggests keeping the button sizes smaller if possible.
> 
> One place where this pattern is used is on the keyboard shortcuts dialog. 
> Attached a screenshot in the email.  Can do some experimentation to determine
> how small the margin can be before tap error rates climb.

sorry, I wasn't clear enough. I didn't mean that the margin would be too big and
look weird. I was thinking about all the many pages that use buttons and these
other elements, and wondering if one of the many pages would be broken by this
change.

The fact that it has been on for a while with the touch optimized build doesn't
give me much confidence, although it's better than nothing. But I'm still wary
without a manual pass over all webui pages because 1) we have a ton of pages and
2) poor spacing or padding or alignment in the past has often persisted for
several releases in a row, showing that most people don't care to file a bug
report when they notice issues, or simply don't consciously notice the issues.

so basically I was looking for an answer like "I manually inspected every page
in chrome://chrome, every dialog I could find in settings, downloads, feedback,
etc."

http://codereview.chromium.org/10837190/diff/6001/chrome/browser/resources/op...
File chrome/browser/resources/options2/language_options.css (right):

http://codereview.chromium.org/10837190/diff/6001/chrome/browser/resources/op...
chrome/browser/resources/options2/language_options.css:135: padding-top: 8px;
On 2012/08/10 16:10:08, kevers wrote:
> Moved from chrome-shared.

thx

[kevers, 2012-08-13 17:18:46]

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

http://codereview.chromium.org/10837190/diff/1/chrome/browser/resources/share...
chrome/browser/resources/shared/css/widgets.css:73: margin-top: 6px;
On 2012/08/10 21:21:08, Evan Stade wrote:
> On 2012/08/10 13:56:03, kevers wrote:
> > On 2012/08/10 02:44:16, Evan Stade wrote:
> > > how much verification did you do that this doesn't make anything look
crazy?
> > 
> > This rule has been in effect for a while now with the touch optimized UI. 
> > Without it, a vertical stack of buttons are nearly touching, which creates
> some
> > tap errors.  The alternative is to use larger buttons; however, UX feedback
> > suggests keeping the button sizes smaller if possible.
> > 
> > One place where this pattern is used is on the keyboard shortcuts dialog. 
> > Attached a screenshot in the email.  Can do some experimentation to
determine
> > how small the margin can be before tap error rates climb.
> 
> sorry, I wasn't clear enough. I didn't mean that the margin would be too big
and
> look weird. I was thinking about all the many pages that use buttons and these
> other elements, and wondering if one of the many pages would be broken by this
> change.
> 
> The fact that it has been on for a while with the touch optimized build
doesn't
> give me much confidence, although it's better than nothing. But I'm still wary
> without a manual pass over all webui pages because 1) we have a ton of pages
and
> 2) poor spacing or padding or alignment in the past has often persisted for
> several releases in a row, showing that most people don't care to file a bug
> report when they notice issues, or simply don't consciously notice the issues.
> 
> so basically I was looking for an answer like "I manually inspected every page
> in chrome://chrome, every dialog I could find in settings, downloads,
feedback,
> etc."

Removed rule in favor of standardizing the formatting of settings pages instead.
 Added "settings-row" as appropriate to fix the vertical spacing.

[kevers, 2012-08-13 18:47:48]

Several places on the settings pages where, depending on language, we have
multi-line instructions.  Without and explicit line-height, the row separation
is language dependent (height = 1.2em in English, 1.3em in Chinese and 1.0em in
Japanese).  The 1.0em case is clearly problematic and makes the text much harder
to read.

Chrome_shared has specific rules for paragraphs, which were not being
consistently applied wherever we had multi-line instructions.  Occasionally, the
instructions were simply enclosed in a span.

An explicit line-height is also required on the "Customize Languages..." dialog
to ensure that text lines in the header and footer are separated.

[Evan Stade, 2012-08-14 18:38:00]

lgtm

http://codereview.chromium.org/10837190/diff/8002/chrome/browser/resources/bo...
File chrome/browser/resources/bookmark_manager/css/bmm.css (right):

http://codereview.chromium.org/10837190/diff/8002/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/css/bmm.css:25: line-height: 30px;
should this be in em?

http://codereview.chromium.org/10837190/diff/8002/chrome/browser/resources/op...
File chrome/browser/resources/options2/browser_options.html (right):

http://codereview.chromium.org/10837190/diff/8002/chrome/browser/resources/op...
chrome/browser/resources/options2/browser_options.html:464: <p><span
id="cloudPrintConnectorLabel" class="settings-row"
why not convert the span to a <p>?

[kevers, 2012-08-14 20:26:52]

http://codereview.chromium.org/10837190/diff/8002/chrome/browser/resources/bo...
File chrome/browser/resources/bookmark_manager/css/bmm.css (right):

http://codereview.chromium.org/10837190/diff/8002/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/css/bmm.css:25: line-height: 30px;
On 2012/08/14 18:38:01, Evan Stade wrote:
> should this be in em?

Used 30px to be same height as element in the tree control in the LHS of the
bookmark manager.  Seems to look best when sizes in the two columns are
precisely aligned.  2em would work out to be 28px, and be shorter than a tree
entry due to its 1px border.

http://codereview.chromium.org/10837190/diff/8002/chrome/browser/resources/op...
File chrome/browser/resources/options2/browser_options.html (right):

http://codereview.chromium.org/10837190/diff/8002/chrome/browser/resources/op...
chrome/browser/resources/options2/browser_options.html:464: <p><span
id="cloudPrintConnectorLabel" class="settings-row"
On 2012/08/14 18:38:01, Evan Stade wrote:
> why not convert the span to a <p>?

Done.

[Evan Stade, 2012-08-14 22:06:38]

http://codereview.chromium.org/10837190/diff/8002/chrome/browser/resources/bo...
File chrome/browser/resources/bookmark_manager/css/bmm.css (right):

http://codereview.chromium.org/10837190/diff/8002/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/css/bmm.css:25: line-height: 30px;
On 2012/08/14 20:26:53, kevers wrote:
> On 2012/08/14 18:38:01, Evan Stade wrote:
> > should this be in em?
> 
> Used 30px to be same height as element in the tree control in the LHS of the
> bookmark manager.  Seems to look best when sizes in the two columns are
> precisely aligned.  2em would work out to be 28px, and be shorter than a tree
> entry due to its 1px border.

that seems like a valid reason. Although you can always use floating-point ems.
regardless, lgtm.

[commit-bot: I haz the power, 2012-08-15 13:29:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kevers@chromium.org/10837190/4013

[commit-bot: I haz the power, 2012-08-15 15:48:48]

Change committed as 151698